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

Issue 137940043: code review 137940043: net/http: add Transport.DialTLS hook (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 2 months ago by bradfitz
Modified:
7 years, 2 months ago
Reviewers:
agl1, adg
CC:
agl, agl1, cee-dub, adg, golang-codereviews
Visibility:
Public.

Description

net/http: add Transport.DialTLS hook Per discussions out of https://codereview.appspot.com/128930043/ and golang-nuts threads and with agl. Fixes Issue 8522

Patch Set 1 #

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

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

Total comments: 1

Patch Set 4 : diff -r c9c8b336ffcc2f515d2693f681852fcf598fbca4 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 5 : diff -r 428dd5a62a9dad3f037c45aaf19ffecca8801b7e https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r 0a5fafdd2343b083457d0baf6487dfce0f01e25f https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+83 lines, -20 lines) Patch
M src/pkg/net/http/transport.go View 1 2 3 4 5 chunks +43 lines, -20 lines 0 comments Download
M src/pkg/net/http/transport_test.go View 1 2 3 4 1 chunk +40 lines, -0 lines 0 comments Download

Messages

Total messages: 15
agl1
Do the use cases require the ability to bypass the usual verification? Can't the callback ...
7 years, 2 months ago (2014-09-02 18:39:12 UTC) #1
bradfitz
I think people only need subtractive. Would you keep the existing signature? Or would the ...
7 years, 2 months ago (2014-09-02 18:42:37 UTC) #2
agl1
On 2014/09/02 18:42:37, bradfitz wrote: > I think people only need subtractive. > > Would ...
7 years, 2 months ago (2014-09-02 18:45:31 UTC) #3
bradfitz
Hello agl@chromium.org, agl@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
7 years, 2 months ago (2014-09-02 19:31:02 UTC) #4
bradfitz
On Tue, Sep 2, 2014 at 11:45 AM, <agl@golang.org> wrote: > On 2014/09/02 18:42:37, bradfitz ...
7 years, 2 months ago (2014-09-02 19:31:53 UTC) #5
agl1
LGTM https://codereview.appspot.com/137940043/diff/40001/src/pkg/crypto/tls/handshake_server.go File src/pkg/crypto/tls/handshake_server.go (right): https://codereview.appspot.com/137940043/diff/40001/src/pkg/crypto/tls/handshake_server.go#newcode602 src/pkg/crypto/tls/handshake_server.go:602: err = c.config.VerifyCertificate(c.verifiedChains) s/c.verifiedChains/chains/
7 years, 2 months ago (2014-09-02 20:07:49 UTC) #6
cee-dub
7 years, 2 months ago (2014-09-03 14:20:41 UTC) #7
bradfitz
Hello agl@chromium.org, agl@golang.org, c@apcera.com (cc: c@apcera.com, golang-codereviews@googlegroups.com), Please take another look.
7 years, 2 months ago (2014-09-05 22:44:56 UTC) #8
bradfitz
PTAL This is now moved to net/http where it addresses more issues (in particular the ...
7 years, 2 months ago (2014-09-05 22:45:49 UTC) #9
agl1
LGTM in general. https://codereview.appspot.com/137940043/diff/60001/src/pkg/net/http/transport.go File src/pkg/net/http/transport.go (right): https://codereview.appspot.com/137940043/diff/60001/src/pkg/net/http/transport.go#newcode70 src/pkg/net/http/transport.go:70: // TLS connections for non-proxied https ...
7 years, 2 months ago (2014-09-05 22:50:40 UTC) #10
bradfitz
You're right, it should be HTTPS. I'll write some tests & get adg or somebody ...
7 years, 2 months ago (2014-09-05 22:52:22 UTC) #11
bradfitz
Hello agl@chromium.org, agl@golang.org, c@apcera.com, adg@golang.org (cc: c@apcera.com, golang-codereviews@googlegroups.com), Please take another look.
7 years, 2 months ago (2014-09-07 17:08:24 UTC) #12
bradfitz
[+adg for test review] Now with a test, and slight control flow tweak from previous ...
7 years, 2 months ago (2014-09-07 17:10:22 UTC) #13
adg
LGTM
7 years, 2 months ago (2014-09-08 00:23:42 UTC) #14
bradfitz
7 years, 2 months ago (2014-09-08 03:48:39 UTC) #15
*** Submitted as https://code.google.com/p/go/source/detail?r=455042166f13 ***

net/http: add Transport.DialTLS hook

Per discussions out of https://codereview.appspot.com/128930043/
and golang-nuts threads and with agl.

Fixes Issue 8522

LGTM=agl, adg
R=agl, c, adg
CC=c, golang-codereviews
https://codereview.appspot.com/137940043
Sign in to reply to this message.

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