proposal: Add GetClientCAs to tls.Config

[magiconair]

tls.Config provides a GetCertificate function for providing TLS certificates dynamically. I suggest to add a GetClientCAs function to provide the same for the ClientCAs field.

Rationale: On a server the ClientCAs field is used for client certificate authentication but to my knowledge it isn't possible to extend the list of client certificates at runtime without interruption of existing connections (restart service or listener) since x509.CertPool is a struct and not safe for use by multiple go routines. A GetClientCAs function would also mirror the GetCertificate function.

I have written a reverse proxy http://github.com/eBay/fabio for which I've added the dynamic reloading of TLS certificates without restart and would like to provide the same functionality for the client cert authentication.

I'm willing to write the change if this is something that could be accepted. Target would be Go 1.8 obviously.

[ianlancetaylor]

CC @agl

[bradfitz]

Ping @agl.

[magiconair]

FWIW, I've implemented the patch yesterday to see how difficult it would be and it looks like it is simple. In essence, I've added a GetClientCAs func() *x509.CertPool function to tls.Config and updated clone(), the handshake, the test and api/next.txt. ~20 lines with comments. The question remains whether this is the right approach.

[bradfitz]

You need tests too, which I imagine will be more than 20 lines.

[magiconair]

True. So far I've only updated the clone() test. But I could factor the new decision logic out into a separate function and write a simple test for that instead of writing a full integration test. Not sure what the preferred approach is.

[bradfitz]

You'll probably want something closer to an integration test than a single unit test. For things like this, it's too easy for a unit test to be misleading.

[magiconair]

OK, so far I was not successful finding a test for the ClientCAs field and its current behavior in the server handshake which I could mimic/enhance but I'll keep digging. Otherwise, I'll create one.

[magiconair]

@bradfitz Is there a way to run a specific test with go tool dist test ? Right now I'm running go tool dist test -v go_test:crypto/tls to run the entire crypto/tls suite but I'd like to run only my TestClientCAs test if possible.

Also, how can I provide the -update parameter as suggested in crypto/tls/handshake_test.go? Working around this by setting the default to true for now.

[bradfitz]

The same way as normal Go tests:

$ go test -v -run=YourNewTest crypto/tls

[magiconair]

Works - of course ... I was using go from another go1.7 directory which then failed :/

Thx for the quick reply.

[magiconair]

OK, so I have a patch with the changes and a full integration test which is an addition to the existing tests. This involved adding a new option to generate_cert.go to create a cert which contains x509.ExtKeyUsageClientAuth and replacing the test certificates in handshake_server_test.go since the new test needs to verify the client certificate and the old test certificates do not support that. Since I'm not a crypto expert a review would be nice :)

The tests check three cases:

• Only ClientCAs
• Only GetClientCAs()
• Prefer GetClientCAs() over ClientCAs

I've generated the updated testdata/* files with the homebrew version OpenSSL 1.0.2h 3 May 2016 of openssl on OSX but I don't imagine that this is the reference implementation that the crytpo/tls test refers to.

@bradfitz @agl How do we proceed?

[magiconair]

Submitted a CL with the code: https://go-review.googlesource.com/#/c/28773/

This is my first on this project so pls bear with me.

[gopherbot]

CL https://golang.org/cl/28773 mentions this issue.

[magiconair]

Changed the signature of GetClientCAs() to GetClientCAs(clientHello *ClientHelloInfo) in the CL to match GetCertificate() and to provide a mechanism to return different CA pools for different server names.

[magiconair]

@bradfitz @agl ping

[bradfitz]

Pung. Left comments.

[magiconair]

Fixed except for one consistency thing. See comments

[bradfitz]

This is very related to #15707 and #15699

Perhaps instead of adding a dozen hooks GetFoo hooks, we should have one for clients and one for servers which returns a new *tls.Config as a function of the ServerHello or ClientGoo, and use the returned config's fields before the base config's fields.

That is, instead of your:

type Config struct {
    ClientCAs *x509.CertPool
    GetClientCAs func(*ClientHelloInfo) *x509.CertPool
}

We could do:

type Config struct {
    ClientCAs *x509.CertPool

    // GetClientConfig returns server TLS configuration specialized for
    // the incoming user. The current supported fields are: ClientCAs.
    // If a supported field is its zero value, the base *tls.Config value is used
    // instead. 
    GetClientConfig func(*ClientHelloInfo) *Config
}

/cc @dpiddy @FiloSottile for thoughts.

[danp]

Yes, please. That is what I was thinking in #15699 (comment). I had it also return error to mirror tls.Config.GetCertificate but maybe it is not required.

[danp]

And as I said there, too, I think to make it truly useful we'd need some more info added to ClientHelloInfo.

[bradfitz]

@dpiddy, whoops, I totally missed that. An error might be nice. If you're doing a database/network lookup to find the config for that incoming connection and fail, it might be better to close the connection than accept it.

[bradfitz]

I think to make it truly useful we'd need some more info added to ClientHelloInfo.

@dpiddy, like what?

[danp]

Again as I said in #15699 (comment) my immediate need was for ALPN info so I can conditionally choose whether to serve, say, http/2 to a client.

[magiconair]

Yes, that is better. Do we want to resolve it recursively since the returned *Config could have another GetConfig() function? I guess not but maybe there is a use-case.

Also, since we need to maintain GetCertificate() I suggest this fallback order:

GetConfig() -> GetCertificate() -> Config

As for extending ClientHelloInfo:

type ClientHelloInfo struct {
    ...

    // ALPNProtocols ...
    ALPNProtocols []string

    // what else ?
}

I can update the patch later to see what this would look like.

[magiconair]

On the same note: What if the returned *Config has a GetCertificate() function set? Ignore or call?

[danp]

@magiconair as @bradfitz said I think it would be good to make clear that only certain items on the Config returned by GetConfig will be respected. Then the recursion and ordering issues can be avoided.

Maybe having ALPN info on the ClientHello isn't absolutely necessary for what I'm trying to do. I could offer h2 or not and the client can accept it or not if it wants. Hopefully I'm thinking that through correctly.

Also, @bradfitz, my last comment went out too quickly and ended up sounding short. That wasn't what I intended, sorry about that. Thanks for helping move this along!

[gopherbot]

CL https://golang.org/cl/30752 mentions this issue.

[magiconair]

@bradfitz @danp I've added an implementation for the suggested GetClientConfig approach in https://golang.org/cl/30752. Since it takes a different approach I've created a separate change. See big comment on approach for patch set 3.

/cc @dpiddy @FiloSottile

[gopherbot]

CL https://golang.org/cl/30790 mentions this issue.

[magiconair]

@agl @bradfitz Shall I submit the patch I've added in the comment of https://golang.org/cl/30790 for replacing hs.c.config, c.config and config in crypto/tls/handshake_server.go with just c.config as a separate CL? This is inconsistent in the current code and I found it a bit confusing.

[bradfitz]

If you left a comment on that CL and @agl hasn't submitted it yet, he'll probably just address it before he submits.

[FiloSottile]

This (the CL 30790 approach) is exactly what we've been experimenting with at Cloudflare in our fork, and seems to work.

Agreed with @danp that we'll need a few more fields in the CHI, but it should be another issue/proposal/CL.