GUACAMOLE-2196: Openbao/Hashicorp Vault extension - #1214
Conversation
corentin-soriano
left a comment
There was a problem hiding this comment.
Thank you for this contribution!
I don't have time to read it in detail right now, but I already have some comments.
|
PS: The build failure for this PR is due to the changes in generate-license-files.sh a few months ago. Basically just have to rename the README files to LICENSE, remove the version from the directory names and reformat the LICENSE files to be the same as the other LICENSE files... I'll rebase this PR, so it builds with the latest Guacamole |
|
Latest commit removes the need from Guacamole to have access to the Vault path "/sys/mounts" and uses the path-help API instead. The path "sys/mounts" give information and metadata on all secret engine mount paths in the Vault, which is possibly a security risk depending on your threat model, whereas the path-help functionality only responses for paths that Guacamole has the permission to access. |
…It compiles but is untested
…re rather than hard code values. Typos
…he start of a testing script and documentation. The documentation will be transfered to guacamole-manual in the future
- Gets ed25519 ssh to actually work - Replace spring-vault sesion manager with our own implement as broken in spring-vault version 2.3 and doesn't treat non renewable tokens by re-authentication in later versions of spring-vault - Reorganisation vault specific code in to a seperate sub-directory - Fully test everything but the LDAP and database secret engines - Update the documentation
…documentation and test script.
… the token permissions in my test script.
…single point, resolve sub-tokens before inclusion, update documentation and remove old README.md
…ifference in creation of ldap static credentials between OpenBao and Hashicorp is only in the role creation, not the use by Guacamole
- Typos (review comment) - Don't print secret values even for debug logging (review comment) - Remove the need for access to sys/mounts that might be a security risk and use the Vault path-help API to get the mount-path and type - Fix missing SSH OTP token secrets after simplification of OpenBaoClient - Some documentation corrections
- Use TokenFilter pattern matcher directly to detect arbitrary token by matching explicitly the allowed pattern modifiers - Ignore pattern modifies in openbao extension
|
@corentin-soriano can you hold off on reviewing this PR? Looking again at #1116 I kind of understand why the author wrote the code as he did, and there is one clear avantage of the code in #1116 for the concurrency in the http requests. Basically I see two possibilities
What I’m thinking of doing, starting from #1116 is
This should end up with a PR functionally equivalent to #1214 but allowing per connection group vaults, with a minimum of work. At that point up to you and the other maintainers to decide if per connection group vaults are a good idea or not. |
@corentin-soriano Frankly after working with #1116, I'm going to close this pull request and create another pull request that combines #1116, #1147 and #1214. To be continued in my next pull request |
…apache#1214 - It supports both OpenBao and Hashicorp Vault - Uses url like tokens of the form "vault://<mount>/<path>/<secret>" - Uses the path-help function of the vault to determine the vault type - Uses spring-vault to communicate with the vault - Adds support for both KV_1 and KV_2 Key-Value secret engines - Adds support for LDAP secret engine and static, dynamic and service accounts - Adds support of the SSH secret engine including both SSH one-time passwords and signed user certificates - Adds supports for the database secret engine, allowing Guacamole itself to obtain its username, password and if the database is configured with additional static values, the URI of the database server and the database itself - Adds the possiblity of including sub-tokens with the Vault tokens (ex: vault://kv1/users{GUAC_USERNAME}/password) - Gets the connectionGroup and User fallback functions in apache#1116 to actually work - Doesn't use a Base64 configuration string, but real Guacamole configuration options - Doesn't use a sanitize function on TextFields of the Form, but rather PasswordField types
…apache#1214 - It supports both OpenBao and Hashicorp Vault - Uses url like tokens of the form "vault://<mount>/<path>/<secret>" - Uses the path-help function of the vault to determine the vault type - Uses spring-vault to communicate with the vault - Adds support for both KV_1 and KV_2 Key-Value secret engines - Adds support for LDAP secret engine and static, dynamic and service accounts - Adds support of the SSH secret engine including both SSH one-time passwords and signed user certificates - Adds supports for the database secret engine, allowing Guacamole itself to obtain its username, password and if the database is configured with additional static values, the URI of the database server and the database itself - Adds the possiblity of including sub-tokens with the Vault tokens (ex: vault://kv1/users{GUAC_USERNAME}/password) - Gets the connectionGroup and User fallback functions in apache#1116 to actually work - Doesn't use a Base64 configuration string, but real Guacamole configuration options - Doesn't use a sanitize function on TextFields of the Form, but rather PasswordField types
This PR was first based on #1143 but its moved a fair way on since then, I have however stolen their JIRA ticket. The main differences are
As this PR is fully documented in the markdown/jinja I won't do in to details of the working of this extension, but rather discuss the problems I encountered
Simplified token names:
The KSM extension uses token names like KEEPER_SERVER_USERNAME as only a single Keeper secret record with relatively well controls key values is used. THis extension allows basically any mount path of teh secret engine and path to the secret record within the Vault and so multiple secret records. Its therefore difficult to see how the KSM token naming method could be projected on to a Vault.
As in #1116 I chose to expose the Vault paths directly in the tokens, allowing the complete freedom
Token paths:
In most cases the token name to use will be the path to the Vault secret record post-pending with the secret value to extract. This has two issues
Secret caching:
When obtaining a secret record from the Vault, the secrets in the record are associated between themselves. For exemple a dynamic LDAP account has a single use username and password. The structure of the base vault extension assumes a function getValue that returns a single secret value. We therefore need to cache the values for a single record to keep the association between common values.
In the getTokens method the VaultSecretService, this is not a problem as all tokens for a single connection are resolved at once and we can set a UUID as a key in the cache to ensure common values are kept together. It also prevents session stealing by timing issues in concurrent connections. For the secrets looked up for guacamole.propertes.vlt that basically only concern secrets without any user context this is no problem. The issue is with tokens looked up in vault-token-mapping.yml. These secrets are looked up individually, but they have a user context. To avoid secret stealing and try to keep common secrets together I used a cache key combining GUAC_USERNAME, USERNAME and the path to the Vault secret record.
By the nature of Vault tokens where the password rotation at each usage, this cache should only be very short term, in the order of a few seconds. I used caffiene for the cache
Spring-Vault :
Due to the Tomcat 9 dependency of Guacamole I was forced to use a version of spring-vault compatible with Java-11, so I'm left using version 2.3.4 of spring-vault, when spring-vault is currently working on version 4.x. This came with many issues, the main one being that the LifecycleAwareSessionManager class of spring-vault in version 2.3.4 is pretty much broken, so I had to reimplement my own SessionManager. The use of spring-vault in this pull-request is therefore more of a promise to be able to simplify the extension when Guacamole migrates to a newer Tomcat
Spring-vault also pulled in a number of other dependencies (spring-framework, micrometre, jspecify) that you might not want to pull in.
SSH ed25519:
The SSH certificate signing reqires Guacamole to be able to generate temporty SSH certificates. I used apach SSHD for this. The Java 11 versions of apache SSHD don't include ed25519 directly and I had to include i2p as a crypto provider for ed25519
LDAP Service accounts:
The LDAP service accounts in the Vault must be checked out and checked back in again after use. This presented two problems for this extension.
Non renewable or expired token
Even in recent spring-vault implementations of LifecycleAwareSessionManager, an expired or non renewable token will cause the SessionManager to just stop. I'd like to use VaultAgent for complex authentification methods via a token sink file. This essentially means that a reauthentication via a call to the function CLientAuthentication.login() of spring-vault should be used if the token is expired or non-renewable. I implemented this in my SessionManager