Skip to content

Fix: prevent stale Vault credential caching and fix default KV secret keys#118

Open
tdferreira wants to merge 4 commits into
premium-minds:masterfrom
tdferreira:fix-stale-cache
Open

Fix: prevent stale Vault credential caching and fix default KV secret keys#118
tdferreira wants to merge 4 commits into
premium-minds:masterfrom
tdferreira:fix-stale-cache

Conversation

@tdferreira
Copy link
Copy Markdown
Contributor

Problem

When the plugin is configured to read credentials from Vault KV1 or KV2 secrets, the handling of username_key / password_key and the in-memory credential cache can produce incorrect and misleading behavior.

There were three related issues:

  1. Empty username_key / password_key fields did not actually default to username / password
  2. Cached credentials could become stale and be incorrectly reused after the key configuration changed
  3. Missing or invalid KV keys were not reported clearly at the point where Vault data was parsed

What was happening

In the UI, the Username key and Password key fields showed placeholder text username and password, which makes it look like those are the defaults. However, placeholders are only visual hints. If the user leaves those fields empty, the saved additional properties are empty strings, not actual default values.

For KV1 / KV2, the plugin then used those empty values as the lookup keys when parsing the Vault response. That led to code paths equivalent to:

  • KV1: data.get("")
  • KV2: data.data.get("")

which return null, even if the secret actually contains valid username and password fields.

This produced a confusing outcome:

  • Vault was contacted successfully
  • The returned secret JSON contained the expected credentials
  • But the plugin resolved both values to null
  • The failure only became visible later during DB login or in temporary debug output

Why it was especially confusing

The plugin also caches fetched credentials in a static ConcurrentHashMap.

Originally, the cache key only contained:

  • Vault address
  • Secret path
  • Secret type

It did not include:

  • username_key
  • password_key

That means a first failed fetch using blank or wrong key names could cache a Credentials object containing null values. If the user later corrected the key names in the UI, the plugin could still reuse the previously cached credentials because the cache key had not changed.

From the user?s perspective, this looked like:

  • "I fixed the configuration"
  • "Vault clearly returns the right secret"
  • "But the plugin still behaves as if the keys are wrong"

In reality, the plugin was often reusing stale cached credentials from an earlier incorrect lookup.

Expected behavior

For KV1 and KV2:

  • If the user leaves username_key and password_key blank, the plugin should use sensible defaults: username and password
  • If the user explicitly provides custom keys, those keys should be used
  • If those explicit keys do not exist in the returned secret, the plugin should fail immediately with a clear error

For DYNAMIC_ROLE and STATIC_ROLE:

  • The plugin should continue using the fixed username / password fields returned by Vault
  • The configurable key fields are not relevant there

Root causes

  1. Visual default vs runtime default mismatch
    The widget showed placeholder text but did not normalize blank values to actual defaults at runtime.

  2. Incomplete cache key
    The cache did not account for the configured KV field names, so changing those fields did not invalidate cached results.

  3. Too-late failure mode
    Missing values in the Vault payload were allowed to propagate as null, instead of being validated where the secret was parsed.

  4. Blank token file handling
    A blank token file field should mean ?use the normal default resolution?, including $HOME/.vault-token.
    This behavior is now made explicit at the provider boundary.

What this PR changes

  • Blank username_key now defaults to username
  • Blank password_key now defaults to password
  • This defaulting only matters for KV1 / KV2; dynamic/static role handling remains unchanged
  • The credential cache key now includes username_key and password_key
  • KV secrets no longer reuse stale cached values from a previous lookup done with different key names
  • KV parsing now fails fast when the configured keys do not exist in the returned secret
  • A blank token file field is treated as unset, so token resolution falls back to the normal default behavior, including $HOME/.vault-token

User-visible impact

This fixes cases where:

  • A Vault KV2 secret correctly contains:
    • data.data.username
    • data.data.password
  • But the plugin still resolves null credentials
  • Or continues using bad credentials after the key fields were corrected in the connection settings

It also improves diagnostics by failing at the Vault parsing step instead of only surfacing as a downstream database authentication failure.

Why this matters

Without this change, users can end up in a state where:

  • Vault is configured correctly
  • The plugin UI appears to be configured correctly
  • The returned secret is correct
  • But the plugin either resolves null credentials or reuses stale ones

That makes the issue very hard to reason about and can easily be mistaken for:

  • A Vault path problem
  • A Vault namespace/token problem
  • A database authentication issue
  • Or a DataGrip connection problem

This PR aligns the runtime behavior with what the UI suggests, makes cache invalidation correct, and ensures incorrect KV key configuration fails in a direct and actionable way.

Considering the nature of this fix, it's recommended to create a new release (v2.0.2?) if this Pull Request is accepted.

@lenz1111
Copy link
Copy Markdown

lenz1111 commented May 9, 2026

I would like it to be merged

@tdferreira
Copy link
Copy Markdown
Contributor Author

@froque could you please review this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants