incorrect classloading of SecurityProviders

[jtnord]

Version

2.12.1

Bug description

The mina plugin attempts work out if EdDSA is supported using a different classloader than it actually uses for using EdDSASecurityProviderUtils leading to ClassNotFoundExceptions

Actual behavior

NoClassDefError

Caused by: java.lang.NoClassDefFoundError: net/i2p/crypto/eddsa/spec/EdDSAParameterSpec
	at org.apache.sshd.common.util.security.SecurityUtils.getEDDSAPublicKeyEntryDecoder(SecurityUtils.java:580)
	at org.apache.sshd.common.config.keys.KeyUtils.<clinit>(KeyUtils.java:187)
	at org.apache.sshd.common.config.keys.loader.openssh.OpenSSHRSAPrivateKeyDecoder.<clinit>

Expected behavior

No classloading issues.
the code uses the same classloader to load EdDSA as it uses to see if EdDSA is supported.

Relevant log output

Other information

if you have a non flat classloader (e.g. Jenkins, or any modular app server) then the code has a non reflective way of determining if something is supported or not.

e.g.

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

Lines 575 to 581 in 9ceba3e

	public static PublicKeyEntryDecoder<? extends PublicKey, ? extends PrivateKey> getEDDSAPublicKeyEntryDecoder() {
	if (!isEDDSACurveSupported()) {
	throw new UnsupportedOperationException(EDDSA + " provider N/A");
	}
	return EdDSASecurityProviderUtils.getEDDSAPublicKeyEntryDecoder();
	}

we see a call to isEDDSACurveSupported before we attempt to use a staic method from the class EdDSASecurityProviderUtils

However

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

Lines 566 to 571 in 9ceba3e

	public static boolean isEDDSACurveSupported() {
	register();
	SecurityProviderRegistrar r = getRegisteredProvider(EDDSA);
	return (r != null) && r.isEnabled() && r.isSupported();
	}

looks for the presence of a security provider called "EdDSA" which in no way reflects if the classes are available or not.

If the "EdDSA" provider was statically registered in the bootclasspath this would work, however it is generally registered by EdDSASecurityProviderRegistrar

As can be seen here

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

Lines 88 to 101 in 9ceba3e

	public boolean isSupported() {
	Boolean supported;
	synchronized (supportHolder) {
	supported = supportHolder.get();
	if (supported != null) {
	return supported.booleanValue();
	}
	Class<?> clazz = ThreadUtils.resolveDefaultClass(getClass(), "net.i2p.crypto.eddsa.EdDSAKey");
	supported = clazz != null;
	supportHolder.set(supported);
	}
	return supported.booleanValue();

this uses ThreadUtils.resolveDefaultClass to see if the class is available which adds a the Threads contextClassLoader into the mix to see if the class is there.
If the class is available in the contextClassloader but not the current classload, then it would load as the Provider is created (eventually) here which uses reflection and in the case of this issue the Threads contextClassLoader.

The created provider is then registered in the JVM.

This provider is then used as the gate for "are the EdDSA classes available" for the regular (current) classloader, which in this case they are not which causes a 💥

whilst it is generally safe to create and Provider dynamically like this, the code that uses the classes from the provider as opposed to the provider itself should take a different approach.
Either they also need to be loaded by reflection (yikes), or the check to see if they are available should not use the provider, but should check the actual class is present in the current classloader (ie ignore the context classloader).
Or providers should not be dynamically registered from a context classloader.

[jtnord]

(of note BouncyCastle also supports ED25519 curves as does JDK > 15) so this whole code loading / registering for EdDSA could be replaced on a modern JVM).