869542 - Allow per-host secrets overrides

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: General, task)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

For things like signing servers, we need to be able to override secrets such as the root password.  My idea for doing that is to add a node-level $secrets_override which specifies an extra secrets file that has higher priority than secrets.csv.  Then we can use

node "signing9.foo" {
    $secrets_override = "signing-secrets"
    include toplevel::server::signing
}

and the secrets module will instruct extlookup to look at ["signing-secrets", "secrets", "secrets-template"]

The extlookup configuration is currently at the global scope.  In theory, this can be scoped to a class, but last time I tried it didn't work very well.  So this *may* not work.  Stay tuned.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Nope, $extlookup_precedence needs to be global.  I might be able to work around that.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

My new plan is to introduce the idea of aspects, based vaguely on aspect-oriented programming.

The background is, we have a primary hierarchy of host types -- the toplevel hierarchy -- which dictates *most* aspects of a host's behavior.  "toplevel" specifies configuration on all hosts, "toplevel::server" specifies servers, "toplevel::server::foopy" specifies foopies, etc.

But some of our needs are orthogonal to that hierarchy:
 * staging hosts
 * loaners
 * high-security hosts (e.g., signing)

So the proposal is to specify a host's aspects as a node-level variable (which is inherited everywhere but global scope):

node "whatever" {
    $aspects = [ "staging", "highsecurity" ]
    include toplevel::server::signing
}

along with some useful utility functions:

  assert_aspect("highsecurity")
    -- fail unless the node has the given aspect
  has_aspect("highsecurity")
    -- true if the node has the given aspect

The hook-in with secrets is in a new function, secret("root_pw_hash").  By default, this does exactly what we're doing now - looks up the given secret with extlookup.  But if the host has aspects, it will first try to look up secrets with each aspect as a suffix.  So for the "whatever" host above, secret("root_pw_hash") would look up, in order:
  root_pw_hash!staging
  root_pw_hash!highsecurity
  root_pw_hash
This will work well with Hiera, too.

We'll document the supported aspects on the wiki.

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug869542.patch

Sweet, that worked!
----
Bug 869542: Add aspects, secret()

The new secret() function looks up a named secret.

Aspects cross-cut the concerns represented by the toplevel hierarchy.
The only currently-defined aspect is "staging", but others will be added
in the future.  The secret() function supports aspects automatically,
looking up secrets by `{name}!{aspect}` before defaulting to `{name}`,
allowing secrets to be altered by aspects easiliy, without special
handling in the modules using the secrets.
----
Feel free to trade around the f?/r? roles, since we all three of us talked about this a few weeks ago.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug869542-r2.patch

This removes an unnecessary file and optimizes the ruby per finch's advice (which he gave right after I made the last attachment):

diff --git a/modules/config/lib/puppet/parser/functions/secret.rb b/modules/config/lib/puppet/parser/functions/secret.rb
index 396e7d3..fe17dac 100755
--- a/modules/config/lib/puppet/parser/functions/secret.rb
+++ b/modules/config/lib/puppet/parser/functions/secret.rb
@@ -7,12 +7,12 @@ module Puppet::Parser::Functions
     name = args[0]
     aspects = lookupvar("aspects")
     aspects = [] if aspects == :undefined
-    aspects.map do |aspect|
+    aspects.inject do |_, aspect|
       begin
         break function_extlookup(["#{name}!#{aspect}"])
       rescue Puppet::Error
         nil # ignore error
       end
-    end.first || function_extlookup([name])
+    end || function_extlookup([name])
   end
 end

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Notes for myself as to what to document:
 - Aspects in general (incl has_aspect and assert_aspect)
 - 'staging' aspect
 - no more $is_mozpool_staging node-level var
 - $aspects node-level var
 - secret() function, interaction with aspects

Comment 6

[Justin Wood (:Callek)]

Comment on attachment 747054 [details] [diff] [review]
bug869542-r2.patch

Review of attachment 747054 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I'm not a big fan of the aspect route, however I have no better idea so in interests of letting the good-enough beat the unknown perfect. f+

A few nits, that I am willing to let rail be a tiebreaker on if you (:dustin) don't agree with me :-)

::: modules/buildmaster/templates/passwords.py.erb
@@ +10,3 @@
>  
>  <% if @fqdn.match "scl1.mozilla.com" %>
>  secrets={

this part bitrots against another patch that recently landed, fwiw ;-) -- but if you take my suggestion re secrets.pp you won't need to do this change anyway

