Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portability fixes for FreeBSD #7869

Closed
wants to merge 4 commits into from
Closed

Conversation

arrowd
Copy link
Contributor

@arrowd arrowd commented Mar 8, 2025

This PR starts with small steps towards porting the master branch to FreeBSD. I will continue this endeavor if there is a positive feedback on this.

@alexey-tikhonov alexey-tikhonov self-requested a review March 10, 2025 08:12
@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Mar 10, 2025
@andreboscatto andreboscatto requested a review from ikerexxe March 11, 2025 13:38
@ikerexxe
Copy link
Contributor

Can you please add some documentation for the xucred structure? I've tried searching online but haven't found any useful resources.

@arrowd
Copy link
Contributor Author

arrowd commented Mar 12, 2025

Hope it makes it clearer.

@arrowd
Copy link
Contributor Author

arrowd commented Mar 12, 2025

Yes, this PR is not enough to make SSSD compile on FreeBSD. I was planning to add more changes once this PR gets accepted.

Or I can just continue adding commits to this PR, if you're open to merging it once it is ready.

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Mar 12, 2025

One PR per "topic" makes sense (like 'ucred vs xcred', for example).
But merging half baked makes me uncomfortable.

I mean, we can merge "complete" (for a given "topic") PR if it looks good even if a whole thing doesn't compile on FreeBSD yet.

Use it to fix build of sss_client/common.c on FreeBSD.
@alexey-tikhonov
Copy link
Member

Let's move libcap related patches to a separate PR.

@arrowd arrowd force-pushed the freebsd-port branch 2 times, most recently from 26819c1 to 584532b Compare March 12, 2025 17:45
@@ -14,6 +14,10 @@
#include <fcntl.h>
#include <unistd.h>

#if !defined(SO_PEERCRED) && defined(LOCAL_PEERCRED)
#define SO_PEERCRED LOCAL_PEERCRED
#endif
Copy link
Member

@alexey-tikhonov alexey-tikhonov Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh... I just realized that below in this file:

#ifdef __OpenBSD__
    struct sockpeercred *cr;
#else
    struct ucred *cr;
#endif

If you really want to make this test portable then you need to fix this place also.

(1) I have no idea why this #ifdef __OpenBSD__ was added, I don't know if SSSD code compiles there.
(@sumit-bose, @pbrezina, do you know / remember?)

(2) we are slowly removing intg-tests anyway, so probably you don't want to bother.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what exactly to fix there?

Copy link
Member

@alexey-tikhonov alexey-tikhonov Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#ifdef __OpenBSD__
    struct sockpeercred *cr;
#else
    struct ucred *cr;
#endif

won't compile on FreeBSD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to fix this, you need to replace it with STRUCT_CRED and then all the code like cr->uid != 0.

But again, probably you can leave this file intact, as it likely will be removed anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, sorry, got a brain glitch. I omitted this change for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh... I just realized that below in this file:

#ifdef __OpenBSD__
    struct sockpeercred *cr;
#else
    struct ucred *cr;
#endif

If you really want to make this test portable then you need to fix this place also.

(1) I have no idea why this #ifdef __OpenBSD__ was added, I don't know if SSSD code compiles there. (@sumit-bose, @pbrezina, do you know / remember?)

(2) we are slowly removing intg-tests anyway, so probably you don't want to bother.

Hi,

not sure, but the code might have benn inspired by the original dbus code https://gitlab.freedesktop.org/dbus/dbus/-/blame/main/dbus/dbus-sysdeps-unix.c?ref_type=heads&page=3#L2305 . In general I think that the OpenBSD ifdef is not strictly needed but I'm not sure if there might someone trying to port to OpenBSD as well.

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it turns out that OpenBSD has SO_PEERCRED like Linux, but the structure is named differently. It might worth reflecting in util_creds.h, but I'd better leave that to a OpenBSD developer.

@@ -45,6 +45,7 @@
#define _(STRING) dgettext (PACKAGE, STRING)
#include "sss_cli.h"
#include "common_private.h"
#include "util/util_creds.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just some notes, not a "change request")
Maybe makes sense to '#undef HAVE_SELINUX' here to avoid pulling <selinux/*> headers... Not sure.
Or maybe better to split "util_creds.h" into two headers... (out of this PR)

@alexey-tikhonov
Copy link
Member

Thank you.
In general looks good to me (barring pollution of client lib namespace with selinux headers).
Let's wait for other reviewers and CI.

Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope it makes it clearer.

Yes, that clarifies how this structure works. The patches LGTM!

As a general note, this project is open to contributions from the free and open source software community. So feel free to create new proposals for changes.

@arrowd
Copy link
Contributor Author

arrowd commented Mar 13, 2025

Absolutely, I plan to continue submitting patches until the master branch returns to a working state on FreeBSD.

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Mar 13, 2025

@arrowd, please take a look at 2ea30fc

If you agree with this patch, then please cherry-pick it to your PR.

@alexey-tikhonov alexey-tikhonov self-assigned this Mar 13, 2025
@alexey-tikhonov
Copy link
Member

As a side note, what's your plan wrt Linux-specific [files] capabilities used by sssd-2.10+?
IIUC, FreeBSD doesn't have it (but uses quite different 'capabilities' model)?

@arrowd
Copy link
Contributor Author

arrowd commented Mar 14, 2025

As a side note, what's your plan wrt Linux-specific [files] capabilities used by sssd-2.10+?
IIUC, FreeBSD doesn't have it (but uses quite different 'capabilities' model)?

In the short term - try to #ifdef it. The capabilities model of Capsicum is indeed very different from Linux, which makes it hard to properly abstract it. We'll see.

so that sss_client code doesn't pull selinux headers.
@alexey-tikhonov
Copy link
Member

@ikerexxe, please take a look at additional patch.

Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, ack

@alexey-tikhonov
Copy link
Member

Pushed PR: #7869

  • master
    • 150d2ee - Move 'STRUCT_CRED' definition into standalone header
    • 38fe14a - Use LOCAL_PEERCRED option instead SO_PEERCRED where appropriate
    • 843aa08 - Extend util_creds.h with xucred case
    • add0ed1 - platform.m4: Teach to look for struct xucred in addition to struct ucred

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport This should go to target branch only. Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants