Skip to content
This repository has been archived by the owner on Apr 17, 2024. It is now read-only.

Go interfaces don't accept and pass through context.Context #255

Closed
balasanjay opened this issue Sep 20, 2019 · 6 comments
Closed

Go interfaces don't accept and pass through context.Context #255

balasanjay opened this issue Sep 20, 2019 · 6 comments

Comments

@balasanjay
Copy link

Some AEAD implementations make network calls, e.g. if you use envelope encryption with a key in a remote key manager. However, none of the crypto interfaces accept context.Context values, so we cannot apply deadlines, or cancellation.

@thaidn
Copy link
Contributor

thaidn commented Oct 3, 2019

Hi there,

Could you please share some code snippets that demonstrate your problem?

@balasanjay
Copy link
Author

There are many places where the Go code performs RPCs to backends. For instance, here: https://github.com/google/tink/blob/master/go/integration/gcpkms/gcp_kms_aead.go#L51.

There is no way for me to provide a deadline for this RPC, nor is there a way for me to cancel this RPC once it is in progress.

Ordinarily, functions that can make RPCs should accept an argument of type context.Context as their first argument and pass it on to any RPCs they make (you'll find that both the GCP and AWS APIs will accept contexts). In this way, cancellation and deadlines can be propagated through Tink to the underlying KMS.

@thaidn
Copy link
Contributor

thaidn commented Oct 8, 2019

Thanks.

Does it work for you if we add a constructor that accepts a context.Context parameter and uses it in encrypt()/decrypt()?

@balasanjay
Copy link
Author

@sophieschmieg and I discussed this on cl/272708100, and arrived at a solution like net/http.Request where there's a (s *SomeAEAD) WithContext(ctx context.Context) *SomeAEAD.

This is not ideal, as I believe it will force an allocation in the common case that this is used as the masterKey for one of the envelope AEADs, but I think on balance, this is the best option.

That said, for new APIs that could theoretically ever be implemented with an RPC internally (or an interface where any implementation might have that property), it should probably accept a Context directly where it makes sense. Also, if y'all ever do a breaking v2, this is a possible change that should be considered for being rolled-in.

@tholenst
Copy link
Contributor

tholenst commented Oct 8, 2019

In C++ and Java, we now have the ability to have multiple interfaces for a single key type.

If we implemented the same for go, we could in principle add a new interface ContextAead which has methods Encrypt and Decrypt taking a Context. This would allow to be backwards compatible.

However, this route will need some design work; I don't know how to do the analogue in go, and I suspect that the best ways would require generics in Go.

@tholenst
Copy link
Contributor

I will close this, a duplicate is at tink-crypto/tink-go#6.

This is because we find it easier to split things per languages, and we already have this duplicate (even though it was filed after this bug).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants