On Tue, 22 Oct 2024 16:19:14 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:
>> Artur Barashev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 13 additional >> commits since the last revision: >> >> - Do exact match on jdk.tls.disabledAlgorithms property >> - Merge branch 'master' into JDK-8341964 >> - Remove duplicate description from docs >> - Re-ordering and simplifying the checks. Restricting new constraint to >> TLS. Updating docs. >> - Revert Copyright on unmodified file >> - Update comments and Copyright >> - Match TLSCipherConstraint constraint against any algorithm. TLSv1.3 test >> case. >> - Merge branch 'master' into JDK-8341964 >> - Add tests >> - Naming update >> - ... and 3 more: https://git.openjdk.org/jdk/compare/ed909bbc...fd3ae924 > > src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java > line 157: > >> 155: */ >> 156: @Override >> 157: public final boolean permits(Set<CryptoPrimitive> primitives, > > I don't think this method should be changed. As the comment previously > stated, this is for completely disabling the algorithm. Your change adds > TLS-specific checking which is also not needed for checking with > `jdk.jar.disabledAlgorithms` and `jdk.certpath.disabledAlgorithms`. If I move TLS cipher suite check from this method to `checkConstraints` then tests fail. In the context of `AlgorithmConstraints` the "algorithm" word means either an actual algorithm like "RSA" or a TLS cipher suite. So in this method we completely disable given TLS cipher suite (aka algorithm) if it matches the criteria. I just updated the comment to avoid any confusion. > src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java > line 393: > >> 391: Matcher matcher; >> 392: >> 393: TLSCipherSegment segment = >> Arrays.stream(TLSCipherSegment.values()) > > This seems overly complicated for what is a existing, simple if() layout. > `else if (entry.endsWith("kx")) { ... }` > `else if (entry.endsWith("authn")) { ...}` > > I also suggesting moving this down farther as the other keywords are more > likely to be used at this time. > > This code is used for jar and certpath checking too, so there may need to be > a check to not allow these new keywords to be used with the other Security > properties. - I designed it to be expandable in the future if needed. For example we may add `bulk` TLS cipher constraint to disable some algorithms to be used as bulk cipher. - We already have checks to not allow TLSCipherConstraint to be linked with any other constraints - I'll work on re-ordering the checks > src/java.base/share/conf/security/java.security line 586: > >> 584: # usage [TLSServer] [TLSClient] [SignedJAR] >> 585: # >> 586: # TLSCipherConstraint: > > These comments may need to be in the tls property, adding that these are > specific constraints for TLS operations. I realize `UsageConstraints` say > TLS in them, but they are used for multiple properties. Maybe the tls > property should follow the layout of the jar property, where it lists the > options and refers to the meanings in certpath. Except for the new property. Yeah, it can be somewhat confusing. The thing is `jdk.tls.disabledAlgorithms` section has the following instruction: `See the specification of "jdk.certpath.disabledAlgorithms" for the syntax of the disabled algorithm string. ` So it looks like the `jdk.certpath.disabledAlgorithms` section is used as the central location for constraint description. Thus, I've decided to put the description for this new constraint there. I'll modify the description to follow the layout of the jar property as suggested. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21577#discussion_r1811574457 PR Review Comment: https://git.openjdk.org/jdk/pull/21577#discussion_r1811576566 PR Review Comment: https://git.openjdk.org/jdk/pull/21577#discussion_r1811599259
