Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Tue, 19 Apr 2022 17:16:29 GMT, Daniel Jeliński wrote: >> During TLS handshake, hundreds of constraints are evaluated to determine >> which cipher suites are usable. Most of the evaluations are performed using >> `HandshakeContext#algorithmConstraints` object. By default that object >> contains a `SSLAlgorithmConstraints` instance wrapping another >> `SSLAlgorithmConstraints` instance. As a result the constraints defined in >> `SSLAlgorithmConstraints` are evaluated twice. >> >> This PR improves the default case; if the user-specified constraints are >> left at defaults, we use a single `SSLAlgorithmConstraints` instance, and >> avoid duplicate checks. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Replace remaining constructors with factory methods Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Wed, 20 Apr 2022 10:28:39 GMT, Daniel Jeliński wrote: >> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java >> line 94: >> >>> 92: AlgorithmConstraints userSpecifiedConstraints, >>> 93: boolean withDefaultCertPathConstraints) { >>> 94: if (nullIfDefault(userSpecifiedConstraints) == null) { >> >> Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation? >> The logic of the block is a little bit hard to understand to me. > > No I don't; it's for the same reason why I'm using `==` and not `equals`: > `DEFAULT` is the only `SSLAlgorithmConstraints` instance that is ever used as > `userSpecifiedConstraints` here. > > `DEFAULT` is used because [SSLConfiguration sets > userSpecifiedAlgorithmConstraints to > SSLAlgorithmConstraints.DEFAULT](https://github.com/openjdk/jdk/blob/6d8d156c97b90a9ab4776c6b42563a962d959741/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L129). > This feels wrong; the name suggests that the constraints should be specified > by user, and should be null if the user doesn't touch them. > `userSpecifiedAlgorithmConstraints` are accessible by > `getSSLParameters().getAlgorithmConstraints()` on SSLEngineImpl and > SSLSocketImpl. Returning `DEFAULT` here also feels wrong; as a user I would > be concerned that setting my own algorithm constraints would replace the > default ones. It doesn't, but that is not immediately apparent. > > We could initialize `userSpecifiedAlgorithmConstraints` to null, and back out > all the other changes from this PR. The only reason why I didn't do that was > because it would change the observable behavior > (`getSSLParameters().getAlgorithmConstraints()` would return `null`). If you > think we can live with that, I'll be happy to do that change. It is not interested to me to use 'null' constraints in ssl configure. I have no more comments. Thank you for the update! - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Wed, 20 Apr 2022 04:19:38 GMT, Xue-Lei Andrew Fan wrote: >> Daniel Jeliński has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Replace remaining constructors with factory methods > > src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java > line 94: > >> 92: AlgorithmConstraints userSpecifiedConstraints, >> 93: boolean withDefaultCertPathConstraints) { >> 94: if (nullIfDefault(userSpecifiedConstraints) == null) { > > Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation? > The logic of the block is a little bit hard to understand to me. No I don't; it's for the same reason why I'm using `==` and not `equals`: `DEFAULT` is the only `SSLAlgorithmConstraints` instance that is ever used as `userSpecifiedConstraints` here. `DEFAULT` is used because [SSLConfiguration sets userSpecifiedAlgorithmConstraints to SSLAlgorithmConstraints.DEFAULT](https://github.com/openjdk/jdk/blob/6d8d156c97b90a9ab4776c6b42563a962d959741/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L129). This feels wrong; the name suggests that the constraints should be specified by user, and should be null if the user doesn't touch them. `userSpecifiedAlgorithmConstraints` are accessible by `getSSLParameters().getAlgorithmConstraints()` on SSLEngineImpl and SSLSocketImpl. Returning `DEFAULT` here also feels wrong; as a user I would be concerned that setting my own algorithm constraints would replace the default ones. It doesn't, but that is not immediately apparent. We could initialize `userSpecifiedAlgorithmConstraints` to null, and back out all the other changes from this PR. The only reason why I didn't do that was because it would change the observable behavior (`getSSLParameters().getAlgorithmConstraints()` would return `null`). If you think we can live with that, I'll be happy to do that change. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Tue, 19 Apr 2022 17:16:29 GMT, Daniel Jeliński wrote: >> During TLS handshake, hundreds of constraints are evaluated to determine >> which cipher suites are usable. Most of the evaluations are performed using >> `HandshakeContext#algorithmConstraints` object. By default that object >> contains a `SSLAlgorithmConstraints` instance wrapping another >> `SSLAlgorithmConstraints` instance. As a result the constraints defined in >> `SSLAlgorithmConstraints` are evaluated twice. >> >> This PR improves the default case; if the user-specified constraints are >> left at defaults, we use a single `SSLAlgorithmConstraints` instance, and >> avoid duplicate checks. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Replace remaining constructors with factory methods src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line 94: > 92: AlgorithmConstraints userSpecifiedConstraints, > 93: boolean withDefaultCertPathConstraints) { > 94: if (nullIfDefault(userSpecifiedConstraints) == null) { Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation? The logic of the block is a little bit hard to understand to me. src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line 97: > 95: return withDefaultCertPathConstraints ? DEFAULT : > DEFAULT_SSL_ONLY; > 96: } > 97: return new SSLAlgorithmConstraints(userSpecifiedConstraints, > withDefaultCertPathConstraints); It would be nice to limit each line within 80 characters, which is useful for terminal users. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
> During TLS handshake, hundreds of constraints are evaluated to determine > which cipher suites are usable. Most of the evaluations are performed using > `HandshakeContext#algorithmConstraints` object. By default that object > contains a `SSLAlgorithmConstraints` instance wrapping another > `SSLAlgorithmConstraints` instance. As a result the constraints defined in > `SSLAlgorithmConstraints` are evaluated twice. > > This PR improves the default case; if the user-specified constraints are left > at defaults, we use a single `SSLAlgorithmConstraints` instance, and avoid > duplicate checks. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Replace remaining constructors with factory methods - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/e4cc8152..2a1f0a1d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8199=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8199=02-03 Stats: 58 lines in 4 files changed: 21 ins; 14 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199