Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/tls: add Config.VerifyConnection callback #36736

Closed
divjotarora opened this issue Jan 24, 2020 · 19 comments
Closed

crypto/tls: add Config.VerifyConnection callback #36736

divjotarora opened this issue Jan 24, 2020 · 19 comments
Labels
FrozenDueToAge NeedsFix Proposal-Accepted Proposal-Crypto Unfortunate
Projects
Milestone

Comments

@divjotarora
Copy link

@divjotarora divjotarora commented Jan 24, 2020

I'd like to do OCSP verification using the VerifyPeerCertificate field of tls.Config. My understanding is that it is not possible to access the stapled OCSP response from the peer in this callback. This is because the stapled response is available on the connection itself through the OCSPResposne method on tls.Conn or through the ConnectionState type. Unless there is a way to access it in the callback, the OCSP verification will have to be done after the handshake has been completed, which isn't ideal because the peer logs will show that the connection was successfully established.

Is there a way to currently access the stapled responses in the verification callback that I've missed? If not, is this possible given how the TLS handshake code is currently written?

@gopherbot gopherbot added this to the Proposal milestone Jan 24, 2020
@FiloSottile FiloSottile added Proposal-Crypto Unfortunate labels Feb 11, 2020
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 11, 2020

I'm afraid you are correct. I can only think of two solutions, both not great: adding a new callback, or adding a global map accessible via something like tls.PeerCertificatesAdditionalData(rawCerts [][]byte) *SomeStruct which gets populated right before any call to VerifyPeerCertificate and cleaned up immediately after the callback returns.

@divjotarora
Copy link
Author

@divjotarora divjotarora commented Feb 11, 2020

@FiloSottile I think of those two suggestions, I'd prefer the additional callback. Also, is it a viable solution to deprecate VerifyPeerCertificate and replace it with a callback that exposes the peer certificates as well as the stapled OCSP response?

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Feb 14, 2020

While not solving the specific problem of not having access to the stapled response in VerifyPeerCertificate it sounds like implementing some of what is discussed in #22274 would solve the problem that instigates this issue (by which I mean if all you want is to validate stapled OCSP responses, the problem with VerifyPeerCertificate can be mostly handwaved away until another time).

@divjotarora
Copy link
Author

@divjotarora divjotarora commented Feb 14, 2020

@rolandshoemaker This is fair, but just adding support for MustStaple verification does not necessarily address all use cases. For example, my use case requires me to manually reach out to an OCSP responder via an HTTP request if the certificate is not a MustStaple cert and a staple is not provided. Having access to the OCSP response inside a callback like VerifyPeerCertificate would still be helpful.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 18, 2020

@katiehockman pointed out that ConnectionState already has all the information needed to verify a peer, and semantically I don't see how we would want things in VerifyPeerCertificate but not in ConnectionState, and vice-versa.

Taking the opportunity to make the semantics of when the callback is invoked more straightforward, we propose this callback.

