On Wed, 20 Apr 2022 04:19:38 GMT, Xue-Lei Andrew Fan <[email protected]> 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
