Add YC KMS provider support #1097
Conversation
kuzaxak
changed the title
astreter
force-pushed
the
yc-kms-support
branch
3 times, most recently
from
5017148 to
ae59188
Compare
kuzaxak
changed the title
|
@hiddeco Could you review pls? What else should be added \ updated? |
|
Have put this on my review list but it might take some time before I can take a proper look at it, as I am not too familiar with the YC KMS client. |
Thank you! |
|
One of the things I can see by quickly scanning this is that it seems to lack a way to overwrite the credentials being used, as was made possible by all my rewritten implementations. This has proven to be of importance when you want to integrate SOPS in another Go application while not relying on runtime environment variables, you can probably get an idea of what I am aiming at by looking at e.g. https://github.com/mozilla/sops/blob/develop/azkv/keysource.go#L97-L111 (or any of the other key source implementations). In addition: generally speaking, every time a request is made the key is reconstructed for that specific request anyway (as can be seen in |
Thank you for detailed feedback. I didn't understand that part and decided to skip it. I think it is easy to fix and we will do that.
Does it mean that we should move @astreter cc |
Yes, I think it would be best to move the client out and replace it with authentication configuration. |
|
@hiddeco Implemented ability to pass credentials programmatically and cleaned up tests a bit. Client instance is created every time as you suggested. To implement tests properly I used GCP approach with passing grpcConn directly. |
|
@hiddeco Hey. Small ping :) We fixed your comments and it will awesome if you could check them. |
|
Have no rights to merge at present, which means my review wouldn't mean much until this is resolved. Which is something I am actively working on together with others. |
|
Unfortunately, I think that all PRs are frozen until the issue is resolved. |
|
any news here? |
|
Any news? :) |
|
Can this be merged? |
|
@kuzaxak do you feel like updating this PR? |
Yes, I can look into it next year |
kuzaxak
force-pushed
the
yc-kms-support
branch
from
e12bd43 to
9904363
Compare
SDK documentation was found [here][1]. Service implemented in the same way as GCP KMS. This is initial commit which provides basic functionality to encrypt and decrypt files. Operations with keys via CLI wasn't added, will be added later. Tests and documentation will be added in the next commit. Resolves issue getsops#1052 Signed-off-by: Vladimir Kuznichenkov <[email protected]> [1]: https://cloud.yandex.com/en/docs/kms/tutorials/encrypt/sdk#auth
gRPC calls `Encrypt` and `Decrypt` are mocked with dummy responses using base64 instead of actual encryption. Since YC KMS response with binary data we are storing it as base64, together with mocked server where we encode it one more time instead of actual encryption we are using double encoding and double decoding in tests cases. Signed-off-by: Vladimir Kuznichenkov <[email protected]>
kuzaxak
force-pushed
the
yc-kms-support
branch
from
9904363 to
d2335e9
Compare
|
Got a bit of time during Christmas. Rebased the changes, tested locally, works fine. Could you please review @felixfontein CC @chaporgin |
|
Hey, @felixfontein can you please have a look when you have time? Thank you in advance! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return nil, fmt.Errorf("cannot create YC KMS service: %w", err) | ||
| } | ||
|
|
||
| decodedCipher, err := base64.StdEncoding.DecodeString(string(key.EncryptedDataKey())) |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
|
Looking forward to the PR being approved. Thanks for the contribution. 🙌 |
|
We have used it successfully with other providers and want to use YC KMS with SOPS. Thanks for your support. :) |
I'm very sorry, but I currently have only very limited time, and I will very likely not spend it on reviewing this (or any other new cloud based KMS PR) PR. Maybe someone else from the maintainer team has time, but I don't. (Generally I would prefer to have a pluggable system where new KMSes (or stores) can be added without having to modify SOPS itself. That would simplify maintaining SOPS and at the same time make supporting new KMSes, or adding larger features to existing ones, a lot easier. Unfortunately that requires quite some legwork, and I currently don't have time to work on that as well...) |
|
The timing issue is completely understandable, and I appreciate you keeping us informed. |
|
+1, also waiting for merge of this pr |
|
@kuzaxak , can u please resolve conflicts ? |
11 participants
SDK documentation was found here. Service implemented in the same way as GCP KMS.
THis PR adding support for YC KMS as an encryption provider.
Resolves issue #1052