GUACAMOLE-2137 : OpenBao/Hashicorp Vault extension

[adb014]

This PR was first based on #1143, but has now combined with #1116 as well, so I stole the Jira ticket of #1116. The main changes are

• It supports both OpenBao and Hashicorp Vault, so this PR also address both GUACAMOLE-2137: Introduce HashiCorp Vault token handler (based on the KSM module) #1116 and GUACAMOLE-2196: OpenBao Vault Integration Extension #1143
• It uses the same idea as GUACAMOLE-2137: Introduce HashiCorp Vault token handler (based on the KSM module) #1116 to have the tokens represented by their paths in the Vault, but uses a URL like token vault://
• Uses the vault path-help function on the path /sys/internal/ui/mounts/ to detect the secret engine type of a path. Access to /sys/mounts is not needed (it presents a security risk)
• It uses spring-vault to talk to the vault
• It adds support for both KV_1 and KV_2 Key/Values secret engines
• It adds support of the LDAP secret engine for static, dynamic and service accounts
• It adds support of the SSH secret engine including both SSH one-time passwords and signed User certificates
• It 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. Note with OpenBao only the username and password is configurable in this manner, but with Hashicorp version 1.10 or later and static fields in the database engine, all of the database arguments can be configured
• 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 GUACAMOLE-2137: Introduce HashiCorp Vault token handler (based on the KSM module) #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
• Finally kept the user vaults as the way its implemented the risk is minimal by default and the adminsitrator really has to mess up to create a risk
• Replace the hand-rolled cache of GUACAMOLE-2137: Introduce HashiCorp Vault token handler (based on the KSM module) #1116 with Caffeine and use for very short term (10 seconds by default cache)

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 the 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. This means that the file
vault-token-mapping.yml is probably not needed,

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 several issues

1. The Vault itself doesn't create the SSH certificates so the secret names "unsigned", "public" and "private" are imposed by Guacamole
2. The LDAP service accounts are checked out via and endpoint "/check-out". I chose to drop the "/check-out" in the token path in the extension
3. Certain secret keys are added for consistency with other endpoints. For example the password for SSH OTP is returned in the value "key". This value is kept but copied to "password". LDAP service account usernames and returned in "service_account_name" which is copied to "username"

Secret caching:
When obtaining a secret record from the Vault, the secrets in the record are associated between themselves. For example 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

SSH ed25519:
The SSH certificate signing reqires Guacamole to be able to generate temporary SSH certificates. I used apache 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. I removed the BouncyCastle Fips crypto provider from #1116, as it wasn't initially used and it prevented the use of ed25519 as FIPS-140 is a relatively old standard that doesn't include or allow elliptique curve crypto

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.

1. Within the extension the GuacamoleTunnel is not yet opened, and so I have no unique connection ID for the extension. I addressed this by assuming that the TunnelConnectEvent will be very rapidly after the call to getTokens in the extension, and setting the connection UUID in the TunnelConnectEvent.
2. The TunnelCloseEvent is a bit random when and if it is called. I wrote the code to check-in the service account if a TunnelCloseEvent is recieved, but as a back-up I fixed the TTL of the checked out accounts to 2 hours to ensure the Vault automatically checks in these accounts. In Guacamole the service accounts check in information is just dropped after 2 hours. If we can get more reliable TunnelCloseEvents on the termination of the sessions, the code as is should just work

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 in expiry. 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

[adb014]

Rebased on staging/1.6.1 and the PR updated

[adb014]

I converted this PR to a Draft as after having run the code through a SAST/linter and taking a deep breath after the
conclusions, there are some serious refactoring to do to make this code ready... I've already made most of these changes my SAST (using PMD) suggested.

Didn't last long in Draft form, 'cause I committed my changes a few hours after following testing

[adb014]

For discussion of my proposed TokenFilter change, please see regex101.com for an example of how the regular expression I propose functions

[ciroiriarte]

I've been going through this branch with a view to helping it land, and built the
TokenFilter test coverage that a core token-grammar change is going to need either
way. Posting what it found, plus a few other things worth a look.

The token pattern change alters existing behavior

I wrote characterization tests for the current parameter token grammar — they pass on
main and pin down existing behavior rather than asserting anything new.

