Netatalk / netatalk

Netatalk is a Free and Open Source AFP fileserver. A *NIX or BSD system running Netatalk is capable of serving many Macintosh clients simultaneously as an AppleShare file server.
https://netatalk.io
GNU General Public License v2.0
330 stars 85 forks source link

Unable to build with wolfssl 5.7.2 #1421

Closed rdmark closed 1 week ago

rdmark commented 3 weeks ago

When building netatalk against wolfssl 5.7.2, compilation fails with the error in the title.

This is a spin-off of the upstream bug https://github.com/wolfSSL/wolfssl/issues/7827

The proposed fix is to include wolfssl/wolfcrypt/settings.h in each netatalk source file that uses wolfssl symbols.

knight-of-ni commented 2 weeks ago

I've been placing most of my notes over on the wolfssl site. This is a wierd one that has me completely stumped at the moment. Despite including settings.h, the build still fails exactly the same as it did before.

The build fails, stating the FIPS_VERSION3_GE macro is not defined, but it is defined right here: https://github.com/wolfSSL/wolfssl/blob/master/wolfssl/wolfcrypt/settings.h#L382

I'm hoping someone else will come along with the same problem and possibly uncover something I have not realized.

knight-of-ni commented 2 weeks ago

ok, we had a eureka moment over at https://github.com/wolfSSL/wolfssl/issues/7827. They identified that, despite meson detecting the external wolfssl libraries and headers, the compiler was finding the bundled settings.h first, which was based off wolfssl 5.7.0, rather than 5.7.2... hence the compiler complaining the macro in question was not defined (the macro in question is new for 5.7.2).

What this means for me, is that I need to delete the wolfssl bundled with netatalk during the rpm build process. I'm not sure if there is anything the Netatalk team should or can do here. I think we still need to merge #1422, but let's wait until we resolve the next issue....

After making that change, however, I ran into a new error:

FAILED: bin/afppasswd/afppasswd.p/afppasswd.c.o 
gcc -Ibin/afppasswd/afppasswd.p -Ibin/afppasswd -I../bin/afppasswd -I. -I.. -Iinclude -I../include -Ietc/afpd -I../etc/afpd -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Wpedantic -std=c11 -DHAVE_CONFIG_H '-D_U_=__attribute__((unused))' -Wno-pedantic -Wno-extra -Wno-all -Wno-deprecated-declarations -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer '-D_PATH_AFPDPWFILE="/etc/netatalk/afppasswd"' '-DVERSION="3.2.7"' -MD -MQ bin/afppasswd/afppasswd.p/afppasswd.c.o -MF bin/afppasswd/afppasswd.p/afppasswd.c.o.d -o bin/afppasswd/afppasswd.p/afppasswd.c.o -c ../bin/afppasswd/afppasswd.c
../bin/afppasswd/afppasswd.c: In function ‘convert_passwd’:
../bin/afppasswd/afppasswd.c:84:14: error: ‘DES_KEY_SZ’ undeclared (first use in this function)
   84 |     if (j <= DES_KEY_SZ)
      |              ^~~~~~~~~~

After some digging I discovered that DES_KEY_SZ was never part of upstream wolfssl but was added to bundled wolfssl in netatalk: https://github.com/Netatalk/netatalk/commit/d1ef115f5a752f721eca330dd3798c88f7ec0d95

We need to either define DES_KEY_SZ in another location in Netatalk project, or make a case with upstream wolfssl team to add it to their project.

Thoughts?

knight-of-ni commented 2 weeks ago

@rdmark As I continue to read through the Netatalk commit history, I noticed that the main branch replaced DES_KEY_SZ with DES_KEY_SIZE through a series of commits (I think having to do with nettle).

DES_KEY_SIZE is defined in upstream wolfcrypt des3.h here.

So I think what we need is to identify the commit(s) used to replace DES_KEY_SZ in these files:

