fix: cert auth method watches cert file change and NewCreds() notific…

[jasonjoo2010]

Hi community and committees,

Recently, our team encountered TLS certificate reloading issue even after adding reload: true to config when using cert method. After checking, it looks like not functioning properly.

Although there was a previous PR(#19002 ) which added certificate reloading capability to auto auth, it doesn't work in the actual use case. After checking further a little bit, I found that the agent blocked at the line auth.go:526 while the server responded with 400 errors at transport layer due to expired certificate. In our case, we use certificates in cloud native applications with 5 days as expiry and refresh the certs every day.

At the same time, the lifetime loop also listens on a channel returned by NewCreds() but it's not implemented in cert method. So it may be the best and shortest path adding this support to make the reloading mechanism work.

Description

So in a nutshell, the reload option for cert method only guarantee AuthClient() return a different client instance when called. It doesn't guarantee the renewed certificate on disk can sooner or later be used in auth. I don't know whether people maintain a good understanding on what this option actually does but it would be better to be more straightforward and more automated in reloading it.

This PR adds implementation of existing NewCreds() method in the interface as well as a new optional option reload_period for the internal file watching loop. And they only take effect when reload is enabled. In common cases, reload is the only option people need to set when they need.

TODO only if you're a HashiCorp employee

• Backport Labels: If this PR is in the ENT repo and needs to be backported, backport
  to N, N-1, and N-2, using the backport/ent/x.x.x+ent labels. If this PR is in the CE repo, you should only backport to N, using the backport/x.x.x label, not the enterprise labels.
  • If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
• ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
  of a public function, even if that change is in a CE file, double check that
  applying the patch for this PR to the ENT repo and running tests doesn't
  break any tests. Sometimes ENT only tests rely on public functions in CE
  files.
• Jira: If this change has an associated Jira, it's referenced either
  in the PR description, commit message, or branch name.
• RFC: If this change has an associated RFC, please link it in the description.
• ENT PR: If this change has an associated ENT PR, please link it in the
  description. Also, make sure the changelog is in this PR, not in your ENT PR.

[hashicorp-cla-app]

All committers have signed the CLA.

[hashicorp-cla-app]

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

[VioletHynes]

Hi there! Thanks for your contribution.

Reading through the description and your submission, this makes sense to me. That being said, I'm not an expert in cert logic, so I'm going to try and get a second set of eyes on this that does have that expertise and talk through this with them before I call this approved.

In either case, I've added some comments that will need to be addressed before this is merged. This is shaping up to be awesome so far :D

[VioletHynes]

To add to the suggestions Scott and I made, this will also need a changelog (a file called 28126.txt in the changelog directory containing a discussion of the improvement) -- you can look at the other files for examples, and I'm happy to review.

You will also want to update the docs around this feature. In particular, there should be a new entry added for the new config option, and you should also update the reload option as appropriate. This file can be found at website/content/docs/agent-and-proxy/autoauth/methods/cert.mdx.

Thanks for the contribution! Excited to get this merged. Please let me know if you have any questions about any of the suggestions.

[jasonjoo2010]

Hi Violet and Scott,

After checking the existing code logic carefully, I realised there were special cases that the users may:

1. Don't give any cert/key files in the config.
2. Only set ca cert.
3. Only set client cert/key.

Although the most common use case is setting all 3 options, we need to cover them in the test cases and keep it backward compatible. So I adjusted the codes a little bit for this.

I also added the release note and relevant module docs, please take a look at them.

Any further suggestions, please let me know.

Cheers

[VioletHynes]

Hi there! Looking better! I've asked a few clarifying questions and I have a few small suggestions, too.

[VioletHynes]

Closes #12233 (as merging this will make enable_reauth_on_new_credentials a supported feature.

[VioletHynes]

This looks good! Just a few small things to get this over the finish line. I agree that we should make enable_reauth_on_new_credentials supported.

If you could remove the bits of text:

	// NOTE: This is unsupported outside of testing and may disappear at any
	// time.

above it (in the two config.go files) that'd be great. We should do that if we're documenting it as officially supported.

[VioletHynes]

Looks great! Just a couple tiny things left before I can approve this and get it merged. Thanks for bearing with me throughout all of the comments, and I really appreciate the quick updates and responses.

[VioletHynes]

Thanks for this! Approved. Apologies it took us some time to get to this, and I appreciate your hard work and readiness to update based on feedback.

I've kicked off CI, and assuming it's all green, I'll get this merged :)

Thank you for your contribution!

[jasonjoo2010]

I saw there were tiny red ones. No worry, let me solve the race condition in test cases, they are only for debugging purpose but we can do it in the right way.

[VioletHynes]

Thanks for the contribution, and the diligence in the added and improved tests.

Merging! 🚢

[jasonjoo2010]

Thanks for your kind and patient review, @VioletHynes and @sgmiller
That's what makes a product great and greater, really appreciate it!