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

When Managing PCS Auth, pcs_cluster_auth Executes Every Puppet Run #500

Open
kahoffman opened this issue Jan 6, 2021 · 1 comment · May be fixed by #525
Open

When Managing PCS Auth, pcs_cluster_auth Executes Every Puppet Run #500

kahoffman opened this issue Jan 6, 2021 · 1 comment · May be fixed by #525
Labels
bug Something isn't working

Comments

@kahoffman
Copy link

Affected Puppet, Ruby, OS and module versions/distributions

  • Puppet: 5.5.20
  • Ruby: 2.0.0.648
  • Distribution: RHEL 7.9
  • Module version: v7.0.0

How to reproduce (e.g Puppet code you use)

class { 'corosync':
  manage_pcsd_service          => true,
  manage_pcsd_auth             => true,
  sensitive_hacluster_password => Sensitive('this-is-the-actual-password'),
  sensitive_hacluster_hash     => Sensitive('a-hash-of-the-passwd-for-the-user-resource'),
}

What are you seeing

When managing pcs auth via this module, puppet runs Exec/pcs_cluster_auth to authenticate pcs nodes on every run. This seems to be by design based on the comments in the code snippet below. Puppet best practice however expects configuration to converge to a stable state. This can never happen if a "change" happens every run. This causes problems for compliance reporting and deployment pipelines that expect clean Puppet runs.

manifests/init.pp

      # Attempt to authorize all members. The command will return successfully
      # if they were already authenticated so it's safe to run every time this
      # is applied.
      # TODO - make it run only once
      exec { 'pcs_cluster_auth':
        command => "pcs cluster auth ${node_string} ${auth_credential_string}",
        path    => $exec_path,
        require => [
          Service['pcsd'],
          User['hacluster'],
        ],
      }
    }

What behavior did you expect instead

Cluster auth should only run once or as needed and not every Puppet run. This will allow Puppet to converge to a stable state.

Output log

Notice: /Stage[main]/Corosync/Exec[pcs_cluster_auth]/returns: executed successfully (corrective)

Any additional information you'd like to impart

@kahoffman kahoffman changed the title When Managing PCS Auth pcs_cluster_auth Executes Every Puppet Run When Managing PCS Auth, pcs_cluster_auth Executes Every Puppet Run Jan 7, 2021
@towo towo added the bug Something isn't working label Dec 28, 2021
@optiz0r
Copy link

optiz0r commented Jun 28, 2022

I've just run into this myself. The pcs docs indicate the var/lib/pcsd/tokens is used to store auth tokens when run as root, which it is when run under puppet using this module. This file is currently a json document containing

{
  "format_version": 3,
  "data_version": 8,
  "tokens": {
    "hostname": "token"
  }
  "ports": {
    "hostname": 2224
  }
}

The simplest naive fix for this issue would be to add an unless clause that checks for the presence of all hostnames in this file. I've just done a quick test of this code, and it appears to do the right thing in my case, even if it is rather hacky.

$auth_check_command = $quorum_members.map |$node| {
  "grep '${node}' /var/lib/pcsd/tokens"
}.join(" && ")

exec { 'authorize_members':
  command => "pcs cluster auth ${node_string} ${auth_credential_string}",
  unless => $auth_check_command,
  path    => $exec_path,
  require => [
    Service['pcsd'],
    User['hacluster'],
    ],
}

Since what goes in the auth file is a token, not the original password, this won't validate that the auth is correct, just that it has previously been authenticated. If for any reason the authentication is invalidated, it would have to be corrected by manually re-running the pcs cluster auth or pcs host auth commands.

This code will also fail if the hostnames used overlap with each other (like foo and foobar), or match any of the terms in the file.

The use of multiple greps pipelined together is because currently a single auth command is run for all quorum members in one go. Alternatively the auth exec could be repeated separately for each quorum member, and then a single grep could check that each specific quorum member is present, which is a bit cleaner.

$quorum_members.each |$node| {
  exec { "authorize_member_$node":
    command => "pcs cluster auth ${node} ${auth_credential_string}",
    unless => "grep '$node' /var/lib/pcsd/tokens",
    path    => $exec_path,
    require => [
      Service['pcsd'],
      User['hacluster'],
      ],
  }
}

A more technically correct check for a single hostname in the tokens hash would be this, but it depends on jq, which may not be available.

jq -e </var/lib/pcsd/tokens ".tokens[\"hostname\"] | if .==null then false else true end"

optiz0r added a commit to optiz0r/puppet-corosync that referenced this issue Jun 28, 2022
The current code for authenticating to quorum members generates a
change event for every puppet run due to the exec having not being
`refreshonly` or having any conditionals. This cases the pcsd tokens
file to be updated regularly as well.

The proposed change splits up the authentication so that it's done
once per quorum member, rather than doing them all in one go. It
also adds a conditional check to see if any authentication token
is already present in the pcsd tokens file, and skips the exec if so.

