code review 128930043: net/http: optional TLSVerifyCallback func for Transport

net/http: optional TLSVerify func for Transport

Expose a policy hook for TLS that may be used, for example, to check for revoked certificates or other specifics in the certificate chain.

Fixes Issue 8522

[cee-dub, 2014-08-12 00:29:03]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-08-13 00:48:45]

https://codereview.appspot.com/128930043/diff/40001/src/pkg/net/http/transpor...
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/128930043/diff/40001/src/pkg/net/http/transpor...
src/pkg/net/http/transport.go:77: // TLSVerifyCallback, if non-nil, is invoked
right before hostname
I think I'd drop the word "Callback". Dial and Proxy above don't say that
they're Hooks or Callbacks in their names.

But I think I'd drop the part about specifying where it's run and drop the
guarantee about being able to mutate the *tls.Config to affect the future
behavior. Feels too spaghetti.

What if instead we just call this TLSVerify and say "TLSVerify optionally
specifies a validity check on new TLS connections. If nil, only the hostname is
checked." You could also say that it's an error to specify both
TLSClientConfig.InsecureSkipVerify and TLSVerifyCallback, and we can return an
error if you try to set both.

Then we can drop the the VerifyHostname part afterwards, and not depend on
mutations to *tls.Config. We might even say the config must not be mutated, lest
the pointer is the same between multiple goroutines. We might also say that the
function should not affect the connection directly. (as opposed to the indirect
affect of returning an error and net/http closing it)

Thoughts?  You might also want to file a bug with background and use cases to
reference in the CL description with a "Fixes Issue nnnn" line at the end.

[cee-dub, 2014-08-13 07:19:57]

https://codereview.appspot.com/128930043/diff/40001/src/pkg/net/http/transpor...
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/128930043/diff/40001/src/pkg/net/http/transpor...
src/pkg/net/http/transport.go:77: // TLSVerifyCallback, if non-nil, is invoked
right before hostname
On 2014/08/13 00:48:44, bradfitz wrote:
> I think I'd drop the word "Callback". Dial and Proxy above don't say that
> they're Hooks or Callbacks in their names.
Done.

> But I think I'd drop the part about specifying where it's run and drop the
> guarantee about being able to mutate the *tls.Config to affect the future
> behavior. Feels too spaghetti.
Done.

> What if instead we just call this TLSVerify and say "TLSVerify optionally
> specifies a validity check on new TLS connections. If nil, only the hostname
is
> checked." You could also say that it's an error to specify both
> TLSClientConfig.InsecureSkipVerify and TLSVerifyCallback, and we can return an
> error if you try to set both. Then we can drop the the VerifyHostname part
> afterwards, and not depend on mutations to *tls.Config.
Mostly done. Seemed a bit more straightforward to say "TLSVerify optionally
specifies
a validity check on new TLS connections. If nil, only the hostname is checked.
Ignored
if TLSClientConfig.InsecureSkipVerify is true." Didn't see a straightforward way
to
return an error if both are set without adding undue complexity.

This change adds some difficulty to testing, since NewTLSServer's certificate is
rejected 
before the callback. Suggestions?

> We might even say the config must not be mutated, lest
> the pointer is the same between multiple goroutines. We might also say that
the
> function should not affect the connection directly. (as opposed to the
indirect
> affect of returning an error and net/http closing it)
I like the spirit of this comment. Any reasonable way to enforce it when passing
these
as pointers, or is godoc sufficient?

> Thoughts?  You might also want to file a bug with background and use cases to
> reference in the CL description with a "Fixes Issue nnnn" line at the end.
Thank you for the feedback. Filed Issue 8522.