main     (0b65b78)   Tests run: 11, Failures: 0, Errors: 0   BUILD SUCCESS
this PR  (d3b3b5e)   Tests run: 11, Failures: 1, Errors: 2   BUILD FAILURE

  TokenFilterGrammarTest.testUnrecognizedModifierYieldsRawValue:74
      expected:<[Value-of-C]> but was:<[${TOKEN_C:BOGUS}]>
  TokenFilterGrammarTest.testUnrecognizedModifierResolvesUnderStrictMode:89
      » GuacamoleTokenUndefinedException
  TokenFilterGrammarTest.testNonTokenTextIsNotTreatedAsToken:113
      » GuacamoleTokenUndefinedException

Those three methods cover six distinct inputs (JUnit reports the first failing
assertion per method):

Input	main	this branch
${TOKEN:BOGUS}	Value-of-C	${TOKEN:BOGUS} (literal)
${TOKEN:lower}	Value-of-C	${TOKEN:lower} (literal)
${TOKEN:BOGUS} (strict)	Value-of-C	throws GuacamoleTokenUndefinedException
${NOT A TOKEN} (strict)	literal	throws GuacamoleTokenUndefinedException
${a/b/c} (strict)	literal	throws GuacamoleTokenUndefinedException
${some.value} (strict)	literal	throws GuacamoleTokenUndefinedException

Two distinct causes:

Unrecognized modifiers. On main the modifier group is (\:(.*))? and the switch
in filter() falls through to default:, appending the token value unchanged. The new
pattern only recognizes LOWER|UPPER|OPTIONAL, and any other :suffix is absorbed into
the token name — so ${TOKEN:BOGUS} becomes a lookup for a token literally named
TOKEN:BOGUS, which is undefined.

Names outside [A-Za-z0-9_]. On main, ${a/b/c} doesn't match the pattern at all,
so it is never a token and is passed through literally. The new pattern matches it, making
it an undefined token. In non-strict mode the output is identical, so this is invisible
in most paths — but strict mode changes from "pass through" to "throw".

Why strict mode matters here specifically

filterStrict() has exactly one caller in the tree, and it's in the vault extension:
VaultUserContext.getTokens(), resolving secret names from vault-token-mapping.yml.
That caller treats the exception as "skip this secret":

catch (GuacamoleTokenUndefinedException e) {
    logger.debug("Secret for token \"{}\" will not be retrieved. ...");
    continue;
}

So for a mapping entry whose secret name contains / or . — normal for vault paths —
behavior changes from "use it as a literal secret name" to "silently skip it", with only a
debug-level log. No error surfaces; the credential just never arrives. That looks like a
regression against the already-shipped KSM extension, and it's the reason I'd suggest the
TokenFilter change be split into its own PR with these tests attached, rather than
riding along with the extension.

I've opened those tests separately as #1231 (GUACAMOLE-2302) against main, since they
apply to the existing grammar regardless of what happens here. If they land, they'd give
any change to the token pattern — this one or a later one — something concrete to be
evaluated against.

Two things I checked that turned out fine: no catastrophic backtracking on the new
pattern (worst case I could construct ran in single-digit ms), and escaping via
$${TOKEN} is unaffected.

TokenFilter.getToken() is public API

