Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Wed, 2 Dec 2020 15:33:20 GMT, Jamil Nimeh wrote: >> SunEC's algorithm name for keys are always "EdDSA", but I know BC returns >> "Ed25519" or "Ed448". > > Filed and took ownership of JDK-8257607 to address BC JCE provider issues for > both XDH and EdDSA when used with SunJSSE. Also, specific to this particular algorithm the signature type can be EdDSA for both BC and SunJCE, regardless of if the key type is EdDSA (SunJCE) or Ed25519 or Ed448 (BC). Creating a signature with algorithm EdDSA and the use of the key from either provider will perform a signature of the proper kind. - PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Fri, 20 Nov 2020 20:05:09 GMT, Weijun Wang wrote: >> src/java.base/share/classes/sun/security/ssl/JsseJce.java line 97: >> >>> 95: */ >>> 96: static final String SIGNATURE_EDDSA = "EdDSA"; >>> 97: >> >> Please update the copyright year. >> >> Is it possible that "ed25519" or "ed448" is used as the signature algorithm, >> especially in the X.509 certificate implementation? > > SunEC's algorithm name for keys are always "EdDSA", but I know BC returns > "Ed25519" or "Ed448". Filed and took ownership of JDK-8257607 to address BC JCE provider issues for both XDH and EdDSA when used with SunJSSE. - PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Fri, 20 Nov 2020 20:22:33 GMT, Jamil Nimeh wrote:
>> src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line
>> 139:
>>
>>> 137: if (cct.isAvailable) {
>>> 138: cct.keyAlgorithm.forEach(key -> {
>>> 139: if (!keyTypes.contains(key)) {
>>
>> Can this ever happen? Why not just `addAll`?
>
> I wanted to make sure we didn't end up with duplicates if two different
> certificate types had the same underlying key type and also to deal with both
> well-formed certificate request messages or ones that erroneously reiterate
> the type. It is definitely an edge case.
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Fri, 20 Nov 2020 20:39:46 GMT, Xue-Lei Andrew Fan wrote: >> JsseJce.isEcAvailable doesn't check for EdDSA availability so I'm not sure >> we want that second clause. I don't think the EdDSA code is implemented in >> the same module as the other EC code is so I don't know if we'd want to >> extend isEcAvailable to include EdDSA. I'll need to go look to see where >> EdDSA is located relative to the EC code. > > Hm, maybe a new isEdDsaAvailable(). The reasons for having isEcAvailable for EC keys do not apply for EdDSA. I don't think we should make any changes to this part of the code right now. - PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Fri, 20 Nov 2020 20:12:47 GMT, Jamil Nimeh wrote:
>> src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 73:
>>
>>> 71: ED448 (0x0808, "ed448", "Ed448",
>>> 72: "EdDSA",
>>> 73: ProtocolVersion.PROTOCOLS_12_13),
>>
>> You may also want to check if EdDSA is available in the following block:
>> - 282 if ("EC".equals(keyAlgorithm)) {
>> + 282 if ("EC".equals(keyAlgorithm) || "EdDSA"... ) {
>> 283 mediator = JsseJce.isEcAvailable();
>> 284 }
>
> JsseJce.isEcAvailable doesn't check for EdDSA availability so I'm not sure we
> want that second clause. I don't think the EdDSA code is implemented in the
> same module as the other EC code is so I don't know if we'd want to extend
> isEcAvailable to include EdDSA. I'll need to go look to see where EdDSA is
> located relative to the EC code.
Hm, maybe a new isEdDsaAvailable().
-
PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Fri, 20 Nov 2020 19:58:23 GMT, Weijun Wang wrote:
>> Jamil Nimeh has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Update test to account for JDK-8202343 fix
>> - Merge
>> - Merge
>> - Applied code review comments to tests
>> - Fix cut/paste error with ECDH-RSA key exchange
>> - Merge
>> - Initial EdDSA/TLS solution
>
> src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 139:
>
>> 137: if (cct.isAvailable) {
>> 138: cct.keyAlgorithm.forEach(key -> {
>> 139: if (!keyTypes.contains(key)) {
>
> Can this ever happen? Why not just `addAll`?
I wanted to make sure we didn't end up with duplicates if two different
certificate types had the same underlying key type and also to deal with both
well-formed certificate request messages or ones that erroneously reiterate the
type. It is definitely an edge case.
-
PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Fri, 20 Nov 2020 18:37:36 GMT, Xue-Lei Andrew Fan wrote:
>> Jamil Nimeh has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Update test to account for JDK-8202343 fix
>> - Merge
>> - Merge
>> - Applied code review comments to tests
>> - Fix cut/paste error with ECDH-RSA key exchange
>> - Merge
>> - Initial EdDSA/TLS solution
>
> src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 73:
>
>> 71: ED448 (0x0808, "ed448", "Ed448",
>> 72: "EdDSA",
>> 73: ProtocolVersion.PROTOCOLS_12_13),
>
> You may also want to check if EdDSA is available in the following block:
> - 282 if ("EC".equals(keyAlgorithm)) {
> + 282 if ("EC".equals(keyAlgorithm) || "EdDSA"... ) {
> 283 mediator = JsseJce.isEcAvailable();
> 284 }
JsseJce.isEcAvailable doesn't check for EdDSA availability so I'm not sure we
want that second clause. I don't think the EdDSA code is implemented in the
same module as the other EC code is so I don't know if we'd want to extend
isEcAvailable to include EdDSA. I'll need to go look to see where EdDSA is
located relative to the EC code.
> src/java.base/share/classes/sun/security/ssl/SSLKeyExchange.java line 48:
>
>> 46: if (authentication != null) {
>> 47: this.authentication = new ArrayList<>();
>> 48: this.authentication.addAll(authentication);
>
> I may use an immutable list, for example List.copyOf(authentication).
That's probably a good idea. It doesn't look like we're making changes to the
List outside of the constructor.
> src/java.base/share/classes/sun/security/ssl/SSLKeyExchange.java line 134:
>
>> 132: SSLAuthentication authType = li.next();
>> 133: auHandshakes =
>> authType.getRelatedHandshakers(handshakeContext);
>> 134: }
>
> I guess for-each loop could be a little bit more lightweight.
Yeah, I thought about that. I just like having all the loop termination
clauses in one place rather than testing in the loop body and breaking. I can
change it to for-each though, no strong feelings one way or the other.
> src/java.base/share/classes/sun/security/ssl/SSLKeyExchange.java line 163:
>
>> 161: SSLAuthentication authType = li.next();
>> 162: auProducers =
>> authType.getHandshakeProducers(handshakeContext);
>> 163: }
>
> Use for-each?
Will change to for-each.
-
PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Fri, 20 Nov 2020 17:31:20 GMT, Xue-Lei Andrew Fan wrote:
>> Jamil Nimeh has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Update test to account for JDK-8202343 fix
>> - Merge
>> - Merge
>> - Applied code review comments to tests
>> - Fix cut/paste error with ECDH-RSA key exchange
>> - Merge
>> - Initial EdDSA/TLS solution
>
> src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 83:
>
>> 81: ECDSA_SIGN ((byte)0x40, "ecdsa_sign",
>> 82: List.of("EC", "EdDSA"),
>> 83: JsseJce.isEcAvailable()),
>
> Would you like to add RFC 8422 to the comment line as well?
I think that's a good idea.
-
PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Fri, 20 Nov 2020 18:09:26 GMT, Xue-Lei Andrew Fan wrote: >> Jamil Nimeh has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - Update test to account for JDK-8202343 fix >> - Merge >> - Merge >> - Applied code review comments to tests >> - Fix cut/paste error with ECDH-RSA key exchange >> - Merge >> - Initial EdDSA/TLS solution > > src/java.base/share/classes/sun/security/ssl/JsseJce.java line 97: > >> 95: */ >> 96: static final String SIGNATURE_EDDSA = "EdDSA"; >> 97: > > Please update the copyright year. > > Is it possible that "ed25519" or "ed448" is used as the signature algorithm, > especially in the X.509 certificate implementation? SunEC's algorithm name for keys are always "EdDSA", but I know BC returns "Ed25519" or "Ed448". - PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Thu, 19 Nov 2020 17:48:34 GMT, Jamil Nimeh wrote:
>> Hello all,
>> This change brings in support for certificates with EdDSA keys (both Ed25519
>> and Ed448) allowing those signature algorithms to be used both on the
>> certificates themselves and used during the handshaking process for messages
>> like CertificateVerify, ServerKeyExchange and so forth.
>
> Jamil Nimeh has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - Update test to account for JDK-8202343 fix
> - Merge
> - Merge
> - Applied code review comments to tests
> - Fix cut/paste error with ECDH-RSA key exchange
> - Merge
> - Initial EdDSA/TLS solution
src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 139:
> 137: if (cct.isAvailable) {
> 138: cct.keyAlgorithm.forEach(key -> {
> 139: if (!keyTypes.contains(key)) {
Can this ever happen? Why not just `addAll`?
-
PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
On Thu, 19 Nov 2020 17:48:34 GMT, Jamil Nimeh wrote:
>> Hello all,
>> This change brings in support for certificates with EdDSA keys (both Ed25519
>> and Ed448) allowing those signature algorithms to be used both on the
>> certificates themselves and used during the handshaking process for messages
>> like CertificateVerify, ServerKeyExchange and so forth.
>
> Jamil Nimeh has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - Update test to account for JDK-8202343 fix
> - Merge
> - Merge
> - Applied code review comments to tests
> - Fix cut/paste error with ECDH-RSA key exchange
> - Merge
> - Initial EdDSA/TLS solution
src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 83:
> 81: ECDSA_SIGN ((byte)0x40, "ecdsa_sign",
> 82: List.of("EC", "EdDSA"),
> 83: JsseJce.isEcAvailable()),
Would you like to add RFC 8422 to the comment line as well?
src/java.base/share/classes/sun/security/ssl/JsseJce.java line 97:
> 95: */
> 96: static final String SIGNATURE_EDDSA = "EdDSA";
> 97:
Please update the copyright year.
Is it possible that "ed25519" or "ed448" is used as the signature algorithm,
especially in the X.509 certificate implementation?
src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 73:
> 71: ED448 (0x0808, "ed448", "Ed448",
> 72: "EdDSA",
> 73: ProtocolVersion.PROTOCOLS_12_13),
You may also want to check if EdDSA is available in the following block:
- 282 if ("EC".equals(keyAlgorithm)) {
+ 282 if ("EC".equals(keyAlgorithm) || "EdDSA"... ) {
283 mediator = JsseJce.isEcAvailable();
284 }
src/java.base/share/classes/sun/security/ssl/SSLKeyExchange.java line 48:
> 46: if (authentication != null) {
> 47: this.authentication = new ArrayList<>();
> 48: this.authentication.addAll(authentication);
I may use an immutable list, for example List.copyOf(authentication).
src/java.base/share/classes/sun/security/ssl/SSLKeyExchange.java line 134:
> 132: SSLAuthentication authType = li.next();
> 133: auHandshakes =
> authType.getRelatedHandshakers(handshakeContext);
> 134: }
I guess for-each loop could be a little bit more lightweight.
src/java.base/share/classes/sun/security/ssl/SSLKeyExchange.java line 163:
> 161: SSLAuthentication authType = li.next();
> 162: auProducers =
> authType.getHandshakeProducers(handshakeContext);
> 163: }
Use for-each?
-
PR: https://git.openjdk.java.net/jdk/pull/1197
Re: RFR: JDK-8166596: TLS support for the EdDSA signature algorithm [v3]
> Hello all, > This change brings in support for certificates with EdDSA keys (both Ed25519 > and Ed448) allowing those signature algorithms to be used both on the > certificates themselves and used during the handshaking process for messages > like CertificateVerify, ServerKeyExchange and so forth. Jamil Nimeh has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Update test to account for JDK-8202343 fix - Merge - Merge - Applied code review comments to tests - Fix cut/paste error with ECDH-RSA key exchange - Merge - Initial EdDSA/TLS solution - Changes: - all: https://git.openjdk.java.net/jdk/pull/1197/files - new: https://git.openjdk.java.net/jdk/pull/1197/files/59aced35..dee70944 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1197&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1197&range=01-02 Stats: 6780 lines in 253 files changed: 3650 ins; 1976 del; 1154 mod Patch: https://git.openjdk.java.net/jdk/pull/1197.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1197/head:pull/1197 PR: https://git.openjdk.java.net/jdk/pull/1197
