utafrali commented on code in PR #2927:
URL: https://github.com/apache/karaf/pull/2927#discussion_r4056954733
##########
jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java:
##########
@@ -239,6 +247,23 @@ 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();
+ return encoded != null && Arrays.equals(encoded,
generatedPublicKey.getEncoded());
Review Comment:
`key.getEncoded()` returning `null` silently causes authentication to fail
with no diagnostic information. The SSH client will just see a rejected key.
Add a debug-level log before returning false:
```java
byte[] encoded = key.getEncoded();
if (encoded == null) {
LOG.debug("Ed25519 key returned null encoding — provider may not support
getEncoded()");
return false;
}
return Arrays.equals(encoded, generatedPublicKey.getEncoded());
```
##########
shell/ssh/src/main/java/org/apache/karaf/shell/ssh/Activator.java:
##########
@@ -169,7 +169,7 @@ 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,[email protected],ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521");
+ String[] sigAlgorithms = getStringArray("sigAlgorithms",
"ssh-rsa,rsa-sha2-256,rsa-sha2-512,[email protected],ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,ssh-ed25519,[email protected]");
Review Comment:
`[email protected]` 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 `[email protected]`
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.
##########
jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java:
##########
@@ -239,6 +247,23 @@ 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");
Review Comment:
`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.
##########
jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyEncodingTest.java:
##########
@@ -209,4 +211,22 @@ public void testEC256_2() throws FailedLoginException,
NoSuchAlgorithmException,
assertFalse(PublickeyLoginModule.equals(publicKey, differentKey));
}
Review Comment:
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:
```java
// 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.
##########
jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java:
##########
@@ -247,6 +272,17 @@ 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}.
+ */
Review Comment:
`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:
```java
/**
* 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);
}
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
