docs: adds decision record for HashiCorp Vault auth refactor #4720
Conversation
| The sole common point between the authentication methods is the ability to provide a `client_token` that can be used to authenticate with HashiCorp Vault. | ||
|
|
||
| ```java | ||
| public interface HashicorpVaultAuth { |
There was a problem hiding this comment.
This name is not intuitive. It should reflect what role it provides.
There was a problem hiding this comment.
Which name would be more intuitive?
Only other fitting name that comes to mind for me would be HashicorpVaultTokenProvider.
There was a problem hiding this comment.
The name has been adjusted.
|
|
||
| ```java | ||
| @NotNull | ||
| private Headers getHeaders() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Moved the token to the parameters of getHeaders.
|
@jimmarino did a pass for the class names. 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 |
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. |
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. |
There was a problem hiding this comment.
I have two major gripes with this PR:
- succinctness: a lot of stuff is explained, that is already covered by documentation. This would make the DR much shorter and more concise.
- 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.
|
|
||
| ## Decision | ||
|
|
||
| Refactor the HashiCorp Vault extension and make it extensible with different methods of authentication. |
There was a problem hiding this comment.
| Refactor the HashiCorp Vault extension and make it extensible with different methods of authentication. | |
| We will refactor the HashiCorp Vault extension and make it extensible with different methods of authentication. |
|
|
||
| ## Rationale | ||
|
|
||
| The current implementation of the authentication in the HashiCorp Vault extension has no way to add new authentication methods. |
There was a problem hiding this comment.
we're not adding (As in: there could be several at runtime), but replacing
| The current implementation of the authentication in the HashiCorp Vault extension has no way to add new authentication methods. | |
| The current implementation of the authentication in the HashiCorp Vault extension has no way to customize/exchange authentication methods. |
|
|
||
| ## Approach | ||
|
|
||
| The refactor will affect only the `vault-hashicorp` extension. |
There was a problem hiding this comment.
| The refactor will affect only the `vault-hashicorp` extension. | |
| The refactor will affect only the `vault-hashicorp` module. |
| } | ||
| ``` | ||
|
|
||
| `HashicorpVaultAuthClientTokenProvider` implementations will be used by the `HashicorpVaultClient` to receive the `client_token` for the request authentication. |
There was a problem hiding this comment.
careful, the hashicorp vault stuff is in the process (#4749) of being refactored. in essence, the HashicorpVaultClient is demoted to being "just" a health service.
| `HashicorpVaultAuthClientTokenProvider` implementations will be used by the `HashicorpVaultClient` to receive the `client_token` for the request authentication. | ||
| More on that in the sections [Hashicorp Vault Auth Provision](#Hashicorp-Vault-Auth-Provision) and [HashiCorp Vault Client](#HashiCorp-Vault-Client) | ||
|
|
||
| ### Haschicorp Vault Auth Extensions |
|
|
||
| For every authentication method, an implementation of the [Hashicorp Vault Auth interface](#hashicorp-vault-auth-interface) is needed. | ||
| Each `HashicorpVaultAuthClientTokenProvider` implementation represents a different authentication method and is packaged inside its own service extension. | ||
| In this way, it can easily be added/removed from the runtime and maintained in one place. |
There was a problem hiding this comment.
spurious. no need to explain.
| Each `HashicorpVaultAuthClientTokenProvider` implementation represents a different authentication method and is packaged inside its own service extension. | ||
| In this way, it can easily be added/removed from the runtime and maintained in one place. | ||
| Due to the possible differences in the needed configuration of different authentication methods, each Service Extension will need its own configuration values specific to the authentication method. | ||
| The exception will be the token authentication, which is already packaged inside the `HashicorpVaultExtension` and used as the default authentication method. |
There was a problem hiding this comment.
no, the token auth is no exception. it also has its own set of config values (token renewal etc.)
|
|
||
| ```java | ||
| @Provider(isDefault = true) | ||
| public HashicorpVaultAuthClientTokenProvider HashicorpVaultAuthClientTokenProvider() { |
There was a problem hiding this comment.
methods are lowercase
| } | ||
| ``` | ||
|
|
||
| `isDefault = true` signifies, that `HashicorpVaultAuthTokenImpl` will be used unless another extension overwrites it by providing its own implementation without the parameter. |
There was a problem hiding this comment.
no need to explain. that is covered extensively in documentation.
|
|
||
| `isDefault = true` signifies, that `HashicorpVaultAuthTokenImpl` will be used unless another extension overwrites it by providing its own implementation without the parameter. | ||
|
|
||
| The provided implementation of `HashicorpVaultAuthClientTokenProvider` is then injected into the `HashicorpVaultExtension` to be used in the `HashicorpVaultClient` later on. |
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