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

feat: add awskms provider #86

Merged
merged 3 commits into from
Jun 9, 2022

Conversation

philomory
Copy link
Contributor

An alternative implementation of the awskms provider, inspired by #41.

This implementation works the way you suggested it should in #41, with ref+awskms://BASE64CIPHERTEXT resolving to the cleartext encoded by said ciphertext. The indirection/ref+vals:// support from #41 is not included.

Note that while I have tested this code and it does work, I am not a Go programmer, so I can't really vouch for its quality. In particular, I didn't understand the intended purpose of having the GetStringMap function in addition to the GetString function; even with an empty implementation, the awskms provider does seem to work, but, I assume it's missing some functionality given that I didn't really properly implement one of the necessary methods.

return string(result.Plaintext), nil
}

func (p *provider) GetStringMap(key string) (map[string]interface{}, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if you could implement this too!
FYI, GetStringMap can be implemented on top of GetString like seen in https://github.com/variantdev/vals/blob/e059ff4fa22925a54038c69a62c51ee500d1c4d2/pkg/providers/s3/s3.go#L71-L82.
It's used when you instructed vals to parse the decrypted/fetched value as a YAML or a JSON object and retrieve only one value within the object (e.g. when you suffixed the URL with #/yaml_or_json_key/in/secret

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's fairly unlikely that anyone would use the KMS provider in this manner; since the ciphertext needs to be embedded directly into the document (and embedded repeatedly if you need to reference it multiple times), you'd usually just encode the value of the specific value needed at each location rather than encoding a much larger document (which means a correspondingly larger cipher text), and repeating that ciphertext in multiple places with different keys.

Nevertheless, I did go ahead and implement it, so if someone does want to use it this way, they can.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely a valid point! Thanks for your insight. I'm still glad you made it feature-parity with other providers so that it is at least consistent across the providers and users might potentially find a way to utilize it in practice.

@philomory
Copy link
Contributor Author

philomory commented Jun 8, 2022

@mumoshu I'm working on addressing your suggestion above, but, I've run into another wrinkle that will also (presumably) affect the SOPS base64 support. + is a potential character in a base64-encoded string, but, it messes up the URL-parsing of the ref+awskms://<data> urls. If I have ref+awskms://AQICAHgAP/ReuysYYYo+m0C9gl0GLJoHC4uGr..., the key string passed to GetString only ends up being AQICAHgAP/ReuysYYYo, i.e. everything up to the +. If you URL-encode the + in your vals reference, it does work... but it adds an additional wrinkle. The URL escape %2F (for /) is valid... but it's not valid in the authority section of a URL (the section after the scheme, up to the first /), apparently, only the path, query string, etc. That means that ref+awskms://AQICAHgAP%2FReuysYYYo%2Bm0C9g... isn't valid, because you can't have a %2F until after the first /. This could be fixed by telling people to only URL-encode the + and = characters in their Base64 input, and not the / characters, but that's kind of obnoxious to set up, so instead I'm introducing an extra leading slash in front of the URL-encoded base64-encoded input, e.g. ref+awskms:///AQICAHgAP%2FReuysYYYo%2Bm0C9g.... This appears to work.

I'll have the updated PR shortly (including support for KMS encryption context, key id, etc). In the mean time, if you can think of a better way to make base64 work in a URL, I'm all ears.

EDIT: Nevermind, I apparently forgot about URL-safe Base64 encoding. Unfortunately, there's no way to trick the AWS CLI to give you the urlsafe encoding up front, but it's still easier than URL-encoding everything.

@philomory
Copy link
Contributor Author

@mumoshu OK, that's fixed now.

@philomory philomory requested a review from mumoshu June 9, 2022 00:23
Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd say you're an incredible Golang dev already! Thanks a lot for your contribution ☺️

README.md Outdated Show resolved Hide resolved
@mumoshu mumoshu merged commit b8264f6 into helmfile:master Jun 9, 2022
@philomory philomory deleted the feature/provider/awskms branch June 9, 2022 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants