Changes in Tink v1.7.0 restrict the ability to import existing key material
Closed this issue · 5 comments
Describe the bug:
The Go library for Tink v1.7.0 changed the Bazel visibility of some packages (such as built protobuf packages) to __subpackages__, restricting their use outside of Tink. Access to these packages is required in order to import existing key material into a Tink keyset.
What was the expected behavior?
Congrats on the 1.7.0 release! 🎉 We're looking to upgrade from 1.6.1 to take advantage of some of the new manager functionality and expanded key types.
One of our existing workflows revolves around importing an existing key (e.g. an ECDSA P-256 key) into a Tink keyset. That is, we don't want Tink to generate this key for us. Instead, the key is provided to us, and we create a keyset from it / add it to an existing keyset.
To do this, we take the key value and marshal it into the expected serialized protobuf format. Essentially we're just reproducing some of the behavior from a key managers NewKeyData call, providing our own data instead.
Here's an example playground: https://go.dev/play/p/HHi888p_3bP.
This no longer works with 1.7.0 since many packages are now restricted to __subpackages__ like the one shown here.
We would expect Bazel to be able to find and use packages within go/proto. Alternatively, if there's a different way to create Tink keys using existing key material that's fine, too! It's possible that I'm missing a workflow here.
How can we reproduce the bug?
If you take the logic in the linked Playground above and try to run it as a test with Bazel support with 1.7.0 then you'll encounter errors that indicate packages such as @com_github_google_tink_go//proto/common_go_proto:common_go_proto are not reachable. The test completes successfully in 1.6.1.
Do you have any debugging information?
Here's an example visibility rule that is restricting the described workflow: https://github.com/google/tink/blob/1.7/go/proto/ecdsa_go_proto/BUILD.bazel#L18
Provide your version information:
- Language: Go
- Version: 1.7.0
- Environment: N/A
Is there anything else you’d like to add?
I appreciate all the hard work y'all are doing to make Tink a great library. Keep up the great work!
Thank you for reporting this. We will discuss how we can help you in the team.
Unfortunately, in the past, Tink had a bit of an inconsistent stance on this: on one hand protos were considered internal API, but sometimes we didn't pay much attention to it. On the other, it wasn't really possible to import and export keys any other way (as you say).
We are currently working on changing this, but the work is not yet in 1.7; we only have it for one key type in Java so far.
We will discuss the options in the team, I don't think there is a very good option yet, but I would like to provide you with a work around until we have an official way to import and export keys in go (this might even be to undo the PR you listed).
Thank you @tholenst! That sounds wonderful, and I really appreciate y'all being open to supporting some workflow for importing existing key material. We definitely see benefits from using Tink's overall key management strategy, even if we occasionally need to work with key material that wasn't itself generated by Tink.
I'll defer to y'all completely on the route y'all would want to take to reintroduce this compatibility, but please don't hesitate to let me know if there's anything I can do on our side to help!
It is weird that you were able to use it in version 1.6.1, because also then it was not public, see here:
https://github.com/google/tink/blob/v1.6.1/go/proto/BUILD.bazel
Only tink_go_proto was public in 1.6.1 public, and it is still public in v1.7.
On the master branch, some targets were public for a while I made a mistake when I updated the build files. I fixed that here:
bff5f8e
But that doesn't include your target.
But these visibility restrictions only apply to users of bazel, users that don't use bazel can access them. This difference is certainly not ideal. We have now decided to make these targets public, since we don't want to force users to not use bazel, and we currently don't have a better option than to directly use the proto files.
Hey there!
I'm sorry I missed this update.
It is weird that you were able to use it in version 1.6.1
I did some digging and I agree. I'll be the first to admit I'm the furthest thing from a Bazel expert, but I don't see the BUILD.bazel I would expect to see in our 1.6.1 tree, though it was added quite some time ago so it's possible there's just some Bazel plumbing that's been updated on our end such that the upgrade brings in those build files.
We have now decided to make these targets public
Fwiw, I do think this is still a good option. I'll continue to dig on my side to figure out why this hasn't impacted us before, but I agree completely with the reasoning you outlined.
I'll update here if I have any new info on our side, but thanks again for looking into this! It's much appreciated. The more I dig into the recent changes in Tink, the more excited I get since there have been some really need additions. Keep up the great work! 🎉