Accept Datatype Sensitive for all Secrets #960

cocker-cc · 2021-06-21T06:55:25Z

No description provided.

ekohl

I haven't really reviewed it in depth. Please don't align parameters.

Other than that I am open to it. We'll need to verify if our installer handles it properly and otherwise patch that.

I do feel like a Puppet function to unwrap if needed makes sense.

manifests/cli.pp

manifests/init.pp

manifests/database/postgresql.pp

ekohl

I'm inclined to prefer this in separate PRs. The reason is that I don't know if our installer handles Sensitive well. Another is that we have some tools that read the sensitive values from our answers. For example, they read a DB password. I'm not saying this is good, but it's a concern that we need to address. Consider it a heads up that this is not a trivial merge for us.

So I'd say that the first one is converting templates from ERB to EPP. This is IMHO an easy PR to merge since it remains compatible.

Then dealing with Sensitive. Perhaps a way to mitigate our installer concerns is to first make sure the module handles Sensitive everywhere but keep defaulting to regular strings. This would allow users of the module to pass in sensitive values while not breaking our installer.

Then a third could be defaulting to Sensitive values.

I also have no experience with Sensitive myself. Whenever I tried it, I found it be quite useless given that you still need to unwrap in many places. I also don't know how well it works with Hiera.

As a last one - should we also change files with sensitive values to show_diff => false?

manifests/cli.pp

manifests/database.pp

manifests/init.pp

spec/classes/foreman_spec.rb

spec/classes/plugin/supervisory_authority_spec.rb

templates/foreman_supervisory_authority.yaml.epp

templates/hammer_etc.yml.epp

templates/hammer_root.yml.epp

manifests/plugin/supervisory_authority.pp

manifests/plugin.pp

cocker-cc · 2021-06-23T15:15:47Z

I'm inclined to prefer this in separate PRs.
So I'd say that the first one is converting templates from ERB to EPP.
Then dealing with Sensitive.
Then a third could be defaulting to Sensitive values.

That sounds like a Plan. So, we close this PR now, and I'll create separate PRs then. Okay?

I also have no experience with Sensitive myself. Whenever I tried it, I found it be quite useless given that you still need to unwrap in many places.

Exactly this is the main Driver for my Effort. Code with or without Sensitive should "just work". The unwrap should happen at the least possible Location – or not at all, when it comes to EPP-Files.

I also don't know how well it works with Hiera.

Not sure, if I understand You right. With Variant[String, Sensitive[String]] the Code will (and should!) run with both Datatypes. But if one insists, that Secrets coming from Hiera have to be of Type Sensitive, you can have something like this in your Module's data/common.yaml:

lookup_options:
  # Hiera delivers String.  To match the Classparameter's Datatype, we have to convert to Sensitive[String]
  '^my_cool_module::.+::secret_\w+$':
    convert_to: Sensitive

In my Environment the Secrets come from Hashicorp Vault using a Function like vault_secrets::lookup, which returns Sensitive natively. My Goal is, to pass these Secrets down to the Component-Modules without any unwrap. So I create Pullrequrests for all these Modules in Question, and foreman is one of them.

ekohl · 2021-07-09T15:40:15Z

So I'd say that the first one is converting templates from ERB to EPP.

This part is now merged. Do you want to rebase this PR or open a new one?

cocker-cc · 2021-07-10T08:26:32Z

I will rebase.

cocker-cc · 2021-07-20T10:54:12Z

Rebasing done.

ekohl · 2021-07-22T12:53:19Z

Looks like something went wrong in the rebase because I see some unrelated commits that are already in master. Also has more conflicts now.

cocker-cc · 2021-07-22T13:53:43Z

Looks like something went wrong in the rebase because I see some unrelated commits that are already in master. Also has more conflicts now.

I squashed and repaired my Branch.

lib/puppet/provider/foreman_config_entry/cli.rb

manifests/params.pp

templates/hammer_root.yml.epp

ekohl · 2021-07-23T08:51:59Z

manifests/cli.pp

+          # epp() returns a Content of Datatype Sensitive, if at least one Parameter is of Datatype Sensitive.
+          # The "unwrap" therefore is not necessary, BUT: I do not know, how to make the Test pass in
+          # spec/classes/foreman_cli_spec.rb:110 otherwise.


Perhaps #976 would help with that.

Can this now be resolved?

I removed the Comment and the unwrap, but did not manage to establish a Rspec-Test.
I amended the Commit.

ekohl

It took a while to circle back to this, but I still see this as a good enhancement and I'd like to see this finished.

manifests/params.pp

manifests/database/postgresql.pp

ekohl · 2022-01-10T11:52:23Z

manifests/cli.pp

+          # epp() returns a Content of Datatype Sensitive, if at least one Parameter is of Datatype Sensitive.
+          # The "unwrap" therefore is not necessary, BUT: I do not know, how to make the Test pass in
+          # spec/classes/foreman_cli_spec.rb:110 otherwise.


Can this now be resolved?

- accept Datatype Sensitive for Puppet-Type foreman_config_entry - accept Datatype Sensitive for CLI-Password - accept Datatype Sensitive for initial Admin-Password - accept Datatype Sensitive for Database-Password - accept Datatype Sensitive for OAuth-Secrets - accept Datatype Sensitive for SMTP-Secrets

ekohl

This looks good to me. Thanks for the big effort.

ekohl · 2022-01-18T11:02:20Z

Sadly it looks like what we were afraid of is true: Kafo doesn't understand the Sensitive data type so it breaks: https://community.theforeman.org/t/foreman-installer-develop-package-release-1035-failed/26898

I'll see about a fix. For inspiration, do you know what's best practice with regards to Hiera?
I see https://puppet.com/docs/puppet/6/securing-sensitive-data.html#lookup-options but could that live in puppet-foreman?