type Config struct {
	// VerifyConnection, if not nil, is called after normal certificate
	// verification and after VerifyPeerCertificate by either a TLS client
	// or server. If it returns a non-nil error, the handshake is aborted
	// and that error results.
	//
	// If normal verification fails then the handshake will abort before
	// considering this callback. This callback will run for all connections
	// regardless of InsecureSkipVerify or ClientAuth settings.
	VerifyConnection func(ConnectionState) error // Go 1.15

While at it, I propose we set ConnectionState.ServerName on the client side, too. This way VerifyConnection has access to the ServerName that net/http or other intermediate libraries have set dynamically (which is currently a problem when trying to override normal verification from a http.Transport.TLSClientConfig).

An open question is whether we should allow setting both VerifyPeerCertificate and VerifyConnection. It feels cleaner to make them mutually exclusive, but VerifyConnection will run when VerifyPeerCertificate wouldn't if a client certificate is not requested or required, so it's simple but not trivial to implement VerifyConnection in terms of VerifyPeerCertificate.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 18, 2020

Particularly happy about how easy it makes reproducing (and then customizing) normal verification.

&tls.Config{
	InsecureSkipVerify: true,
	VerifyConnection: func(cs ConnectionState) error {
		opts := x509.VerifyOptions{
			DNSName:       cs.ServerName,
			Intermediates: x509.NewCertPool(),
		}
		for _, cert := range cs.PeerCertificates[1:] {
			opts.Intermediates.AddCert(cert)
		}
		_, err := cs.PeerCertificates[0].Verify(opts)
		return err
	},
}

@rsc
Copy link
Contributor

@rsc rsc commented Feb 26, 2020

Is the proposal now to add VerifyConnection, not "provide access to stapled OCSP response in VerifyPeerCertificate"?

@katiehockman
Copy link
Contributor

@katiehockman katiehockman commented Feb 26, 2020

@rsc that's correct. The proposal is for a new callback, Config.VerifyConnection.

@divjotarora
Copy link
Author

@divjotarora divjotarora commented Feb 26, 2020

@katiehockman Will this also involve deprecating the existing VerifyPeerCertificate callback or will both be part of the public API?

@katiehockman
Copy link
Contributor

@katiehockman katiehockman commented Feb 26, 2020

@divjotarora no I don't expect VerifyPeerCertificate to be formally deprecated as a result of this proposal. There is still an open question about how VerifyConnection and VerifyPeerCertificate will work together though, as @FiloSottile outlined in #36736 (comment)

An open question is whether we should allow setting both VerifyPeerCertificate and VerifyConnection. It feels cleaner to make them mutually exclusive, but VerifyConnection will run when VerifyPeerCertificate wouldn't if a client certificate is not requested or required, so it's simple but not trivial to implement VerifyConnection in terms of VerifyPeerCertificate.

@FiloSottile FiloSottile changed the title proposal: crypto/tls: Provide access to stapled OCSP response in VerifyPeerCertificate callback proposal: crypto/tls: add Config.VerifyConnection callback Feb 26, 2020
@divjotarora
Copy link
Author

@divjotarora divjotarora commented Feb 28, 2020

@FiloSottile @katiehockman As part of this, would it be possible to improve some of the documentation for the tls.ConnectionState type? For example, one open question from me: when using tls.ConnectionState.VerifiedChains, can I assume that the 0th element of a chain is the peer certificate and the element directly after is the issuer? I originally made this assumption, but https://tools.ietf.org/html/rfc8446#section-4.4.2 says:

   Note: Prior to TLS 1.3, "certificate_list" ordering required each
   certificate to certify the one immediately preceding it; however,
   some implementations allowed some flexibility.  Servers sometimes
   send both a current and deprecated intermediate for transitional
   purposes, and others are simply configured incorrectly, but these
   cases can nonetheless be validated properly.  For maximum
   compatibility, all implementations SHOULD be prepared to handle
   potentially extraneous certificates and arbitrary orderings from any
   TLS version, with the exception of the end-entity certificate which
   MUST be first.

which makes it sound like I can't assume this? Some documentation improvements to explicitly state what each chain in VerifiedChains looks like would help tremendously.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 28, 2020

@divjotarora Very happy to make such usability changes. Could you open a new issue for it so we don't lose track of it?

@divjotarora
Copy link
Author

@divjotarora divjotarora commented Feb 28, 2020

@FiloSottile Thanks for the quick response! I filed #37572. Would you be able to answer my question about VerifiedChains either here or on that issue? I've tried reading through the client handshake code in crypto/tls a few times but I can't come up with a conclusive answer.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 4, 2020

Marking this discussion ongoing for the proposal minutes, but it seems like we're headed toward a likely accept. I just want to be sure to let the discussion finish.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 11, 2020

Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 11, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Mar 25, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 25, 2020
@FiloSottile FiloSottile added NeedsFix and removed Proposal-FinalCommentPeriod labels Mar 26, 2020
@FiloSottile FiloSottile removed this from the Proposal milestone Mar 26, 2020
@FiloSottile FiloSottile added this to the Go1.15 milestone Mar 26, 2020
@FiloSottile FiloSottile changed the title proposal: crypto/tls: add Config.VerifyConnection callback crypto/tls: add Config.VerifyConnection callback Mar 26, 2020
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Mar 31, 2020

To @katiehockman for implementation. Reminder that we decided to also set ConnectionState.ServerName on the client side, and once this is in I will also replace the VerifyPeerCertificate example with this, which will be so much cleaner.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 20, 2020

Change https://golang.org/cl/229122 mentions this issue: crypto/tls: add Config.VerifyConnection callback

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 24, 2020

Change https://golang.org/cl/239560 mentions this issue: crypto/tls: replace VerifyPeerCertificate example with VerifyConnection

gopherbot pushed a commit that referenced this issue Jun 24, 2020
Look at how much better it is!

Updates #36736

Change-Id: I53a314a103a42dd869c05823fa50f37d70f9d283
Reviewed-on: https://go-review.googlesource.com/c/go/+/239560
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
@golang golang locked and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix Proposal-Accepted Proposal-Crypto Unfortunate
Projects
Proposals
Accepted
Development

No branches or pull requests

6 participants