search

tink-crypto / tink-go-awskms

Extension to Tink Go that provides AWS KMS integration
https://developers.google.com/tink
Apache License 2.0
6 stars 3 forks source link

feat: support the option to use aws-sdk-v2 #5

Closed spennymac closed 1 month ago

spennymac commented 5 months ago

With AWS announcing in January that the v1 AWS SDK for Go will have its support ending on July 31, 2025, I decide to take a stab at introducing non-breaking change to support the v2 SDK.

I chose to propagate all the AWS SDK options to the client and remove any support for parsing the region from key arn or loading credentials from csv/ini files. All of this extra functionality is still available through the sdk options, refer to AWS's documentation .

Let me know what you think.

juergw commented 5 months ago

Thanks for looking into implementing this. We certainly should have support for V2.

But this implementation looks overly complicated to me. Would it be better to simply have one new ClientOption "WithKMSV2"? And then require that the user always needs to create these KMS v2 client themselve. Then, we don't need to handle the options of these clients ourselves.

The timeout could be another ClientOption. (But I guess the timeout will not be needed anymore once https://github.com/tink-crypto/tink-go/issues/6 is resolved.)

spennymac commented 5 months ago

👋 Thank you for your feedback!

But this implementation looks overly complicated to me. Would it be better to simply have one new ClientOption "WithKMSV2"? And then require that the user always needs to create these KMS v2 client themselve. Then, we don't need to handle the options of these clients ourselves.

This is what I initially planned, and then pivoted when I realized the current v1 implementation offered up ways to specify credentials, and we now need the timeout. The implementation tries its best to separate all options, ensuring that the code is not burdened with the responsibility to constantly check whether a v1-specific option has been already been set, it only needs to check this one place.

Removing the load and kms options is a easy change, which leads us to the timeout 🤔

The timeout could be another ClientOption. (But I guess the timeout will not be needed anymore once tink-crypto/tink-go#6 is resolved.)

Would this timeout now apply to the v1-sdk as well, since it would become a 'v1 option'? If so, we would have to change the v1 Encrypt/Decrypt calls to use their WithContext variant and make sure we aren't changing any current timeout behavior. Or add new branches to use WithContext if the timeout is set. If not, I'm not sure of a good way to let users know this is only valid for v2, simply naming it with some v2 prefix/suffix?

Finally, would you say a v2 implementation would be blocked until [tink-crypto/tink-go#6] is complete?

spennymac commented 5 months ago

Would this be more inline with what you are expecting? https://github.com/spennymac/tink-go-awskms/pull/2/files

juergw commented 5 months ago

Yes. But I think it should be even simpler. I don't think we want to add the timeout, because I think that will not be needed anymore once we add the context directly to the remote encrypt/decrypt calls, which we will add soon. I think the additional API really should just be a WithV2KMS option. And I don't think we need the "Aead builder", I'd prefer to not refactor the code, just add the new KMS and don't change the old code path.

spennymac commented 5 months ago

Yes. But I think it should be even simpler. I don't think we want to add the timeout, because I think that will not be needed anymore once we add the context directly to the remote encrypt/decrypt calls, which we will add soon. I think the additional API really should just be a WithV2KMS option. And I don't think we need the "Aead builder", I'd prefer to not refactor the code, just add the new KMS and don't change the old code path.

Hmm.. alright. Seeing as how the timeout is needed, I'm not sure how best to do this. I'll hold off until the context change comes through, and then it wont be needed.