Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ setupClassPath() {
CLASSPATH="${CLASSPATH}:${KARAF_HOME}/system/org/apache/sshd/sshd-sftp/@@sshd.version@@/sshd-sftp-@@sshd.version@@.jar"
CLASSPATH="${CLASSPATH}:${KARAF_HOME}/system/org/fusesource/jansi/jansi/@@jansi.version@@/jansi-@@jansi.version@@.jar"
CLASSPATH="${CLASSPATH}:${KARAF_HOME}/system/org/jline/jline/@@jline.version@@/jline-@@jline.version@@.jar"
# bouncycastle provides the EdDSA support sshd needs to read ed25519 keys
CLASSPATH="${CLASSPATH}:${KARAF_HOME}/system/org/bouncycastle/bcprov-jdk18on/@@bouncycastle.version@@/bcprov-jdk18on-@@bouncycastle.version@@.jar"
CLASSPATH="${CLASSPATH}:${KARAF_HOME}/system/org/bouncycastle/bcpkix-jdk18on/@@bouncycastle.version@@/bcpkix-jdk18on-@@bouncycastle.version@@.jar"
CLASSPATH="${CLASSPATH}:${KARAF_HOME}/system/org/bouncycastle/bcutil-jdk18on/@@bouncycastle.version@@/bcutil-jdk18on-@@bouncycastle.version@@.jar"
}

init() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ set CLASSPATH=%CLASSPATH%;%KARAF_HOME%\system\org\apache\sshd\sshd-scp\@@sshd.ve
set CLASSPATH=%CLASSPATH%;%KARAF_HOME%\system\org\apache\sshd\sshd-sftp\@@sshd.version@@\sshd-sftp-@@sshd.version@@.jar
set CLASSPATH=%CLASSPATH%;%KARAF_HOME%\system\org\jline\jline\@@jline.version@@\jline-@@jline.version@@.jar
set CLASSPATH=%CLASSPATH%;%KARAF_HOME%\system\org\fusesource\jansi\jansi\@@jansi.version@@\jansi-@@jansi.version@@.jar
rem bouncycastle provides the EdDSA support sshd needs to read ed25519 keys
set CLASSPATH=%CLASSPATH%;%KARAF_HOME%\system\org\bouncycastle\bcprov-jdk18on\@@bouncycastle.version@@\bcprov-jdk18on-@@bouncycastle.version@@.jar
set CLASSPATH=%CLASSPATH%;%KARAF_HOME%\system\org\bouncycastle\bcpkix-jdk18on\@@bouncycastle.version@@\bcpkix-jdk18on-@@bouncycastle.version@@.jar
set CLASSPATH=%CLASSPATH%;%KARAF_HOME%\system\org\bouncycastle\bcutil-jdk18on\@@bouncycastle.version@@\bcutil-jdk18on-@@bouncycastle.version@@.jar

:EXECUTE
set arg1=%~1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ completionMode = GLOBAL

#
# Override allowed SSH host key signature algorithms.
# Default: ssh-rsa,rsa-sha2-256,rsa-sha2-512,sk-ecdsa-sha2-nistp256@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521
# Default: ssh-rsa,rsa-sha2-256,rsa-sha2-512,sk-ecdsa-sha2-nistp256@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,ssh-ed25519,sk-ssh-ed25519@openssh.com
#
# sigAlgorithms = ssh-rsa,rsa-sha2-256,rsa-sha2-512,sk-ecdsa-sha2-nistp256@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521
# sigAlgorithms = ssh-rsa,rsa-sha2-256,rsa-sha2-512,sk-ecdsa-sha2-nistp256@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,ssh-ed25519,sk-ssh-ed25519@openssh.com

#
# Override moduli-url.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import java.security.spec.InvalidParameterSpecException;
import java.security.spec.KeySpec;
import java.security.spec.RSAPublicKeySpec;
import java.security.spec.X509EncodedKeySpec;
import java.util.Arrays;
import java.util.Base64;
import java.util.Collections;
import java.util.HashMap;
Expand All @@ -61,9 +63,15 @@

