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

proposal: crypto/x509: add Context to VerifyOptions #61576

Open
rolandshoemaker opened this issue Jul 25, 2023 · 5 comments
Open

proposal: crypto/x509: add Context to VerifyOptions #61576

rolandshoemaker opened this issue Jul 25, 2023 · 5 comments
Assignees
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@rolandshoemaker
Copy link
Member

Currently, there is no way to cancel or put a deadline on certificate verification operations, which due to the graph nature of chain building (and the associated signature verifications) can be somewhat expensive.

I propose we add a context.Context to the VerifyOptions struct, which would then be checked during path building. This can then be populated during TLS handshakes using the existing context.

type VerifyOptions struct {
	...

	// Context is the context used for certificate chain building and
	// verification. If nil, verification time is unbounded.
	Context context.Context
}

cc @FiloSottile

@rolandshoemaker rolandshoemaker added this to the Proposal milestone Jul 25, 2023
@rolandshoemaker rolandshoemaker self-assigned this Jul 25, 2023
@Jorropo
Copy link
Member

Jorropo commented Jul 25, 2023

To me it is confusing to cancel code that do not block on IO.
How granular should opts.Context.Err() checked by the implementations ?
Between every elements in the chain ?
Between every round of cryptography ?
Should it use internal runtime options and a context callback to raise a panic (that would be recovered) inside the crypto code from the canceling goroutine (so as fast as possible) ?

@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jul 25, 2023
@mateusz834
Copy link
Member

Is there any reason for not doing it this (common) way instead?

func (c *Certificate) VerifyContext(ctx context.Context, opts VerifyOptions) (chains [][]*Certificate, err error) {

Is this going to involve goroutine creation for platform verifiers? Or is this not going to be used there?

@rolandshoemaker
Copy link
Member Author

I don't have a clear memory of why we didn't consider VerifyContext, but perhaps @FiloSottile does. I cannot come up with any strong reason against it versus using the VerifyOptions, and it clearly does have significant precedent.

For platform verifiers, I don't think we want to spawn goroutines and return early, since the work will still be happening, which may be expensive, and could not reasonably be cancelled. As with a couple of features of the verifiers, at least initially contexts would only be a native verifier only feature.

Since contexts are not currently wired into cryptography code (and it seems unlikely we'll do that, at least at this point) the context will only be consulted between chain building operations (i.e. each time we consider a new certificate for addition to an in process chain, essentially at each invocation in a DFS).

@rittneje
Copy link

My understanding is that Verify (and by extension, VerifyContext) returns all the chains that it can build. However, in most cases (e.g., TLS handshake) you really only need one valid chain. So if the context expires in the middle of chain building, but we have at least one valid chain by then, what is the expected behavior?

@FiloSottile
Copy link
Contributor

I don't have a clear memory of why we didn't consider VerifyContext, but perhaps @FiloSottile does. I cannot come up with any strong reason against it versus using the VerifyOptions, and it clearly does have significant precedent.

Nope, not sure why we went with VerifyOptions.Context instead of VerifyContext. Maybe just because it was there and we're used to adding things to it. VerifyContext sounds straightforwardly like the right choice, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

No branches or pull requests

6 participants