Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(976)

Issue 5448093: crypto/tls: Make TLS Client Authentication work according to the spec (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 8 months ago by jra
Modified:
8 years, 7 months ago
Reviewers:
CC:
mkrautz, agl1, bradfitz, golang-dev, mikkel_krautz.dk
Visibility:
Public.

Description

crypto/tls: Improve TLS Client Authentication Fix incorrect marshal/unmarshal of certificateRequest. Add support for configuring client-auth on the server side. Fix the certificate selection in the client side. Update generate_cert.go to new time package Fixes issue 2521.

Patch Set 1 #

Patch Set 2 : diff -r 85e087089edf https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 85e087089edf https://go.googlecode.com/hg/ #

Total comments: 9

Patch Set 4 : diff -r b16a53f58594 https://code.google.com/p/go/ #

Patch Set 5 : diff -r b16a53f58594 https://code.google.com/p/go/ #

Total comments: 9

Patch Set 6 : diff -r 7ec969250bfc https://go.googlecode.com/hg/ #

Total comments: 8

Patch Set 7 : diff -r 7ec969250bfc https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+906 lines, -117 lines) Patch
M src/pkg/crypto/tls/common.go View 1 2 3 4 5 6 4 chunks +26 lines, -6 lines 0 comments Download
M src/pkg/crypto/tls/generate_cert.go View 1 2 3 4 5 2 chunks +4 lines, -3 lines 0 comments Download
M src/pkg/crypto/tls/handshake_client.go View 1 2 3 4 5 6 5 chunks +51 lines, -26 lines 0 comments Download
M src/pkg/crypto/tls/handshake_messages.go View 1 4 chunks +16 lines, -15 lines 0 comments Download
M src/pkg/crypto/tls/handshake_server.go View 1 2 3 4 5 6 4 chunks +65 lines, -22 lines 0 comments Download
M src/pkg/crypto/tls/handshake_server_test.go View 1 2 3 4 5 6 9 chunks +733 lines, -44 lines 0 comments Download
M src/pkg/crypto/tls/tls.go View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/crypto/x509/cert_pool.go View 1 2 3 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 13
jra
Hello agl@chromium.org (cc: golang-dev@googlegroups.com, mikkel@krautz.dk), I'd like you to review this change to https://go.googlecode.com/hg/
8 years, 8 months ago (2011-12-03 01:08:16 UTC) #1
mkrautz
A few comments. http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go#newcode162 src/pkg/crypto/tls/common.go:162: AuthenticateClientRequired bool This does not require ...
8 years, 8 months ago (2011-12-03 02:07:35 UTC) #2
jra
http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go#newcode162 src/pkg/crypto/tls/common.go:162: AuthenticateClientRequired bool Maybe I should just s/valid//. Because what ...
8 years, 8 months ago (2011-12-03 02:42:44 UTC) #3
agl1
http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go#newcode156 src/pkg/crypto/tls/common.go:156: AuthenticateClient bool I think the API should look something ...
8 years, 8 months ago (2011-12-05 17:29:30 UTC) #4
agl1
Please let me know when I should take another look. Cheers AGL
8 years, 8 months ago (2011-12-10 21:47:17 UTC) #5
jra
Hello krautz@gmail.com, agl@golang.org (cc: golang-dev@googlegroups.com, mikkel@krautz.dk), Please take another look.
8 years, 7 months ago (2011-12-22 16:02:18 UTC) #6
jra
Hello krautz@gmail.com, agl@golang.org (cc: golang-dev@googlegroups.com, mikkel@krautz.dk), Please take another look.
8 years, 7 months ago (2011-12-22 16:03:09 UTC) #7
bradfitz
Could you change this CL's subject line to be more descriptive? We've already had 11,046 ...
8 years, 7 months ago (2011-12-22 19:02:03 UTC) #8
bradfitz
(Drive-by comments. Consider agl the real reviewer still.) http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/common.go#newcode164 src/pkg/crypto/tls/common.go:164: // ...
8 years, 7 months ago (2011-12-22 19:13:06 UTC) #9
jra
http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/common.go#newcode164 src/pkg/crypto/tls/common.go:164: // TLS Client Authentication. On 2011/12/22 19:13:06, bradfitz wrote: ...
8 years, 7 months ago (2012-01-03 02:04:17 UTC) #10
agl1
http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake_client.go File src/pkg/crypto/tls/handshake_client.go (right): http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake_client.go#newcode192 src/pkg/crypto/tls/handshake_client.go:192: c2, ok := x509.ParseCertificates(cert.Certificate[0]) I'd be happy to add ...
8 years, 7 months ago (2012-01-03 15:33:24 UTC) #11
jra
PTAL http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake_client.go File src/pkg/crypto/tls/handshake_client.go (right): http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake_client.go#newcode192 src/pkg/crypto/tls/handshake_client.go:192: c2, ok := x509.ParseCertificates(cert.Certificate[0]) On 2012/01/03 15:33:24, agl1 ...
8 years, 7 months ago (2012-01-05 00:28:51 UTC) #12
agl1
8 years, 7 months ago (2012-01-05 17:05:50 UTC) #13
*** Submitted as http://code.google.com/p/go/source/detail?r=d620ce23ebe4 ***

crypto/tls: Improve TLS Client Authentication

Fix incorrect marshal/unmarshal of certificateRequest.
Add support for configuring client-auth on the server side.
Fix the certificate selection in the client side.
Update generate_cert.go to new time package

Fixes issue 2521.

R=krautz, agl, bradfitz
CC=golang-dev, mikkel
http://codereview.appspot.com/5448093

Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b