8346129: Simplify EdDSA & XDH curve name usage #23647
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| import sun.security.util.DisabledAlgorithmConstraints; | ||
|
|
||
| import java.security.CryptoPrimitive; | ||
| import java.security.KeyPairGenerator; | ||
| import java.security.PrivateKey; | ||
| import java.security.Security; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
|
|
||
| /* | ||
| * @test | ||
| * @bug 8346129 | ||
| * @modules java.base/sun.security.util | ||
| * @summary Check DisabledAlgorithmConstraints using EdDSA & XDH against | ||
| * permit(Set<CryptoPrimitive>, String, AP) and | ||
| * permit(Set<CryptoPrimitive>, Key). Results will differ based | ||
| * on method used. The first method only can compare the String with the | ||
| * algorithms. The second can check the algorithm and NamedParameterSpec | ||
| * while results in more 'false' cases. | ||
| * | ||
| * @run main/othervm DisabledAlgorithmPermits Ed25519 | ||
| * @run main/othervm DisabledAlgorithmPermits Ed448 | ||
| * @run main/othervm DisabledAlgorithmPermits EdDSA | ||
| * @run main/othervm DisabledAlgorithmPermits X25519 | ||
| * @run main/othervm DisabledAlgorithmPermits X448 | ||
| * @run main/othervm DisabledAlgorithmPermits XDH | ||
| */ | ||
|
|
||
| public class DisabledAlgorithmPermits { | ||
| public static void main(String[] args) throws Exception { | ||
| String algorithm = args[0]; | ||
| Security.setProperty("x", algorithm); | ||
|
|
||
| // Expected table are the expected results from the test | ||
| List<TestCase> expected = switch (algorithm) { | ||
| case "Ed25519" -> | ||
| Arrays.asList( | ||
| new TestCase("EdDSA", true), | ||
| new TestCase("Ed25519", false), | ||
| new TestCase("Ed448", true), | ||
| new TestCase("X448", true), | ||
| new TestCase(1,"EdDSA", false), | ||
| new TestCase(1,"Ed25519", false), | ||
| new TestCase(1,"Ed448", true), | ||
| new TestCase(1,"X448", true)); | ||
| case "Ed448" -> | ||
| Arrays.asList( | ||
| new TestCase("EdDSA", true), | ||
| new TestCase("Ed25519", true), | ||
| new TestCase("Ed448", false), | ||
| new TestCase("X448", true), | ||
| new TestCase(1,"EdDSA", true), | ||
| new TestCase(1,"Ed25519", true), | ||
| new TestCase(1,"Ed448", false), | ||
| new TestCase(1,"X448", true)); | ||
| case "EdDSA" -> | ||
| Arrays.asList( | ||
| new TestCase("EdDSA", false), | ||
| new TestCase("Ed25519", true), | ||
|
Contributor
There was a problem hiding this comment. Why should the above pass? If you disable
Contributor
Author
There was a problem hiding this comment. I'm confused by this comment. With removing the hardcoded aliases in AbstractAlgorithmConstraints, which is what I thought you had suggested, EdDSA and Ed25519 are now separate as the check is effectively a string compare check against the disabledAlgorithm list The second half of that case statement has a key that can check against both EdDSA and the NPS. With respect to
Contributor
There was a problem hiding this comment. Yes, I remember that. I understand there will be multiple checks in TLS and CertPath. Do we have existing tests on that level?
Contributor
Author
There was a problem hiding this comment. We have other tests that check this. |
||
| new TestCase("Ed448", true), | ||
| new TestCase("X448", true), | ||
| new TestCase(1,"EdDSA", false), | ||
| new TestCase(1,"Ed25519", false), | ||
| new TestCase(1,"Ed448", false), | ||
| new TestCase(1,"X448", true)); | ||
| case "X25519" -> | ||
| Arrays.asList( | ||
| new TestCase("XDH", true), | ||
| new TestCase("X25519", false), | ||
| new TestCase("X448", true), | ||
| new TestCase("Ed448", true), | ||
| new TestCase(1, "XDH", false), | ||
| new TestCase(1, "X25519", false), | ||
| new TestCase(1, "X448", true), | ||
| new TestCase(1, "Ed448", true)); | ||
| case "X448" -> | ||
| Arrays.asList( | ||
| new TestCase("XDH", true), | ||
| new TestCase("X25519", true), | ||
| new TestCase("X448", false), | ||
| new TestCase("Ed448", true), | ||
| new TestCase(1, "XDH", true), | ||
| new TestCase(1, "X25519", true), | ||
| new TestCase(1, "X448", false), | ||
| new TestCase(1, "Ed448", true)); | ||
| case "XDH" -> | ||
| Arrays.asList( | ||
| new TestCase("XDH", false), | ||
| new TestCase("X25519", true), | ||
| new TestCase("X448", true), | ||
| new TestCase("Ed448", true), | ||
| new TestCase(1, "XDH", false), | ||
| new TestCase(1, "X25519", false), | ||
| new TestCase(1, "X448", false), | ||
| new TestCase(1, "Ed448", true)); | ||
| default -> null; | ||
| }; | ||
|
|
||
| Objects.requireNonNull(expected, "algorithm being tested " + | ||
| algorithm + " not in expected table"); | ||
| System.out.println("---"); | ||
| var dac = new DisabledAlgorithmConstraints("x"); | ||
| System.out.println("disabled algorithms = " + Security.getProperty("x")); | ||
|
|
||
| // Using only testType 0, this tests that permit(Set<>, String, null) | ||
| // will check only the algorithm against the disabled list | ||
| expected.stream().filter(n->n.testType == 0).forEach(tc -> { | ||
| boolean r = dac.permits(Set.of(CryptoPrimitive.SIGNATURE), | ||
| tc.testAlgo, null); | ||
| System.out.print("\tpermits(Set.of(CryptoPrimitive.SIGNATURE), \"" + | ||
| tc.testAlgo + "\", null): " + r + " : " ); | ||
| if (r != tc.expected) { | ||
| System.out.println("failed."); | ||
| throw new AssertionError("failed. Expected " + | ||
| tc.expected); | ||
| } | ||
|
Member
There was a problem hiding this comment. I suggest replacing this check with
Contributor
Author
There was a problem hiding this comment. If I did that, then I couldn't println to stdout. I'd rather keep it as is. |
||
| System.out.println("pass"); | ||
| }); | ||
|
|
||
| // Using only testType 1, this tests permit(Set<>, Key) that will look | ||
| // at both the key.getAlgorithm() and the key.getParams().getName() | ||
| // against the disabled list | ||
| expected.stream().filter(n->n.testType == 1).forEach(tc -> { | ||
| PrivateKey k; | ||
| try { | ||
| k = KeyPairGenerator.getInstance(tc.testAlgo).generateKeyPair(). | ||
| getPrivate(); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| boolean r = dac.permits(Set.of(CryptoPrimitive.SIGNATURE), k); | ||
| System.out.print("\tpermits(Set.of(CryptoPrimitive.SIGNATURE), " + | ||
| tc.testAlgo + " privkey): " + r + " : " ); | ||
| if (r != tc.expected) { | ||
| System.out.println("failed."); | ||
| throw new AssertionError("failed. Expected " + | ||
| tc.expected); | ||
| } | ||
|
|
||
| System.out.println("pass"); | ||
| }); | ||
| System.out.println("---"); | ||
| } | ||
|
|
||
| record TestCase(int testType, String testAlgo, boolean expected) { | ||
| TestCase(String testAlgo, boolean expected) { | ||
| this(0, testAlgo, expected); | ||
| } | ||
| } | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Do you plan to have a unit test for
AlgorithmCheckerchanges? It looks like certificates usingED25519algorithm didn't match that check before. It would be useful to have a test where we disableED25519in java.security and then try to use a certificate withED25519algorithm.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.
This is checked by an existing test