GH-590: Be more careful with Bouncy Castle in FIPS environment #591

tomaswolf · 2024-08-31T17:31:54Z

Decouple the registrar name from the security provider name. Also check whether the BC RandomGenerator exists at all; in BC-FIPS, it doesn't.

olamy · 2024-09-01T05:13:36Z

Thanks for the PR but hardcoded "BC" gives me this:

java.lang.IllegalArgumentException: BouncyCastle not registered
	at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.ValidateUtils.createFormattedException(ValidateUtils.java:213)
	at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.ValidateUtils.throwIllegalArgumentException(ValidateUtils.java:179)
	at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.ValidateUtils.checkTrue(ValidateUtils.java:156)
	at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.security.SecurityUtils.getBouncycastleEncryptedPrivateKeyInfoDecryptor(SecurityUtils.java:546)
	at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.config.keys.loader.pem.PKCS8PEMResourceKeyPairParser.decryptKeyPairs(PKCS8PEMResourceKeyPairParser.java:107)

olamy · 2024-09-01T05:16:50Z

as long as there is this check still using the hardcoded BC this will not work

mina-sshd/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityUtils.java

Line 396 in 40a08a4

public static boolean isBouncyCastleRegistered() {

olamy · 2024-09-01T07:49:40Z

There is a test for this with this PR jenkinsci/mina-sshd-api-plugin#114

tomaswolf · 2024-09-01T21:03:37Z

The exception you show above shows that you are trying to load an encrypted PEM key. (Starts with "-----BEGIN ENCRYPTED PRIVATE KEY-----").

AFAIK that is not possible in a FIPS environments as FIPS has no PBE/PBES2.

So the exception is totally correct.

olamy · 2024-09-01T21:09:37Z

Ah. Do you have any link for this?

olamy · 2024-09-01T21:16:19Z

TBI have been always thinking this openssl genrsa -des3 -out rsa2048 2048 would generate a FIPS compatible key.

tomaswolf · 2024-09-01T21:28:20Z

Hm. Actually, I had a link, but can't find it anymore. Anyway, it said bc-fips had no pbe... (not that FIPS didn't have it). Might be technically true since I don't see any such code in bundle bc-fips, but there's also bundle bcpkix-fips which seems to have the classes I was missing. I'll keep trying.

olamy · 2024-09-02T06:18:59Z

that's failing my tests here jenkinsci/mina-sshd-api-plugin#114

tomaswolf · 2024-09-02T15:30:15Z

@olamy: All right. The original idea was sound after all. With this PR at commit 9518312 and the following patch applied to your test jenkinsci/mina-sshd-api-plugin#114 that test succeeds.