-    public String getToken(String name) {
+    public String getToken(String name, String modifier) {

guacamole-ext is the published extension API, so dropping the one-arg form breaks
third-party extensions that call it. Keeping it as an overload delegating to the new
method would resolve this.

Sub-token substitution bypasses canonicalize()

Separate from the above. guacamole-vault-base routes token values through
VaultSecretService.canonicalize() — VaultUserContext.createFilter() overrides
setToken() specifically so a value can't alter the secret name it lands in.

This extension implements canonicalize() (URLEncoder.encode(...)), but I can't find a
call to it: the single-brace sub-token path in HvSecretService.applyToken() does a raw
replace.

value = filter.filter(value);
if (!value.contains("${") && token.contains(placeholder) && !token.contains("$$" + placeholder)) {
    token = token.replace(placeholder, value);
}

filter.filter(value) expands nested ${...} tokens but doesn't canonicalize, and in
getValue() the filter is a plain new TokenFilter() rather than the canonicalizing
subclass. So for vault://kv1/users/{GUAC_USERNAME}/password, an identifier containing
/ or .. changes which path is read. Under LDAP/SSO those identifiers aren't always
constrained, and Vault policy only contains this when the Guacamole token is already
tightly scoped.

Running each substituted value through canonicalize() in applyToken() would cover it,
and probably also the escaping edge case in the // FIXME above that function.

Cache isolation differs between the two entry points

getTokens() isolates per invocation:

final String key = UUID.randomUUID().toString();

getValue() doesn't:

final String key = guacUsername + "-" + username + "-" +
        finalName.substring(0, finalName.lastIndexOf('/'));

Fine for KV. For SSH OTP and LDAP dynamic/service accounts it means a secret can be shared
between concurrent resolutions within the cache window, and the lastIndexOf('/')
truncation puts everything under a path into one entry regardless of engine. Might be
worth making the cache engine-aware.

Minor: the description says a 10 second cache, DEFAULT_CACHE_LIFETIME is 5000.

Offer

Happy to take on the conflict resolution against main, or run an interop matrix across
HashiCorp OSS and OpenBao for the engines you support — I have a test environment set up
for both. Say what would help rather than duplicate what you already have in flight.

[adb014]

I've been going through this branch with a view to helping it land, and built the TokenFilter test coverage that a core token-grammar change is going to need either way. Posting what it found, plus a few other things worth a look.

Thanks for this, and you're right that teh TokenFIlter is the only change to the main Guacamole code so it needs particular care so that the existing KSM module is not impacted.

I wrote characterization tests for the current parameter token grammar — they pass on main and pin down existing behavior rather than asserting anything new.

However, the test suite you've built implements the existing behaviour, whereas the request here was to modify this behaviour to allow URL like token parameters. A complex example of a token parameter this Hashicorp/OpenBao extension supports is

vault://ssh/guacamole/server1/sign/public?key_type=ec&key_bits=521

So pinning down the minimal set of changes to the existing tokenFilter is a good thing, but changes will be needed to support tokens of the above form. My approach in the filter proposed esentially let a lot of things through. If this causes problems for KSM a minimal regular expression to match url like token as above

[a-z]+:\/(?:\/[a-zA-Z0-9_-]+)+(?:\?(?:[^=&?#:]+=[^&#]*(?:&[^=&?#]+=[^&#:]*)*)?)?

So an alternative tokenfilter might be to replace

      * entire token itself.
      */
-    private final Pattern tokenPattern = Pattern.compile("(.*?)(^|.)(\\$\\{([A-Za-z0-9_]*)(\\:(.*))?\\})");
+    private final Pattern tokenPattern = Pattern.compile("(.*?)(^|.)(\\$\\{([A-Za-z0-9_]*|[a-z]+:\\/(?:\\/[a-zA-Z0-9_-]+)+(?:\\?(?:[^=&?#]+=[^&#:]*(?:&[^=&?#]+=[^&#:]*)*)?)?)(\\:([^:]*))?\\})");
 
     /**

This is probably a better starting point for the regular expression if the goal is to make minimal changing to the existing behaviour

main     (0b65b78)   Tests run: 11, Failures: 0, Errors: 0   BUILD SUCCESS
this PR  (d3b3b5e)   Tests run: 11, Failures: 1, Errors: 2   BUILD FAILURE

  TokenFilterGrammarTest.testUnrecognizedModifierYieldsRawValue:74
      expected:<[Value-of-C]> but was:<[${TOKEN_C:BOGUS}]>
  TokenFilterGrammarTest.testUnrecognizedModifierResolvesUnderStrictMode:89
      » GuacamoleTokenUndefinedException
  TokenFilterGrammarTest.testNonTokenTextIsNotTreatedAsToken:113
      » GuacamoleTokenUndefinedException

Those three methods cover six distinct inputs (JUnit reports the first failing assertion per method):

Input main this branch
${TOKEN:BOGUS} Value-of-C ${TOKEN:BOGUS} (literal)
${TOKEN:lower} Value-of-C ${TOKEN:lower} (literal)
${TOKEN:BOGUS} (strict) Value-of-C throws GuacamoleTokenUndefinedException
${NOT A TOKEN} (strict) literal throws GuacamoleTokenUndefinedException
${a/b/c} (strict) literal throws GuacamoleTokenUndefinedException
${some.value} (strict) literal throws GuacamoleTokenUndefinedException

My initial TokenFilter didn't try to keep any existing behavior so this doesn't surprise me at all... Can you try with the TokenFilter propsoed above ?

After that we really need to tie down exactly what behavior in the token filter will casue problems with KSM

Why strict mode matters here specifically

filterStrict() has exactly one caller in the tree, and it's in the vault extension: VaultUserContext.getTokens(), resolving secret names from vault-token-mapping.yml. That caller treats the exception as "skip this secret":

Ok, so here is the key statement... The initial proposed changes completely modified the behavior of unmatched tokens for
the token mapping yaml file...

So for a mapping entry whose secret name contains / or . — normal for vault paths — behavior changes from "use it as a literal secret name" to "silently skip it", with only a debug-level log. No error surfaces; the credential just never arrives. That looks like a regression against the already-shipped KSM extension, and it's the reason I'd suggest the TokenFilter change be split into its own PR with these tests attached, rather than riding along with the extension.

I think the proposed TokenFilter above that matches more strictly url like vault tokens might address most of this concern. It can't entiremy address it as if you write a token that matches the url token filter it will match and have this behavior, but that is the desired outcome for me.

guacamole-ext is the published extension API, so dropping the one-arg form breaks third-party extensions that call it. Keeping it as an overload delegating to the new method would resolve this.

I initially dropped the one argument form as this was done in #1116.. No problem to keep both forms

Sub-token substitution bypasses canonicalize()

Separate from the above. guacamole-vault-base routes token values through VaultSecretService.canonicalize() — VaultUserContext.createFilter() overrides setToken() specifically so a value can't alter the secret name it lands in.

This extension implements canonicalize() (URLEncoder.encode(...)), but I can't find a call to it: the single-brace sub-token path in HvSecretService.applyToken() does a raw replace.

Yeah I didn't quite understand the use of this function either, so I just duplicated what KSM did... If what you say above is true (I don't doubt it) then KSM also has this same issue, except ...

value = filter.filter(value);
if (!value.contains("${") && token.contains(placeholder) && !token.contains("$$" + placeholder)) {
    token = token.replace(placeholder, value);
}

filter.filter(value) expands nested ${...} tokens but doesn't canonicalize, and in getValue() the filter is a plain new TokenFilter() rather than the canonicalizing subclass. So for vault://kv1/users/{GUAC_USERNAME}/password, an identifier containing / or .. changes which path is read. Under LDAP/SSO those identifiers aren't always constrained, and Vault policy only contains this when the Guacamole token is already tightly scoped.

Running each substituted value through canonicalize() in applyToken() would cover it, and probably also the escaping edge case in the // FIXME above that function.

I only matched sub-tokens

GUAC_USERNAME
HOSTNAME
USERNAME
GATEWAY
GATEWAY_USER

These allow the use of ".." or "/" in their values ? None of these values are under user control so the security risk of the path traversal attack you describe is low, but ok I'll canonicalize the token before substitution

Cache isolation differs between the two entry points

getTokens() isolates per invocation:

final String key = UUID.randomUUID().toString();

getValue() doesn't:

final String key = guacUsername + "-" + username + "-" +
        finalName.substring(0, finalName.lastIndexOf('/'));

Fine for KV. For SSH OTP and LDAP dynamic/service accounts it means a secret can be shared between concurrent resolutions within the cache window, and the lastIndexOf('/') truncation puts everything under a path into one entry regardless of engine. Might be worth making the cache engine-aware.

Firstly using url like token the need to the yaml token file essentially disappears as there is no token that can't be described directly in the connections. I hestitated to entirely drop the use of the yaml file as uneeded..

However yes you have put your finger on an issue I was entirely aware of (one of the reasons I hestitated to drop the getValue code path) that getValue looks up each token value independently... The problem is imagine the yaml values

LINUX_SSH_PUBLIC: vault://ssh/guacamole/server1/sign/public
LINUX_SSH_PRIVATE: vault://ssh/guacamole/server1/sign/private

These two values are associated with each other and must be calculated together. So as each is looked up indenpendantly in getValue(), the cache resolution of these two values within the cache window is necessary to allow them to be associated with each other.

Note that the 'engine' is coded into the vault path used in the cache and so the cache resolution doesn't allow secret resolution between two different secret engines..

I also coded the username into the cache path to avoid users usurping the secrets of another user... I don't believe I've left a possible attack path in this code, but if you still think it is an issue, best solution is just to entirely drop the yaml file support for the Hashicorp tokens as it really isn't as needed as form KSM.

Minor: the description says a 10 second cache, DEFAULT_CACHE_LIFETIME is 5000.

The problem of code written over several weeks.. SUre I'll fix this

Offer

Happy to take on the conflict resolution against main, or run an interop matrix across HashiCorp OSS and OpenBao for the engines you support — I have a test environment set up for both. Say what would help rather than duplicate what you already have in flight.

Frankly, I haven't looked at this code for a little while as I've moved on the other thing during the wait for review... If you want to try out the above TokenFilter with your test code that would be great

[adb014]

Here is a regex101 link to the proposed TokenFilter for testing. Please note that the escape charceters \ will need to be doubled when including in Guacamole