Skip to content

Use Deferred node_encrypt function on passwords#18

Open
genebean wants to merge 1 commit intomainfrom
use_node_encrypt
Open

Use Deferred node_encrypt function on passwords#18
genebean wants to merge 1 commit intomainfrom
use_node_encrypt

Conversation

@genebean
Copy link
Copy Markdown
Contributor

@genebean genebean commented Feb 19, 2019

This is a breaking change for Puppet < 6 as it requires the use of the Deferred type. The idea here is to limit the exposure of passwords passed to the user resource as much as possible. This is especially important on Windows nodes as their passwords are moved around in plain text ( 😿 ).

Note: This is built on top of #15 - do not merge before that one. The real diff is available at release_100...use_node_encrypt

@mikkergimenez
Copy link
Copy Markdown
Contributor

This looks reasonable to me, how are we planning on testing it?

@genebean
Copy link
Copy Markdown
Contributor Author

Testing via spec testing here then validation in our environment

@genebean
Copy link
Copy Markdown
Contributor Author

@binford2k - If you have time I could use some help understanding how to mock the encrypt and decrypt functions... I thought I had it figured out but nothing I have tired works.

@dylanratcliffe
Copy link
Copy Markdown

Well this is the first time that I've seen node_encrypt::secret() and it is cool as shit. Deferred functions really is exactly what this module has been waiting for to make it just a seamless beautiful experience. I love it @binford2k.

The way I mock functions like this in onceover is by doing a:

let(:pre_condition) {
  pp = <<-END
    function node_encrypt::secret ($foo) { $foo }
  END
}

@genebean
Copy link
Copy Markdown
Contributor Author

Thanks @dylanratcliffe! That works locally :) Can you remind me what the pp part is / does?

@genebean genebean marked this pull request as ready for review February 19, 2019 22:02
@genebean genebean changed the base branch from release_100 to master February 19, 2019 22:04
@genebean
Copy link
Copy Markdown
Contributor Author

@mikkergimenez how do you feel about the testing changes that are part of this PR? Do they cover things well enough?

@genebean
Copy link
Copy Markdown
Contributor Author

Note: This is built on top of #15 - do not merge before that one. The real diff is available at release_100...use_node_encrypt

@dylanratcliffe
Copy link
Copy Markdown

@genebean The pp part is just a variable, it means nothing at all. The let() method is relying on the block to return a value, and in Ruby blocks will always return whatever the last statement returned. In the case of variable assignment the returned value is that of the left operand. I think you could get rid of the variable assignment entirely and have have the heredoc and it would probably still work. The only reason I have it there is because that's what the person I copied did and I didn't understand it at the time.

@genebean genebean force-pushed the use_node_encrypt branch 2 times, most recently from 0ceef7d to e4fbc70 Compare February 21, 2019 16:02
@genebean
Copy link
Copy Markdown
Contributor Author

@dylanratcliffe Turns out Rubocop said I had to kill off the pp anyhow :) The result is

let(:pre_condition) do
  'function node_encrypt::secret ($foo) { $foo }'
end

@genebean genebean self-assigned this Feb 21, 2019
@genebean
Copy link
Copy Markdown
Contributor Author

This needs testing before merging.

@genebean genebean force-pushed the use_node_encrypt branch 2 times, most recently from 7053bcd to 6b5cc83 Compare February 21, 2019 19:54
@genebean
Copy link
Copy Markdown
Contributor Author

tested on our canary nodes then verified I could still log in with name and password

@genebean
Copy link
Copy Markdown
Contributor Author

@dylanratcliffe / @binford2k - I have rearranged a couple of things here and the seem to be working based on running this branch on some canary nodes. Having said that, now that I am taking a variable that is a Sensitive string and wrapping it in Deferred the way I am mocking node_encrypt is no longer working. Below is a copy of how I am currently mocking this... would love any help you can provide

let(:pre_condition) do
  'function node_encrypt::secret ($foo) { $foo }'
end

The failing test is of account::user which has this param: Optional[Sensitive] $password = undef, In that defined type some testing is done then it calls the OS-specific defined type. If a password was provided it is wrapped like so before being passed on: node_encrypt::secret($password) The OS-specific manifests each have this param: Optional[Deferred] $password = undef,

Aside from the testing issue I am not totally sure that I am correctly using node_encrypt::secret() here. Naturally, the goal is to keep the password either out of the catalog or encrypted if it is in there.

This is a breaking change for Puppet < 6 as it requires the use of the
Deferred type.
@genebean
Copy link
Copy Markdown
Contributor Author

Just rebased. Will work on getting this merged soon (it got forgotten about)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants