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
