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

Puppetdb Client *ALWAYS* uses token header even when cert/key is configured #3296

Open
donoghuc opened this issue Apr 8, 2024 · 4 comments
Labels
Bug Bug reports and fixes.

Comments

@donoghuc
Copy link
Member

donoghuc commented Apr 8, 2024

Describe the Bug

In the case a token AND cert based auth methods are configured for puppetdb, the client always includes the token in the header for puppetdb requests. This results in the case where puppetdb queries fail even when a valid cert/key pair is configured.

Expected Behavior

Puppetdb client should be able to use either token or cert/key auth.

Options:

  1. Client could have cert/key take precedence over token
  2. Client could try with token, on 401/403 response could retry with cert/key
@donoghuc donoghuc added the Bug Bug reports and fixes. label Apr 8, 2024
donoghuc added a commit to donoghuc/bolt that referenced this issue Apr 8, 2024
Previously token was always included in header. With this commit, try POST requests with token in header. If that returns 401 AND cert/key is configured, retry with auth header excluded.
@nmburgan
Copy link

nmburgan commented Apr 8, 2024

I think what makes sense to me without introducing a new flag or something is:

  1. If either token OR cert is specified, use it.
  2. If both are specified, try the token first. If that fails, record that it failed and don't try using it again, and use the cert/key. If the cert/key is invalid, let that then fail, perhaps with a message saying both methods failed.

@donoghuc
Copy link
Member Author

donoghuc commented Apr 8, 2024

I think what is a bit confounding is that if cert/key are invalid it will fail regardless of the token. If this were net-new I would just choose to prefer cert based auth when both are specified (IE only include token if cert/key are not specified). But in order to not break anybody relying on token being included in header even if certs are configured I wanted to not disturb that (specifically thinking of the case where you would prefer the identity piece of an rbac token if possible). I bring this up in response to 2, specifically i think that the configuration will get a bit out of control with complexity if we try to use token only even if cert/key are incorrectly configured. We are not currently at risk of breaking anybody with a change today in this regard (if you have bad certs configured, regardless of your token config its not going to connect).

@nmburgan
Copy link

nmburgan commented Apr 8, 2024

I see, didn't realize there was a use case for both. In that case, I think at least just not including the token again if the token was invalid to avoid repeat failures seems reasonable.

donoghuc added a commit to donoghuc/bolt that referenced this issue Apr 8, 2024
Previously token was always included in header. With this commit, try POST requests with token in header. If that returns 401 AND cert/key is configured, retry with auth header excluded.

!feature

* **Try cert based auth, failback to cert based auth for puppetdb** ([puppetlabs#3296](puppetlabs#3296))

  When both a token and cert are specified, try using the token in the
  x-auth header for puppetdb POSTs. If that responds with a 401, try
  again with the token excluded.
@donoghuc
Copy link
Member Author

donoghuc commented Apr 9, 2024

I'm tempted to simplify this to follow the pattern in the puppetdb CLI. Specifically, if cert is configured, do not use token. I think that really simplifies things and is easier to understand. It also probably helps as a forcing function to ensure there is not ambiguous config files in practice. I can put up a separate PR with that approach and make sure to add a log message to warn that token wont be used when cert based auth is configured.

donoghuc added a commit to donoghuc/bolt that referenced this issue Apr 11, 2024
Previously regardless of using certs any puppetdb token (either read from default location OR configured in settings) would be sent in x-authentication header for puppetdb requests. In the case a cert is configured, do not include this as the puppetdb endpoint will 401 in the case a valid cert but revoked token is presented.
donoghuc added a commit to donoghuc/bolt that referenced this issue Apr 11, 2024
Previously regardless of using certs any puppetdb token (either read from default location OR configured in settings) would be sent in x-authentication header for puppetdb requests. In the case a cert is configured, do not include this as the puppetdb endpoint will 401 in the case a valid cert but revoked token is presented.

!bug

* **Prefer cert based auth over token for puppetdb** ([puppetlabs#3296](puppetlabs#3296))

  When both a token and cert are computed for puppetdb config, only use
  cert auth. This matches behavior of other puppetdb CLI tools.
donoghuc added a commit that referenced this issue Apr 12, 2024
(GH-3296) Prefer cert auth to token auth for puppetdb client
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug reports and fixes.
Projects
None yet
Development

No branches or pull requests

2 participants