-
Notifications
You must be signed in to change notification settings - Fork 666
[ssh] Support ed25519 keys using bouncycastle #2927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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 { | ||
|
|
@@ -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"); | ||
| 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()); | ||
| } | ||
|
|
@@ -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. | ||
| */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
/**
* 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);
}
...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
||
|
|
@@ -209,4 +211,26 @@ public void testEC256_2() throws FailedLoginException, NoSuchAlgorithmException, | |
| assertFalse(PublickeyLoginModule.equals(publicKey, differentKey)); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: // 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -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"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This basically mirrors the behavior before: |
||
| String welcomeBanner = getString("welcomeBanner", null); | ||
| String moduliUrl = getString("moduli-url", null); | ||
| boolean sftpEnabled = getBoolean("sftpEnabled", true); | ||
|
|
||
There was a problem hiding this comment.
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 viaBouncyCastleSecurityProviderRegistrar. That sequencing dependency is invisible here. The caughtGeneralSecurityExceptionwill surface it as aFailedLoginException, 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.There was a problem hiding this comment.
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.