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

olamy · 2024-08-31T02:52:49Z

Version

2.13.2

Bug description

When using using registar created for BCFIPS (which means any BC classes are available within the class path).

I got

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:553)
  at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.config.keys.loader.pem.PKCS8PEMResourceKeyPairParser.decryptKeyPairs(PKCS8PEMResourceKeyPairParser.java:107)

I tried to "trick" it :) using the name BC here https://github.com/jenkinsci/mina-sshd-api-plugin/pull/114/files#diff-5440105bdcdf53b86acce84166b9884f497eb6908da1d68b82ec974aa0fd83e1R45

But turns into:

  Caused: java.lang.NoClassDefFoundError: org/bouncycastle/crypto/prng/RandomGenerator
  at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.security.bouncycastle.BouncyCastleRandomFactory.create(BouncyCastleRandomFactory.java:43)
  at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.util.security.bouncycastle.BouncyCastleRandomFactory.create(BouncyCastleRandomFactory.java:28)
  at PluginClassLoader for mina-sshd-api-common//org.apache.sshd.common.random.SingletonRandomFactory.<init>(SingletonRandomFactory.java:38)
with BCFIPS (classic BC class not available in the classpth) this is this class https://javadoc.io/doc/org.bouncycastle/bc-fips/latest/org/bouncycastle/crypto/fips/FipsSecureRandom.html

But the idea is to make this random factory more configurable.

Actual behavior

see bug description

Expected behavior

Able to use BouncyCastle FIPS

Relevant log output

No response

Other information

No response

The text was updated successfully, but these errors were encountered:

olamy · 2024-08-31T02:53:04Z

PR #589

tomaswolf · 2024-08-31T08:48:38Z

I'm not a fan of yet another system property. There's too many already.

I see three things:

Why even use a BouncyCastleRandomFactory and BouncyCastleRandom at all? Why can't we just use Java's own SecureRandom?
The PKCS8PEMResourceKeyPairParser should be changed such that encrypted keys are parsed in a separate class, and we register that separate parser only if the required BC support is present.
If we want to make this RandomFactory overrideable, use the ServiceLoader. If a service can be found for RandomFactory, use that; otherwise fall back to the current code.

olamy · 2024-08-31T09:28:32Z

I'm not a fan of yet another system property. There's too many already.

The one for using BCFIPS instead of BC will be complicated to not have.

I see three things:

Why even use a BouncyCastleRandomFactory and BouncyCastleRandom at all? Why can't we just use Java's own SecureRandom?

Definitely agree with that. SecureRandom is even compatible with FIPS.
Maybe would be better to use only the current JceRandom but with SecureRandom.getInstanceStrong()

The PKCS8PEMResourceKeyPairParser should be changed such that encrypted keys are parsed in a separate class, and we register that separate parser only if the required BC support is present.

not sure to follow. do you mean some new method in SecurityProviderRegistrar?

If we want to make this RandomFactory overrideable, use the ServiceLoader. If a service can be found for RandomFactory, use that; otherwise fall back to the current code.

I don;t have real need for this, I just need something working when using BCFIPS, but I didn't want to break the current code too much. This sounds like a better idea to do it.
I have no real strong opinion, but I'm happy to add some ServiceLoader to support this.
Just let me kow.

tomaswolf · 2024-08-31T09:41:42Z

not sure to follow. do you mean some new method in SecurityProviderRegistrar?

No. The PKCS8PEMResourceKeyPairParser currently handles unencrypted and encrypted keys. For encrypted keys it relies on BC because of numerous bugs in PBES2 in earlier Java versions.

The PKCS8PEMResourceKeyPairParser is unconditionally registered in PEMResourceParserUtils.

