Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Fri, 4 Feb 2022 16:35:22 GMT, Sean Mullan wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> More update for the sec and impl > > src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 713: > >> 711: * in this list or may not use the recommended name for a certain >> 712: * signature scheme. >> 713: * > > Regarding my previous comment about providers not supporting these new > methods, I think something like the following might suffice: > > > @apiNote Note that a provider may not have been updated to support this > method and in that case may return null instead of the default signature > schemes for pre-populated objects. > @implNote The SunJSSE provider supports this method. It looks like a typo. Replaced "pre-populated objects" with "connection populated objects", and added to the spec. - PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Fri, 4 Feb 2022 16:34:14 GMT, Sean Mullan wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> More update for the sec and impl
>
> src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 735:
>
>> 733: * {@link #setSignatureSchemes} method has not been called, this
>> method
>> 734: * should return the default signature schemes for connection
>> populated
>> 735: * objects, or {@code null} for pre-populated objects.
>
> I think this sentence should be part of the specification and not the
> implementation note since ideally you would want all provider implementations
> to behave this way (assuming they supported this method):
>
> "If the {@link #setSignatureSchemes} method has not been called, this method
> should return the default signature schemes for connection populated objects,
> or {@code null} for pre-populated objects."
I agreed.
> src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 752:
>
>> 750:
>> 751: /**
>> 752: * Sets the prioritized array of signature scheme names that
>
> If the system properties are set, does it override this API? I think it is
> important to mention that here in an @implNote. I know you discuss them in
> `getSignatureSchemes` but I think a developer is more likely to want to know
> the behavior of the properties for this method especially if they will
> override this setting.
I though it is a spec of the get method. But I agree with you that it could be
helpful if we have it in the set method as well. Updated.
> src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 761:
>
>> 759: * Names Specification. Providers may support signature schemes
>> not defined
>> 760: * in this list or may not use the recommended name for a certain
>> 761: * signature scheme.
>
> Did you want to say something about what the behavior should be if a provider
> does not support one of these schemes? Is it ignored, or is an exception
> thrown?
Just as your suggested in a later comment, an apiNote was added.
> src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 762:
>
>> 760: * in this list or may not use the recommended name for a certain
>> 761: * signature scheme.
>> 762: *
>
> Regarding my previous comment about providers not supporting these new
> methods, I think something like the following might suffice:
>
>
> @apiNote Note that a provider may not have been updated to support this
> method and in that case may ignore the schemes that are set.
> @implNote The SunJSSE provider supports this method.
Thank you for the suggestion. Updated.
-
PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Wed, 2 Feb 2022 19:12:56 GMT, Xue-Lei Andrew Fan wrote:
>> This update is to support signature schemes customization for individual
>> (D)TLS connection. Please review the CSR as well:
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8280495
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8280494
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> More update for the sec and impl
Changes requested by mullan (Reviewer).
src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 713:
> 711: * in this list or may not use the recommended name for a certain
> 712: * signature scheme.
> 713: *
Regarding my previous comment about providers not supporting these new methods,
I think something like the following might suffice:
@apiNote Note that a provider may not have been updated to support this method
and in that case may return null instead of the default signature schemes for
pre-populated objects.
@implNote The SunJSSE provider supports this method.
src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 735:
> 733: * {@link #setSignatureSchemes} method has not been called, this
> method
> 734: * should return the default signature schemes for connection
> populated
> 735: * objects, or {@code null} for pre-populated objects.
I think this sentence should be part of the specification and not the
implementation note since ideally you would want all provider implementations
to behave this way (assuming they supported this method):
"If the {@link #setSignatureSchemes} method has not been called, this method
should return the default signature schemes for connection populated objects,
or {@code null} for pre-populated objects."
src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 752:
> 750:
> 751: /**
> 752: * Sets the prioritized array of signature scheme names that
If the system properties are set, does it override this API? I think it is
important to mention that here in an @implNote. I know you discuss them in
`getSignatureSchemes` but I think a developer is more likely to want to know
the behavior of the properties for this method especially if they will override
this setting.
src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 761:
> 759: * Names Specification. Providers may support signature schemes not
> defined
> 760: * in this list or may not use the recommended name for a certain
> 761: * signature scheme.
Did you want to say something about what the behavior should be if a provider
does not support one of these schemes? Is it ignored, or is an exception thrown?
src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 762:
> 760: * in this list or may not use the recommended name for a certain
> 761: * signature scheme.
> 762: *
Regarding my previous comment about providers not supporting these new methods,
I think something like the following might suffice:
@apiNote Note that a provider may not have been updated to support this method
and in that case may ignore the schemes that are set.
@implNote The SunJSSE provider supports this method.
-
PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Thu, 3 Feb 2022 21:30:59 GMT, Sean Mullan wrote:
> > > On a related issue, have you given any thought as to what the behavior
> > > should be if a 3rd-party JSSE provider is not updated to support these
> > > new methods? I don't know of a good way to address that since the API is
> > > not part of the provider implementation. We need a way to query what
> > > parameters a given provider supports, I think.
> >
> >
> > It's a good question. A 3rd party provider need to support to the new APIs.
> > Otherwise, the spec does not really work except to use the default values.
> > It might be overweighted if the Java SE API has to detect the
> > implementation details.
>
> Fair point. However, I think then we have to make some changes to the
> specification wording that state that these new methods depend on support
> from the underlying provider. In other words something like
> `SSLParameters.setSignatureSchemes(new String[] {"ecdsa_secp521r1_sha512"})`
> could be silently ignored if the provider doesn't support the new method.
> It's unfortunate because a developer may miss this point or it may go
> undetected, whereas throwing an `UnsupportedOperationException` would
> probably be detected (but which won't work for this API). I do note that
> previous methods we have added to SSLParameters have probably had this same
> issue. But I think we should try to address it better with these methods.
> I'll suggest some wording changes.
>
> To help reduce this potential issue, we can make sure we mention this issue
> in the release notes, and encourage 3rd party providers to add support for
> these methods when they add support for JDK 18.
>
> Open to other ideas though.
Good points. I will try to add an apiNote.
-
PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Wed, 2 Feb 2022 22:41:56 GMT, Xue-Lei Andrew Fan wrote:
> > On a related issue, have you given any thought as to what the behavior
> > should be if a 3rd-party JSSE provider is not updated to support these new
> > methods? I don't know of a good way to address that since the API is not
> > part of the provider implementation. We need a way to query what parameters
> > a given provider supports, I think.
>
> It's a good question. A 3rd party provider need to support to the new APIs.
> Otherwise, the spec does not really work except to use the default values. It
> might be overweighted if the Java SE API has to detect the implementation
> details.
Fair point. However, I think then we have to make some changes to the
specification wording that state that these new methods depend on support from
the underlying provider. In other words something like
`SSLParameters.setSignatureSchemes(new String[] {"ecdsa_secp521r1_sha512"})`
could be silently ignored if the provider doesn't support the new method. It's
unfortunate because a developer may miss this point or it may go undetected,
whereas throwing an `UnsupportedOperationException` would probably be detected
(but which won't work for this API). I do note that previous methods we have
added to SSLParameters have probably had this same issue. But I think we should
try to address it better with these methods. I'll suggest some wording changes.
To help reduce this potential issue, we can make sure we mention this issue in
the release notes, and encourage 3rd party providers to add support for these
methods when they add support for JDK 18.
Open to other ideas though.
-
PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Wed, 2 Feb 2022 22:13:07 GMT, Sean Mullan wrote: > On a related issue, have you given any thought as to what the behavior should > be if a 3rd-party JSSE provider is not updated to support these new methods? > I don't know of a good way to address that since the API is not part of the > provider implementation. We need a way to query what parameters a given > provider supports, I think. It's a good question. A 3rd party provider need to support to the new APIs. Otherwise, the spec does not really work except to use the default values. It might be overweighted if the Java SE API has to detect the implementation details. - PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Wed, 2 Feb 2022 19:12:56 GMT, Xue-Lei Andrew Fan wrote: >> This update is to support signature schemes customization for individual >> (D)TLS connection. Please review the CSR as well: >> CSR: https://bugs.openjdk.java.net/browse/JDK-8280495 >> RFE: https://bugs.openjdk.java.net/browse/JDK-8280494 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More update for the sec and impl > - PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Wed, 2 Feb 2022 19:12:56 GMT, Xue-Lei Andrew Fan wrote: >> This update is to support signature schemes customization for individual >> (D)TLS connection. Please review the CSR as well: >> CSR: https://bugs.openjdk.java.net/browse/JDK-8280495 >> RFE: https://bugs.openjdk.java.net/browse/JDK-8280494 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More update for the sec and impl On a related issue, have you given any thought as to what the behavior should be if a 3rd-party JSSE provider is not updated to support these new methods? I don't know of a good way to address that since the API is not part of the provider implementation. We need a way to query what parameters a given provider supports, I think. - PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
On Wed, 2 Feb 2022 19:12:56 GMT, Xue-Lei Andrew Fan wrote: >> This update is to support signature schemes customization for individual >> (D)TLS connection. Please review the CSR as well: >> CSR: https://bugs.openjdk.java.net/browse/JDK-8280495 >> RFE: https://bugs.openjdk.java.net/browse/JDK-8280494 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More update for the sec and impl Ok, great. I'll need another day or so to review the implementation changes. - PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v9]
> This update is to support signature schemes customization for individual > (D)TLS connection. Please review the CSR as well: > CSR: https://bugs.openjdk.java.net/browse/JDK-8280495 > RFE: https://bugs.openjdk.java.net/browse/JDK-8280494 Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: More update for the sec and impl - Changes: - all: https://git.openjdk.java.net/jdk/pull/7252/files - new: https://git.openjdk.java.net/jdk/pull/7252/files/b5161f75..6b74767e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7252&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7252&range=07-08 Stats: 71 lines in 4 files changed: 22 ins; 14 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/7252.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7252/head:pull/7252 PR: https://git.openjdk.java.net/jdk/pull/7252
