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

Easy way of decrypting fields #19

Open
daroczig opened this issue Aug 6, 2018 · 8 comments
Open

Easy way of decrypting fields #19

daroczig opened this issue Aug 6, 2018 · 8 comments

Comments

@daroczig
Copy link

daroczig commented Aug 6, 2018

Would you be open/interested in a PR introducing new YAML types for encrypted config values?

See eg https://github.com/daroczig/dbr/blob/master/R/config.R#L32

image

(Image source: http://bit.ly/user2018-dbr)

^^ Although that only supports Amazon KMS for now, but extending it with new types would be similarly easy. This way I could drop the custom functions from my dbr pkg and use config.

@jjallaire
Copy link
Member

@jcheng5, @wch, and @trestletech, how does this dovetail with other work/thinking we have done on this front? (I know we have it on our radar to support encrypted config using another mechanism, seems like the !kms extension described here should be orthogonal to that but wanted to check to be sure.

@trestletech
Copy link

I'm not aware of anything on our radar around encrypted config values on the R side. Looping @slopp in for broader perspective, though.

@jcheng5
Copy link
Member

jcheng5 commented Aug 6, 2018

I haven't thought much about this recently but this looks like a nice extension. @hadley @gaborcsardi may have more thoughts.

@gaborcsardi
Copy link

There are two things here IMO. Encrypted fields are one, and if there are good use cases for them, then they make sense. I can't really judge this.

Second, using the "type system" of YAML, so the parsed config will be typed. E.g. something like

shinydemo:
   host: !type kms |
     AAAbadcafe...

and then the parser could just add a "kms" S3 class to the host object. This seems easy enough to implement, and maybe not too bad to use. One would need to traverse the parsed list after parsing, though.

OTOH, AFAICT with !expr it is already possible to write this, and more:

shinydemo:
   host: !expr kms_decrpyt("
     AAAbadcafe...")

@daroczig
Copy link
Author

daroczig commented Aug 6, 2018

OTOH, AFAICT with !expr it is already possible to write this, and more

I'd rather avoid putting R function calls in the YAML config if possible so that we can keep that language-independent. Eg one could use the same YAML config both from R and Python -- implementing in both languages how to deal with the kms or other custom types (although the above example with the RMySQL::MySQL() is probably confusing from this point of view, sorry for that).

@gaborcsardi
Copy link

I'd rather avoid putting R function calls in the YAML config if possible so that we can keep that language-independent.

Yeah, that is a good point for sg like !type. Maybe namespace-d with package names:

shinydemo:
   host: !type AWR.KMS::kms |
     AAAbadcafe...

But then you'd need to walk the list to decrypt, or config would need to be able to pass handlers to the yaml package.

@daroczig
Copy link
Author

daroczig commented Aug 6, 2018

Maybe namespace-d with package names

Not sure: trying to keep the YAML language-independent, and for a ciphertext to be decrypted via Amazon KMS, kms type makes more sense to me than AWR.KMS::kms -- especially when I'm using the very same YAML config from Python :) And then config could know how to deal with kms, eg using AWR.KMS::kms_decrypt.

But then you'd need to walk the list to decrypt, or config would need to be able to pass handlers to the yaml package.

I think that's fairly easy and already done in 1-1 lines in my own config implementation at https://github.com/daroczig/dbr/blob/master/R/config.R#L12

@jjallaire
Copy link
Member

I agree that we should keep the YAML language-independent. I also agree that since we won't likely end up with more than a handful to a couple-dozen of these handlers (we currently have 0) that we don't need a formal namespacing mechanism per se. That said, since kms isn't a protocol or other generic term, I think you do need a preface of some kind (as one could imagine additional "key management services" from other providers_. You might want to use aws-kms

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

No branches or pull requests

5 participants