Patch:

 public class FIPSBouncyCastleSecurityProviderRegistar extends AbstractSecurityProviderRegistrar {
 
     public FIPSBouncyCastleSecurityProviderRegistar() {
-        super("BCFIPS");
+        super("BC");
+    }
+
+    @Override
+    public String getProviderName() {
+        return "BCFIPS";
     }
 
     @Override

Test run succeeds:

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.665 s -- in io.jenkins.plugins.mina_sshd_api.core.bouncycaslte_registar.TestBCFIPSRegistar

Note that I did not include the customizeable RandomFactory. Not sure it's needed; it'll use JceRandom with BCFIPS anyway. (And JceRandom is changed to use SecureRandom.getInstanceStrong().)

Two observations:

The typo in the package name I mentioned before is io.jenkins.plugins.mina_sshd_api.core.bouncycaslte_registar. That should probably be "bouncycastle_registrar".
I think typical applications will include either the BCFIPS bundles or the normal BC bundles, but not both. Mix and match won't work anyway. In such typical applications, there shouldn't be a need for an extra registrar at all, since the built-in BouncyCastleSecurityRegistrar can handle either, depending on what is accessible or what security providers are installed already. I presume no application would have both the BCFIPS and the BC provider installed.

Your use case seems to be in Jenkins and apparently both libraries (BC and BCFIPS) may be present. That use case may still need its special-purpose registrar. Also you mentioned you'd not want to use the SunJCE AES, so a system property to disable the SunJCEWrapper registrar would be needed anyway if you don't override the registrars. (If you do -- as is currently the case -- that SunJCEWrapper won't be registered at all.)

tomaswolf · 2024-09-02T17:08:38Z

And in FIPSRegistarInitPlugin two lines can be removed:

         if(FIPS140.useCompliantAlgorithms() && Security.getProvider("BCFIPS") != null) {
             LOG.info("register FIPSBouncyCastleSecurityProviderRegistar");
             System.setProperty("org.apache.sshd.security.registrars", "io.jenkins.plugins.mina_sshd_api.core.bouncycaslte_registar.FIPSBouncyCastleSecurityProviderRegistar");
-            System.setProperty("org.apache.sshd.security.defaultProvider", "BCFIPS");
-            System.setProperty("org.apache.sshd.common.util.security.bouncycastle.provider.name", "BCFIPS");
         } else {
             LOG.config("not needed to register FIPSBouncyCastleSecurityProviderRegistar");
         }

olamy · 2024-09-02T21:35:18Z

@tomaswolf Thanks a lot!!

@olamy: All right. The original idea was sound after all. With this PR at commit 9518312 and the following patch applied to your test jenkinsci/mina-sshd-api-plugin#114 that test succeeds.

Patch:
 public class FIPSBouncyCastleSecurityProviderRegistar extends AbstractSecurityProviderRegistrar {
 
     public FIPSBouncyCastleSecurityProviderRegistar() {
-        super("BCFIPS");
+        super("BC");
+    }
+
+    @Override
+    public String getProviderName() {
+        return "BCFIPS";
     }
 
     @Override
Test run succeeds:
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.665 s -- in io.jenkins.plugins.mina_sshd_api.core.bouncycaslte_registar.TestBCFIPSRegistar
Note that I did not include the customizeable RandomFactory. Not sure it's needed; it'll use JceRandom with BCFIPS anyway. (And JceRandom is changed to use SecureRandom.getInstanceStrong().)

Right so it's not needed anymore.

Two observations:

The typo in the package name I mentioned before is io.jenkins.plugins.mina_sshd_api.core.bouncycaslte_registar. That should probably be "bouncycastle_registrar".

Haha good catch. I was checking if String constants in System.setProperty were right but the problem was something else. Uhm I need some new glasses...

I think typical applications will include either the BCFIPS bundles or the normal BC bundles, but not both. Mix and match won't work anyway. In such typical applications, there shouldn't be a need for an extra registrar at all, since the built-in BouncyCastleSecurityRegistrar can handle either, depending on what is accessible or what security providers are installed already. I presume no application would have both the BCFIPS and the BC provider installed.

Your use case seems to be in Jenkins and apparently both libraries (BC and BCFIPS) may be present. That use case may still need its special-purpose registrar. Also you mentioned you'd not want to use the SunJCE AES, so a system property to disable the SunJCEWrapper registrar would be needed anyway if you don't override the registrars. (If you do -- as is currently the case -- that SunJCEWrapper won't be registered at all.)

No we don't. If Jenkins is running in FIPS mode, the classic BC is not registered (not easy to figure when looking at the poms only but it's here https://github.com/jenkinsci/bouncycastle-api-plugin/blob/e27176eb46cbe94c1d1de6a9f318e3d538c34a4a/src/main/java/jenkins/bouncycastle/api/BouncyCastlePlugin.java#L27)

olamy · 2024-09-02T21:37:22Z

And in FIPSRegistarInitPlugin two lines can be removed:

         if(FIPS140.useCompliantAlgorithms() && Security.getProvider("BCFIPS") != null) {
             LOG.info("register FIPSBouncyCastleSecurityProviderRegistar");
             System.setProperty("org.apache.sshd.security.registrars", "io.jenkins.plugins.mina_sshd_api.core.bouncycaslte_registar.FIPSBouncyCastleSecurityProviderRegistar");
-            System.setProperty("org.apache.sshd.security.defaultProvider", "BCFIPS");

this one ^ is still needed because we want to use BCFIPS.

       System.setProperty("org.apache.sshd.common.util.security.bouncycastle.provider.name", "BCFIPS");
   } else {
       LOG.config("not needed to register FIPSBouncyCastleSecurityProviderRegistar");
   }

Running few tests and this is looking really great. Thanks for your help.

tomaswolf · 2024-09-02T22:00:39Z

this one ^ is still needed because we want to use BCFIPS.

Right.

However, for FIPS mode I think we may have another problem: we use our own ChaCha20-Poly1305 implementation, and we use the bcyrypt KDF for encrypted OpenSSH "new format" keys.

Probably we should have a way to disable those if you want to run in FIPS mode?

olamy · 2024-09-03T01:44:42Z

this one ^ is still needed because we want to use BCFIPS.

Right.

However, for FIPS mode I think we may have another problem: we use our own ChaCha20-Poly1305 implementation, and we use the bcyrypt KDF for encrypted OpenSSH "new format" keys.

right as far as I understand ChaCha20-Poly1305 is not FIPS compliant.

Regarding OpenSSH "new format" keys, I'm not quite sure.
with new do you mean this from 2018? :) https://www.openssh.com/txt/release-7.8
If this, it looks this format should be rejected on a FIPS env

Probably we should have a way to disable those if you want to run in FIPS mode?

tomaswolf · 2024-09-03T06:51:06Z

I'm not quite sure. with new do you mean this from 2018? :) https://www.openssh.com/txt/release-7.8

Yes: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.key

I also think neither ChaCha20-Poly1305 nor bcrypt are covered by FIPS.

For ChaCha20-Poly1305 we have two options: either we say it's the user's responsibility to configure the SshClient appropriately by removing the cipher via configuration (SshClient.setCipherFactories()), or we add a flag (with system property and setter) in SecurityUtils and then hard-disable this Cipher if the flag is set.

The bcrypt thing is less critical. I don't know which KDF's are FIPS-approved, but I'd be surprised if bcrypt was. If not, anyone using this OpenSSH format for stored keys would deviate from FIPS anyway, so we could say it's entirely the user's responsibility. (As is using only PEM keys with FIPS-approved encryptions and KDFs.) Or we could use the aforementioned flag to also disable bcrypt (with the effect that such keys cannot be read or written in FIPS mode). Or we might only disable writing such files, while still allowing to read them.

jtnord · 2024-09-03T08:45:32Z

The bcrypt thing is less critical. I don't know which KDF's are FIPS-approved, but I'd be surprised if bcrypt was.

for BouncyCastle FIPS see section 6 of https://downloads.bouncycastle.org/fips-java/docs/BC-FJA-UserGuide-1.0.2.pdf

Key Agreements are in Section 5 of the same document.

olamy · 2024-09-03T08:55:26Z

I'm not quite sure. with new do you mean this from 2018? :) https://www.openssh.com/txt/release-7.8

Yes: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.key

I also think neither ChaCha20-Poly1305 nor bcrypt are covered by FIPS.

For ChaCha20-Poly1305 we have two options: either we say it's the user's responsibility to configure the SshClient appropriately by removing the cipher via configuration (SshClient.setCipherFactories()), or we add a flag (with system property and setter) in SecurityUtils and then hard-disable this Cipher if the flag is set.

LGTM

The bcrypt thing is less critical. I don't know which KDF's are FIPS-approved, but I'd be surprised if bcrypt was. If not, anyone using this OpenSSH format for stored keys would deviate from FIPS anyway, so we could say it's entirely the user's responsibility. (As is using only PEM keys with FIPS-approved encryptions and KDFs.) Or we could use the aforementioned flag to also disable bcrypt (with the effect that such keys cannot be read or written in FIPS mode). Or we might only disable writing such files, while still allowing to read them.

what about something such a general flag fips enabled? Will turn reponsibility to the library but could be a great user feature?

tomaswolf · 2024-09-03T20:29:11Z

So here's another commit on top of this PR for this single flag. See the commit message.

With this, I think you don't need the FIPSBouncyCastleSecurityProviderRegistar anymore, and the FIPSRegistarInitPlugin can just do

System.setProperty("org.apache.sshd.security.fipsEnabled", "true");
System.setProperty("org.apache.sshd.security.defaultProvider", "BCFIPS");

If the flag is set, a number of non-FIPS-approved crypto-algorithms get disabled (ed25519, curve25519, curve448, sntrup761, bcrypt, chacha20-poly1305).

Disabling sntrup761 and curve25519 means that this FIPS mode is left without any post-quantum key exchange method.
FIPS 203 has come out recently; it specifies the post-quantum ML-KEM key encapsulation method. There is a draft RFC for incorporating this into SSH as key exchange algorithm. Once OpenSSH has support for this and Bouncy Castle also provides these ML-KEM instances, we can add them easily. At that point there would be mlkem1024nistp384-sha384 available as (I believe) FIPS-compliant post-quantum algorithm. When we do have that algorithm, I think we should keep it enabled even in FIPS mode.

Can you test that this works for your use case in Jenkins?

olamy · 2024-09-03T20:57:37Z

@tomaswolf Thanks a lot. I will test this.

Signed-off-by: Olivier Lamy <olamy@apache.org>

olamy · 2024-09-04T02:00:54Z

@tomaswolf I did some successful testing.
But I will still need the registar because the environment where it's running is very restrictive and even have a SecurityManager installed to block registration of SecurityProvider.
So without using the registar, the Apache Mina is trying to register the BCFIPS and BC :)

Caused: java.lang.IllegalArgumentException: Failed to register BC as a JCE provider
  at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.security.SecurityUtils.registerSecurityProvider(SecurityUtils.java:534)
  at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.security.SecurityUtils.register(SecurityUtils.java:489)
  at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.security.SecurityUtils.isBouncyCastleRegistered(SecurityUtils.java:438)

tomaswolf · 2024-09-04T06:51:46Z

That's strange. I did test this with your change, and it worked like a charm.

I see the bug: at

mina-sshd/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityProviderRegistrar.java

Line 310 in 890f93a

    
           (registrar == null) ? null : registrar.getName(), "No name for registrar=%s", registrar);

we must use registrar.getProviderName()!

olamy · 2024-09-04T07:49:55Z

Yes, correct.
I definitely mix up some local build of snapshots from various PR, branches...
Sorry for the wrong alert. I haven't been to reproduce this (and I really tried :))
All looks good to me to me to be merged.
And released?

tomaswolf · 2024-09-04T07:51:50Z

Edited my comments. There is indeed a bug; I missed one place where getName() must be replaced by getProviderName().

olamy · 2024-09-04T07:53:13Z

Edited my comments. There is indeed a bug; I missed one place where getName() must be replaced by getProviderName().

ah so I need to test again because I'm confused :)

tomaswolf · 2024-09-04T07:53:30Z

I'll push an update this evening.

olamy · 2024-09-04T08:09:01Z

Thanks.
By the way, I confirm I can reproduce. Sorry I made other changes in my test and forgot to correctly setup -Djava.security.manager= which is obviously mandatory to reproduce that.

Decouple the registrar name from the security provider name. In the BouncyCastleSecurityRegistrar, check also for BCFIPS if regular BC cannot be found. Also check whether the BC RandomGenerator exists at all; in BCFIPS, it doesn't.

Fall back to new SecureRandom() if we get a NoSuchAlgorithmException, which should never occur unless the JVM is wrongly configured. Every JVM must support at least one strong PRNG.

tomaswolf · 2024-09-04T16:05:12Z

I'll push an update this evening.

Done. The bug fix is https://github.com/apache/mina-sshd/compare/31234defad60f723ba5f3de4d2a7527784e46e49..8092f28b9f379e3652907a8cad3916e1163bc646#diff-b775cafcbee766e042f74d6790cbaa143b5f175756db29140865a41222e7b94aR310 .

Hope I didn't miss anything else.

olamy · 2024-09-05T02:19:45Z

@tomaswolf All is working fine.
Thanks a lot for your help!

tomaswolf · 2024-09-05T06:41:33Z

Thanks for testing. I'll finish this this evening (documentation needs some updates still), then merge.

And released?

I have a few other things to fix or clean-up first. But after that I'll ask our release manager if we could do a release.

Add a flag in SecurityUtils to enable FIPS mode. In FIPS mode, algorithms known to be not FIPS-compliant are had disabled and not available. The BouncyCastleSecurityRegistrar only considers bc-fips, and the SunJCESecurityRegistrar and the EdDSASecurityRegistrar are disabled. The ChaCha20-Poly1305 cipher is disabled, ed25519 signatures are disabled, the bcrypt KDF used in OpenSSH-format encrypted private keys[1] is disabled, and the curve25519 and curve448 key exchange methods are disabled. Also disabled is the post-quantum sntrup761x25519-sha512 key exchange method. These disabled algorithms are not approved in FIPS 140. The flag can be set via a system property or by calling SecurityUtils.setFipsMode(). The system property is "org.apache.sshd.security.fipsEnabled" and takes the boolean value "true". Any other value does not enable FIPS mode. [1] https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.key

Signed-off-by: Olivier Lamy <olamy@apache.org>

tomaswolf force-pushed the gh-590 branch from 5ccfe4f to d220b95 Compare August 31, 2024 17:34

tomaswolf mentioned this pull request Aug 31, 2024

Cannot use Mina SSH client in a FIPS constrained environment #590

Closed

tomaswolf force-pushed the gh-590 branch from d220b95 to f6fa4fb Compare August 31, 2024 20:47

tomaswolf force-pushed the gh-590 branch from f6fa4fb to ad546f6 Compare September 1, 2024 20:58

tomaswolf force-pushed the gh-590 branch from ad546f6 to 9518312 Compare September 2, 2024 15:11

olamy mentioned this pull request Sep 2, 2024

Add option to be able to use BCFIPS when BC is blocked in a FIPS environment #589

Closed

tomaswolf force-pushed the gh-590 branch 2 times, most recently from 56571c6 to 31234de Compare September 3, 2024 20:10

olamy added a commit to olamy/mina-sshd-api-plugin that referenced this pull request Sep 3, 2024

simplify with last changes from Mina PR apache/mina-sshd#591

2834796

Signed-off-by: Olivier Lamy <olamy@apache.org>

tomaswolf added 2 commits September 4, 2024 18:01

JceRandom: use SecureRandom.getInstanceStrong()

3cc9d48

Fall back to new SecureRandom() if we get a NoSuchAlgorithmException, which should never occur unless the JVM is wrongly configured. Every JVM must support at least one strong PRNG.

tomaswolf force-pushed the gh-590 branch from 31234de to 8092f28 Compare September 4, 2024 16:03

olamy approved these changes Sep 5, 2024

View reviewed changes

tomaswolf force-pushed the gh-590 branch from 8092f28 to 980d94b Compare September 5, 2024 17:48

tomaswolf merged commit 980d94b into apache:master Sep 5, 2024
7 checks passed

olamy added a commit to olamy/mina-sshd-api-plugin that referenced this pull request Oct 3, 2024

simplify with last changes from Mina PR apache/mina-sshd#591

750cbc6

Signed-off-by: Olivier Lamy <olamy@apache.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-590: Be more careful with Bouncy Castle in FIPS environment #591

GH-590: Be more careful with Bouncy Castle in FIPS environment #591

tomaswolf commented Aug 31, 2024 •

edited

Loading

olamy commented Sep 1, 2024

olamy commented Sep 1, 2024 •

edited

Loading

olamy commented Sep 1, 2024

tomaswolf commented Sep 1, 2024

olamy commented Sep 1, 2024

olamy commented Sep 1, 2024 •

edited

Loading

tomaswolf commented Sep 1, 2024

olamy commented Sep 2, 2024

tomaswolf commented Sep 2, 2024 •

edited

Loading

tomaswolf commented Sep 2, 2024

olamy commented Sep 2, 2024

olamy commented Sep 2, 2024

tomaswolf commented Sep 2, 2024

olamy commented Sep 3, 2024 •

edited

Loading

tomaswolf commented Sep 3, 2024

jtnord commented Sep 3, 2024 •

edited

Loading

olamy commented Sep 3, 2024

tomaswolf commented Sep 3, 2024

olamy commented Sep 3, 2024

olamy commented Sep 4, 2024

tomaswolf commented Sep 4, 2024 •

edited

Loading

olamy commented Sep 4, 2024 •

edited

Loading

tomaswolf commented Sep 4, 2024

olamy commented Sep 4, 2024

tomaswolf commented Sep 4, 2024

olamy commented Sep 4, 2024

tomaswolf commented Sep 4, 2024

olamy commented Sep 5, 2024

tomaswolf commented Sep 5, 2024

GH-590: Be more careful with Bouncy Castle in FIPS environment #591

GH-590: Be more careful with Bouncy Castle in FIPS environment #591

Conversation

tomaswolf commented Aug 31, 2024 • edited Loading

olamy commented Sep 1, 2024

olamy commented Sep 1, 2024 • edited Loading

olamy commented Sep 1, 2024

tomaswolf commented Sep 1, 2024

olamy commented Sep 1, 2024

olamy commented Sep 1, 2024 • edited Loading

tomaswolf commented Sep 1, 2024

olamy commented Sep 2, 2024

tomaswolf commented Sep 2, 2024 • edited Loading

tomaswolf commented Sep 2, 2024

olamy commented Sep 2, 2024

olamy commented Sep 2, 2024

tomaswolf commented Sep 2, 2024

olamy commented Sep 3, 2024 • edited Loading

tomaswolf commented Sep 3, 2024

jtnord commented Sep 3, 2024 • edited Loading

olamy commented Sep 3, 2024

tomaswolf commented Sep 3, 2024

olamy commented Sep 3, 2024

olamy commented Sep 4, 2024

tomaswolf commented Sep 4, 2024 • edited Loading

olamy commented Sep 4, 2024 • edited Loading

tomaswolf commented Sep 4, 2024

olamy commented Sep 4, 2024

tomaswolf commented Sep 4, 2024

olamy commented Sep 4, 2024

tomaswolf commented Sep 4, 2024

olamy commented Sep 5, 2024

tomaswolf commented Sep 5, 2024

tomaswolf commented Aug 31, 2024 •

edited

Loading

olamy commented Sep 1, 2024 •

edited

Loading

olamy commented Sep 1, 2024 •

edited

Loading

tomaswolf commented Sep 2, 2024 •

edited

Loading

olamy commented Sep 3, 2024 •

edited

Loading

jtnord commented Sep 3, 2024 •

edited

Loading

tomaswolf commented Sep 4, 2024 •

edited

Loading

olamy commented Sep 4, 2024 •

edited

Loading