::: modules/config/lib/puppet/parser/functions/secret.rb
@@ +5,5 @@
> +module Puppet::Parser::Functions
> +  newfunction(:secret, :type => :rvalue, :arity => 1) do |args|
> +    name = args[0]
> +    aspects = lookupvar("aspects")
> +    aspects = [] if aspects == :undefined

(IANA ruby dev): I would also love some logic to turn a scalar into an array, if the following ruby doesn't do-the-right-thing in the presence of a scalar string

::: modules/config/manifests/secrets.pp
@@ -1,2 @@
> -# This Source Code Form is subject to the terms of the Mozilla Public
> -# License, v. 2.0. If a copy of the MPL was not distributed with this

I'm not a fan of killing config::secrets like this, fwiw. I find it useful to know what secrets exist and what they are used for, in one file.

It would be better (imho) to use secret() here rather than changing all the erb's etc everywhere to make use of them elsewhere. aiui aspects will still just work since the catalog is parsed on a per-machine basis

::: modules/mozpool/manifests/daemon.pp
@@ +6,1 @@
>      include mozpool::settings

adding packages:httpd looks unrelated to this patchset, and probably should be ripped out to a new bug.

::: modules/users/manifests/builder/account.pp
@@ +16,4 @@
>  
>      case $::operatingsystem {
>          CentOS, Ubuntu: {
> +            if (secret("builder_pw_hash") == '') {

despite my suggestion of using secrets.pp for all secrets anyway, I actually like using the secret function in actual .pp files, rather than the variable. It sure makes this file cleaner.

Primarily since puppet variables are always confusing.

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Yes, there will be lots of bitrot over the next while.

I don't want to list all of the secrets in a module -- that's very WET and difficult to keep in sync.  The secrets-template.csv is a list of all secrets, with documentation on the wiki.

I don't think we should support $aspects = "foo", and the ruby as written will throw an error in that case.

There's still something wrong here - in my subsequent patch, I'm getting aspect names instead of secrets.  So I may revise the patch slightly.

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Ah, the fix was

diff --git a/modules/config/lib/puppet/parser/functions/secret.rb b/modules/config/lib/puppet/parser/functions/secret.rb
index fe17dac..e918ba4 100755
--- a/modules/config/lib/puppet/parser/functions/secret.rb
+++ b/modules/config/lib/puppet/parser/functions/secret.rb
@@ -7,7 +7,7 @@ module Puppet::Parser::Functions
     name = args[0]
     aspects = lookupvar("aspects")
     aspects = [] if aspects == :undefined
-    aspects.inject do |_, aspect|
+    aspects.inject(nil) do |_, aspect|
       begin
         break function_extlookup(["#{name}!#{aspect}"])
       rescue Puppet::Error

otherwise if aspects has length 1, inject just returns the element.  Rail, r? with that change in mind, please -- I'll spare you a new patch for this one-liner.

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 747054 [details] [diff] [review]
bug869542-r2.patch

Review of attachment 747054 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me (including the small fixes you posted).

::: modules/config/manifests/secrets.pp
@@ -3,5 @@
> -# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> -# the keys in this file are documented in
> -#   https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain#Secrets
> -# if you add a new key here, add it to the wiki as well!
> -class config::secrets {

Die, secrets.pp, die. I always hated it because if you add a secret you have to add it in 3 files.

::: modules/mozpool/manifests/daemon.pp
@@ +13,5 @@
> +        user => 'apache',
> +        environment => [ "MOZPOOL_CONFIG=${::mozpool::settings::config_ini}" ],
> +        require => [
> +            Class['mozpool::virtualenv'],
> +            Class['packages::httpd'],

I think, that these hunks sneaked in somehow from another bug. Meh, they don't look bad. :)

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

Those hunks were about a missing dependency between the mozpool setup (which needs the 'apache' user to exist) and packages::httpd.  It looks like I unthinkingly adjusted whitespace there, too.  Yes, they're harmless.

Thanks, then.  I'll get this landed and merged to puppet320 shortly, after verifying everything's right in the secrets files.

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

ugh, typo in the merge: http://hg.mozilla.org/build/puppet/rev/f01d89cf1779

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

The fix above landed quickly enough to not affect any hosts.

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

There were a bunch of bustages in 3.2.0, all solved.