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

first pass at simple encryption #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

binford2k
Copy link

This isn't intended to be a complete solution yet, but I wanted to give
you a proof of concept I banged out. The encryption is completely
transparent to the end user. To use it, simply pass the parameter of
encrypted => true and it's encrypted & decrypted for you.

Example usage:

datacat_fragment { 'encryption test':
  target => $target,
  data   => {
    name => 'fred',
  },
  encrypted => true,
}

This isn't intended to be a complete solution yet, but I wanted to give
you a proof of concept I banged out. The encryption is completely
transparent to the end user. To use it, simply pass the parameter of
`encrypted => true` and it's encrypted & decrypted for you.

Example usage:

``` Puppet
datacat_fragment { 'encryption test':
  target => $target,
  data   => {
    name => 'fred',
  },
  encrypted => true,
}
```
@richardc
Copy link
Owner

This does look very cool as a starting place. Thanks.

It'll be desirable to extend this to structured data values, though I remember ruby json parsers/emitters are inconsistent between versions when dealing with a simple JSON document like "this is a string", so the obvious 'encode as JSON, encrypt'/'decrypt, JSON parse' pairing may not be portable.

I'll be travelling for the next few days, but will try and pick this up next week.

@binford2k
Copy link
Author

Yeah, I considered the JSON dance and figured it would be better to just get the POC up to start thinking about. Obviously, this also only works on the CA master too, unless some sort of cert distribution is done. 10am with a couple beers in me[1] wasn't really the time to be making those design decisions though.

[1] http://xkcd.com/323/

@binford2k
Copy link
Author

Well nuts. @adrienthebo just informed me that validate is called client-side. I'll investigate alternatives.

The cop-out way would be to call a puppet function on the data being passed in, but I'd like to avoid that if possible.

@richardc
Copy link
Owner

I wonder if we can do something with generate to mutate the resource, iirc it fires farily late in catalog assembly.

@adrienthebo
Copy link

@adrienthebo
Copy link

Are there any other ways that I can crush your hopes and dreams?

@@ -17,4 +17,19 @@
newparam(:data) do
desc 'A hash of data to be merged for this resource.'
end

newparam(:encrypted) do
desc 'Whether the data values should be encrypted over the wire.'

Choose a reason for hiding this comment

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

s/over the while/in the compiled catalog/

This conditionally loads my node_encrypt library, and then allows
users to transparently node_encrypt() any data items.
@binford2k
Copy link
Author

Updated to use my module for encryption. Ping @richardc

if defined?(Puppet_X::Binford2k::NodeEncrypt)
fragments.each do |fragment|
fragment[:data].each do |key,value|
next unless Puppet_X::Binford2k::NodeEncrypt.encrypted?(value)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not keen on this particular check as it smells like it's checking for a magic number, and it also seems too soft. I think I'd sooner have an additional explicit flag on the fragment, something more like:

fragments.each do |fragment|
  if fragment[:encrypted]
    fragment[:data].each do |key,value|
      fragment[:data][key] = Puppet_X::Binford2k::NodeEncrypt.decrypt(value)
    end
  end
end

Then it's less automagic and more explicit.

Copy link
Author

Choose a reason for hiding this comment

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

It's checking for the guard string -----BEGIN PKCS7-----. I can't figure a more robust way to do this transparently to the end-user. I'd totally be happy if you found a good way to do this.

I suppose you could require an additional encrypted => true parameter to be set, but that might make it awkward because all values or no values would be marked as encrypted.

Copy link
Author

Choose a reason for hiding this comment

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

maybe we could do both approaches, flag the fragment with a parameter, then iterate each value like I'm currently doing. Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

That's what my code fragment was meant to be showing, unless you mean it could optimistically check if the value looks encrypted so a warning can be issued if it wasn't flagged as being encrypted? More like:

fragments.each do |fragment|
  fragment[:data].each do |key,value|
    if Puppet_X::Binford2k::NodeEncrypt.encrypted?(value)
      if fragment[:encrypted]
        fragment[:data][key] = Puppet_X::Binford2k::NodeEncrypt.decrypt(value)
      else
        warn "Fragment looked encrypted but wasn't flagged as being encrypted"
      end
    end
  end
end

@richardc
Copy link
Owner

richardc commented Jan 4, 2016

Cool that you've found a way, I have one comment on the approach though, I think it's better to fail harder/earlier than to try and maybe decrypt a thing if it looks like it might be encrypted.

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.

3 participants