public class PublickeyLoginModule extends AbstractKarafLoginModule {

private final Logger LOG = LoggerFactory.getLogger(PublickeyLoginModule.class);
private static final Logger LOG = LoggerFactory.getLogger(PublickeyLoginModule.class);

private static final String USERS_FILE = "users";
private static final String ED25519_IDENTIFIER = "ssh-ed25519";
private static final int ED25519_KEY_LENGTH = 32;
// DER prefix of a X.509 SubjectPublicKeyInfo holding a 32 bytes long ed25519 key (RFC 8410)
private static final byte[] ED25519_X509_PREFIX = {
0x30, 0x2a, 0x30, 0x05, 0x06, 0x03, 0x2b, 0x65, 0x70, 0x03, 0x21, 0x00
};
private static final Map<String, String> nistSecMap;

static {
Expand Down Expand Up @@ -239,6 +247,27 @@ public static boolean equals(PublicKey key, String storedKey) throws FailedLogin
PublicKey generatedPublicKey = keyFactory.generatePublic(keySpec);

return key.equals(generatedPublicKey);
} else if (ED25519_IDENTIFIER.equals(identifier)) {
// OpenSSH stores an ed25519 key as the raw 32 bytes of the compressed point.
// The key implementation depends on the registered provider (for instance
// BouncyCastle), so compare the X.509 encodings instead of the key objects.
int size = dis.readInt();
if (size != ED25519_KEY_LENGTH) {
return false;
}
byte[] bytes = new byte[size];
dis.readFully(bytes);

KeyFactory keyFactory = KeyFactory.getInstance("Ed25519");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KeyFactory.getInstance("Ed25519") relies on a JCA provider that supports Ed25519. Native JDK support only arrived in JDK 15; Karaf supports JDK 11+. On JDK 11-14, this call succeeds only if BouncyCastle is already registered in the JVM security provider list, which MINA SSHD does on startup via BouncyCastleSecurityProviderRegistrar. That sequencing dependency is invisible here. The caught GeneralSecurityException will surface it as a FailedLoginException, so it fails safely — but the message will be cryptic. A comment noting the BC dependency (and the JDK 15 floor for BC-free operation) would help future maintainers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, interesting background. See my comment on lift to JDK 21 below.

KeySpec publicKeySpec = new X509EncodedKeySpec(x509Ed25519(bytes));
PublicKey generatedPublicKey = keyFactory.generatePublic(publicKeySpec);

byte[] encoded = key.getEncoded();
if (encoded == null) {
LOG.debug("Unable to compare ed25519 key, the provider does not support getEncoded()");
return false;
}
return Arrays.equals(encoded, generatedPublicKey.getEncoded());
} else {
throw new FailedLoginException("Unsupported key type " + key.getClass().toString());
}
Expand All @@ -247,6 +276,22 @@ public static boolean equals(PublicKey key, String storedKey) throws FailedLogin
}
}

/**
* Wraps the raw bytes of an ed25519 public key in a X.509 SubjectPublicKeyInfo structure,
* so that it can be read by a {@link KeyFactory}. The prefix encodes the total length,
* so the key has to be exactly {@link #ED25519_KEY_LENGTH} bytes long.
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x509Ed25519 has an implicit contract that rawKey must be exactly 32 bytes — the 0x2a length byte in ED25519_X509_PREFIX encodes exactly 42 (10 prefix bytes + 32 key bytes). If this method is ever called with a different size, it will silently produce malformed DER. The caller already guards this, but the method itself should either validate or document the contract:

/**
 * Wraps the raw bytes of an ed25519 public key (must be exactly {@value ED25519_KEY_LENGTH} bytes)
 * in a X.509 SubjectPublicKeyInfo structure so that it can be read by a {@link KeyFactory}.
 */
