On Wed, 19 Feb 2025 13:26:49 GMT, Artur Barashev <abaras...@openjdk.org> wrote:
>> Currently when a signature scheme constraint is specified with >> "jdk.tls.disabledAlgorithms" property we don't differentiate between >> signatures used to sign a TLS handshake exchange and the signatures used in >> TLS certificates: >> https://datatracker.ietf.org/doc/html/rfc8446#section-4.2.3 > > Artur Barashev has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo in java.security documentation src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 223: > 221: // Handshake signature scope > 222: public static final Set<SSLCryptoScope> HANDSHAKE_SCOPE = > 223: > Collections.unmodifiableSet(EnumSet.of(SSLCryptoScope.HANDSHAKE)); I think you get the same with `Set.of(SSLCryptoScope.HANDSHAKE)` src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java line 220: > 218: > 219: // Checks if algorithm is disabled for the given TLS scopes. > 220: public boolean permits(String algorithm, Set<String> scopes) { Is there a reason you decided to have the caller make it into a `Set<String>` instead of passing SSLCryptoScope down? src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java line 978: > 976: } > 977: > 978: private boolean cachedCheckAlgorithm( The only reason you need `scopes` here is because of `checkAlgorithmTlsScopes`, which I believe can go away. If so, then you can just pass a modified algorithm+scopes value from `permit(algorithm, Set<String>)` as all the other calls to this caching method have scopes as `null`. I still have to think if `scopes` as part of the `cacheKey` can be avoided. src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java line 1042: > 1040: } > 1041: > 1042: private boolean checkAlgorithmTlsScopes( If you're able to use `algorithmConstraints` with `UsageConstraint`, you won't need this scopes-specific method src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java line 1064: > 1062: } > 1063: > 1064: private Map<String, Set<String>> getDisabledAlgorithmScopes() { I don't think this new Map is necessary. `algorithmConstraints` would already contain this as a `UsageConstraint` with the algorithm as the key. I think if there was a UsageConstraint.permits(Set<SSLCryptoScopes>) you could have a few benefits. First, pushing the conversion from SSLCryptoScope enums to String would be more efficient as no constraint, it would cause no conversion. Secondly, you could disregard a UsageConstraint that had a non-null `nextConstraint`, that would help your previous concerns about & It also unifies all `usage` under one class. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r1966676283 PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r1966652176 PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r1966668087 PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r1966667670 PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r1966650043