$ grep -r DES_KEY_SZ ./
./include/wolfssl/openssl/des.h:#define DES_KEY_SZ            (sizeof(DES_cblock))
./etc/uams/uams_randnum.c:  uint8_t key[DES_KEY_SZ*2];
./etc/uams/uams_randnum.c:    if (j <= DES_KEY_SZ)
./etc/uams/uams_randnum.c:      if (j <= DES_KEY_SZ)
./etc/uams/uams_randnum.c:    for (i = j = 0; i < DES_KEY_SZ; i++, j += 2) {
./bin/afppasswd/afppasswd.c:    if (j <= DES_KEY_SZ)
./bin/afppasswd/afppasswd.c:    if (j <= DES_KEY_SZ)
./bin/afppasswd/afppasswd.c:    for (i = j = 0; i < DES_KEY_SZ; i++, j += 2) {

Include these changes with pr #1422 and then release Netatalk 3.2.8? I would like to test the proposed changes before the release is official. What do you think?

rdmark commented 2 weeks ago

Thanks for working on this! I wonder if we want to bump the bundled wolfssl code to 5.7.2 as well?

We still need to support nettle so let's test that changing the symbol name doesn't break linking with nettle.

Do you want to file a PR with the proposed changeset?

Regarding testing 3.2.8 before the official release: What do you need to make this happen? Is a git tag sufficient, or do you need release tarballs too?

knight-of-ni commented 2 weeks ago

After looking into this further, I was not able to find small commit(s) I could easily cherry pick from the main branch. The changes I was looking at were part of much larger commits, which seemed like scope creep. That would make netatalk 3.2 branch look more like the 4.0 milestone.

Shifting directions, I settled on these commits: https://github.com/knight-of-ni/Netatalk/commit/847036e55604baff0bcb202ec5a6a5da99108cd7 https://github.com/knight-of-ni/Netatalk/commit/7dccaa5520a03c4a0272ce5951e1dfb19ecee423

The first is the same as #1422, but refactored for netatalk-3-2 branch. The second defines DES_KEY_SZ if it isn't already defined. I'll add a PR for this.

After patching netatalk 3.2.7 with these commits and deleting bundled wolfssl library, netatalk now builds successfully against external wolfssl 5.7.2.

I would absolutely recommend we upgrade the bundled wolfssl to 5.7.2. There are multiple CVE's that have been fixed: https://github.com/wolfSSL/wolfssl/releases/tag/v5.7.2-stable

rdmark commented 2 weeks ago

Good stuff! Yep, let's bump the bundled wolfssl to 5.7.2 across the board. Is this something you would be open to working on, as well?

and deleting bundled wolfssl library

Do you have to delete the bundled library (libatalk/ssl and include/wolfssl) in its entirety to get it to build with the system library? I wonder if there is a more graceful way to make it work?

At a related note, we have similar issues on Arch Linux and NetBSD. Note to self to do further troubleshooting with the insights in this thread. https://github.com/Netatalk/netatalk/issues/1354

rdmark commented 1 week ago

The changeset between 5.7.0 and 5.7.2 is surprisingly massive for a bugfix release: https://github.com/wolfSSL/wolfssl/compare/v5.7.0-stable...v5.7.2-stable

It can clearly not be applied as a patch like this. The better approach would probably be to iterate over wolfssl source files that were adopted into netatalk and identify those covered by the changeset.

knight-of-ni commented 1 week ago

I created pr #1425 to upgrade wolfssl to 2.7.2. Let's see if the CI workflows like this change.

I just got back from vacation and noticed RHEL 10 just branched. I'm going to be tied up with other priorities for the moment.

rdmark commented 1 week ago

Thanks for the PR! Only the Debian/Ubuntu jobs passed, curiously. (edit: obviously, because they use the system wolfssl library, and not the bundled one.) And frankly, I don't think we need all those new source files. We deliberately bundled a subset of WolfSSL – only the parts we strictly need (it's a highly modular library.)

Let me take a stab at it and see how it goes.

rdmark commented 1 week ago

I think the main cause of the failures is because all of the EMBEDDED_SSL preprocessor macros got reverted, which is a local modification to the bundled WolfSSL code... It's a bit unfortunate because it makes upgrades like this cumbersome. If you have a suggestion on how to handle this more gracefully, I'm all ears. :)

But in the meantime I will merge the updated code while maintaining the local modifications...

For reference, see https://github.com/Netatalk/netatalk/pull/1036

rdmark commented 1 week ago

In addition, updates for the actual implementation code was missing (everything under libatalk/ssl ). This compiles at least now: https://github.com/Netatalk/netatalk/pull/1426

knight-of-ni commented 1 week ago

I'm certainly not trying to tell you how to run this project, but bringing in the entire wolfssl library would have some advantages. One would be able to use a bot to check for new versions and warn if vulnerabilities exists. You can also auto-generate a pull request when new versions are available, and then let the CI workflows do their thing on it. Nextcloud project does this. It's really slick.

