gridcf / gct

Grid Community Toolkit
Apache License 2.0
46 stars 30 forks source link

Add missing header file #210

Closed fscheiner closed 1 year ago

fscheiner commented 1 year ago

Fixes #207


Looking at https://github.com/gridcf/gct/actions/runs/3940255855/jobs/6780376794#step:3:13057 this interestingly now also affects our builds on ~Rocky Linux~ CentOS Stream 9 ~(and most likely also CentOS Stream 9)~ - it didn't in the past runs. I don't know why this starts to happen now, maybe a change in the used OpenSSL on these distributions.


Corrected: I misread things at first.

ellert commented 1 year ago

Fedora and RHEL/CentOS/... adds an extra header file to their packaged version of openssl 3+ named openssl/fips.h. That header defines a FIPS_mode() macro:

# define FIPS_mode() EVP_default_properties_is_fips_enabled(NULL)

for backward compatibility with earlier versions of openssl. It is a Fedora/Redhat specific addition though. Debian does not provide such a header and macro, so its use is not portable.

Until a recent update of openssl in Fedora and RHEL 9, the patch added a line

#include <openssl/fips.h>

to openssl/crypto.h, so the macro was available without any additional #include statements. In the latest openssl update this #include statement was dropped and an explicit

#include openssl/fips.h

is needed to use the macro. The GCT source has added its own replacement for the macro in gsi_openssh/source/fips_mode_replacement.h (different from the one provided by Fedora/Redhat). By using its own replacement macro the code is more portable, but since it broke by the openssl update it seems the extra header was not included everywhere it is needed, and before used a mixture of the Fedora/Redhat and GCT versions of the macro in different places.

msalle commented 1 year ago

In addition to adding this header to gsi_openssh/source/kexgexc.c, since the FIPS_mode() macro is called in this file, it probably should be removed from gsi_openssh/source/kexgex.c, since it is not called there. The include seem to have been in the right file, but was moved to the wrong file in commit 607c9a1

The replacment macro used in fips_mode_replacement.h is different from the one defined by Fedora/Redhat. Should GCT use the same?

I think the Fedora/Redhat one is definitely the better one and looks like it should give more or less the same return values as the original FIPS_enabled. Don't remember where the current fix came from.

fscheiner commented 1 year ago

I think the Fedora/Redhat one is definitely the better one and looks like it should give more or less the same return values as the original FIPS_enabled. Don't remember where the current fix came from.

IIRC you did create that for me to support OpenSSL 3.0 - but let me check the relevant commits to be sure. I think this was also added for Linux distributions that don't have FIPS related stuff, because the in-tree GSI-OpenSSH should compile "everywhere else than Fedora", too. I hence mainly tested the compilation of the in-tree GSI-OpenSSH on non-Fedora distributions, assuming that it will compile on Fedora anyhow, so might not have been aware of Fedora shipping that extra ~file~ include with their version of OpenSSL 3.x. Or we implemented our own solution just because of the implicit target of non-Fedora distributions, I don't remember but will have a look on the relevant commits and my email archive at that time.

For now I'll tidy up this PR (incl. removal of the include from the wrong file) and stay with our GCT solution. And with the planned next in-tree version of GSI-OpenSSH look at the FIPS stuff again, but if it's not portable, I'd say we stay with our solution.

msalle commented 1 year ago

I think the Fedora/Redhat one is definitely the better one and looks like it should give more or less the same return values as the original FIPS_enabled. Don't remember where the current fix came from.

IIRC you did create that for me to support OpenSSL 3.0 - but let me check the relevant commits to be sure. I think this was also added for Linux distributions that don't have FIPS related stuff, because the in-tree GSI-OpenSSH should compile "everywhere else than Fedora", too. I hence mainly tested the compilation of the in-tree GSI-OpenSSH on non-Fedora distributions, assuming that it will compile on Fedora anyhow, so might not have been aware of Fedora shipping that extra ~file~ include with their version of OpenSSL 3.x. Or we implemented our own solution just because of the implicit target of non-Fedora distributions, I don't remember but will have a look on the relevant commits and my email archive at that time.

For now I'll tidy up this PR (incl. removal of the include from the wrong file) and stay with our GCT solution. And with the planned next in-tree version of GSI-OpenSSH look at the FIPS stuff again, but if it's not portable, I'd say we stay with our solution.

The Fedora/RedHat solution should also work for us, we just need to use a different #define:

# define FIPS_mode() EVP_default_properties_is_fips_enabled(NULL)

instead of

# define FIPS_mode() 0

So perhaps use Fedora/RedHat's header when available and otherwise our own but with the same substitution?

fscheiner commented 1 year ago

The Fedora/RedHat solution should also work for us, we just need to use a different #define:

# define FIPS_mode() EVP_default_properties_is_fips_enabled(NULL)

instead of

# define FIPS_mode() 0

Indeed, that's what I concluded now, too, when starting from the top of the thread. And as we would only make that define if OpenSSL 3.x is available it wouldn't make a difference if older OpenSSL versions are used, because also no additional header is included by the OpenSSL package of the OS.

So perhaps use Fedora/RedHat's header when available and otherwise our own but with the same substitution?

If we use the same definition, it should not make a difference, right? And for OSes that ship with the definition in their OpenSSL 3.x packages (see https://github.com/gridcf/gct/pull/210#issuecomment-1399243533) we can't "uninclude" that header anyhow.

So in addition to the original change of this PR, (1) I'll also change the definition of FIPS_mode() accordingly and (2) remove the unneeded include in gsi_openssh/source/kexgex.c