My suggestion was to factor out the encrypted key handling into a separate parser, which would implement OptionalFeature, and which would be registered only if the required BC classes are present. (And/or BC was registered through a registrar. All this registrar stuff seems way overdesigned to me anyway. I don't quite understand why one would want to have yet another layer on top of Java's SecurityProvider. Why can't we just use whatever SecurityProviders are installed?)

Re: ServiceLoader: seems simpler than using ThreadUtils and manual class loading. Java has a mechanism to such things, so why not use it. (It's another area of the code that I don't quite get why we have it at all. What's the problem with ServiceLoader that prompted the creation of these home-grown thread utils?)

olamy · 2024-08-31T10:00:17Z

not sure to follow. do you mean some new method in SecurityProviderRegistrar?

No. The PKCS8PEMResourceKeyPairParser currently handles unencrypted and encrypted keys. For encrypted keys it relies on BC because of numerous bugs in PBES2 in earlier Java versions.

The PKCS8PEMResourceKeyPairParser is unconditionally registered in PEMResourceParserUtils.

My suggestion was to factor out the encrypted key handling into a separate parser, which would implement OptionalFeature, and which would be registered only if the required BC classes are present. (And/or BC was registered through a registrar. All this registrar stuff seems way overdesigned to me anyway. I don't quite understand why one would want to have yet another layer on top of Java's SecurityProvider. Why can't we just use whatever SecurityProviders are installed?)

I kinda agree, but maybe this should be done as part of another issue, including some bigger refactoring.
Here it's only a bug fix because it's blocking system that wanted to use Mina SSH on a FIPS constrained env (which means JGit is not usable for SSH git repo)

Re: ServiceLoader: seems simpler than using ThreadUtils and manual class loading. Java has a mechanism to such things, so why not use it. (It's another area of the code that I don't quite get why we have it at all. What's the problem with ServiceLoader that prompted the creation of these home-grown thread utils?)

I don't have the history here. But there could be plenty of classloader issues (OSGI?).
I will change the code to use classloader for this random.

tomaswolf · 2024-08-31T14:39:49Z

Re: property:

If I understand it right, the BC classes are there (with the same package names), but the provider name is not BC but BCFIPS? If so, all we have to do is:

De-couple the registrar name ("BC") from the security provider name ("BC" or "BCFIPS")
Use "BC" only if we want to get the registrar, and otherwise use the provider's name.

Then we don't need a property for this.

Re: ServiceLoader and OSGi:

ServiceLoader in an environment where there are partitioned classloaders (like OSGi, or Plexus classworlds) can be made to work. In OSGi, a very simple approach is to place services in fragments for the host bundle (org.apache.sshd.osgi in that case). Classes in fragments get put on the bundle's class path and are accessible via the normal BundleClassloader, so they will be found. We use that in JGit extensively. Otherwise, using an SPI bridge like SPI Fly is an option.

Using ThreadUtils and manual classloading doesn't prevent these problems.

I'd go with ServiceLoader for a possible override, and otherwise silently fall back to JceRandom if the BC random cannot be used. BouncyCastleRandom should implement OptionalFeature, and test whether the needed BC classes are present in its isSupported() method. BouncyCastleRandomFactory.isSupported() should then just forward to BouncyCastleRandom.isSupported(). And SecurityUtils.getRandomFactory() can then become

  // Optionally use serviceloader to load some override
  if (isBouncyCastleRegistered() && BouncyCastleRandomFactory.INSTANCE.isSupported()) {
    return BouncyCastleRandomFactory.INSTANCE;
  }
  return JceRandomFactory.INSTANCE;

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

tomaswolf · 2024-08-31T17:34:44Z

PR #591 perhaps explains better what I have in mind.

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.

tomaswolf · 2024-08-31T21:13:18Z

(Off-topic) Just noticed: using the bc-fips JAR in an OSGi environment might be difficult. I don't see any OSGi headers in the MANIFEST.MF. Plus the version number is unrelated to the normal BC. (BC also has LTS versions of the regular bundles that again use different version numbers, which might also pose some challenges in OSGi.)

The general idea in PR #591 is that the BouncyCastleSecurityRegistrar should simply work with either, normal BC or BCFIPS.

BTW; in https://github.com/jenkinsci/mina-sshd-api-plugin/pull/114/files#diff-9f68200faaabb5a0022f5eaa9de98ae4f9136bb2b6a766d698b8bc47203ad698R48 I notice two things:

A typo in the package name.
bc-fips does not have any native code, so its AES implementation is Java-only. SunJCE has a native AES implementation that is much faster. You probably also want to enable the org.apache.sshd.common.util.security.SunJCESecurityProviderRegistrar (name "SunJCEWrapper") to use the SunJCE AES (if that is FIPS compliant).

olamy · 2024-09-01T00:59:24Z

Re: property:

If I understand it right, the BC classes are there (with the same package names), but the provider name is not BC but BCFIPS? If so, all we have to do is:

I didn't check but I would not rely on BC and BCFIPS using the same classes (BCRandon is already different :))

De-couple the registrar name ("BC") from the security provider name ("BC" or "BCFIPS")

Use "BC" only if we want to get the registrar, and otherwise use the provider's name.

Then we don't need a property for this.

Re: ServiceLoader and OSGi:

ServiceLoader in an environment where there are partitioned classloaders (like OSGi, or Plexus classworlds) can be made to work. In OSGi, a very simple approach is to place services in fragments for the host bundle (org.apache.sshd.osgi in that case). Classes in fragments get put on the bundle's class path and are accessible via the normal BundleClassloader, so they will be found. We use that in JGit extensively. Otherwise, using an SPI bridge like SPI Fly is an option.

Using ThreadUtils and manual classloading doesn't prevent these problems.

I'd go with ServiceLoader for a possible override, and otherwise silently fall back to JceRandom if the BC random cannot be used. BouncyCastleRandom should implement OptionalFeature, and test whether the needed BC classes are present in its isSupported() method. BouncyCastleRandomFactory.isSupported() should then just forward to BouncyCastleRandom.isSupported(). And SecurityUtils.getRandomFactory() can then become

Sounds good. And no real need of ServiceLoader.

  // Optionally use serviceloader to load some override
  if (isBouncyCastleRegistered() && BouncyCastleRandomFactory.INSTANCE.isSupported()) {
    return BouncyCastleRandomFactory.INSTANCE;
  }
  return JceRandomFactory.INSTANCE;

olamy · 2024-09-01T01:12:10Z

(Off-topic) Just noticed: using the bc-fips JAR in an OSGi environment might be difficult. I don't see any OSGi headers in the MANIFEST.MF. Plus the version number is unrelated to the normal BC. (BC also has LTS versions of the regular bundles that again use different version numbers, which might also pose some challenges in OSGi.)

I'm not using OSGI so I can't test but agree this might be a problem.

The general idea in PR #591 is that the BouncyCastleSecurityRegistrar should simply work with either, normal BC or BCFIPS.

BTW; in https://github.com/jenkinsci/mina-sshd-api-plugin/pull/114/files#diff-9f68200faaabb5a0022f5eaa9de98ae4f9136bb2b6a766d698b8bc47203ad698R48 I notice two things:

A typo in the package name.

? not sure to understand.
This looks right to me according to this class https://github.com/jenkinsci/mina-sshd-api-plugin/pull/114/files#diff-5440105bdcdf53b86acce84166b9884f497eb6908da1d68b82ec974aa0fd83e1

bc-fips does not have any native code, so its AES implementation is Java-only. SunJCE has a native AES implementation that is much faster. You probably also want to enable the org.apache.sshd.common.util.security.SunJCESecurityProviderRegistrar (name "SunJCEWrapper") to use the SunJCE AES (if that is FIPS compliant).

I would rather stay with BC FIPS which is definitely registered as FIPS compliant. I don't think SunJCE AES is FIPS compliant.

I will test #591 (thanks for this)
The problem I have is to have unit test here for this as they need to fork another jvm with passing a modified -Djava.security.properties= or modifying the surefire start with -Djava.security.properties= but this means all the tests will use (or we can configure surefire to use 2 executions with some excludes/includes)

Override canExtractKeyPairs so that we don't even try the parser if BC is not registered. The decryption is implemented only for Bouncy Castle because many Java versions have various bugs and limitations regarding PBES2.

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.

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

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.

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

tomaswolf · 2024-09-05T18:32:09Z

PR #591 is merged.

olamy mentioned this issue Aug 31, 2024

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

Closed

tomaswolf self-assigned this Sep 5, 2024

tomaswolf added this to the 2.14.0 milestone Sep 5, 2024

tomaswolf closed this as completed Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

olamy commented Aug 31, 2024

olamy commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

olamy commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

olamy commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

olamy commented Sep 1, 2024

olamy commented Sep 1, 2024 •

edited

Loading

tomaswolf commented Sep 5, 2024

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

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

Comments

olamy commented Aug 31, 2024

Version

Bug description

Actual behavior

Expected behavior

Relevant log output

Other information

olamy commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

olamy commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

olamy commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

tomaswolf commented Aug 31, 2024

olamy commented Sep 1, 2024

olamy commented Sep 1, 2024 • edited Loading

tomaswolf commented Sep 5, 2024

olamy commented Sep 1, 2024 •

edited

Loading