RFR: 8349583: Add mechanism to disable signature schemes based on their TLS scope [v3]

[Anthony Scarpino]

On Wed, 19 Feb 2025 13:26:49 GMT, Artur Barashev <abarashev at 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