Issue 7880043: code review 7880043: crypto/tls: support TLS 1.1.

Issue 7880043: code review 7880043: crypto/tls: support TLS 1.1. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 9 months ago by agl1

Modified:
12 years, 6 months ago

Reviewers:
bradfitz

CC:
golang-dev, bradfitz

Visibility:
Public.

Description

crypto/tls: support TLS 1.1. The significant change between TLS 1.0 and 1.1 is the addition of an explicit IV in the case of CBC encrypted records. Support for TLS 1.1 is needed in order to support TLS 1.2.

Patch Set 1 : diff -r 79a1fec35802 https://code.google.com/p/go/ #

Total comments: 8

Patch Set 2 : diff -r 5993bd419cf0 https://code.google.com/p/go/ #

Patch Set 3 : diff -r 1a43728b3f8f https://code.google.com/p/go/ #

Created: 12 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+587 lines, -67 lines)			Patch
M	doc/go1.2.txt	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/pkg/crypto/cipher/cbc.go	View		2 chunks	+14 lines, -0 lines	0 comments	Download
M	src/pkg/crypto/tls/cipher_suites.go	View		5 chunks	+9 lines, -8 lines	0 comments	Download
M	src/pkg/crypto/tls/common.go	View	1	4 chunks	+46 lines, -17 lines	0 comments	Download
M	src/pkg/crypto/tls/conn.go	View	1	8 chunks	+66 lines, -27 lines	0 comments	Download
M	src/pkg/crypto/tls/handshake_client.go	View		2 chunks	+4 lines, -4 lines	0 comments	Download
M	src/pkg/crypto/tls/handshake_client_test.go	View	1 2	2 chunks	+175 lines, -0 lines	0 comments	Download
M	src/pkg/crypto/tls/handshake_server.go	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	src/pkg/crypto/tls/handshake_server_test.go	View		3 chunks	+261 lines, -0 lines	0 comments	Download
M	src/pkg/crypto/tls/key_agreement.go	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/pkg/crypto/tls/prf.go	View		4 chunks	+4 lines, -4 lines	0 comments	Download
M	src/pkg/crypto/tls/prf_test.go	View		4 chunks	+4 lines, -4 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

agl1

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/

12 years, 6 months ago (2013-05-24 18:30:25 UTC) #1

bradfitz

https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/common.go#newcode215 src/pkg/crypto/tls/common.go:215: // If zero, then the maximum version is TLS ...

12 years, 6 months ago (2013-05-24 18:40:07 UTC) #2

agl1

12 years, 6 months ago (2013-05-24 20:34:36 UTC) #3

bradfitz

LGTM On Fri, May 24, 2013 at 1:34 PM, <agl@golang.org> wrote: > > https://codereview.appspot.**com/7880043/diff/16001/src/** > ...

12 years, 6 months ago (2013-05-24 20:40:55 UTC) #4

LGTM



On Fri, May 24, 2013 at 1:34 PM, <agl@golang.org> wrote:

