[go] crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in RFC5289

Jacob Haven (Gerrit)

unread,

Jan 24, 2015, 5:17:16 PM1/24/15

to Ian Lance Taylor, Adam Langley, golang-co...@googlegroups.com

Jacob Haven uploaded a change:
https://go-review.googlesource.com/3265

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Generalizes PRF calculation for TLS 1.2 to support arbitrary hashes
(SHA-384 instead of SHA-256).
Testdata were all updated to correspond with the new cipher suites in the
handshake.

Change-Id: I3d9fc48c19d1043899e38255a53c80dc952ee08f
---
M src/crypto/tls/cipher_suites.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_client_test.go
M src/crypto/tls/handshake_server.go
M src/crypto/tls/handshake_server_test.go
M src/crypto/tls/prf.go
M src/crypto/tls/prf_test.go
M src/crypto/tls/testdata/Client-TLSv10-ClientCert-ECDSA-ECDSA
M src/crypto/tls/testdata/Client-TLSv10-ClientCert-ECDSA-RSA
M src/crypto/tls/testdata/Client-TLSv10-ClientCert-RSA-ECDSA
M src/crypto/tls/testdata/Client-TLSv10-ClientCert-RSA-RSA
M src/crypto/tls/testdata/Client-TLSv10-ECDHE-ECDSA-AES
M src/crypto/tls/testdata/Client-TLSv10-ECDHE-RSA-AES
M src/crypto/tls/testdata/Client-TLSv10-RSA-RC4
M src/crypto/tls/testdata/Client-TLSv11-ECDHE-ECDSA-AES
M src/crypto/tls/testdata/Client-TLSv11-ECDHE-RSA-AES
M src/crypto/tls/testdata/Client-TLSv11-RSA-RC4
M src/crypto/tls/testdata/Client-TLSv12-ALPN
M src/crypto/tls/testdata/Client-TLSv12-ALPN-NoMatch
M src/crypto/tls/testdata/Client-TLSv12-ClientCert-ECDSA-ECDSA
M src/crypto/tls/testdata/Client-TLSv12-ClientCert-ECDSA-RSA
M src/crypto/tls/testdata/Client-TLSv12-ClientCert-RSA-ECDSA
M src/crypto/tls/testdata/Client-TLSv12-ClientCert-RSA-RSA
M src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES
A src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES-256-GCM-384
M src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES-GCM
A src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES256-GCM-SHA384
M src/crypto/tls/testdata/Client-TLSv12-ECDHE-RSA-AES
M src/crypto/tls/testdata/Client-TLSv12-RSA-RC4
M src/crypto/tls/testdata/Server-SSLv3-RSA-3DES
M src/crypto/tls/testdata/Server-SSLv3-RSA-AES
M src/crypto/tls/testdata/Server-SSLv3-RSA-RC4
M src/crypto/tls/testdata/Server-TLSv10-ECDHE-ECDSA-AES
M src/crypto/tls/testdata/Server-TLSv10-RSA-3DES
M src/crypto/tls/testdata/Server-TLSv10-RSA-AES
M src/crypto/tls/testdata/Server-TLSv10-RSA-RC4
M src/crypto/tls/testdata/Server-TLSv11-FallbackSCSV
M src/crypto/tls/testdata/Server-TLSv11-RSA-RC4
M src/crypto/tls/testdata/Server-TLSv12-ALPN
M src/crypto/tls/testdata/Server-TLSv12-ALPN-NoMatch
M src/crypto/tls/testdata/Server-TLSv12-CipherSuiteCertPreferenceECDSA
M src/crypto/tls/testdata/Server-TLSv12-CipherSuiteCertPreferenceRSA
M src/crypto/tls/testdata/Server-TLSv12-ClientAuthRequestedAndECDSAGiven
M src/crypto/tls/testdata/Server-TLSv12-ClientAuthRequestedAndGiven
M src/crypto/tls/testdata/Server-TLSv12-ClientAuthRequestedNotGiven
M src/crypto/tls/testdata/Server-TLSv12-ECDHE-ECDSA-AES
M src/crypto/tls/testdata/Server-TLSv12-IssueTicket
M src/crypto/tls/testdata/Server-TLSv12-IssueTicketPreDisable
M src/crypto/tls/testdata/Server-TLSv12-RSA-3DES
M src/crypto/tls/testdata/Server-TLSv12-RSA-AES
M src/crypto/tls/testdata/Server-TLSv12-RSA-AES-GCM
A src/crypto/tls/testdata/Server-TLSv12-RSA-AES256-GCM-SHA384
M src/crypto/tls/testdata/Server-TLSv12-RSA-RC4
M src/crypto/tls/testdata/Server-TLSv12-Resume
M src/crypto/tls/testdata/Server-TLSv12-ResumeDisabled
M src/crypto/tls/testdata/Server-TLSv12-SNI
56 files changed, 1,985 insertions(+), 1,758 deletions(-)

