Issue 107400043: code review 107400043: crypto/tls: Added dynamic alternative to NameToCertific...
|
Created:
9 years, 10 months ago by ox Modified:
9 years, 8 months ago CC:
golang-codereviews, gobot, agl1 Visibility:
Public. |
Descriptioncrypto/tls: Added dynamic alternative to NameToCertificate map for SNI
Revised version of https://codereview.appspot.com/81260045/
Patch Set 1 #Patch Set 2 : diff -r 767350c547a0 https://code.google.com/p/go #Patch Set 3 : diff -r 767350c547a0 https://code.google.com/p/go #
Total comments: 1
Patch Set 4 : diff -r 767350c547a0 https://code.google.com/p/go #
Total comments: 13
Patch Set 5 : diff -r fda4c0d14c53 https://code.google.com/p/go #Patch Set 6 : diff -r fda4c0d14c53 https://code.google.com/p/go #Patch Set 7 : diff -r 6b696a34e0af https://code.google.com/p/go #Patch Set 8 : diff -r 6b696a34e0af https://code.google.com/p/go #
Total comments: 3
Patch Set 9 : diff -r 6b696a34e0af https://code.google.com/p/go #Patch Set 10 : diff -r 88999b670ed9 https://code.google.com/p/go #Patch Set 11 : diff -r 37c1e2e62ea2 https://code.google.com/p/go #Patch Set 12 : diff -r 37c1e2e62ea2 https://code.google.com/p/go #Patch Set 13 : diff -r 37c1e2e62ea2 https://code.google.com/p/go #Patch Set 14 : diff -r 37c1e2e62ea2 https://code.google.com/p/go #
MessagesTotal messages: 18
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/107400043/diff/40001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): https://codereview.appspot.com/107400043/diff/40001/src/pkg/crypto/tls/common... src/pkg/crypto/tls/common.go:234: // of getCertificateForName will be used. that's not a helpful comment because getCertificateForName is not an exported symbol
Sign in to reply to this message.
R=agl@golang.org (assigned by r@golang.org)
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, gobot@golang.org, agl@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/common... src/pkg/crypto/tls/common.go:234: // retrieved from NameToCertificate. If NameToCertificate is nil, the first wrap to 80, since everything else is. https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/common... src/pkg/crypto/tls/common.go:236: CertificateForName func(name string) (*Certificate, error) I suspect that this interface will prove inadequate in the future. How about the function takes a structure as its single argument? Something like: type CertificateSelectionInfo struct { SNIName string Conn net.Conn ECDSAOk bool } That will allow selection based on SNI, destination IP and whether the client is ECDSA capable. (All these and more are used in certificate selection in Google servers.) https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/conn_t... File src/pkg/crypto/tls/conn_test.go (right): https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/conn_t... src/pkg/crypto/tls/conn_test.go:93: t.Errorf("unable to get certificate for name: %s", err) The error message should include the name that failed, i.e. t.Errorf("unable to get certificate for name %s: %s", name, err) https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/handsh... File src/pkg/crypto/tls/handshake_server_test.go (right): https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/handsh... src/pkg/crypto/tls/handshake_server_test.go:261: // expectAlert, if true, expect alert during handshaking with server // expectAlert, if true, indicates that a fatal alert should be returned when handshaking with the server. https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/handsh... src/pkg/crypto/tls/handshake_server_test.go:525: dynamicConfig := new(Config) config := *testConfig https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/handsh... src/pkg/crypto/tls/handshake_server_test.go:548: dynamicConfig := new(Config) config := *testConfig
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, gobot@golang.org, agl@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Responded to all comments. https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/common... src/pkg/crypto/tls/common.go:234: // retrieved from NameToCertificate. If NameToCertificate is nil, the first On 2014/07/22 15:56:21, agl1 wrote: > wrap to 80, since everything else is. Acknowledged. https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/common... src/pkg/crypto/tls/common.go:236: CertificateForName func(name string) (*Certificate, error) ECDSAOk depends on the certificate returned by the existing config.getCertificateForName() function, and thus isn't yet available to be passed to that function. We could pass in the cipher suites sent in the client hello. In fact, it's tempting just to pass the whole client hello (or an equivalent struct, since that's private). On 2014/07/22 15:56:21, agl1 wrote: > I suspect that this interface will prove inadequate in the future. > > How about the function takes a structure as its single argument? Something like: > > type CertificateSelectionInfo struct { > SNIName string > Conn net.Conn > ECDSAOk bool > } > > That will allow selection based on SNI, destination IP and whether the client is > ECDSA capable. > > (All these and more are used in certificate selection in Google servers.) https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/conn_t... File src/pkg/crypto/tls/conn_test.go (right): https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/conn_t... src/pkg/crypto/tls/conn_test.go:93: t.Errorf("unable to get certificate for name: %s", err) On 2014/07/22 15:56:21, agl1 wrote: > The error message should include the name that failed, i.e. > > t.Errorf("unable to get certificate for name %s: %s", name, err) Acknowledged. https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/handsh... File src/pkg/crypto/tls/handshake_server_test.go (right): https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/handsh... src/pkg/crypto/tls/handshake_server_test.go:261: // expectAlert, if true, expect alert during handshaking with server On 2014/07/22 15:56:21, agl1 wrote: > // expectAlert, if true, indicates that a fatal alert should be returned when > handshaking with the server. Acknowledged. https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/handsh... src/pkg/crypto/tls/handshake_server_test.go:525: dynamicConfig := new(Config) Ah, this is a little less verbose, got it. On 2014/07/22 15:56:21, agl1 wrote: > config := *testConfig https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/handsh... src/pkg/crypto/tls/handshake_server_test.go:525: dynamicConfig := new(Config) On 2014/07/22 15:56:21, agl1 wrote: > config := *testConfig Acknowledged.
Sign in to reply to this message.
https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/common.go File src/pkg/crypto/tls/common.go (right): https://codereview.appspot.com/107400043/diff/60001/src/pkg/crypto/tls/common... src/pkg/crypto/tls/common.go:236: CertificateForName func(name string) (*Certificate, error) On 2014/08/02 01:42:04, ox1 wrote: > ECDSAOk depends on the certificate returned by the existing > config.getCertificateForName() function, and thus isn't yet available to be > passed to that function. We could pass in the cipher suites sent in the client > hello. In fact, it's tempting just to pass the whole client hello (or an > equivalent struct, since that's private). Internally we know that we use P-256 for our certs, so ECDSAOk is something that we can statically calculate, but fair point that such limitations aren't always true. I think passing the cipher suites, curves and point formats would be reasonable. In any case, it's very likely that we'll need to pass more in the future so an "opts" structure is very likely to be needed.
Sign in to reply to this message.
I've added a new struct to capture some of the client hello information, which I'm now passing to the GetCertificate method for dynamically looking up a certificate.
Sign in to reply to this message.
LGTM with nits. https://codereview.appspot.com/107400043/diff/140001/src/pkg/crypto/tls/commo... File src/pkg/crypto/tls/common.go (right): https://codereview.appspot.com/107400043/diff/140001/src/pkg/crypto/tls/commo... src/pkg/crypto/tls/common.go:205: type ClientHello struct { ClientHelloInfo I think.
Sign in to reply to this message.
Okay, I think this may be the one ... https://codereview.appspot.com/107400043/diff/140001/src/pkg/crypto/tls/commo... File src/pkg/crypto/tls/common.go (right): https://codereview.appspot.com/107400043/diff/140001/src/pkg/crypto/tls/commo... src/pkg/crypto/tls/common.go:205: type ClientHello struct { Haha, I had considered that same name myself and then opted for the concise version. Since you also thought of the name, I'm happy to use the longer version. On 2014/08/05 00:36:45, agl1 wrote: > ClientHelloInfo I think. https://codereview.appspot.com/107400043/diff/140001/src/pkg/crypto/tls/commo... src/pkg/crypto/tls/common.go:205: type ClientHello struct { On 2014/08/05 00:36:45, agl1 wrote: > ClientHelloInfo I think. Done.
Sign in to reply to this message.
Nearly :) I landed https://codereview.appspot.com/108710046/ in order to clear out my working directory to patch this in, but they conflict. If you hg sync and reupload, it should fix that.
Sign in to reply to this message.
Synced and merged handshake_server_test
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=957bd50e2b82 *** crypto/tls: Added dynamic alternative to NameToCertificate map for SNI Revised version of https://codereview.appspot.com/81260045/ LGTM=agl R=golang-codereviews, gobot, agl, ox CC=golang-codereviews https://codereview.appspot.com/107400043 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
update doc/go1.4.text please.
Sign in to reply to this message.
Added entry to doc/go1.4.txt Apologies for the extra patches - I'm new to using the code review extension.
Sign in to reply to this message.
On 2014/08/06 21:59:34, ox.to.a.cart wrote: > Added entry to doc/go1.4.txt Please create another CL just for doc/go1.4.txt change, this CL has already been submitted.
Sign in to reply to this message.
On 2014/08/07 00:15:38, minux wrote: > On 2014/08/06 21:59:34, ox.to.a.cart wrote: > > Added entry to doc/go1.4.txt > Please create another CL just for doc/go1.4.txt change, this CL has > already been submitted. Done - https://codereview.appspot.com/117670046/
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