>
> https://codereview.appspot.**com/7880043/diff/16001/src/**
>
pkg/crypto/tls/common.go<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/common.go>
> File src/pkg/crypto/tls/common.go (right):
>
> https://codereview.appspot.**com/7880043/diff/16001/src/**
>
pkg/crypto/tls/common.go#**newcode215<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/common.go#newcode215>
> src/pkg/crypto/tls/common.go:**215: // If zero, then the maximum version
> is TLS 1.1.
> On 2013/05/24 18:40:07, bradfitz wrote:
>
>> rather than promise exactly TLS 1.1, could you use more vague wording
>>
> saying
>
>> that it's currently TLS 1.1?
>>
>
> Done.
>
>
> https://codereview.appspot.**com/7880043/diff/16001/src/**
>
pkg/crypto/tls/conn.go<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/conn.go>
> File src/pkg/crypto/tls/conn.go (right):
>
> https://codereview.appspot.**com/7880043/diff/16001/src/**
>
pkg/crypto/tls/conn.go#**newcode232<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/conn.go#newcode232>
> src/pkg/crypto/tls/conn.go:**232: type CBCMode interface {
> On 2013/05/24 18:40:07, bradfitz wrote:
>
>> doc comment
>>
>
> Exporting this was actually a mistake. Have renamed to cbcMode and have
> added a doc comment.
>
>
> https://codereview.appspot.**com/7880043/diff/16001/src/**
>
pkg/crypto/tls/conn.go#**newcode240<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/conn.go#newcode240>
> src/pkg/crypto/tls/conn.go:**240: func (hc *halfConn) decrypt(b *block)
> (bool, int, alert) {
> On 2013/05/24 18:40:07, bradfitz wrote:
>
>> I'd name these result parameters, keeping the English and not using
>>
> naked
>
>> returns.
>>
>
> Done.
>
>
> https://codereview.appspot.**com/7880043/diff/16001/src/**
>
pkg/crypto/tls/handshake_**client_test.go<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/handshake_client_test.go>
> File src/pkg/crypto/tls/handshake_**client_test.go (right):
>
> https://codereview.appspot.**com/7880043/diff/16001/src/**
>
pkg/crypto/tls/handshake_**client_test.go#newcode78<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/handshake_client_test.go#newcode78>
> src/pkg/crypto/tls/handshake_**client_test.go:78: conn := Client(record,
> &config)
> On 2013/05/24 18:40:07, bradfitz wrote:
>
>> GetTestConfig already returns a *Config, so this is **Config?
>>
>
> Ah, no. This is me failing to remove some testing code. Reverted.
>
>
https://codereview.appspot.**com/7880043/<https://codereview.appspot.com/7880...
>

bradfitz

LGTM but update the new go1.2.txt file when submitting. On Fri, May 24, 2013 at ...

12 years, 6 months ago (2013-05-24 20:41:22 UTC) #5

LGTM but update the new go1.2.txt file when submitting.



On Fri, May 24, 2013 at 1:40 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
>
>
> On Fri, May 24, 2013 at 1:34 PM, <agl@golang.org> wrote:
>
>>
>> https://codereview.appspot.**com/7880043/diff/16001/src/**
>>
pkg/crypto/tls/common.go<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/common.go>
>> File src/pkg/crypto/tls/common.go (right):
>>
>> https://codereview.appspot.**com/7880043/diff/16001/src/**
>>
pkg/crypto/tls/common.go#**newcode215<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/common.go#newcode215>
>> src/pkg/crypto/tls/common.go:**215: // If zero, then the maximum version
>> is TLS 1.1.
>> On 2013/05/24 18:40:07, bradfitz wrote:
>>
>>> rather than promise exactly TLS 1.1, could you use more vague wording
>>>
>> saying
>>
>>> that it's currently TLS 1.1?
>>>
>>
>> Done.
>>
>>
>> https://codereview.appspot.**com/7880043/diff/16001/src/**
>>
pkg/crypto/tls/conn.go<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/conn.go>
>> File src/pkg/crypto/tls/conn.go (right):
>>
>> https://codereview.appspot.**com/7880043/diff/16001/src/**
>>
pkg/crypto/tls/conn.go#**newcode232<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/conn.go#newcode232>
>> src/pkg/crypto/tls/conn.go:**232: type CBCMode interface {
>> On 2013/05/24 18:40:07, bradfitz wrote:
>>
>>> doc comment
>>>
>>
>> Exporting this was actually a mistake. Have renamed to cbcMode and have
>> added a doc comment.
>>
>>
>> https://codereview.appspot.**com/7880043/diff/16001/src/**
>>
pkg/crypto/tls/conn.go#**newcode240<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/conn.go#newcode240>
>> src/pkg/crypto/tls/conn.go:**240: func (hc *halfConn) decrypt(b *block)
>> (bool, int, alert) {
>> On 2013/05/24 18:40:07, bradfitz wrote:
>>
>>> I'd name these result parameters, keeping the English and not using
>>>
>> naked
>>
>>> returns.
>>>
>>
>> Done.
>>
>>
>> https://codereview.appspot.**com/7880043/diff/16001/src/**
>>
pkg/crypto/tls/handshake_**client_test.go<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/handshake_client_test.go>
>> File src/pkg/crypto/tls/handshake_**client_test.go (right):
>>
>> https://codereview.appspot.**com/7880043/diff/16001/src/**
>>
pkg/crypto/tls/handshake_**client_test.go#newcode78<https://codereview.appspot.com/7880043/diff/16001/src/pkg/crypto/tls/handshake_client_test.go#newcode78>
>> src/pkg/crypto/tls/handshake_**client_test.go:78: conn := Client(record,
>> &config)
>> On 2013/05/24 18:40:07, bradfitz wrote:
>>
>>> GetTestConfig already returns a *Config, so this is **Config?
>>>
>>
>> Ah, no. This is me failing to remove some testing code. Reverted.
>>
>>
https://codereview.appspot.**com/7880043/<https://codereview.appspot.com/7880...
>>
>
>

agl1

12 years, 6 months ago (2013-06-05 00:03:59 UTC) #6

*** Submitted as https://code.google.com/p/go/source/detail?r=4e008dc85dc8 ***

crypto/tls: support TLS 1.1.

The significant change between TLS 1.0 and 1.1 is the addition of an explicit IV
in the case of CBC encrypted records. Support for TLS 1.1 is needed in order to
support TLS 1.2.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/7880043

Expand All Messages | Collapse All Messages