--
https://go-review.googlesource.com/3265

Adam Langley (Gerrit)

unread,

Jan 27, 2015, 8:50:10 PM1/27/15

to Jacob Haven, golang-co...@googlegroups.com

Adam Langley has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 1:

(7 comments)

https://go-review.googlesource.com/#/c/3265/1/src/crypto/tls/handshake_server.go
File src/crypto/tls/handshake_server.go:

Line 113: defer func() {
Wouldn't it be much easier to setup the finishedHash after the call to
readClientHello() in serverHandshake()?

Line 253: var sessionTicket = append([]uint8{},
hs.clientHello.sessionTicket...)
What's the motivation for this?

https://go-review.googlesource.com/#/c/3265/1/src/crypto/tls/prf.go
File src/crypto/tls/prf.go:

Line 67: // as defined in RFC 5246, section 5, with hash determinted by
cipher suite.
"with the given hash" -- the cipher suite doesn't get to this function.

Line 121: hash
tls12Hash

Line 136: hash
tls12Hash

Line 148: hash
tls12Hash

Line 170: hash
tls12Hash

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-HasComments: Yes

Jacob Haven (Gerrit)

unread,

Jan 28, 2015, 5:25:22 PM1/28/15

to Adam Langley, golang-co...@googlegroups.com

Reviewers: Adam Langley

Jacob Haven uploaded a new patch set:

56 files changed, 1,922 insertions(+), 1,713 deletions(-)

Jacob Haven (Gerrit)

unread,

Jan 28, 2015, 5:33:26 PM1/28/15

to Adam Langley, golang-co...@googlegroups.com

Reviewers: Adam Langley

Jacob Haven uploaded a new patch set:

56 files changed, 1,918 insertions(+), 1,712 deletions(-)

David Gil (Gerrit)

unread,

Jan 29, 2015, 4:38:17 PM1/29/15

to Jacob Haven, Adam Langley, golang-co...@googlegroups.com

David Gil has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 3:

(2 comments)

https://go-review.googlesource.com/#/c/3265/3/src/crypto/tls/cipher_suites.go
File src/crypto/tls/cipher_suites.go:

Line 257: func hashForCipherSuite(ciphersuite uint16) crypto.Hash {
It seems like it would look cleaner to replace this with a field in the
cipherSuite struct.

https://go-review.googlesource.com/#/c/3265/3/src/crypto/tls/prf.go
File src/crypto/tls/prf.go:

Line 67: determinted
In general: s/determinted/determined/

But, AGL's suggestion was:

s/determinted by the given hash/with the given hash/

And I'd prefer "using the given hash".

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>

Gerrit-Reviewer: David Gil <dg...@yahoo-inc.com>
Gerrit-HasComments: Yes

Jacob Haven (Gerrit)

unread,

Jan 29, 2015, 5:15:04 PM1/29/15

to David Gil, Adam Langley, golang-co...@googlegroups.com

Jacob Haven uploaded a new patch set:

56 files changed, 1,927 insertions(+), 1,729 deletions(-)

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 4:00:40 PM2/3/15

to Adam Langley, David Gil, golang-co...@googlegroups.com

Jacob Haven has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 4:

Are there any further comments on this change? Anything holding it back
from being merged?

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: David Gil <dg...@yahoo-inc.com>

Gerrit-Reviewer: Jacob Haven <ja...@cloudflare.com>
Gerrit-HasComments: No

Adam Langley (Gerrit)

unread,

Feb 3, 2015, 4:49:27 PM2/3/15

to Jacob Haven, David Gil, golang-co...@googlegroups.com

Adam Langley has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 4:

> Are there any further comments on this change? Anything holding it
> back from being merged?

You've updated the code to reflect the feedback, but haven't posted replies
to anything. Are they sitting as drafts perchance?

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 4:59:21 PM2/3/15

to Adam Langley, David Gil, golang-co...@googlegroups.com

Jacob Haven has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 1:

(7 comments)

https://go-review.googlesource.com/#/c/3265/1/src/crypto/tls/handshake_server.go
File src/crypto/tls/handshake_server.go:

Line 113: defer func() {
> Wouldn't it be much easier to setup the finishedHash after the call to
> read

Sure. I just wanted to keep thing in the same function/keep serverHandshake
simple, but that removes the need for the hs.suite!=nil check so it's
probably better to move it.

Line 253: var sessionTicket = append([]uint8{},
hs.clientHello.sessionTicket...)
> What's the motivation for this?

c.decryptTicket destructively modifies the sessionTicket. Thus, if we don't
make a copy, the the clientHello will be modified and we won't be able to
write it out correctly to the finishedHash later.

https://go-review.googlesource.com/#/c/3265/1/src/crypto/tls/prf.go
File src/crypto/tls/prf.go:

Line 67: // as defined in RFC 5246, section 5, with hash determinted by
cipher suite.
> "with the given hash" -- the cipher suite doesn't get to this function.

Done

Line 121: hash
> tls12Hash
Done

Line 136: hash
> tls12Hash
Done

Line 148: hash
> tls12Hash
Done

Line 170: hash
> tls12Hash
Done

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: David Gil <dg...@yahoo-inc.com>
Gerrit-Reviewer: Jacob Haven <ja...@cloudflare.com>

Gerrit-HasComments: Yes

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 5:00:04 PM2/3/15

to Adam Langley, David Gil, golang-co...@googlegroups.com

Jacob Haven has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 3:

(2 comments)

https://go-review.googlesource.com/#/c/3265/3/src/crypto/tls/cipher_suites.go
File src/crypto/tls/cipher_suites.go:

Line 257: func hashForCipherSuite(ciphersuite uint16) crypto.Hash {
> It seems like it would look cleaner to replace this with a field in the
> cip

Perhaps. It's only used for TLS1.2 PRF determination, though. For all
existing (non-SHA-384) cipher suites, it requires repeating `crypto.SHA256`
as a new `tls12Hash` field in the cipherSuite struct.

https://go-review.googlesource.com/#/c/3265/3/src/crypto/tls/prf.go
File src/crypto/tls/prf.go:

Line 67: determinted
> In general: s/determinted/determined/

I agree, "using" sounds best.

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 5:01:18 PM2/3/15

to Adam Langley, David Gil, golang-co...@googlegroups.com

Jacob Haven has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 3:

> > Are there any further comments on this change? Anything holding
> it
> > back from being merged?
>
> You've updated the code to reflect the feedback, but haven't posted
> replies to anything. Are they sitting as drafts perchance?

Indeed they were. Thanks, I was sitting here wondering why there were no
replies...

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: David Gil <dg...@yahoo-inc.com>
Gerrit-Reviewer: Jacob Haven <ja...@cloudflare.com>

Gerrit-HasComments: No

Adam Langley (Gerrit)

unread,

Feb 3, 2015, 5:19:44 PM2/3/15

to Jacob Haven, David Gil, golang-co...@googlegroups.com

Adam Langley has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 4: Code-Review+1

(1 comment)

Looks good apart from one nit in the tests.

https://go-review.googlesource.com/#/c/3265/4/src/crypto/tls/handshake_server_test.go
File src/crypto/tls/handshake_server_test.go:

Line 570: command:
[]string{"openssl", "s_client", "-no_ticket", "-cipher", "ECDHE-RSA-AES128-GCM-SHA256"},
Should be AES256 in the command line here.

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: David Gil <dg...@yahoo-inc.com>
Gerrit-Reviewer: Jacob Haven <ja...@cloudflare.com>

Gerrit-HasComments: Yes

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 5:31:12 PM2/3/15

to David Gil, Adam Langley, golang-co...@googlegroups.com

Jacob Haven has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 4:

(1 comment)

Good catch, *fixed*

https://go-review.googlesource.com/#/c/3265/4/src/crypto/tls/handshake_server_test.go
File src/crypto/tls/handshake_server_test.go:

Line 570: command:
[]string{"openssl", "s_client", "-no_ticket", "-cipher", "ECDHE-RSA-AES128-GCM-SHA256"},
> Should be AES256 in the command line here.

Ah, the perils of copy-paste. Updated to "ECDHE-RSA-AES256-GCM-SHA384".

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 5:31:24 PM2/3/15

to Adam Langley, David Gil, golang-co...@googlegroups.com

Reviewers: Adam Langley

Jacob Haven uploaded a new patch set:

56 files changed, 1,927 insertions(+), 1,729 deletions(-)

Adam Langley (Gerrit)

unread,

Feb 3, 2015, 7:02:04 PM2/3/15

to Jacob Haven, David Gil, golang-co...@googlegroups.com

Adam Langley uploaded a new patch set:

M src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES-GCM
A src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES256-GCM-SHA384
M src/crypto/tls/testdata/Client-TLSv12-ECDHE-RSA-AES
M src/crypto/tls/testdata/Client-TLSv12-RSA-RC4

M src/crypto/tls/testdata/Server-TLSv12-ALPN
M src/crypto/tls/testdata/Server-TLSv12-ALPN-NoMatch
A src/crypto/tls/testdata/Server-TLSv12-RSA-AES256-GCM-SHA384
31 files changed, 1,185 insertions(+), 968 deletions(-)

Adam Langley (Gerrit)

unread,

Feb 3, 2015, 7:04:48 PM2/3/15

to Jacob Haven, David Gil, golang-co...@googlegroups.com

Adam Langley uploaded a new patch set:

M src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES-GCM
A src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES256-GCM-SHA384
M src/crypto/tls/testdata/Client-TLSv12-ECDHE-RSA-AES
M src/crypto/tls/testdata/Client-TLSv12-RSA-RC4

Adam Langley (Gerrit)

unread,

Feb 3, 2015, 7:07:23 PM2/3/15

to Jacob Haven, David Gil, golang-co...@googlegroups.com

Adam Langley has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 7:

Jacob: please review the changes that I made at:
https://go-review.googlesource.com/#/c/3265/5..7

If you're happy with that then please reset the author:

At https://go-review.googlesource.com/#/c/3265, see the "Download" menu at
the top-right, copy the "cherry pick" line and:

git checkout master
git checkout -b reauthor
<paste cherry-pick line>
git commit --amend --reset-author
<save without changes>
git push origin HEAD:refs/for/master

Then post a note to the review.

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: David Gil <dg...@yahoo-inc.com>
Gerrit-Reviewer: Jacob Haven <ja...@cloudflare.com>

Gerrit-HasComments: No

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 7:11:32 PM2/3/15

to Adam Langley, David Gil, golang-co...@googlegroups.com

Jacob Haven has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 7: Code-Review+1

(2 comments)

Yep, LGTM.

https://go-review.googlesource.com/#/c/3265/7/src/crypto/tls/cipher_suites.go
File src/crypto/tls/cipher_suites.go:

Line 77:
That was a strange transposition. :)

Line 274: 0xc030
Ah, good catch. Definitely ECDHE, not ECDH.

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: David Gil <dg...@yahoo-inc.com>
Gerrit-Reviewer: Jacob Haven <ja...@cloudflare.com>

Gerrit-HasComments: Yes

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 7:16:05 PM2/3/15

to Adam Langley, David Gil, golang-co...@googlegroups.com

Reviewers: Adam Langley

Jacob Haven uploaded a new patch set:

M src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES-GCM
A src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES256-GCM-SHA384
M src/crypto/tls/testdata/Client-TLSv12-ECDHE-RSA-AES
M src/crypto/tls/testdata/Client-TLSv12-RSA-RC4

M src/crypto/tls/testdata/Server-TLSv12-ALPN
M src/crypto/tls/testdata/Server-TLSv12-ALPN-NoMatch
A src/crypto/tls/testdata/Server-TLSv12-RSA-AES256-GCM-SHA384
31 files changed, 1,185 insertions(+), 968 deletions(-)

Jacob Haven (Gerrit)

unread,

Feb 3, 2015, 7:18:02 PM2/3/15

to Adam Langley, David Gil, golang-co...@googlegroups.com

Jacob Haven has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 7:

Okay, everything looks good. Ready to merge?

--
https://go-review.googlesource.com/3265
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: David Gil <dg...@yahoo-inc.com>
Gerrit-Reviewer: Jacob Haven <ja...@cloudflare.com>

Gerrit-HasComments: No

Adam Langley (Gerrit)

unread,

Feb 3, 2015, 7:18:13 PM2/3/15

to Jacob Haven, David Gil, golang-co...@googlegroups.com

Adam Langley has posted comments on this change.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Patch Set 8: Code-Review+2

Adam Langley (Gerrit)

unread,

Feb 3, 2015, 7:18:16 PM2/3/15

to Jacob Haven, golang-...@googlegroups.com, David Gil, golang-co...@googlegroups.com

Adam Langley has submitted this change and it was merged.

crypto/tls: add support for AES_256_GCM_SHA384 cipher suites specified in
RFC5289

Generalizes PRF calculation for TLS 1.2 to support arbitrary hashes
(SHA-384 instead of SHA-256).
Testdata were all updated to correspond with the new cipher suites in the
handshake.

Change-Id: I3d9fc48c19d1043899e38255a53c80dc952ee08f

Reviewed-on: https://go-review.googlesource.com/3265
Reviewed-by: Adam Langley <a...@golang.org>

M src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES-GCM
A src/crypto/tls/testdata/Client-TLSv12-ECDHE-ECDSA-AES256-GCM-SHA384
M src/crypto/tls/testdata/Client-TLSv12-ECDHE-RSA-AES
M src/crypto/tls/testdata/Client-TLSv12-RSA-RC4

M src/crypto/tls/testdata/Server-TLSv12-ALPN
M src/crypto/tls/testdata/Server-TLSv12-ALPN-NoMatch
A src/crypto/tls/testdata/Server-TLSv12-RSA-AES256-GCM-SHA384
31 files changed, 1,185 insertions(+), 968 deletions(-)

Approvals:
Adam Langley: Looks good to me, approved

Reply all

Reply to author

Forward