docs: adds decision record for HashiCorp Vault auth refactor

[SaschaIsele]

What this PR changes/adds

This PR adds a decision record that outlines a refactor of the authentication inside the vault-hashicorp extension.

Why it does that

Currently, the only possible authentication method for HashiCorp Vault is token authentication which is hardcoded into the extension.
To pave the way for additonal authentication methods, the authentication of vault-hashicorp extension needs to be extensible.
This PR outlines the changes needed to achieve interchangeable authentication.

Further notes

This version of the decision record removes the registry and spi in favor of a default implementation that can be overwritten.

Linked Issue(s)

Closes #4672
Related discussion: #4374
Previous closed PR: #4673

[jimmarino]

A few comments, but otherwise I'm fine with this.

[jimmarino]

This name is not intuitive. It should reflect what role it provides.

[SaschaIsele]

Which name would be more intuitive?
Only other fitting name that comes to mind for me would be HashicorpVaultTokenProvider.

[jimmarino]

That name is better IMO

[SaschaIsele]

The name has been adjusted.

[jimmarino]

At first look, I would prefer the token to be passed into this function (also valueAuth to be renamed because it is not intuitive) rather than computed from a member at this point

[SaschaIsele]

Passing the token into this function instead of retrieving it inside would lead to duplicate code in four places.
While it would only be one line, the call to retrieve the token has to be made either way inside the public methods of the HashicorpVaultClient.
Doing it during header generation in a central place seems the most sensible to me.

[jimmarino]

I'm talking about the vaultAuth.vaultToken() call. That value can be passed to the method instead of having the method rely on a class member. I generally don't like getXXX methods that rely on members since it is not apparent what the inputs of the method are.

[SaschaIsele]

Moved the token to the parameters of getHeaders.

[jimmarino]

Can you please distinguish class names better before I proceed with a further review? Specifically, HashicorpVaultAuthTokenProvider and HashicorpVaultTokenAuth are extremely confusing.

[SaschaIsele]

@jimmarino did a pass for the class names.
HashicorpVaultAuthTokenProvider was renamed to HashicorpVaultAuthClientTokenProvider to make it readily apparent, what it provides.
HashicorpVaultTokenAuth was renamed to HashicorpVaultAuthTokenImpl to signify it as the implementation for the token authentication method of HashiCorp Vault.

Also saw the PR #4749 from @paullatzelsperger which already takes care of the refactoring part for the current token authentication and would reroute the changes specified for the HashicorpVaultClientto the HashicorpVault.java and HashicorpVaultHealthService, depending on which PR gets merged first.

[jimmarino]

@jimmarino did a pass for the class names. HashicorpVaultAuthTokenProvider was renamed to HashicorpVaultAuthClientTokenProvider to make it readily apparent, what it provides. HashicorpVaultTokenAuth was renamed to HashicorpVaultAuthTokenImpl to signify it as the implementation for the token authentication method of HashiCorp Vault.

Also saw the PR #4749 from @paullatzelsperger which already takes care of the refactoring part for the current token authentication and would reroute the changes specified for the HashicorpVaultClientto the HashicorpVault.java and HashicorpVaultHealthService, depending on which PR gets merged first.

Sorry, but these names are completely confusing and do not align. For example:

@Provider(isDefault = true)
public HashicorpVaultAuthClientTokenProvider HashicorpVaultAuthClientTokenProvider() {
    return new HashicorpVaultAuthTokenImpl();
}

This makes what should be simple code difficult to follow and is a sign that the concepts underpinning this PR are not fully thought through. I think you should go back and clarify this PR before asking the committers to review it again.

[paullatzelsperger]

@SaschaIsele

Also saw the PR #4749 from @paullatzelsperger which already takes care of the refactoring part for the current token authentication and would reroute the changes specified for the HashicorpVaultClientto the HashicorpVault.java and HashicorpVaultHealthService, depending on which PR gets merged first.

that PR just cleans up a - frankly speaking - poor implementation of the HashicorpVault, and it adds a facility to do remote signing/verifying and key rotation.

[paullatzelsperger]

I have two major gripes with this PR:

1. succinctness: a lot of stuff is explained, that is already covered by documentation. This would make the DR much shorter and more concise.
2. design: a few services seem to emerge from the Hashicorp Vault area (again, PR feat: implement Hashicorp Vault signing service #4749):

• a secure key-value store (the "vault") -> HashicorpVault
• a signing service HashicorpSignatureService
• a health check service HashicorpHealthService
  all of these services share the same auth method. I think this DR should mention that.

[SaschaIsele]

@jimmarino the concepts for the authentication interface are taken straight from HashiCorp Vault.
The interface exists to provide the client_token needed for authentication with HashiCorp Vault and the authentication method currently implemented by the extension is called Token auth method.
While the naming can be confusing, it is the terminology used by HashiCorp Vault.

[SaschaIsele]

@paullatzelsperger I cut a few explanations to make the decision record more concise, incorporated the changes from #4749 and made sure to mention the different services that use the authentication method.
Let me know how it looks to you now.

[jimmarino]

@jimmarino the concepts for the authentication interface are taken straight from HashiCorp Vault. The interface exists to provide the client_token needed for authentication with HashiCorp Vault and the authentication method currently implemented by the extension is called Token auth method. While the naming can be confusing, it is the terminology used by HashiCorp Vault.

Names like HashicorpVaultAuthTokenImpl are not terminology used by Hashicorp Vault. Please review the previous example I raised and others like it in the PR. Until those are cleaned up and you indicate as such, I will not review or lift my request for changes to this PR.

[SaschaIsele]

Kept the naming simple with the interface HashicorpVaultTokenProvider and its default implementation HashicorpVaultTokenProviderImpl which are the only two new classes.
Added a very brief explanation for HashicorpVaultTokenProviderImpl and kept the DR more concise overall.

[github-actions]

This pull request is stale because it has been open for 7 days with no activity.

[ronjaquensel]

Superseded by #4802