Copilot commented on code in PR #933: URL: https://github.com/apache/mina-sshd/pull/933#discussion_r4337308907
########## sshd-common/src/main/java/org/apache/sshd/common/util/security/mldsa/SignatureMLDSA.java: ########## @@ -0,0 +1,75 @@ +/* + * 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.security.SignatureException; +import java.util.Map; +import java.util.Objects; + +import org.apache.sshd.common.session.SessionContext; +import org.apache.sshd.common.signature.AbstractSignature; +import org.apache.sshd.common.util.ValidateUtils; +import org.apache.sshd.common.util.security.MLDSAParameterSets; + +/** + * An ML-DSA signature in pure mode with an empty context. + * + * @see <a href="https://datatracker.ietf.org/doc/draft-sfluhrer-ssh-mldsa/";>draft-sfluhrer-ssh-mldsa</a> + * @author <a href="mailto:[email protected]";>Apache MINA SSHD Project</a> + */ +public class SignatureMLDSA extends AbstractSignature { + private final MLDSAParameterSets params; + + /** + * Creates a signature for a parameter set. + * + * @param params the parameter set + */ + public SignatureMLDSA(MLDSAParameterSets params) { + super(Objects.requireNonNull(params, "No ML-DSA parameters provided").getJceName(), params.getSshName()); + this.params = params; + } + + @Override + public boolean verify(SessionContext session, byte[] sig) throws Exception { + byte[] data = sig; + String sshName = params.getSshName(); + Map.Entry<String, byte[]> encoding = extractEncodedSignature(data, sshName::equalsIgnoreCase); + if (encoding != null) { + String keyType = encoding.getKey(); + ValidateUtils.checkTrue(sshName.equals(keyType), "Mismatched key type: %s", keyType); + data = encoding.getValue(); + } + + return doVerify(data); + } Review Comment: The decoding path accepts the key type case-insensitively (`equalsIgnoreCase`) but then enforces a case-sensitive match and throws on mismatch. This can turn an otherwise "invalid signature" into an exception (and makes behavior dependent on the peer’s casing). Make the logic consistent: either (a) use strict matching when extracting (so mismatched casing won’t unwrap), or (b) validate with `equalsIgnoreCase` and treat mismatches as a normal verification failure (return `false`) rather than throwing. ########## sshd-common/src/main/java/org/apache/sshd/common/util/security/bouncycastle/BouncyCastleSecurityProviderRegistrar.java: ########## @@ -108,6 +113,34 @@ 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 { + Provider provider = getSecurityProvider(); + if (provider == null) { + return Boolean.FALSE; + } + if (KeyFactory.class.isAssignableFrom(entityType)) { + KeyFactory.getInstance(name, provider); + } else if (KeyPairGenerator.class.isAssignableFrom(entityType)) { + KeyPairGenerator.getInstance(name, provider); + } else if (Signature.class.isAssignableFrom(entityType)) { + Signature.getInstance(name, provider); + } else { + return Boolean.FALSE; + } + return Boolean.TRUE; + } catch (GeneralSecurityException | RuntimeException e) { + return Boolean.FALSE; + } + }).booleanValue(); + if (!available) { + return false; + } + return super.isSecurityEntitySupported(entityType, name); + } Review Comment: `MLDSAParameterSets.fromJceName(name)` intentionally recognizes case-/hyphen-insensitive variants (e.g., "mldsa44"), but the probe still uses the original `name` when calling `KeyFactory.getInstance(...)` / `Signature.getInstance(...)`. This can incorrectly cache/report "unsupported" even when the provider supports the canonical JCA algorithm. Normalize to the canonical `variant.getJceName()` (and use that for `probeKey` + `super.isSecurityEntitySupported`), and consider handling the generic `ML-DSA` name separately since it may not be a real JCA algorithm ID. ########## sshd-core/src/test/java/org/apache/sshd/common/signature/SignatureFactoriesTest.java: ########## @@ -110,6 +119,14 @@ private static void addTests( @BeforeAll static void setupClientAndServer() throws Exception { sshd = CoreTestSupportUtils.setupTestFullSupportServer(SignatureFactoriesTest.class); + List<NamedFactory<Signature>> serverSignatures = new ArrayList<>(sshd.getSignatureFactories()); + for (MLDSAParameterSets params : MLDSAParameterSets.values()) { + BuiltinSignatures factory = BuiltinSignatures.fromFactoryName(params.getSshName()); + if (factory.isSupported()) { + serverSignatures.add(factory); + } + } + sshd.setSignatureFactories(serverSignatures); Review Comment: This appends ML-DSA factories to the existing list without checking whether they’re already present. If the test server’s default signature factories already include ML-DSA (now that they’re built-ins), this can introduce duplicates, making negotiation order harder to reason about and potentially masking issues. Prefer de-duplicating by factory name before `setSignatureFactories(...)`. ########## sshd-core/src/test/java/org/apache/sshd/common/signature/SignatureFactoriesTest.java: ########## @@ -199,4 +216,21 @@ protected void testKeyPairProvider( s.auth().verify(AUTH_TIMEOUT.multipliedBy(3)); } } + + @ParameterizedTest(name = "mldsa={0}") + @EnumSource(MLDSAParameterSets.class) + void mldsaUserPublicKeyAuth(MLDSAParameterSets params) throws Exception { + BuiltinSignatures factory = BuiltinSignatures.fromFactoryName(params.getSshName()); + Assumptions.assumeTrue(factory.isSupported()); + KeyPair hostKey = SecurityUtils.getKeyPairGenerator(params.getJceName()).generateKeyPair(); + KeyPair userKey = SecurityUtils.getKeyPairGenerator(params.getJceName()).generateKeyPair(); + sshd.setKeyPairProvider(KeyPairProvider.wrap(hostKey)); + client.setSignatureFactories(Collections.singletonList(factory)); + try (ClientSession s = client.connect(getCurrentTestName(), TEST_LOCALHOST, port) + .verify(CONNECT_TIMEOUT).getSession()) { + s.addPublicKeyIdentity(userKey); + s.auth().verify(AUTH_TIMEOUT); + assertTrue(s.isAuthenticated()); + } Review Comment: This test mutates shared `sshd` and `client` configuration (`setKeyPairProvider`, `setSignatureFactories`) without restoring previous values. If tests are ever executed concurrently (or if another test runs after this within the same JVM and reuses the same static server/client), this can cause cross-test interference and flakiness. Capture the previous key pair provider/signature factory list and restore them in a `finally` block (or scope these settings to a per-test server/client instance). -- 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]
