On Sun, 23 Feb 2025 02:08:12 GMT, Anthony Scarpino <ascarp...@openjdk.org>
wrote:
>> 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/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.
I thought about it, there are multiple issues associated with this approach:
- Different code path and logic behind current `UsageConstraint`
implementation. I think we discussed it already at our meeting and we agreed to
intercept this special TLS usage before it's consumed by constraints class.
- We can't just disregard a UsageConstraint that had a non-null nextConstraint,
we can have multiple scopes.
- The ampersand `&` is actually used between different constraints (`usage` and
`keysize` for example). For the `usage` constraint we have a space-separated
list of usages, and we can't mix TLS-specific usages with other usages.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r1968227461
