Go implementation

[burke]

I'd PR this, but it has no code in common with the previous ruby implementation, so it wouldn't provide a whole lot of value.

Instead, see https://github.com/Shopify/ejson

Highlights:

• Implements EJSON v1.0 RFC except with nacl/box instead of PKCS7.
• Elliptic Curve Crypto rather than Prime Factorization (RSA)
  • DJB code rather than OpenSSL code
  • Relative difficulty of ECC problem allows much shorter keys and ciphertexts => more tractable EJSON files
• Makefile generates rubygem and debian package
  • Saner distribution to production nodes
• Simplified key management
  • /opt/ejson/keys directory contains files named with full public key, containing private key.
  • ejson decrypt looks up key referenced by ejson file in /opt/ejson/keys
• We can link this codebase into our container init script so that we don't have to shell out to ruby for decryption.
• Improved tests

I'll keep poking at this over the next couple of days, but it's basically ready for feedback. The code is pretty well compartmentalized and documented; I don't think it'll be very hard to grok what's going on. Start with README.md, then ejson.go and cmd/ejson/*.go.

Review/cc any combination of: @Shopify/stack, @Shopify/secops. It can easily wait until BFCM if you're busy.

[mutemule]

Implements EJSON v1.0 RFC except with nacl/box instead of PKCS7

:heart:

I am a huge fan of NaCl, and quite wary of the PKCS standards.

[EiNSTeiN-]

djb :100:

[burke]