This is convergent, but comes with two minor costs:
- if quorum member hostnames overlap (e.g. `foo` and `foobar`),
  then the condition will match perhaps incorrectly and one
  hostname may not get added
- If for any reason the authentication token becomes invalid,
  puppet will not correct it, and manual intervention will be
  required (in the form of a `pcs cluster auth` or `pcs host auth`
  command)

A `pcs host deauth` command would be handled correctly, and puppet
would do a corrective re-auth.

Fixes voxpupuli#500
@optiz0r optiz0r linked a pull request Jun 28, 2022 that will close this issue
optiz0r added a commit to optiz0r/puppet-corosync that referenced this issue Jun 29, 2022
The current code for authenticating to quorum members generates a
change event for every puppet run due to the exec having not being
`refreshonly` or having any conditionals. This cases the pcsd tokens
file to be updated regularly as well.

The proposed change splits up the authentication so that it's done
once per quorum member, rather than doing them all in one go. It
also adds a conditional check to see if any authentication token
is already present in the pcsd tokens file, and skips the exec if so.

This is convergent, but comes with two minor costs:
- if quorum member hostnames overlap (e.g. `foo` and `foobar`),
  then the condition will match perhaps incorrectly and one
  hostname may not get added
- If for any reason the authentication token becomes invalid,
  puppet will not correct it, and manual intervention will be
  required (in the form of a `pcs cluster auth` or `pcs host auth`
  command)

A `pcs host deauth` command would be handled correctly, and puppet
would do a corrective re-auth.

Fixes voxpupuli#500
optiz0r added a commit to optiz0r/puppet-corosync that referenced this issue Jun 29, 2022
The current code for authenticating to quorum members generates a
change event for every puppet run due to the exec having not being
`refreshonly` or having any conditionals. This cases the pcsd tokens
file to be updated regularly as well.

The proposed change splits up the authentication so that it's done
once per quorum member, rather than doing them all in one go. It
also adds a conditional check to see if any authentication token
is already present in the pcsd tokens file, and skips the exec if so.

This is convergent, but comes with two minor costs:
- if quorum member hostnames overlap (e.g. `foo` and `foobar`),
  then the condition will match perhaps incorrectly and one
  hostname may not get added
- If for any reason the authentication token becomes invalid,
  puppet will not correct it, and manual intervention will be
  required (in the form of a `pcs cluster auth` or `pcs host auth`
  command)

A `pcs host deauth` command would be handled correctly, and puppet
would do a corrective re-auth.

Fixes voxpupuli#500
optiz0r added a commit to optiz0r/puppet-corosync that referenced this issue Jun 29, 2022
The current code for authenticating to quorum members runs the auth
command on every puppet run. This both updates the credentials on
disk, and generates a puppet change event, which are btoh undesirable.

The proposed change checks to ensure all quorum members have an auth
token in the credentials file, and updates auth for all members if
any one member is missing. This results in a convergent state.

There is a caveat, in that what gets stored in the credentials file
is not the original password, but an auth token. There does not seem
to be a pcs command to check the tokens are still valid. So this code
is only checking for presenence of auth tokens, not correctness.
If the authentication token is later invalided, puppet will not correct
this. It would be necessary to manually run the `pcs host auth` or
`pcs cluster auth` commands to fix it.

Fixes voxpupuli#500
optiz0r added a commit to optiz0r/puppet-corosync that referenced this issue Jun 29, 2022
The current code for authenticating to quorum members runs the auth
command on every puppet run. This both updates the credentials on
disk, and generates a puppet change event, which are btoh undesirable.

The proposed change checks to ensure all quorum members have an auth
token in the credentials file, and updates auth for all members if
any one member is missing. This results in a convergent state.

There is a caveat, in that what gets stored in the credentials file
is not the original password, but an auth token. There does not seem
to be a pcs command to check the tokens are still valid. So this code
is only checking for presenence of auth tokens, not correctness.
If the authentication token is later invalided, puppet will not correct
this. It would be necessary to manually run the `pcs host auth` or
`pcs cluster auth` commands to fix it.

Fixes voxpupuli#500
optiz0r added a commit to optiz0r/puppet-corosync that referenced this issue Jun 29, 2022
The current code for authenticating to quorum members runs the auth
command on every puppet run. This both updates the credentials on
disk, and generates a puppet change event, which are btoh undesirable.

The proposed change checks to ensure all quorum members have an auth
token in the credentials file, and updates auth for all members if
any one member is missing. This results in a convergent state.

There is a caveat, in that what gets stored in the credentials file
is not the original password, but an auth token. There does not seem
to be a pcs command to check the tokens are still valid. So this code
is only checking for presenence of auth tokens, not correctness.
If the authentication token is later invalided, puppet will not correct
this. It would be necessary to manually run the `pcs host auth` or
`pcs cluster auth` commands to fix it.

Fixes voxpupuli#500
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants