RFR: 8255867: SignatureScheme JSSE property does not preserve ordering in handshake messages

[Jamil Nimeh-2]

This fix adjusts the SunJSSE provider's handling of the jdk.tls.[client | server].SignatureSchemes property and its effect on messages that assert the signature_algorithms and signature_algorithms_cert extensions, or supported_signature_algorithms vectors like those used in TLS 1.2 CertificateRequest messages.  With this change, the ordering of the signature algorithms in the property value will be preserved in the ordering as integer identifiers in the messages itself.  Prior to this fix the property algorithms will be asserted, but in the order as shown in the sun.security.ssl.SignatureAlgorithms enumeration.

This does not affect the default ordering of these signature schemes when the property is not given a value.

JBS: https://bugs.openjdk.java.net/browse/JDK-8255867

-------------

Commit messages:
 - 8255867: SignatureScheme JSSE property does not preserve ordering in handshake messages

Changes: https://git.openjdk.java.net/jdk/pull/2658/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2658&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255867
  Stats: 384 lines in 2 files changed: 380 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2658.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2658/head:pull/2658

PR: https://git.openjdk.java.net/jdk/pull/2658

[Jamil Nimeh-2]

On Sat, 20 Feb 2021 01:56:37 GMT, Jamil Nimeh <[hidden email]> wrote:

> This fix adjusts the SunJSSE provider's handling of the jdk.tls.[client | server].SignatureSchemes property and its effect on messages that assert the signature_algorithms and signature_algorithms_cert extensions, or supported_signature_algorithms vectors like those used in TLS 1.2 CertificateRequest messages.  With this change, the ordering of the signature algorithms in the property value will be preserved in the ordering as integer identifiers in the messages itself.  Prior to this fix the property algorithms will be asserted, but in the order as shown in the sun.security.ssl.SignatureAlgorithms enumeration.
>
> This does not affect the default ordering of these signature schemes when the property is not given a value.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8255867

src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 43:

> 41: import java.util.Map;
> 42: import java.util.Set;
> 43: import java.util.stream.Collectors;

Remnant from an earlier attempt at a fix, will remove.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2658

[Jamil Nimeh-2]

> This fix adjusts the SunJSSE provider's handling of the jdk.tls.[client | server].SignatureSchemes property and its effect on messages that assert the signature_algorithms and signature_algorithms_cert extensions, or supported_signature_algorithms vectors like those used in TLS 1.2 CertificateRequest messages.  With this change, the ordering of the signature algorithms in the property value will be preserved in the ordering as integer identifiers in the messages itself.  Prior to this fix the property algorithms will be asserted, but in the order as shown in the sun.security.ssl.SignatureAlgorithms enumeration.
>
> This does not affect the default ordering of these signature schemes when the property is not given a value.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8255867

Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision:

  Remove unnecessary import

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2658/files
  - new: https://git.openjdk.java.net/jdk/pull/2658/files/49247253..9b9d967f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2658&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2658&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2658.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2658/head:pull/2658

PR: https://git.openjdk.java.net/jdk/pull/2658

[Xue-Lei Andrew Fan]

On Mon, 22 Feb 2021 17:07:58 GMT, Jamil Nimeh <[hidden email]> wrote:

>> This fix adjusts the SunJSSE provider's handling of the jdk.tls.[client | server].SignatureSchemes property and its effect on messages that assert the signature_algorithms and signature_algorithms_cert extensions, or supported_signature_algorithms vectors like those used in TLS 1.2 CertificateRequest messages.  With this change, the ordering of the signature algorithms in the property value will be preserved in the ordering as integer identifiers in the messages itself.  Prior to this fix the property algorithms will be asserted, but in the order as shown in the sun.security.ssl.SignatureAlgorithms enumeration.
>>
>> This does not affect the default ordering of these signature schemes when the property is not given a value.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8255867
>
> Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision:
>
>   Remove unnecessary import

test/jdk/sun/security/ssl/SignatureScheme/SigSchemePropOrdering.java line 90:

> 88:     private static final String SIG_SCHEME_STR =
> 89:             "rsa_pkcs1_sha256,rsa_pss_rsae_sha256,rsa_pss_pss_sha256," +
> 90:             "ed448,ed25519,ecdsa_secp256r1_sha256";

It have been a while that we are trying to avoid the use the binary keystore files in test.  It would be nice that if new test cases could use the javax/net/ssl/templates/javax/net/ssl/templates instead.

src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 387:

