Add FreeBSD support, round 1 #7765

fluffykhv · 2024-12-16T10:30:17Z

Port patches from FreeBSD ports tree (https://github.com/freebsd/freebsd-ports/tree/main/security/sssd2/files), round one

alexey-tikhonov · 2024-12-16T11:51:27Z

Hi @fluffykhv,
thank you for the contribution.

Is "Add FreeBSD support, round 1" a single huge patch in FreeBSD repo?
If this is a set of patches, wouldn't it make sense to have a commit per patch to make the review easier?

emaste · 2024-12-26T17:59:11Z

Patches in the FreeBSD ports collection are typically per-file rather than per-logical-change, so it may not be straightforward to recreate an incremental patch set. If you look at the URL above you'll see the per-file patches. This is admittedly not ideal from the perspective of collaboration with an upstream.

Looking at the history (e.g. https://github.com/freebsd/freebsd-ports/commits/main/security/sssd2/files) gives some insight, and you can see some logical changes that could be applied individually e.g. freebsd/freebsd-ports@9a40ff8.

If you want a patch set of logical changes we'll have to curate the patches in the ports tree.

alexey-tikhonov · 2025-01-06T17:13:24Z

Well, that's going to be tough...

As I'm not familiar with FreeBSD, it would help greatly if you could provide a rationale for additional includes.
What especially strikes out:

<sys/param.h> (what's there?)
<sys/types.h> (why?)
<sys/time.h> (why?)
everywhere.

I think I'll cherry pick trivial / non-questionable patches in a separate PR, and once it's merged, you'll have to rebase.
But in general it would be great to have as little #ifdef __FreeBSD__ in the code as feasible.

In case of headers that might be finding a common ground where possible and, where it's impossible, putting conditional includes in a single common header to reduce clutter overall.

Wrt prctl(): there is HAVE_PRCTL - can it be used instead?
Btw, patches do not touch https://github.com/SSSD/sssd/blob/master/src/util/capabilities.c#L23 -- is this because FreeBSD didn't pick sssd-2.10+ yet?

arrowd · 2025-01-06T17:19:53Z

As I'm not familiar with FreeBSD, it would help greatly if you could provide a rationale for additional includes.

The general answer is - the transitive dependencies between FreeBSD headers are different from the Linux ones. It often comes up when porting software for FreeBSD that a single #include <foo.h> line works for Linux, because at also pulls in bar.h, but on FreeBSD these headers are decoupled from each other, so you have to include both in the source file. I think Dima can provide you exact error messaged if you're interested.

In case of headers that might be finding a common ground where possible and, where it's impossible, putting conditional includes in a single common header to reduce clutter overall.

If I'm not mistaken, POSIX dictates what header should be included to be able to use a given struct or function. So it is not matter of finding a common ground between different OSes, but simply following the standard.

Btw, patches do not touch https://github.com/SSSD/sssd/blob/master/src/util/capabilities.c#L23 -- is this because FreeBSD didn't pick sssd-2.10+ yet?

Yes, we're still at 2.9 at the moment. Hopefully we'll catch up soon.

alexey-tikhonov · 2025-01-06T17:35:07Z

If I'm not mistaken, POSIX dictates what header should be included to be able to use a given struct or function.

Where it is POSIX mandated it will be "non questionable" and it is where some help with pointers would be appreciated. It will be just WAY slower for non FreeBSD user to figure out a reason for inclusion of particular header.

src/util/server.c

+    /*
    CatchSignal(SIGSEGV, sig_segv_abrt);
    CatchSignal(SIGABRT, sig_segv_abrt);
+    */


alexey-tikhonov · 2025-01-21T10:01:04Z

src/confdb/confdb.c

@@ -21,6 +21,9 @@

 #include "config.h"

+#ifdef __FreeBSD__
+#include <sys/param.h>


Since b928dbe , "util/util.h" included below, includes <sys/param.h>

But what in confdb.c requires it?

alexey-tikhonov · 2025-01-21T10:03:27Z

src/krb5_plugin/common/radius_kdcpreauth.c

@@ -18,6 +18,9 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */

+#ifdef __FreeBSD__
+#include <sys/param.h>


Since b928dbe , "util/util.h" included below, includes <sys/param.h>

But what in radius_kdcpreauth.c requires it?

alexey-tikhonov · 2025-01-21T10:12:30Z

src/lib/winbind_idmap_sss/winbind_idmap_sss.c

@@ -22,6 +22,8 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */

+#include <sys/types.h>
+#include <sys/time.h>


Why are those 2 headers required?

alexey-tikhonov · 2025-01-21T10:13:11Z

src/lib/winbind_idmap_sss/winbind_idmap_sss.h

@@ -29,6 +29,8 @@
 #include <stdbool.h>

 #include <core/ntstatus.h>
+#include <unistd.h>
+#include <time.h>


alexey-tikhonov · 2025-01-21T10:17:45Z

src/p11_child/p11_child_common.c

@@ -27,7 +27,9 @@
 #include <stdlib.h>
 #include <string.h>
 #include <popt.h>
+#ifndef __FreeBSD__
 #include <sys/prctl.h>


There is still a call to prctl() in this file. How do you compile it?

arrowd · 2025-01-21T10:25:35Z

Let's approach this from a different angle. Would you be OK with enabling FreeBSD CI for this project via Cirrus CI? I will be happy to provide a configuration file, but you'll need to enable the Cirrus application for this repo.

With CI in place we can work through all the FreeBSD compilation errors, so it'd be clear what actual problem a commit fixes.

alexey-tikhonov · 2025-01-21T10:30:55Z

It seems huge problem for adoption of sssd-2.10+ by FreeBSD will be usage of 'files capabilities' (Linux specific) and management of effective/permitted 'capabilities' (again, as meant in Linux) via prctl()

You are patching sssd-2.9 where prctl() is used to set dumpable flag and it is easy to cut it off.
It will be much more complicated for sssd-2.10+...

alexey-tikhonov · 2025-01-21T10:32:58Z

src/providers/ad/ad_common.c

@@ -19,6 +19,9 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
+#ifdef __FreeBSD__
+#include <sys/param.h>


arrowd · 2025-01-21T10:33:01Z

All right, let's close this for now. I'll reach our folks that are maintaining the SSSD port and ask them if they're working on 2.10.

But in case they are, the question about the CI stands.

alexey-tikhonov · 2025-01-21T10:33:32Z

src/providers/ad/ad_pac.h

@@ -32,6 +32,8 @@
 #ifdef ldb_val
 #error Please make sure to include ad_pac.h before ldb.h
 #endif
+#include <unistd.h>
+#include <time.h>


alexey-tikhonov · 2025-01-21T10:34:17Z

src/providers/ad/ad_pac_common.c

@@ -21,6 +21,9 @@
 */


+#include <sys/types.h>
+#include <sys/time.h>


What for?
Also note you already added time.h to ad_pac.h that is included here.

alexey-tikhonov · 2025-01-21T10:41:16Z

src/util/sss_bsd_errno.h

+    Authors:
+        Lukas Slebodnik <[email protected]>
+
+    Copyright (C) 2013 Red Hat


Where is this file from?

alexey-tikhonov · 2025-01-21T10:48:24Z

src/util/sss_bsd_errno.h

+
+#include <errno.h>
+
+#define BSD_ERR_MASK (0xB5DE <<16)


A comment would help to understand this mask.

alexey-tikhonov · 2025-01-21T10:48:31Z

src/util/sss_bsd_errno.h

+
+#define BSD_ERR_MASK (0xB5DE <<16)
+
+#ifndef EUCLEAN


EUCLEAN, EMEDIUMTYPE, EOWNERDEAD and ECONNRESET aren't used.

alexey-tikhonov · 2025-01-21T10:51:35Z

src/providers/data_provider/dp_modules.c

@@ -23,6 +23,9 @@
 #include "providers/data_provider/dp.h"
 #include "providers/data_provider/dp_private.h"
 #include "providers/backend.h"
+#ifdef __FreeBSD__
+#include "util/sss_bsd_errno.h"


Please include it once in util/util_errors.h

alexey-tikhonov · 2025-01-21T10:51:54Z

src/providers/data_provider_be.c

@@ -25,6 +25,8 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>


alexey-tikhonov · 2025-01-21T10:56:05Z

src/providers/ipa/ipa_deskprofile_rules_util.c

@@ -23,6 +23,8 @@
 #include "providers/ipa/ipa_deskprofile_rules_util.h"
 #include "providers/ipa/ipa_deskprofile_private.h"
 #include "providers/ipa/ipa_rules_common.h"
+#include <sys/types.h>
+#include <signal.h>


alexey-tikhonov · 2025-01-21T12:46:59Z

src/util/util.c

+    struct tm tm;
+    time_t t = 0;
+    tzset();
+    localtime_r(&t, &tm);


Man page for 'localtime_r' says:

It need not set tzname, timezone, and daylight.

so looks like tzset() isn't needed

alexey-tikhonov · 2025-01-21T15:46:22Z

src/util/util_creds.h

@@ -64,12 +64,25 @@ typedef void * SEC_CTX;
 #endif /* done HAVE_SELINUX */

 #include <sys/socket.h>
+#ifdef __FreeBSD__
+#include <sys/param.h>
+#include <sys/ucred.h>


I think

sssd/src/external/platform.m4

Line 46 in e76849b

AC_CHECK_MEMBERS([struct ucred.pid, struct ucred.uid, struct ucred.gid], , ,

should be extended to detect availability of xucred

arrowd · 2025-01-21T15:48:58Z

@alexey-tikhonov Like I wrote in #7765 (comment) I see little sense in reviewing this PR in its current state. We (FreeBSD) should start over with master branch and this PR is better be closed.

I still want to hear if you would like us to prepare a FreeBSD CI.

alexey-tikhonov · 2025-01-21T15:52:32Z

src/responder/nss/nsssrv_mmap_cache.c

@@ -1445,8 +1446,14 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name,
    /* Attempt allocation several times, in case of EINTR */
    for (int i = 0; i < POSIX_FALLOCATE_ATTEMPTS; i++) {
        ret = posix_fallocate(mc_ctx->fd, 0, mc_ctx->mmap_size);
-        if (ret != EINTR)
-            break;
+        if (ret != EINTR && ret == EINVAL) {


ret != EINTR && part doesn't make sense.

alexey-tikhonov · 2025-01-21T16:12:11Z

src/tests/cmocka/test_authtok.c

@@ -28,6 +28,7 @@
 #include "tests/cmocka/common_mock.h"

 #include "util/authtok.h"
+#include "util/sss_endian.h"


This header defines htole32 and le32toh that aren't used in this file.

alexey-tikhonov · 2025-01-21T16:33:00Z

@arrowd,

@alexey-tikhonov Like I wrote in #7765 (comment) I see little sense in reviewing this PR in its current state. We (FreeBSD) should start over with master branch and this PR is better be closed.

Well, there are some differences, but most of questions will be still valid.

In general I'd like to propose to use ./configure checks more extensively instead of abrupt '#ifdef FreeBSD' inserts.

Could you please take a look at #7808 - will the check of '_XOPEN_SOURCE' work around absence of 'timezone' on FreeBSD?

I still want to hear if you would like us to prepare a FreeBSD CI.

That's more question to @pbrezina.

But there is no CI automation that would not need a maintenance. And we already have quite a lot of items that likes to get broken...

In any case, I would really appreciate comments for '#include'-s being added.
With or without CI, somebody needs to remove those 1-by-1 to see a specific compilation error.

arrowd · 2025-01-21T16:48:26Z

Could you please take a look at #7808 - will the check of '_XOPEN_SOURCE' work around absence of 'timezone' on FreeBSD?

Not sure what's being talked about here - FreeBSD does have timezone. I'll say this again - this PR is prepared very poorly and shouldn't be looked at.

alexey-tikhonov · 2025-01-21T17:09:25Z

Could you please take a look at #7808 - will the check of '_XOPEN_SOURCE' work around absence of 'timezone' on FreeBSD?

Not sure what's being talked about here - FreeBSD does have timezone.

I thought this PR gets rids of its usage for a reason...
At least from a quick glance - man tzset on FreeBSD doesn't mention extern long timezone;

I'll say this again - this PR is prepared very poorly and shouldn't be looked at.

Ok.

fluffykhv added 5 commits December 16, 2024 13:26

Add FreeBSD support, round 1

b7ea335

src/util/child_common.c: fix typo

c356553

src/util/child_common.c: fix condition

913d158

src/util/child_common.c: rewrite condition

86898c4

src/sss_client/pam_sss_gss.c: rewrite #ifdef guards fround macros

005689f

github-advanced-security bot found potential problems Jan 6, 2025

View reviewed changes

src/util/server.c

Comment on lines +324 to +327

/*

CatchSignal(SIGSEGV, sig_segv_abrt);

CatchSignal(SIGABRT, sig_segv_abrt);

*/

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.

alexey-tikhonov self-assigned this Jan 7, 2025

alexey-tikhonov self-requested a review January 7, 2025 11:51

Merge branch 'SSSD:master' into master

6eacf76

alexey-tikhonov reviewed Jan 21, 2025

View reviewed changes

src/util/sss_bsd_errno.h

Authors:

Lukas Slebodnik <[email protected]>

Copyright (C) 2013 Red Hat

Copy link

Member

alexey-tikhonov Jan 21, 2025

Choose a reason for hiding this comment

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

Where is this file from?

alexey-tikhonov reviewed Jan 21, 2025

View reviewed changes

src/util/sss_bsd_errno.h

#include <errno.h>

#define BSD_ERR_MASK (0xB5DE <<16)

Copy link

Member

alexey-tikhonov Jan 21, 2025

Choose a reason for hiding this comment

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

A comment would help to understand this mask.

alexey-tikhonov reviewed Jan 21, 2025

View reviewed changes

alexey-tikhonov closed this Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FreeBSD support, round 1 #7765

Add FreeBSD support, round 1 #7765

fluffykhv commented Dec 16, 2024

alexey-tikhonov commented Dec 16, 2024

emaste commented Dec 26, 2024

alexey-tikhonov commented Jan 6, 2025 •

edited

Loading

arrowd commented Jan 6, 2025 •

edited

Loading

alexey-tikhonov commented Jan 6, 2025

alexey-tikhonov Jan 21, 2025 •

edited

Loading

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

arrowd commented Jan 21, 2025

alexey-tikhonov commented Jan 21, 2025

alexey-tikhonov Jan 21, 2025

arrowd commented Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

arrowd commented Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov Jan 21, 2025

alexey-tikhonov commented Jan 21, 2025

arrowd commented Jan 21, 2025

alexey-tikhonov commented Jan 21, 2025

Add FreeBSD support, round 1 #7765

Add FreeBSD support, round 1 #7765

Conversation

fluffykhv commented Dec 16, 2024

alexey-tikhonov commented Dec 16, 2024

emaste commented Dec 26, 2024

alexey-tikhonov commented Jan 6, 2025 • edited Loading

arrowd commented Jan 6, 2025 • edited Loading

alexey-tikhonov commented Jan 6, 2025

alexey-tikhonov Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arrowd commented Jan 21, 2025

alexey-tikhonov commented Jan 21, 2025

Choose a reason for hiding this comment

arrowd commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arrowd commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-tikhonov commented Jan 21, 2025

arrowd commented Jan 21, 2025

alexey-tikhonov commented Jan 21, 2025

alexey-tikhonov commented Jan 6, 2025 •

edited

Loading

arrowd commented Jan 6, 2025 •

edited

Loading

alexey-tikhonov Jan 21, 2025 •

edited

Loading