On Fri, 25 Oct 2024 19:40:38 GMT, Artur Barashev <abaras...@openjdk.org> wrote:
>> The current syntax of the jdk.tls.disabledAlgorithms makes it difficult to >> disable algorithms that affect both the key exchange and authentication >> parts of a TLS cipher suite. For example, if you add "RSA" to the >> jdk.tls.disabledAlgorithms security property, it disables all cipher suites >> that use RSA, whether it is for key exchange or authentication. If you only >> want to disable cipher suites that use RSA for key exchange, the only >> workaround is to list the whole cipher suite name, so an exact match is >> done, but if there are many cipher suites that use that key exchange >> algorithm, this becomes cumbersome. >> >> We should extend the syntax of the property to be able to distinguish >> between different cryptographic primitives used in the cipher suite. I think >> adding a new constraint something like: >> >> TLSCipherConstraint: kx | authn >> >> So when disabling TLS_RSA suites, you would add "RSA kx" to the property. > > 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`. 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. 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21577#discussion_r1811031724 PR Review Comment: https://git.openjdk.org/jdk/pull/21577#discussion_r1811279872 PR Review Comment: https://git.openjdk.org/jdk/pull/21577#discussion_r1811369052
