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

Initial implementation #1

Merged
merged 27 commits into from Sep 3, 2019

Conversation

@stevehodgkiss
Copy link
Member

commented Sep 2, 2019

Uses EJSON as a library, and adds in KMS integration on top (same as EJSONWrapper), along with the env command from Shopify/ejson2env

Usage:

❯ ave someprofile -- ./build/bin/darwin-amd64 keygen --kms-key-id alias/alias --aws-region us-east-1
Private Key: 70b69adc70[snip]
EJSON File:
{
  "_public_key": "47fdcf0f1eceb435723fb0d18fa72d877c5b1c0e778e2f219a180446195fcc01",
  "_private_key_enc": "AQICAHjrmkq9Xr6590aQ07GTOh9g5XRV38fn7Gxlb8dXKzZO0gHMfruQJFvSfV9S6dU5MbX[snip]"
}

❯ ave someprofile -- ./build/bin/darwin-amd64 decrypt test.ejson --aws-region us-east-1
{
  "_public_key": "76d95d767ca4c15b3e712c819d30ed9f2956f0eba56f99dbf53989184cdf785f",
  "_private_key_enc": "AQICAHjrmkq9Xr6590aQ07GTOh9g5XRV38fn7Gxlb8dXKzZO0gExiLgYaaXx0M3VKH5TwIbe[snip]",
  "environment": {
    "test": "secret"
  }
}

@stevehodgkiss stevehodgkiss force-pushed the initial-impl branch from a33bebe to 957cb1a Sep 2, 2019

@petervandoros
Copy link

left a comment

LGTM 👍 with some comments for your consideration.

