On Fri, 7 Mar 2025 19:07:16 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> rename getNamedCurveFromKey
>
> src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
> line 274:
>
>> 272: yield List.of();
>> 273: }
>> 274: yield List.of(nc.getNameAndAliases());
>
> Do you want to add `EC` itself to the list? I am asking because for EdDSA you
> added both the algorithm name and the parameter set name.
No, "EC" isn't an alias for a particular curve.
> src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
> line 276:
>
>> 274: yield List.of(nc.getNameAndAliases());
>> 275: }
>> 276: default -> List.of(key.getAlgorithm(),
>> KeyUtil.getAlgorithm(key));
>
> What if these 2 are the same string?
Are you concerned about duplicate checks?
> test/jdk/sun/security/util/AlgorithmConstraints/DisabledAlgorithmPermits.java
> line 86:
>
>> 84: Arrays.asList(
>> 85: new TestCase("EdDSA", false),
>> 86: new TestCase("Ed25519", true),
>
> Why should the above pass? If you disable `EdDSA` and you are still allowed
> `Signature.getInstance("Ed25519")`? If this is because it will reject
> whatever EdDSA key later? Why both check `CryptoPrimitive.SIGNATURE` at all?
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 `CryptoPrimitive.SIGNATURE`, it just a value used in the test,
it can't be null.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1985768201
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1985768272
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1985768304
