docs: adds decision record for HashiCorp Vault auth refactor #4673
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contributors manual, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.
|
please respect the process, which is: raise an issue, wait until it is triaged and ready for development, and then provide a PR. [edit]: The issue should already outline the technical approach, so that it can be discussed in a committers' meeting. It is readily apparent that the approach you proposed here requires some more discussion. |
There was a problem hiding this comment.
I have a few questions, as noted by my inline comments.
My primary question is: Why does this need to be an extension point (SPI)? There are a finite number of authentication methods supported by Hashicorp Vault, and external system integration is not required. Can't the setup be simplified by having the extension support different configurations with a default fallback? There appears to be no need for an SPI or registry, just an internal interface that supports the different methods and an implementation that is passed by the extension at boot based on configuration to the vault client service.
Also, the DR should address which methods will be supported and how they will be tested.
| * [Kubernetes auth](https://developer.hashicorp.com/vault/docs/auth/kubernetes) | ||
| * [AppRole auth](https://developer.hashicorp.com/vault/docs/auth/approle) | ||
|
|
||
| It goes against the EDC extensibility model and needs to be remedied. |
There was a problem hiding this comment.
It's a bit of a nit, but the EDC extension model does not dictate that everything must be extensible. Also, we don't necessarily want to support all of these methods because we have to test them.
| * [AppRole auth](https://developer.hashicorp.com/vault/docs/auth/approle) | ||
|
|
||
| It goes against the EDC extensibility model and needs to be remedied. | ||
| Additionally, extracting the current implementation of the token authentication into its own extension will improve readability and maintainability of the code. |
There was a problem hiding this comment.
Again a bit of a nit, but creating an extension point does not improve readability. Arguably, it makes the code more complex. The reason to provide extensibility is to allow for more flexible security and to support existing security standards a deployer may have.
| ### Haschicorp Vault Auth Service Extension | ||
|
|
||
| For every authentication method, an implementation of the [Hashicorp Vault Auth interface](#hashicorp-vault-auth-interface) is needed. | ||
| Each `HashicorpVaultAuth` implementation represents a different authentication method and is packaged inside its own service extension. |
There was a problem hiding this comment.
Why can't this be a default service?
| A new config value is introduced to the HashiCorp Vault Extension named `edc.vault.hashicorp.auth.method`. | ||
| `edc.vault.hashicorp.auth.method` governs which `HashicorpVaultAuth` implementation is used from `HashicorpVaultAuthRegistry` and is persisted in `HashicorpVaultSettings`. | ||
|
|
||
| For testing and demo purposes, another setting called `edc.vault.hashicorp.auth.token.fallback` is introduced. |
There was a problem hiding this comment.
This sounds like it can be a default service without the need to special-case it
| } | ||
| ``` | ||
|
|
||
| ### Hashicorp Vault Auth Registry |
There was a problem hiding this comment.
Why is a registry used? Isn't it the case that only one Vault auth extension is used at a time? Also, isn't this easier to just create a set of auth services that implement the various methods and have the extension enable one of those based on configuration (or a default fallback)? that would remove the complexity of having an extension point and forcing distributions to enable modules.
|
This pull request is stale because it has been open for 7 days with no activity. |
What this PR changes/adds
This PR adds a decision record that outlines a refactor of the authentication inside the
vault-hashicorpextension.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-hashicorpextension needs to be extensible.This PR outlines the changes needed to achieve interchangeable authentication.
Further notes
List other areas of code that have changed but are not necessarily linked to the main feature. This could be method
signature changes, package declarations, bugs that were encountered and were fixed inline, etc.
Linked Issue(s)
Closes #4672
Related discussion: #4374
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.