I added manpages which are installed with the debian package and vendored with the rubygem (unfortunately rubygems can't install manpages globally). ejson help, ejson help encrypt, etc., will now launch man for the relevant manpage.

[eapache]

What happens if you try to encrypt:

"_starts_with_an_underscore": {
  "no_leading_underscore": "123123123123123123123"
}

I'm assuming it does the simple thing, and encrypts the value anyways, I'm just wondering if that's the nicest behaviour or if it should at least be documented?

[burke]

@eapache it's not in the manpage, but it is in README.md: https://github.com/Shopify/ejson/tree/go-impl#format (see point 6). I'll add it to ejson.5.

[burke]

(I want to err on the side of encrypting too much rather than too little, so this is the behaviour I want, though I'm open to argument on that point)

[eapache]

:+1: to explanation and documenting it

[burke]

https://github.com/Shopify/ejson/commit/61c5d48cb697255bc5f02a29fa5304670dde2f01

[burke]

Also fun: http://shopify.github.io/ejson/

[sirupsen]

@burke any plans to add schemas to the Go version?

[burke]

Yeah, but I plan to do that in a separate codebase, probably just a gem that plugs in to rails. There's no reason that bit has to clutter the ejson codebase.

[eapache]

• https://github.com/Shopify/ejson/blob/go-impl/ejson.go#L47 is there a reason to log.Fatal instead of just returning the error and letting the caller do what it wants?
• https://github.com/Shopify/ejson/blob/go-impl/ejson.go#L40 should we fail fast if we know the file isn't writable?
• https://github.com/Shopify/ejson/blob/go-impl/ejson.go#L119 dunno if this is a legit concern or not, but is there verification we could/should be doing to avoid path attacks with ..?
• https://github.com/Shopify/ejson/blob/go-impl/ejson.go#L122 dislike losing the original err value here

[burke]

1. :+1:, will change.
2. That's just there because Go doesn't really support implicitly rewriting a file with the current mode. I have to fetch the current mode and pass it to ioutil.WriteFile, that's all.
3. I hadn't really factored that into my threat model, but it's probably worth thinking about. I'll do something a little smarter.
4. That error is directly displayed to CLI users so I wanted the message to be specific, but it would be nice to embed the old err.Error() into the new error string. Will do.

[burke]

Actually, I think it's unreasonable to prevent traversal here: The only input value is the keydir, which is allowed to be anywhere on the system.

I made the other two changes.

[eapache]

The only input value is the keydir, which is allowed to be anywhere on the system.

The other input is the key; is there an escalation where a user can generate a fake public key to escape the key-dir?

[burke]

I don't think so; if they do, they've found a bug in printf's %x formatter, not so much in ejson. I don't personally feel we need to be quite that defensive. I like the train of thought though, that hadn't occurred to me.

[eapache]

• https://github.com/Shopify/ejson/blob/go-impl/ejson.go#L68 libraries should not unconditionally write to stdout; (probably worth copying sarama and using a package-global Logger that the importing package can set up appropriately?)
• https://github.com/Shopify/ejson/blob/go-impl/json/walker.go#L6-L10 annoying; after reading the rest of that walking code, I'm wondering if it might not be simpler to do the read-manipulate-dump with an explicit sort? It would force keys ascii-betical, which isn't great, but it would fix the diff issue without the annoying third-party scanner.

[burke]

1. Done; I just moved that printf up to the cmd/ejson package.
2. Surprisingly, with all the type assertions you need to accomplish it the read-manipulate-dump way in Go's type system, this method is actually simpler -- though I'll take another run at trying to simplify that code this afternoon.

Also, I really like that the current system preserves the input formatting. Explicit ordering would interfere with semantic grouping, might put _public_key in a weird place in the file, and would just generally violate user expectations.

I'll try to make the json stuff better, but I don't think there's a better option than the scanner.

[eapache]

• https://github.com/Shopify/ejson/blob/go-impl/crypto/boxed_message.go#L10
  • prefer \A and \z to ^ and $ - json can't contain embedded newlines I don't think but just as best-practice
  • we have known lengths for the first two :-separated groups and we know all three are base64-encoded; extra strictness is probably a good thing, unless it comes at the cost of too much readability
• https://github.com/Shopify/ejson/blob/go-impl/crypto/boxed_message.go#L26-L27 do these have to be pointers? I think it makes Load simpler if it can just decode right into the struct, and the boxedMessage is already a pointer.
• https://github.com/Shopify/ejson/blob/go-impl/crypto/crypto.go#L57 what is the reason for explicitly using /dev/random instead of whatever is provided by rand.Reader from crypto/rand?
• https://github.com/Shopify/ejson/blob/go-impl/crypto/crypto.go#L77 I think this is a typo, you mean given a "public" key?

[eapache]

• https://github.com/Shopify/ejson/blob/go-impl/cmd/ejson/version.go I have always hated this ruby convention, and I really don't think we need to follow it here. I'd be perfectly happy with this constant just at the top of main.go, but whatever. Not exactly a big deal :P
• https://github.com/Shopify/ejson/blob/go-impl/cmd/ejson/main.go#L15 I don't think this is necessary, and it will cause an error exit code even when the user explicitly passes --help
• https://github.com/Shopify/ejson/blob/go-impl/cmd/ejson/actions.go#L15 seeing it in use I think I'd prefer the explicit EncryptFileInPlace especially since DecryptFile does not overwrite in place, otherwise the expectation will be that they reverse each other

[eapache]

Lots of minor comments, but on the whole I really like this, :+1:

[burke]

• [x] \A \z
• [x] regex strictness
• [x] depointerize nonce and key (the reason I did this is that nacl takes pointers as args, and this is just what I refactored into, but you're right, it would be nicer to just & them when I pass them)
• [x] In Go 1.4+, crypto.rand uses /dev/urandom (really, getrandom without GRND_RANDOM). I don't love the idea of using a PRNG for key generation, especially when reading from /dev/random is so trivial. Thoughts?
• [x] private/public typo
• [x] The version file is actually because of this
• [x] os.Exit(1) after manpage: It's because that will only ever happen if the syscall.Exec fails. (syscall.Exec is a true exec). So the Exit(1) here is appropriate, as it indicates the manpage wasn't found or something.
• [x] Rename API to EncryptFileInPlace

[eapache]

especially when reading from /dev/random is so trivial. Thoughts?

It is, I think, the only thing keeping the library part here from being cross-platform (making the whole thing cross-platform would also require ripping out the man-page bits). Security-specific discussion has been taken offline.

[burke]

I replied to your email, but TL;DR: It's not the hugest of deals but I figured I'd do it right.

[burke]

I have no real desire to support this on Windows, so I don't feel too bad about reading from /dev/* directly in that regard.

[sirupsen]

I'll take a look at this, this is cool.

[burke]

Back to crypto/rand: https://github.com/Shopify/ejson/commit/629edb0e177a5b230f5b6357dcfb1e8ea3177139

[burke]

To summarize our discussion offline, /dev/urandom is fine but Go's PRNG is not. Go will use /dev/urandom on linux, and probably only falls back to the bad implenetation on Windows and maybe plan9.

[sirupsen]

First of all, it's a nicely commented and structured project. Good job!

Few comments reading through it:

• I don't think you need to supply valid permissions to writeFile from a stat, open(2) only uses permissions if O_CREAT is set iirc.
• We should take an -o flag, otherwise someone redirecting output to a file might run into nasty bugs as they try to parse an error as JSON.
• What's the reason for the Ruby gem? Shouldn't we just have people install the package? Is it due to OS X etc?
• Are public keys really always 32 bytes?
• I assume the reason for gojson because of the map randomization? And we don't want to sort it for people either, so we can't do that hack either.
• What happens when the key is an array? Why's it allowed?

With this, I'm not sure I'm seeing how schemas could be built on top of it in a reasonable way where everything isn't encrypted?

[burke]

• [x] permissions/writeFile: correct me if I'm missing something, but ioutil.WriteFile requires a mode: http://golang.org/pkg/io/ioutil/#WriteFile
• [x] yeah, ok, that's fair. I'll add an -o flag.
• [x] gem-vs-package: It makes distributing updates much easier, since we can stick it in Gemfiles. I could build an OS X package very easily though.
• [x] 32-byte keys: Yes. ECC is much safer per key-bit than RSA. Nacl/box in particular always uses 256-bit public and private keys.
• [x] gojson: Yup. I also like that the user can structure their JSON however they want and we don't interfere with that structure.
• [x] keys are never arrays, but you probably meant values. It's allowed because:
  • It would be more work to disallow it
  • It could make sense if you wanted to have multiple API keys or something: "tokens": ["token1", "token2"] for rotation or load-balancing or whatever.
• [ ] schemas: I was thinking we could have it assert that, for example, either of {,_}description is present, so for each metadata attribute you could choose whether or not to encrypt it. I'm going to write the validator on the plane tomorrow I think.

[burke]

Also, it's probably worth pointing out that gojson is actually the builtin encoding/json library, only with some lowercase letters made uppercase to expose the scanner API.

[eapache]

Also, it's probably worth pointing out that gojson is actually the builtin encoding/json library

...from some unspecified point in the past. Better than something hand-rolled I guess, but still a bit icky. Alas.

[burke]

...from some unspecified point in the past. Better than something hand-rolled I guess, but still a bit icky. Alas.

Agree on all counts.