Add mod_aggregate to ssh_auth to manage authorized_keys

[mchugh19]

Not sure if this is an appropriate way to handle this scenario. Comments requested!

What does this PR do?

Adds a mod_aggregate function to the ssh_auth state. This then builds up a list of the ssh keys from this and subsequent ssh_auth.present declarations, for each user. The present function then uses this list to determine what ssh_keys are being managed by salt, and removes the rest.

What issues does this PR fix or reference?

#13340

Previous Behavior

ssh_auth.present was only able to add keys. Any old keys not managed by salt need to be removed by specifying them in a ssh_auth.absent state.

New Behavior

By specifying aggregate: True for the ssh_auth.present state, ssh keys not added by salt will be removed.

Example

When adding the Asomekeyh ssh key, another Aoldkey= was found for this user. Since it was not provisioned by salt, it was removed.

Here is a state to ensure that Asomekeyh is present. In the past Aoldkey= had been added to the host, but has been forgotten and is not managed by salt.

key1:
  ssh_auth.present:
    - user: testuser
    - enc: ssh-rsa
    - comment: testuser 1
    - aggregate: True
    - names:
      - Asomekeyh
      - MotherkeyB

#key2:
#  ssh_auth.present:
#    - user: testuser
#    - enc: ssh-rsa
#    - comment: 2
#    - names:
#      - Aoldkey=

By running this state, ssh_auth.present ensured the Asomekeyh was present, but also removed any keys found for the testuser user which were not managed by salt.

          ID: key1
    Function: ssh_auth.present
        Name: Asomekeyh
      Result: True
     Comment: The authorized host key Asomekeyh is already present for user testuser
     Started: 16:08:57.859383
    Duration: 9.498 ms
     Changes:
              ----------
              Aoldkey=:
                  Key removed

Tests written?

No

Commits signed with GPG?

No

[rallytime]

I have one small comment here.

[rallytime]

Can you change this to Neon?

[mchugh19]

Whoops! Yep, will do.

[gtmanfred]

Can we expose this so that it can be used directly as well, instead of only through mod_aggregate?

[gtmanfred]

Probably just needs to be added to the docs for this function.

[mchugh19]

Hmmm. Yes, but that's harder.
At the moment, since ssh_keys is only ever set by the mod_aggregate function, the singular ssh_auth.present is able to get away with having ssh_keys only perform the delete on keys that aren't managed by salt. Actually adding the keys is performed by the current code in present. If we expose ssh_keys, then you would have to modify ssh_auth.present to be capable of adding all those ssh_keys as well.

So this PR currently handles "managing" ssh_authorized with:

key1:
  ssh_auth.present:
    - user: testuser
    - enc: ssh-rsa
    - comment: testuser 1
    - aggregate: True
    - names:
      - keyNumber1

key2:
  ssh_auth.present:
    - user: testuser
    - enc: ssh-rsa
    - comment: testuser 1
    - aggregate: True
    - names:
      - keyNumber2

Each of those ssh_auth.presents performs the key add. Then one of them gets extended by mod_aggregate which adds ssh_keys=['keyNumber1', 'keyNumber2']. Thus this PR only uses ssh_keys to detect what is being added, in order to determine which existing keys should be removed.

If ssh_keys were exposed, I'd expect it to be a synonym for names and thus actually perform the add and removal of everything else. I think names is supported by the state loader, and thus to get this function to support ssh_keys means that we have to add logic to loop.

If that's all correct it's doable, but a bit more invasive. Is that worth it? Or am I missing something?

[gtmanfred]

Ok, removing the ssh keys is not the purpose of mod_aggregate.

The purpose of mod_aggregate is to combine all of the calls into just one call.

See how it is used in pkg.installed where it makes only one call to the system package manager, instead of once per package state.

I am not sure that we should make this change if it is going to start removing keys that are not managed by the aggregate. @saltstack/team-core opinions?

[mchugh19]

Agreed. Hence the initial "I don't know if this is kosher" comment.

In order to perform authorized_keys management where keys not managed by salt are removed, we would need to know about all the keys that salt is managing. Today that means a series of ssh_auth.present stanzas. Because they are indepent, it forces the use of mod_aggregate. This overloads the use of mod_aggregate. In the pkg state it aggregates calls while in this use it aggregates and allows the multiple calls to more completely manage the authorized_keys file. Possibly a hack, or possibly awesome?!

If there are cleaner ways of accomplishing this, I'm for that as well. But a definitely a review is needed before any potential merge.

[gtmanfred]

yeah, i just want to have more input from the rest of the core team to see if we are ok with this.

[terminalmage]

This is definitely not how we want to use aggregate.

[mchugh19]

@gtmanfred @terminalmage @cachedout is there a recommendation for how to support #13340 functionality?

ssh_auth.present allows for passing various arguments and options, which is why this PR only attempted to handle removing unlisted keys and continued to use independent ssh_auth.present declarations to add. Perhaps for the simple use-case an "ssh-keys" key could be created which adds those keys, and removes all others? This would require all keys listed to have the same options, but perhaps that is an acceptable trade off?

[mchugh19]

Closing this out in favor of a different approach