Per-record transit key names #75
Conversation
This adds support for dynamically computing a transit key name on each encrypt/decrypt request, which allows an application to use a separate key per record, or using an encryption key per belongs_to relation.
|
|
||
| vault_attribute :credit_card, | ||
| key: ->(record) { "user_#{record.user.id}" } | ||
| ``` |
There was a problem hiding this comment.
@chrisarcand While writing this example, I realized that maybe it should be required that a proc take the record as an argument, otherwise it won't have the context to deterministically create a key name for the record.
| belongs_to :user | ||
|
|
||
| vault_attribute :credit_card, | ||
| key: :vault_key |
There was a problem hiding this comment.
Accepting a symbol is potentially backwards-incompatible, as existing behavior for passing a symbol would be to coerce it to a string.
We could either:
- Bump the version such that we note this is a breaking change.
- Check if the object responds to the given method. If not, coerce it to a string and print a warning to stdout, and remove this behavior in a future version.
There was a problem hiding this comment.
For me, 1) would be a better option. It's true that supporting both as per option 2 would be nicer for people expecting symbols to coerce, but it would be odd for proper uses where symbols are intended to invoke methods, but instead get coerced to string. It seems like in that case it would be possible that a bunch of data would end up getting encrypted using a static key instead of the intended dynamic one.
I think pinning the gem version to retain old behavior is reasonable, and bumping the version up for the new functionality is probably the way to go. Also, just IMO, passing a symbol and expecting/assuming it gets converted to a string is pretty weird, and AFAIK this was just coincidence that it worked, and not an intentional design decision.
There was a problem hiding this comment.
Here's an example diff of option 2 for posterity: https://gist.github.com/justincampbell/5f2ca1ad78ff3d6ea8eb33bbb7989297
There was a problem hiding this comment.
Especially given that this gem is still marked < 1.0, I'm also in favor of option 1, and agree with Ryan sentiment about accidental encryption.
You can also guide users, too - perhaps you should tag on to the expected NoMethodError here about this behavior. (to be removed in a few versions).
👍🏻
There was a problem hiding this comment.
Looking great! Left some comments about the BC concerns. For transparency, we've been chatting about reducing this to accept a Proc only as well. I'm on board with that, or keeping this PR how it is, given we follow option 1) mentioned in the rollout plan comment above.
| belongs_to :user | ||
|
|
||
| vault_attribute :credit_card, | ||
| key: :vault_key |
There was a problem hiding this comment.
For me, 1) would be a better option. It's true that supporting both as per option 2 would be nicer for people expecting symbols to coerce, but it would be odd for proper uses where symbols are intended to invoke methods, but instead get coerced to string. It seems like in that case it would be possible that a bunch of data would end up getting encrypted using a static key instead of the intended dynamic one.
I think pinning the gem version to retain old behavior is reasonable, and bumping the version up for the new functionality is probably the way to go. Also, just IMO, passing a symbol and expecting/assuming it gets converted to a string is pretty weird, and AFAIK this was just coincidence that it worked, and not an intentional design decision.
There was a problem hiding this comment.
Looks good, though either docs stating the necessary changes to upgrade to the next version and/or raising an error with a bit more context when a symbol is given and a method doesn't exist (because it could be a user running in to this breaking change) would be good to add before shipping (in another PR before a release, or here, whatever)
|
@justincampbell I suggest a slightly different approach here, which is to use Vault's ability to have derived keys. (https://www.vaultproject.io/api/secret/transit/index.html#derived). The documentation is a bit subtle here, and I will nudge the Vault team. In essence, there are concrete keys that Vault needs to hold on to, those are the named keys in the transit backend, anything like "transit/keys/foo". For each key we need to hold on to some meta data, and there is some upper bound of reasonable keys to manage (16K, maybe 32K), but not unlimited. My concern with doing a dynamic key per record is you can quickly force Vault into having millions of keys. To handle this, Vault supports derived keys. These are keys that aren't stored, but are dynamically derived from a unique context. The common pattern is to use a concrete base key, such as "foo". From there, you provide a unique context, maybe a user UUID or something, which derives a sub key from foo. That key is then used to perform the operation and then thrown away. This way, Vault only needs to manage a reasonable number of physical keys. I would recommend using that, since otherwise we run the risk of creating an enormous number of keys in Vault. |
|
Thanks everyone! Closing this in favor of #78. |
This adds support for dynamically computing a transit key name on each encrypt/decrypt request, which allows an application to use a separate key per record, or using an encryption key per belongs_to relation.