https://codereview.appspot.com/128930043/diff/60001/src/pkg/net/http/transpor...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/128930043/diff/60001/src/pkg/net/http/transpor...
src/pkg/net/http/transport_test.go:1914: t.Errorf("expected error: rejected by
TLSVerify, got: %s", err)
This currently fails (err is nil) due to the interaction with
InsecureSkipVerify. Suggestions for testing with httptest.NewTLSServer when
InsecureSkipVerify is false or an alternative change to the implementation?

[cee-dub, 2014-08-13 16:36:04]

Slept on it and came up with simpler code.

> You could also say that it's an error to specify both
Done.

> This change adds some difficulty to testing, since NewTLSServer's certificate
is
> rejected before the callback. Suggestions?
Haven't figured out a clean solution to test this yet.

> We might even say the config must not be mutated, lest the pointer is the 
> same between multiple goroutines. We might also say that the function 
> should not affect the connection directly. (as opposed to the indirect 
> affect of returning an error and net/http closing it)
Added godoc.

[agl1, 2014-08-13 18:06:47]

net/http already allows one to handle doing the connection itself, right? If so,
is this a sufficiently common use case that it needs special support?

(Also, it looks like this callback overrides the VerifyHostname but actually it
doesn't because crypto/tls does that internally now.)

[pdp, 2014-08-13 18:19:54]

On 2014/08/13 18:06:47, agl1 wrote:
> net/http already allows one to handle doing the connection itself, right? If
so,
> is this a sufficiently common use case that it needs special support?
> 
> (Also, it looks like this callback overrides the VerifyHostname but actually
it
> doesn't because crypto/tls does that internally now.)

For context, as the person who originally wrote the patch: it was not intended
to override VerifyHostname nor to preempt InsecureSkipVerify; security APIs
which require the caller to reimplement base functionality before adding their
code which was the reason for the hook tends to result in insecure usage of the
API, especially as fixes are made over time to the base checks but not reflected
into the replacements.  OpenSSL's SSL_CTX_set_cert_verify_callback is an example
of the wrong way to handle this, providing maximum flexibility but at the cost
of encouraging verification fragmentation and turning the security landscape
into a jagged nightmare of 500 applications, 80% of which are missing some
subset of security-impacting verification fixes (WAG percentage based on
pessimism).

This change is not intended to allow completely changing what it means to verify
a certificate.  It was intended to add _additional_ policy for verification,
with revoked certificates being the primary driving motivation.  Our client
application for accessing our product has a blacklist of certificates which we
revoked (in Heartbleed fallout) and adds checks to ensure that confidential
information does not get sent to a server using a presumed-compromised private
key.

There's a place for _replacement_ verification functions, but they should not be
the primary API, because most of the people using the API won't need that and
will get it wrong.

[bradfitz, 2014-09-02 16:40:42]

Per a chat with agl last week, I prepared
https://codereview.appspot.com/137940043 which should cover this.

It has no tests at the moment though.



On Wed, Aug 13, 2014 at 11:19 AM, <phil@apcera.com> wrote:

> On 2014/08/13 18:06:47, agl1 wrote:
>
>> net/http already allows one to handle doing the connection itself,
>>
> right? If so,
>
>> is this a sufficiently common use case that it needs special support?
>>
>
>  (Also, it looks like this callback overrides the VerifyHostname but
>>
> actually it
>
>> doesn't because crypto/tls does that internally now.)
>>
>
> For context, as the person who originally wrote the patch: it was not
> intended to override VerifyHostname nor to preempt InsecureSkipVerify;
> security APIs which require the caller to reimplement base functionality
> before adding their code which was the reason for the hook tends to
> result in insecure usage of the API, especially as fixes are made over
> time to the base checks but not reflected into the replacements.
> OpenSSL's SSL_CTX_set_cert_verify_callback is an example of the wrong
> way to handle this, providing maximum flexibility but at the cost of
> encouraging verification fragmentation and turning the security
> landscape into a jagged nightmare of 500 applications, 80% of which are
> missing some subset of security-impacting verification fixes (WAG
> percentage based on pessimism).
>
> This change is not intended to allow completely changing what it means
> to verify a certificate.  It was intended to add _additional_ policy for
> verification, with revoked certificates being the primary driving
> motivation.  Our client application for accessing our product has a
> blacklist of certificates which we revoked (in Heartbleed fallout) and
> adds checks to ensure that confidential information does not get sent to
> a server using a presumed-compromised private key.
>
> There's a place for _replacement_ verification functions, but they
> should not be the primary API, because most of the people using the API
> won't need that and will get it wrong.
>
> https://codereview.appspot.com/128930043/
>

[bradfitz, 2014-09-29 19:32:33]

R=close

Addressed by https://codereview.appspot.com/137940043