Copilot commented on code in PR #932:
URL: https://github.com/apache/mina-sshd/pull/932#discussion_r4326411597
##########
sshd-common/src/main/java/org/apache/sshd/common/config/keys/KeyUtils.java:
##########
@@ -167,6 +170,11 @@ public final class KeyUtils {
static {
+ for (MLDSAParameterSets params : MLDSAParameterSets.values()) {
+ if (SecurityUtils.isMLDSASupported(params.getJceName())) {
+ registerPublicKeyEntryDecoder(new
MLDSAPublicKeyDecoder(params));
+ }
+ }
Review Comment:
Registering ML-DSA decoders conditionally at class-initialization time means
ML-DSA key parsing support can be permanently missing if a provider is
installed later (e.g., application adds a provider after `KeyUtils` is first
loaded). To avoid order-dependent behavior, register the decoders
unconditionally and let decode/encode fail with a clear
`GeneralSecurityException` when no provider is available, or make decoder
registration lazy in the decoder lookup path.
##########
sshd-common/src/main/java/org/apache/sshd/common/util/security/bouncycastle/BouncyCastleSecurityProviderRegistrar.java:
##########
@@ -108,6 +113,28 @@ public boolean isSecurityEntitySupported(Class<?>
entityType, String name) {
return false;
}
+ if (SecurityUtils.ML_DSA.equalsIgnoreCase(name) ||
MLDSAParameterSets.fromJceName(name) != null) {
+ String probeKey = entityType.getName() + ":" +
name.toUpperCase(Locale.ROOT);
+ boolean available =
mlDSAServiceSupportHolder.computeIfAbsent(probeKey, k -> {
+ try {
+ if (KeyFactory.class.isAssignableFrom(entityType)) {
+ KeyFactory.getInstance(name, getSecurityProvider());
+ } else if
(KeyPairGenerator.class.isAssignableFrom(entityType)) {
+ KeyPairGenerator.getInstance(name,
getSecurityProvider());
+ } else {
+ return Boolean.FALSE;
+ }
+ return Boolean.TRUE;
+ } catch (GeneralSecurityException e) {
+ return Boolean.FALSE;
+ }
+ }).booleanValue();
Review Comment:
`getSecurityProvider()` may be `null` (e.g., BC not present/registered), in
which case `KeyFactory.getInstance(name, null)` /
`KeyPairGenerator.getInstance(name, null)` can throw an unchecked
`IllegalArgumentException`/`NullPointerException` (not a
`GeneralSecurityException`). That would cause `isSecurityEntitySupported` to
throw instead of returning `false`. Consider retrieving the provider once,
returning `false` if it’s `null`, and/or broadening the catch to include
runtime exceptions arising from a missing provider.
##########
sshd-common/src/main/java/org/apache/sshd/common/util/security/mldsa/MLDSAUtils.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.util.security.mldsa;
+
+import java.io.IOException;
+import java.security.GeneralSecurityException;
+import java.security.Key;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.Arrays;
+import java.util.Objects;
+
+import org.apache.sshd.common.util.ValidateUtils;
+import org.apache.sshd.common.util.io.der.ASN1Object;
+import org.apache.sshd.common.util.io.der.DERParser;
+import org.apache.sshd.common.util.io.der.DERWriter;
+import org.apache.sshd.common.util.security.MLDSAParameterSets;
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+/**
+ * Utilities to extract ML-DSA key bytes and construct keys using RFC 9881
encodings.
+ *
+ * @see <a href="https://www.rfc-editor.org/rfc/rfc9881";>RFC 9881</a>
+ * @author <a href="mailto:[email protected]";>Apache MINA SSHD Project</a>
+ */
+public final class MLDSAUtils {
+ private static final byte[] OID_PREFIX = { 0x60, (byte) 0x86, 0x48, 0x01,
0x65, 0x03, 0x04, 0x03 };
+
+ private MLDSAUtils() {
+ throw new UnsupportedOperationException("No instance allowed");
+ }
+
+ /**
+ * Retrieves the parameter set of an ML-DSA key.
+ *
+ * @param key the key
+ * @return the parameter set
+ * @throws InvalidKeySpecException if the key is not an ML-DSA key or its
encoding is invalid
+ */
+ public static MLDSAParameterSets getParameters(Key key) throws
InvalidKeySpecException {
+ Objects.requireNonNull(key, "No key provided");
+ MLDSAParameterSets named =
MLDSAParameterSets.fromJceName(key.getAlgorithm());
+ if (named != null) {
+ return named;
+ }
Review Comment:
`getParameters` returns a parameter set based solely on `key.getAlgorithm()`
for recognized names, without validating that the key’s encoding (OID) matches
that claimed algorithm. A crafted/custom `Key` implementation could be
misidentified as ML-DSA (impacting `KeyUtils.getKeyType()` / `getKeySize()`
decisions) even if its encoding is unrelated. Consider validating the algorithm
OID when `getEncoded()` is available (and rejecting mismatches), even when the
algorithm name matches a known ML-DSA variant.
##########
sshd-common/src/test/java/org/apache/sshd/common/util/buffer/keys/BufferPublicKeyParserTest.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.util.buffer.keys;
+
+import java.security.NoSuchAlgorithmException;
+import java.security.PublicKey;
+import java.security.spec.InvalidKeySpecException;
+import java.util.Arrays;
+import java.util.Collections;
+
+import org.apache.sshd.common.SshException;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+import org.apache.sshd.common.util.security.MLDSAParameterSets;
+import org.apache.sshd.common.util.security.SecurityUtils;
+import org.apache.sshd.common.util.security.mldsa.MLDSAUtils;
+import org.apache.sshd.util.test.JUnitTestSupport;
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
+
+@Tag("NoIoTestCase")
+class BufferPublicKeyParserTest extends JUnitTestSupport {
+ @ParameterizedTest
+ @EnumSource(MLDSAParameterSets.class)
+ void defaultParserSupportMatchesProvider(MLDSAParameterSets params) {
+ boolean supported =
SecurityUtils.isMLDSASupported(params.getJceName());
+ assertEquals(supported,
MLDSABufferPublicKeyParser.INSTANCE.isKeyTypeSupported(params.getSshName()));
+ assertEquals(supported,
BufferPublicKeyParser.DEFAULT.isKeyTypeSupported(params.getSshName()));
+ }
+
+ @Test
+ void unknownTypesAndEmptyParser() {
+ for (String type : new String[] { "", "ssh-mldsa-99", "ML-DSA-44",
"ssh-unknown" }) {
+
assertFalse(MLDSABufferPublicKeyParser.INSTANCE.isKeyTypeSupported(type));
+
assertFalse(BufferPublicKeyParser.DEFAULT.isKeyTypeSupported(type));
+ assertFalse(BufferPublicKeyParser.EMPTY.isKeyTypeSupported(type));
+ assertThrows(NoSuchAlgorithmException.class,
+ () -> BufferPublicKeyParser.DEFAULT.getRawPublicKey(type,
new ByteArrayBuffer()));
+ assertThrows(InvalidKeySpecException.class,
+ () ->
MLDSABufferPublicKeyParser.INSTANCE.getRawPublicKey(type, new
ByteArrayBuffer()));
+ }
+ assertSame(BufferPublicKeyParser.EMPTY,
BufferPublicKeyParser.aggregate(Collections.emptyList()));
+
assertFalse(MLDSABufferPublicKeyParser.INSTANCE.isKeyTypeSupported(null));
+ }
+
+ @ParameterizedTest
+ @EnumSource(MLDSAParameterSets.class)
+ void rawPublicKeyRoundTripAndWireFormat(MLDSAParameterSets params) throws
Exception {
+ PublicKey key = generatePublicKey(params);
+ byte[] expected = MLDSAUtils.getRawBytes(key);
+ ByteArrayBuffer buffer = new ByteArrayBuffer();
+ buffer.putRawPublicKey(key);
+ byte[] wire = buffer.getCompactData();
+
+ assertEquals(params.getSshName(), buffer.getString());
+ assertArrayEquals(expected, buffer.getBytes());
+ assertEquals(0, buffer.available());
+
+ ByteArrayBuffer input = new ByteArrayBuffer(wire);
+ PublicKey decoded = input.getRawPublicKey();
+ assertArrayEquals(expected, MLDSAUtils.getRawBytes(decoded));
+ assertEquals(0, input.available());
+
+ ByteArrayBuffer explicit = new ByteArrayBuffer(wire);
+ assertArrayEquals(expected, MLDSAUtils.getRawBytes(
+
explicit.getRawPublicKey(MLDSABufferPublicKeyParser.INSTANCE)));
+ assertEquals(0, explicit.available());
+ }
+
+ @ParameterizedTest
+ @EnumSource(MLDSAParameterSets.class)
+ void lengthPrefixedPublicKeyRoundTrip(MLDSAParameterSets params) throws
Exception {
+ PublicKey key = generatePublicKey(params);
+ ByteArrayBuffer buffer = new ByteArrayBuffer();
+ buffer.putPublicKey(key);
+ buffer.putInt(0x12345678);
+ PublicKey decoded = buffer.getPublicKey();
+ assertArrayEquals(MLDSAUtils.getRawBytes(key),
MLDSAUtils.getRawBytes(decoded));
+ assertEquals(0x12345678, buffer.getInt(), "Parser must leave the next
field intact");
+ assertEquals(0, buffer.available());
+ }
+
+ @ParameterizedTest
+ @EnumSource(MLDSAParameterSets.class)
+ void malformedRawKeys(MLDSAParameterSets params) {
+
Assumptions.assumeTrue(SecurityUtils.isMLDSASupported(params.getJceName()),
"ML-DSA provider unavailable");
+ for (int size : new int[] { 0, params.getPublicKeySize() - 1,
params.getPublicKeySize() + 1 }) {
+ ByteArrayBuffer buffer = new ByteArrayBuffer();
+ buffer.putString(params.getSshName());
+ buffer.putBytes(new byte[size]);
+ assertThrows(SshException.class, buffer::getRawPublicKey, "Invalid
public key size " + size);
+ }
+
+ ByteArrayBuffer truncated = new ByteArrayBuffer();
+ truncated.putString(params.getSshName());
+ truncated.putInt(params.getPublicKeySize());
+ truncated.putRawBytes(new byte[params.getPublicKeySize() - 1]);
+ assertThrows(SshException.class, truncated::getRawPublicKey);
+ }
+
+ @ParameterizedTest
+ @EnumSource(MLDSAParameterSets.class)
+ void mismatchedVariantAndTruncatedOuterString(MLDSAParameterSets params)
throws Exception {
+ PublicKey key = generatePublicKey(params);
+ MLDSAParameterSets other
+ = params == MLDSAParameterSets.ML_DSA_44 ?
MLDSAParameterSets.ML_DSA_65 : MLDSAParameterSets.ML_DSA_44;
+
Assumptions.assumeTrue(SecurityUtils.isMLDSASupported(other.getJceName()),
"Other variant unavailable");
+ ByteArrayBuffer mismatched = new ByteArrayBuffer();
+ mismatched.putString(other.getSshName());
+ mismatched.putBytes(MLDSAUtils.getRawBytes(key));
+ assertThrows(SshException.class, mismatched::getRawPublicKey);
+
+ ByteArrayBuffer encoded = new ByteArrayBuffer();
+ encoded.putPublicKey(key);
+ byte[] bytes = encoded.getCompactData();
+ ByteArrayBuffer truncated = new ByteArrayBuffer(Arrays.copyOf(bytes,
bytes.length - 1));
+ assertThrows(RuntimeException.class, truncated::getPublicKey);
Review Comment:
This assertion is very broad and may hide unexpected failures (e.g.,
`NullPointerException` vs. an intended parsing exception). Prefer asserting the
most specific exception type thrown by `getPublicKey()` for truncated input in
this codebase (e.g., `BufferException`/`SshException`, depending on the actual
behavior) to keep the test diagnostic and stable.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