{
"_public_key": "6b8280f86aff5f48773f63d60e655e2f3dd0dd7c14f5fecb5df22936e5a3be52",
"_private_key_enc": "S2Fybjphd3M6a21zOnVzLWVhc3QtMToxMTExMjIyMjMzMzM6a2V5L2JjNDM2NDg1LTUwOTItNDJiOC05MmEzLTBhYThiOTM1MzZkYwAAAAAycRX5OBx6xGuYOPAmDJ1FombB1lFybMP42s7PGmoa24bAesPMMZtI9V0w0p0lEgLeeSvYdsPuoPROa4bwnQxJB28eC6fHgfWgY7jgDWY9uP/tgzuWL3zuIaq+9Q==",
"environment": {

This comment has been minimized.

Copy link
@petervandoros

petervandoros Sep 2, 2019

It appears like the format is the same as the ejson2env tool, which has a top level environment key with key/value pairs.

What's the thinking behind using the top level environment key instead of straight key/values at the top level like the ejson format?

IMHO, I don't think we need the environment key at all. If folks want to export all secrets as environment variables, then they can use the ejsonkms env command.

This comment has been minimized.

Copy link
@stevehodgkiss

stevehodgkiss Sep 2, 2019

Author Member

The decrypt command doesn't care if there's a nested environment key. It will just decrypt the file and output its contents. This example uses an environment key because it's demonstrated in the example below. The env command would be the main way we decrypt files using this tool, and that currently looks inside a special environment key (code is from Shopify/ejson2env).

This comment has been minimized.

Copy link
@petervandoros

petervandoros Sep 2, 2019

As mentioned in Slack, I though you wrote ejson2env 🤦‍♂ . And so i thought you could change the way it worked.

But it makes sense that you can add top level key/value pairs but only the key/value pairs under environment are used by the env command.

kms.go Outdated
func newKmsClient(awsRegion string) *kms.KMS {
awsSession := session.Must(session.NewSession())
awsSession.Config.WithRegion(awsRegion)
if flag.Lookup("test.v") != nil { // is there a better way to do this?

This comment has been minimized.

Copy link
@petervandoros

petervandoros Sep 2, 2019

I don't much Go, but I know the ruby aws sdk allows for mocking aws clients for testing. For Go, it appears like you can do that with https://docs.aws.amazon.com/sdk-for-go/api/service/kms/kmsiface/. Not sure how useful it is in this case.

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

@stevehodgkiss Is your comment related to checking if it should use the fake URL due to being in a test environment? I usually do something similar to what you have below it but only override the Endpoint option if it's set, otherwise use the default.

overriddenKMSURL := os.Getenv("AWS_KMS_URL")
if overriddenKMSURL != "" {
    awsClient.Endpoint = overriddenKMSURL
}

This comment has been minimized.

Copy link
@stevehodgkiss

stevehodgkiss Sep 3, 2019

Author Member

Yeah. I think I'll go with your suggestion @jacobbednarz. I was looking for a way to only make that behaviour accessible within tests, but it's a bit clunky this way. And it's nice to be able to run ejsonkms against the local-kms server for testing.

secret123
```

Note that only secrets under the "environment" key will be exported using the `env` command.

This comment has been minimized.

Copy link
@petervandoros
all: binaries
binaries: build/bin/linux-amd64 build/bin/darwin-amd64

build/bin/linux-amd64: $(GOFILES)

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

There is also mitchellh/gox if you don't want to roll this by hand or start to use cross platform libraries.

This comment has been minimized.

Copy link
@stevehodgkiss

stevehodgkiss Sep 3, 2019

Author Member

The approach here is basically a bare bones version of ejson's Makefile. Will check out gox.

actions.go Outdated Show resolved Hide resolved
actions.go Outdated Show resolved Hide resolved
actions.go Outdated Show resolved Hide resolved
actions.go Outdated Show resolved Hide resolved
actions.go Outdated
return nil
}

func envAction(ejsonFilePath string, quiet bool, awsRegion string) error {

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

More of a personal thing but you can also combine the type definitions to save some duplication if you don't care about re-ordering the parameters.

Suggested change
func envAction(ejsonFilePath string, quiet bool, awsRegion string) error {
func envAction(ejsonFilePath, awsRegion string, quiet bool) error {
"os"
"testing"

. "github.com/smartystreets/goconvey/convey"

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

Not something I've used but looks good 👍

This comment has been minimized.

Copy link
@stevehodgkiss
ejsonkms.go Outdated
)

// Keygen generates keys and prepares an EJSON file with them
func Keygen(kmsKeyID string, awsRegion string) (string, string, string, error) {

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

We can combine the type definitions here

Suggested change
func Keygen(kmsKeyID string, awsRegion string) (string, string, string, error) {
func Keygen(kmsKeyID, awsRegion string) (string, string, string, error) {

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

Something else to note here is that I will generally look at lots of return values like this (string, string, string, error) as perhaps a signal to pass back a particular type and then let the client pick off the methods or fields it needs. In this case, we can use my code example from another comment and instead pass back the struct defined or individual fields on it instead.

This isn't a big deal as this will work, it's just not using the type system to the fullest.

ejsonkms.go Outdated

k, ok := obj["_private_key_enc"]
if !ok {
return "", errors.New("Missing _private_key_enc field")

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

errors in Go generally start lowercased (which ends up matching stack traces, etc. nicely)

Suggested change
return "", errors.New("Missing _private_key_enc field")
return "", errors.New("missing _private_key_enc field")

This comment has been minimized.

Copy link
@patrobinson

patrobinson Sep 3, 2019

This check can be avoided by making obj a struct:

type ejsonFile {
    encryptedPrivateKey string `json:"_private_key_enc"`
}

Then when you unmarshal, it will throw an error if that field is not present.

ejsonkms.go Outdated
if !ok {
return "", errors.New("Missing _private_key_enc field")
}
ks, ok = k.(string)

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

Here, I would use the shorthand version of the Go error handling seeing how the line is quite short.

Suggested change
ks, ok = k.(string)
if ks, ok = k.(string); !ok {
return "", errors.New("Couldn't cast _private_key_enc to string")
}

This comment has been minimized.

Copy link
@patrobinson

patrobinson Sep 3, 2019

Same as above, json unmarshal will do the type coercion for you

ejsonkms_test.go Outdated Show resolved Hide resolved
// io.Writer.
func ExportEnv(w io.Writer, values map[string]string) {
for key, value := range values {
fmt.Fprintf(w, "export %s=%s\n", key, shell.Escape(value))

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

nice use of shell.Escape 💯

This comment has been minimized.

Copy link
@stevehodgkiss

stevehodgkiss Sep 3, 2019

Author Member

This came from ejson2env. Unfortunately it couldn't be used as a library like ejson could. https://github.com/Shopify/ejson2env/blob/master/cmd/ejson2env/env.go#L46

go.mod Show resolved Hide resolved
@@ -0,0 +1,14 @@
module github.com/envato/ejsonkms

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

Do we want this to be < 1.12? If so, we'll also need to run go mod vendor to vendor the dependencies locally to the project.

kms.go Outdated Show resolved Hide resolved
kms.go Outdated Show resolved Hide resolved
kms.go Outdated Show resolved Hide resolved
Name: "o",
Usage: "print output to the provided file, rather than stdout",
},
cli.StringFlag{

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Sep 2, 2019

Member

I wonder if it's possible to make aws-region a global flag instead of subcommand to remove some duplication 🤔

@jacobbednarz
Copy link
Member

left a comment

This looks great @stevehodgkiss! No blockers from me, just a bunch of Go-isms if you choose to use them 🙂

@patrobinson
Copy link

left a comment

Take a look at github.com/spf13/cobra for an easy way to structure cli go tools

Otherwise looks good

actions.go Outdated Show resolved Hide resolved
So(err.Error(), ShouldContainSubstring, "Missing _private_key_enc")
})
})
}

This comment has been minimized.

Copy link
@patrobinson

patrobinson Sep 3, 2019

You can achieve similar functionality with Example Tests which check the standard out is what is expected. But this works too

ejsonkms.go Outdated

k, ok := obj["_private_key_enc"]
if !ok {
return "", errors.New("Missing _private_key_enc field")

This comment has been minimized.

Copy link
@patrobinson

patrobinson Sep 3, 2019

This check can be avoided by making obj a struct:

type ejsonFile {
    encryptedPrivateKey string `json:"_private_key_enc"`
}

Then when you unmarshal, it will throw an error if that field is not present.

ejsonkms.go Outdated
if !ok {
return "", errors.New("Missing _private_key_enc field")
}
ks, ok = k.(string)

This comment has been minimized.

Copy link
@patrobinson

patrobinson Sep 3, 2019

Same as above, json unmarshal will do the type coercion for you

return nil, errNoEnv
}

envMap, ok := rawEnv.(map[string]interface{})

This comment has been minimized.

Copy link
@patrobinson

patrobinson Sep 3, 2019

I wont' harp on about it any more, but using structs makes this boilerplate type checking/coercion go away :)

This comment has been minimized.

Copy link
@stevehodgkiss

stevehodgkiss Sep 3, 2019

Author Member

I might address this in a future PR. The code is from ejson2env and is working fine.

@stevehodgkiss

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

Just to be clear, the code here is basically a fork of how things work in ejson/ejson2env (with the addition of the KMS stuff being the only "new" bit). Hence the use of certain libraries ejson uses (Convey, the cli tool etc).

Thanks for the reviews so far! Will take a look at addressing them today.

@orien
orien approved these changes Sep 3, 2019
Copy link
Member

left a comment

Caveat to review: My go-lang skills are pretty non-existent at this point.

@stevehodgkiss stevehodgkiss merged commit 55630da into master Sep 3, 2019

@stevehodgkiss stevehodgkiss deleted the initial-impl branch Sep 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.