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: expose all presented certs in error type on handshake failure #48152

Open
ptagrawal opened this issue Sep 2, 2021 · 15 comments
Open

crypto/tls: expose all presented certs in error type on handshake failure #48152

ptagrawal opened this issue Sep 2, 2021 · 15 comments

Comments

@ptagrawal
Copy link

@ptagrawal ptagrawal commented Sep 2, 2021

What version of Go are you using (go version)?

$ go version
1.14.12

Does this issue reproduce with the latest release?

Not sure

What operating system and processor architecture are you using (go env)?

BusyBox OS, ARM architecture

go env Output
$ go env

What did you do?

This is related to another issue #48151 (comment)

What did you expect to see?

What did you see instead?

@seankhliao seankhliao changed the title Handshake should return a different error type that wraps the original x509 error in a struct that also includes all the presented certs. proposal: crypto/tls: expose all presented certs in error type on handshake failure Sep 2, 2021
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Sep 2, 2021

@gopherbot gopherbot added this to the Proposal milestone Sep 2, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Sep 8, 2021
@rsc rsc moved this from Incoming to Active in Proposals Oct 13, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Oct 13, 2021

Generally this sounds good. What's the proposed API? An error wrapper type for certificate validation failure that includes the peer certificates? Should it also be raise on a failed VerifyPeerCertificates or VerifyConnection?

How is this different from #48151 and why does that issue say "it's done wrong for client certificates"? We could generally use a more detailed "what did you do" / "what did you expect" / "what did you see".

@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

Ping @ptagrawal for answers to @FiloSottile's questions.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 10, 2021

ping @ptagrawal

@ptagrawal
Copy link
Author

@ptagrawal ptagrawal commented Nov 11, 2021

An error wrapper type for certificate validation failure that includes the peer certificates?
Yes, because, this would help debug what certs were presented while trying to make the connection.

Should it also be raise on a failed VerifyPeerCertificates or VerifyConnection?
Not sure about this, but I believe, if VerifyPeerCertificate is set, its upto up to parse and verify the certificate which I guess, we could anyways do in case of an error. So, maybe not required.

How is this different from #48151 and why does that issue say "it's done wrong for client certificates"?
So, I mentioned that because, looking at the logs I couldn't find the cert in the err.Error() where as the string preceeding that called out "failed to verify client certificate". I hoped the returned error would contain the certificate that was presented.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 1, 2021

It sounds like the answer is to suggest a new error wrapper type that will hold the certificates and be returned on validation failure?

Alternately, is it possible to get this info using a custom VerifyPeerCertificates? Or is there no way to invoke the usual code in that case?

@rsc
Copy link
Contributor

@rsc rsc commented Jan 5, 2022

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Jan 28, 2022

You can do it with VerifyConnection/VerifyPeerCertificates, although you have to maintain some state at a higher lexical scope and it's kind of hard to associate a VerifyConnection/VerifyPeerCertificates invocation with an error returned by Handshake.

Assuming it's possible to unwrap the Handshake return value to get an error as returned by VerifyConnection/VerifyPeerCertificates, one can implement the certificate check there and return a wrapped error.

Still, setting InsecureSkipVerify to bypass the normal checks, then reimplementing them just to return an error wrapper to get access to the certificates is quite burdensome.

I wouldn't be opposed to a new error wrapper with an UnverifiedCertificates field, that can be accessed with errors.As.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 2, 2022

It sounds like we need an API, then, specifically a name for the error.

type UnverifiedCertificatesError struct {
    UnverifiedCertificates []*x509.Certificate
}

func (e *UnverifiedCertificatesError) Error() string { ... }

Is this the API?

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 3, 2022

Maybe the error name is CertificateVerificationError?

@rsc
Copy link
Contributor

@rsc rsc commented Feb 9, 2022

OK, so it sounds like:

type CertificateVerificationError struct {
    UnverifiedCertificates []*x509.Certificate
}

func (e * CertificateVerificationError) Error() string { ... }

Does anyone object to that API?

@rsc
Copy link
Contributor

@rsc rsc commented Feb 16, 2022

Does anyone object to the API in the preceding comment?

@rsc rsc moved this from Active to Likely Accept in Proposals Feb 23, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Feb 23, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@FiloSottile FiloSottile removed this from the Proposal milestone Mar 2, 2022
@FiloSottile FiloSottile added this to the Go1.19 milestone Mar 2, 2022
@FiloSottile FiloSottile self-assigned this Mar 2, 2022
@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 2, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Mar 2, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/tls: expose all presented certs in error type on handshake failure crypto/tls: expose all presented certs in error type on handshake failure Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Development

No branches or pull requests

5 participants