Re: [PR] GUACAMOLE-2137 : OpenBao/Hashicorp Vault extension [guacamole-client]

["ciroiriarte (via GitHub)" <gi...@apache.org>]

ciroiriarte commented on PR #1216:
URL: https://github.com/apache/guacamole-client/pull/1216#issuecomment-5014111455

   
   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":
   
   ```java
   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
   
   ```java
   -    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.
   
   ```java
   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:
   
   ```java
   final String key = UUID.randomUUID().toString();
   ```
   
   `getValue()` doesn't:
   
   ```java
   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.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org