cocker-cc · 2022-01-18T12:21:14Z

For inspiration, do you know what's best practice with regards to Hiera? I see https://puppet.com/docs/puppet/6/securing-sensitive-data.html#lookup-options but could that live in puppet-foreman?

Sure. lookup_options is the Way to go.
Hiera in general delivers a plain String, But with lookup_options Hiera is able to convert ithe String to Sensitive[String] before passing it over to Puppet.
Where can I have a Look at the Hiera-File in Question?

ekohl · 2022-01-18T12:28:04Z

Right now it's failing on parsing the data types for its own validation. We have a list of data types:
https://github.com/theforeman/kafo/blob/9ad08513b0b9ecbb31fea784bba2379ec74b5308/lib/kafo/data_type.rb#L114-L133
Perhaps right now we should implement it as a subclass of String. See:

alexjfisher · 2022-01-18T14:28:24Z

Perhaps right now we should implement it as a subclass of String

Probably ok. I've used Sensitive[Hash] as a data type before, but I think that's pretty rare and something you might never use in Foreman modules.

ekohl · 2022-01-18T14:30:51Z

Technically I suppose we can implement it as a transparent container. I might have a try.

ekohl · 2022-01-20T12:00:46Z

I took a stab at it: theforeman/kafo#345. I haven't tested it end to end, but at least it recognizes the type. I also submitted #1018 which I think should close the loop.

ekohl · 2022-01-20T15:25:51Z

templates/hammer_root.yml.epp

-  :username: '<%= $username %>'
-  :password: '<%= $password %>'
+  :username:<%= if $username { " '${username}'" } %>
+  :password:<%= if $password { " '${password}'" } %>


Somehow this doesn't work for me. In #1018 I'm adding a test, but doesn't unwrap the string. I also checked if it perhaps was the interpolation, but that also doesn't appear to be it.

theforeman-bot added Needs testing Not yet reviewed labels Jun 21, 2021

ekohl requested changes Jun 21, 2021

View reviewed changes

manifests/cli.pp Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jun 21, 2021

cocker-cc changed the title ~~Sensitive for foreman plugin config~~ Accept Datatype Sensitive for all Secrets Jun 21, 2021

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jun 21, 2021

ekohl reviewed Jun 21, 2021

View reviewed changes

manifests/init.pp Outdated Show resolved Hide resolved

ekohl reviewed Jun 21, 2021

View reviewed changes

manifests/database/postgresql.pp Outdated Show resolved Hide resolved

ekohl reviewed Jun 21, 2021

View reviewed changes

ekohl added Waiting on contributor and removed Needs re-review labels Jul 9, 2021

ehelms mentioned this pull request Jul 19, 2021

Release 18.0.0 #973

Merged

5 tasks

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 20, 2021

ekohl reviewed Jul 22, 2021

View reviewed changes

lib/puppet/provider/foreman_config_entry/cli.rb Outdated Show resolved Hide resolved

manifests/params.pp Outdated Show resolved Hide resolved

templates/hammer_root.yml.epp Outdated Show resolved Hide resolved

ekohl reviewed Jul 23, 2021

View reviewed changes

ekohl added the Enhancement label Jul 23, 2021

ekohl reviewed Jan 10, 2022

View reviewed changes

ekohl added Waiting on contributor and removed Needs re-review labels Jan 10, 2022

theforeman-bot removed the Waiting on contributor label Jan 17, 2022

theforeman-bot added the Needs re-review label Jan 17, 2022

ekohl approved these changes Jan 17, 2022

View reviewed changes

theforeman-bot removed the Needs re-review label Jan 17, 2022

ekohl merged commit 699f944 into theforeman:master Jan 17, 2022

ekohl reviewed Jan 20, 2022

View reviewed changes

ekohl added the skip-changelog label Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept Datatype Sensitive for all Secrets #960

Accept Datatype Sensitive for all Secrets #960

cocker-cc commented Jun 21, 2021

ekohl left a comment

ekohl left a comment

cocker-cc commented Jun 23, 2021

ekohl commented Jul 9, 2021

cocker-cc commented Jul 10, 2021

cocker-cc commented Jul 20, 2021

ekohl commented Jul 22, 2021

cocker-cc commented Jul 22, 2021

ekohl Jul 23, 2021

ekohl Jan 10, 2022

cocker-cc Jan 17, 2022

ekohl left a comment

ekohl Jan 10, 2022

ekohl left a comment

ekohl commented Jan 18, 2022

cocker-cc commented Jan 18, 2022

ekohl commented Jan 18, 2022

alexjfisher commented Jan 18, 2022

ekohl commented Jan 18, 2022

ekohl commented Jan 20, 2022

ekohl Jan 20, 2022

Accept Datatype Sensitive for all Secrets #960

Accept Datatype Sensitive for all Secrets #960

Conversation

cocker-cc commented Jun 21, 2021

ekohl left a comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

cocker-cc commented Jun 23, 2021

ekohl commented Jul 9, 2021

cocker-cc commented Jul 10, 2021

cocker-cc commented Jul 20, 2021

ekohl commented Jul 22, 2021

cocker-cc commented Jul 22, 2021

ekohl Jul 23, 2021

Choose a reason for hiding this comment

ekohl Jan 10, 2022

Choose a reason for hiding this comment

cocker-cc Jan 17, 2022

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

ekohl Jan 10, 2022

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

ekohl commented Jan 18, 2022

cocker-cc commented Jan 18, 2022

ekohl commented Jan 18, 2022

alexjfisher commented Jan 18, 2022

ekohl commented Jan 18, 2022

ekohl commented Jan 20, 2022

ekohl Jan 20, 2022

Choose a reason for hiding this comment