> 385:                 config.signatureSchemes.isEmpty() ?
> 386:                 Arrays.asList(SignatureScheme.values()) :
> 387:                 config.signatureSchemes;

I would like to have two more indents for the '?' operator.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2658

[Jamil Nimeh-2]

On Mon, 22 Feb 2021 18:56:10 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

>> Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Remove unnecessary import
>
> test/jdk/sun/security/ssl/SignatureScheme/SigSchemePropOrdering.java line 90:
>
>> 88:     private static final String SIG_SCHEME_STR =
>> 89:             "rsa_pkcs1_sha256,rsa_pss_rsae_sha256,rsa_pss_pss_sha256," +
>> 90:             "ed448,ed25519,ecdsa_secp256r1_sha256";
>
> It have been a while that we are trying to avoid the use the binary keystore files in test.  It would be nice that if new test cases could use the javax/net/ssl/templates/javax/net/ssl/templates instead.

I should be able to rework it into an extension of SSLEngineTemplate.

> src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 387:
>
>> 385:                 config.signatureSchemes.isEmpty() ?
>> 386:                 Arrays.asList(SignatureScheme.values()) :
>> 387:                 config.signatureSchemes;
>
> I would like to have two more indents for the '?' operator.

No problem, will do.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2658

[Jamil Nimeh-2]

> This fix adjusts the SunJSSE provider's handling of the jdk.tls.[client | server].SignatureSchemes property and its effect on messages that assert the signature_algorithms and signature_algorithms_cert extensions, or supported_signature_algorithms vectors like those used in TLS 1.2 CertificateRequest messages.  With this change, the ordering of the signature algorithms in the property value will be preserved in the ordering as integer identifiers in the messages itself.  Prior to this fix the property algorithms will be asserted, but in the order as shown in the sun.security.ssl.SignatureAlgorithms enumeration.
>
> This does not affect the default ordering of these signature schemes when the property is not given a value.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8255867

Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision:

  Fix indentation, make test a derivation of SSLEngineTemplate

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2658/files
  - new: https://git.openjdk.java.net/jdk/pull/2658/files/9b9d967f..20498ee3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2658&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2658&range=01-02

  Stats: 88 lines in 3 files changed: 12 ins; 49 del; 27 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2658.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2658/head:pull/2658

PR: https://git.openjdk.java.net/jdk/pull/2658

[Xue-Lei Andrew Fan]

On Mon, 22 Feb 2021 21:28:57 GMT, Jamil Nimeh <[hidden email]> wrote:

>> This fix adjusts the SunJSSE provider's handling of the jdk.tls.[client | server].SignatureSchemes property and its effect on messages that assert the signature_algorithms and signature_algorithms_cert extensions, or supported_signature_algorithms vectors like those used in TLS 1.2 CertificateRequest messages.  With this change, the ordering of the signature algorithms in the property value will be preserved in the ordering as integer identifiers in the messages itself.  Prior to this fix the property algorithms will be asserted, but in the order as shown in the sun.security.ssl.SignatureAlgorithms enumeration.
>>
>> This does not affect the default ordering of these signature schemes when the property is not given a value.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8255867
>
> Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix indentation, make test a derivation of SSLEngineTemplate

Looks good to me.  Thanks!

-------------

Marked as reviewed by xuelei (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2658

[Jamil Nimeh-2]

On Sat, 20 Feb 2021 01:56:37 GMT, Jamil Nimeh <[hidden email]> wrote:

> This fix adjusts the SunJSSE provider's handling of the jdk.tls.[client | server].SignatureSchemes property and its effect on messages that assert the signature_algorithms and signature_algorithms_cert extensions, or supported_signature_algorithms vectors like those used in TLS 1.2 CertificateRequest messages.  With this change, the ordering of the signature algorithms in the property value will be preserved in the ordering as integer identifiers in the messages itself.  Prior to this fix the property algorithms will be asserted, but in the order as shown in the sun.security.ssl.SignatureAlgorithms enumeration.
>
> This does not affect the default ordering of these signature schemes when the property is not given a value.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8255867

This pull request has now been integrated.

Changeset: a30fb4fc
Author:    Jamil Nimeh <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/a30fb4fc
Stats:     356 lines in 3 files changed: 342 ins; 0 del; 14 mod

8255867: SignatureScheme JSSE property does not preserve ordering in handshake messages

Reviewed-by: xuelei

-------------

PR: https://git.openjdk.java.net/jdk/pull/2658