private static byte[] x509Ed25519(byte[] rawKey) {
    if (rawKey.length != ED25519_KEY_LENGTH) {
        throw new IllegalArgumentException("Ed25519 raw key must be " + ED25519_KEY_LENGTH + " bytes, got " + rawKey.length);
    }
    ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. check added.

private static byte[] x509Ed25519(byte[] rawKey) {
if (rawKey.length != ED25519_KEY_LENGTH) {
throw new IllegalArgumentException("An ed25519 key must be " + ED25519_KEY_LENGTH
+ " bytes long, got " + rawKey.length);
}
byte[] encoded = new byte[ED25519_X509_PREFIX.length + rawKey.length];
System.arraycopy(ED25519_X509_PREFIX, 0, encoded, 0, ED25519_X509_PREFIX.length);
System.arraycopy(rawKey, 0, encoded, ED25519_X509_PREFIX.length, rawKey.length);
return encoded;
}

private static String readString(DataInputStream dis) throws IOException {
int size = dis.readInt();
byte[] bytes = new byte[size];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import java.security.spec.InvalidParameterSpecException;
import java.security.spec.KeySpec;
import java.security.spec.RSAPublicKeySpec;
import java.security.spec.X509EncodedKeySpec;
import java.util.Base64;

import javax.security.auth.login.FailedLoginException;

Expand Down Expand Up @@ -209,4 +211,26 @@ public void testEC256_2() throws FailedLoginException, NoSuchAlgorithmException,
assertFalse(PublickeyLoginModule.equals(publicKey, differentKey));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test covers the happy path and a different-key rejection, which is good. It is missing a test for the malformed-size path: size != ED25519_KEY_LENGTH. Something like:

// A base64 blob whose wire-format size field claims 31 bytes instead of 32
// should be rejected cleanly rather than throwing an exception
String badSizeKey = "...";
assertFalse(PublickeyLoginModule.equals(publicKey, badSizeKey));

This is low risk given the guard is simple, but it would pin the behaviour.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Test
public void testEd25519() throws FailedLoginException, NoSuchAlgorithmException, InvalidKeySpecException {
// Generated using: ssh-keygen -t ed25519
String storedKey = "AAAAC3NzaC1lZDI1NTE5AAAAIOiBWR+V72VeSjf4d2spgw2jmh95+LgE8GkCmZiFZQCd";
// the same key as X.509 SubjectPublicKeyInfo
String x509Key = "MCowBQYDK2VwAyEA6IFZH5XvZV5KN/h3aymDDaOaH3n4uATwaQKZmIVlAJ0=";

KeyFactory keyFactory = KeyFactory.getInstance("Ed25519");
KeySpec keySpec = new X509EncodedKeySpec(Base64.getDecoder().decode(x509Key));
PublicKey publicKey = keyFactory.generatePublic(keySpec);

assertTrue(PublickeyLoginModule.equals(publicKey, storedKey));

// Make sure a different stored key does not work
String differentKey = "AAAAC3NzaC1lZDI1NTE5AAAAIH0XVMRvA3FXSjqjRzqCIpqWaSRH5HxWRfwWqKEXayqu";
assertFalse(PublickeyLoginModule.equals(publicKey, differentKey));

// A key of the wrong length is rejected instead of being passed to the key factory
String truncatedKey = "AAAAC3NzaC1lZDI1NTE5AAAAHwABAgMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4=";
assertFalse(PublickeyLoginModule.equals(publicKey, truncatedKey));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,9 @@ protected SshServer createSshServer(SessionFactory sessionFactory) {
String[] macs = getStringArray("macs", "hmac-sha2-512,hmac-sha2-256");
String[] ciphers = getStringArray("ciphers", "aes256-ctr,aes192-ctr,aes128-ctr");
String[] kexAlgorithms = getStringArray("kexAlgorithms", "ecdh-sha2-nistp521,ecdh-sha2-nistp384,ecdh-sha2-nistp256,diffie-hellman-group-exchange-sha256");
String[] sigAlgorithms = getStringArray("sigAlgorithms", "ssh-rsa,rsa-sha2-256,rsa-sha2-512,sk-ecdsa-sha2-nistp256@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521");
// the sk-* algorithms are accepted by the server, but hardware backed keys are not
// supported by the publickey login module yet
String[] sigAlgorithms = getStringArray("sigAlgorithms", "ssh-rsa,rsa-sha2-256,rsa-sha2-512,sk-ecdsa-sha2-nistp256@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,ssh-ed25519,sk-ssh-ed25519@openssh.com");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sk-ssh-ed25519@openssh.com is added to sigAlgorithms here (and in the config file comment), but PublickeyLoginModule.equals() only handles "ssh-ed25519". A client authenticating with a FIDO2 hardware ed25519 key will hit the throw new FailedLoginException("Unsupported key type...") branch. This is consistent with how the existing sk-ecdsa-sha2-nistp256@openssh.com entry is handled, but it is worth a comment next to that value noting that hardware-backed sk keys are not yet supported in the login module.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically mirrors the behavior before: sk-ecdsa-sha2-nistp256@openssh.com is there as well. It it also accepted by the server but not yet supported in PublicKeyLoginModule. This is pretty much a different PR in my view.
So lets go with the comment for now.

String welcomeBanner = getString("welcomeBanner", null);
String moduliUrl = getString("moduli-url", null);
boolean sftpEnabled = getBoolean("sftpEnabled", true);
Expand Down
Loading