Admitadley that also means you would have to find a way to build netatalk w/o local modifications to the bundled wolfssl library.

rdmark commented 1 week ago

This seems increasingly like a good idea to me, after spending over 2 hours last night manually merging everything.

And in case local modifications have to be kept, I would lean towards storing those as a patch that can be applied to the upstream code. This would serve both as a tool and as documentation.

rdmark commented 1 week ago

Closing this since the original issue has been resolved.

Further work will be done in https://github.com/Netatalk/netatalk/issues/1432 and https://github.com/Netatalk/netatalk/issues/1433

rdmark commented 1 week ago

@knight-of-ni Did you dynamically test the DHX UAM after building with the packaged wolfssl 5.7.2, by any chance? I can't get it to actually load its symbols when building against the upgraded bundled library. A bit stumped right now.

Sep 06 21:40:34 alien afpd[467940]: uam_load(uams_dhx.so): failed to load: /usr/local/lib/x86_64-linux-gnu/netatalk/uams_dhx.so: undefined symbol: wolfSSL_DH_get0_key
Sep 06 21:40:34 alien afpd[467940]: uam: uams_dhx.so load failure
knight-of-ni commented 1 week ago

No, I don't know of a way to test DHX communication w/o an old Mac running Os 9.... now you have me wondering if I can virtualize mac os 9 on a Mac Air w/ intel cpu... or wait... I've got a pre-unibody macbook pro lying around. Oh great... there goes another week of my time.

How did you get to that point? Did you just start Netatalk and saw those errors immediately, or did you have to initiate a connection from an old Mac?

During my tests building rpms, I noticed Netatalk was finding and using the header files in the bundled wolfssl 2.7.0, despite detecting external wolfssl 2.7.2. That might cause a problem like the one you described.

rdmark commented 1 week ago

This error occurs immediately when you launch netatalk. No Mac required. Just keep an eye on the syslog. It is not a fatal error, netatalk skips the DHX UAM and proceeds, so you won't notice unless you look closely at the logs.

I would recommend SheepShaver for emulating OS8 or OS9! It runs great on my own Intel MBA.

knight-of-ni commented 1 week ago

This is what I see in journalctl when I start Netatalk 3.2.7 built against wolfssl 5.7.2:

Sep 06 12:33:09 vFedora systemd[1]: Starting netatalk.service - Netatalk AFP fileserver for Macintosh clients...
Sep 06 12:33:09 vFedora systemd[1]: netatalk.service: Can't open PID file /run/lock/netatalk/netatalk (yet?) after start: No such file or directory
Sep 06 12:33:09 vFedora netatalk[7350]: Netatalk AFP server starting
Sep 06 12:33:09 vFedora netatalk[7350]: Registered with Zeroconf
Sep 06 12:33:09 vFedora audit[1]: SERVICE_START pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel msg='unit=netatalk comm="systemd" exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=? res=success'
Sep 06 12:33:09 vFedora systemd[1]: Started netatalk.service - Netatalk AFP fileserver for Macintosh clients.
Sep 06 12:33:09 vFedora cnid_metad[7353]: CNID Server listening on localhost:4700
Sep 06 12:33:09 vFedora audit[7338]: USER_END pid=7338 uid=1002 auid=1002 ses=3 subj=kernel msg='op=PAM:session_close grantors=pam_keyinit,pam_limits,pam_keyinit,pam_limits,pam_systemd,pam_unix acct="root" exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/0 res=success'
Sep 06 12:33:09 vFedora sudo[7338]: pam_unix(sudo:session): session closed for user root
Sep 06 12:33:09 vFedora audit[7338]: CRED_DISP pid=7338 uid=1002 auid=1002 ses=3 subj=kernel msg='op=PAM:setcred grantors=pam_env,pam_unix acct="root" exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/0 res=success'
Sep 06 12:33:09 vFedora afpd[7352]: Netatalk AFP/TCP listening on fe80::c280:8124:1e1f:c3d9:548

Looks like I got a timing problem with the PID file (it does indeed exist on the filesystem). Everything else looks good.

If it helps, I have packages of wolfssl and netatalk hosted in a Copr repo here: https://copr.fedorainfracloud.org/coprs/kni/wolfssl/

rdmark commented 1 week ago

Thanks, yes your output looks fine. I'll keep trying with the bundled library.