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

Pass potentially Sensitive params as Sensitive #1018

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jan 20, 2022

In 699f944 the parameters started to accept Sensitive but it didn't default to Sensitive. They also weren't converting data coming from Hiera. This adds a data-in-modules setup and sets lookup_options for those. This means Kafo (which heavily relies on Hiera) will pass sensitive values.

Currently a draft so I can verify it end to end with the installer.

@ekohl
Copy link
Member Author

ekohl commented Jan 20, 2022

Oh, this is an interesting one:

2022-01-20 12:16:54 [ERROR ] [configure] Evaluation Error: Error while evaluating a Method call, Class[Foreman::Cli]: parameter 'password' expects a value of type Undef, String, or Sensitive[String], got Sensitive[Undef] (file: /usr/share/gems/gems/kafo-6.4.0/modules/kafo_configure/manifests/init.pp, line: 16, column: 39) on node centos8.wisse.example.com

Because it converts it to Sensitive unconditionally, it doesn't work.

@ekohl
Copy link
Member Author

ekohl commented Jan 20, 2022

After changing the data type this doesn't appear to work?

2022-01-20 12:24:09 [ERROR ] [configure] Evaluation Error: Error while evaluating a Function Call, lambda: parameter 'password' expects a value of type Undef, String, or Sensitive[String], got Sensitive[Undef] (file: /usr/share/foreman-installer/modules/foreman/manifests/cli.pp, line: 92, column: 18) on node centos8.wisse.example.com

@ekohl ekohl force-pushed the sensitive-v2 branch 4 times, most recently from 120efb3 to 447bf08 Compare January 20, 2022 15:36
In 699f944 the parameters started to
accept Sensitive but it didn't default to Sensitive. They also weren't
converting data coming from Hiera. This adds a data-in-modules setup and
sets lookup_options for those. This means Kafo (which heavily relies on
Hiera) will pass sensitive values.

The commit also missed settings.yml.erb and database.yml.erb which now
also unwraps if needed. Ideally this would be converted to EPP instead
but this fixes it in the short term. However, hammer_root.yml.epp
somehow doesn't properly render the sensitive data. That's why it's
unwrapper for now, just to make it work. A test case is added to prevent
future regressions.

It also changes the data type to accept Sensitive[Undef] which is needed
if Hiera unconditionally converts the value to Sensitive.

Fixes: 699f944
@ekohl
Copy link
Member Author

ekohl commented Jan 20, 2022

@cocker-cc this isn't pretty, but I couldn't get it to work otherwise. Please have a look.

@cocker-cc
Copy link
Contributor

Hiera itself will

  • either find the Password in YAML – then it will convert it to Sensitive[String] according to lookup_options
  • or it will not find the Password, then the Lookup will produce an Error

Hiera will not return undef (and neither will return Sensitive[undef] as a Consequence).

This means IMHO: If the EPP receives Sensitive[undef], then the Cast to Sensitive has to happen in Puppetcode after the Hiera-Lookup.

@ekohl
Copy link
Member Author

ekohl commented Jan 20, 2022

That does not reflect my findings in our installer. That is because our installer copies all answers and persists them. This results in Sensitive[Undef]. Hence why I chose these data types.

I also couldn't get https://puppet.com/docs/puppet/6/securing-sensitive-data.html#epp-templates to work. Using scope in epp resulted in:

Evaluation Error: A substring operation does not accept a String as a character index. Expected an Integer (file: /home/ekohl/dev/puppet-foreman/spec/fixtures/modules/foreman/templates/hammer_root.yml.epp

@cocker-cc
Copy link
Contributor

My Thoughts on this – without having had a deep Insight into the Code:

Here as an Example-Commit I see:

Variant[Optional[String], Sensitive[Optional[String]]] $password = $foreman::cli::params::password,

Sensitive[Optional[Any]] does not make Sense, because noone wants to accept Sensitive[undef] to pass in. (Optional allows undef)
Instead it should read like

Optional[Variant[String, Sensitive[String]]] $password

Furthermore:

$db_password = Sensitive(extlib::cache_data('foreman_cache_data', 'db_password', extlib::random_password(32)))

Eventually extlib::cache_data() is returning undef? In this Case you would get Sensitive(undef) – exactly, what your Error-Message complains about.
So instead it should read something like

$_cached_pw = extlib::cache_data('foreman_cache_data', 'db_password', extlib::random_password(32))
$db_password = $_cached_pw ? {
  undef   => undef,
  default => Sensitive($_cached_pw),
}

The other Variables are accordingly.

@ekohl
Copy link
Member Author

ekohl commented Jan 24, 2022

For now I want to revert it: #1019. This week we're planning to do module releases for Foreman 3.2 and it needs to be stable. Reverting it now is the quickest path. It can be added in the release candidate phase. Once it's reverted, I'll update this PR to make a complete implementation.

@jhoblitt
Copy link
Contributor

jhoblitt commented May 4, 2022

What about a function that converts Sensitive[undef] to simply undef but passes any other data through. It would be ugly to have to use such a function pretty much everywhere but it might work.

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.

4 participants