Skip to content

feat: Support Vault client token renewal#201

Merged
clement0010 merged 24 commits intomasterfrom
feat/support-token-renewal
Mar 5, 2026
Merged

feat: Support Vault client token renewal#201
clement0010 merged 24 commits intomasterfrom
feat/support-token-renewal

Conversation

@clement0010
Copy link
Copy Markdown
Contributor

@clement0010 clement0010 commented Feb 19, 2026

⚠️ Branched off from #190

Changes

  • Create a new VaultClient type that wraps the Vault API client with token lifecycle management that watches token expiry, re-authenticates on failure, and retries login.
  • Start Vault client token renewal loop in SSHProxy.Start(), so we can gracefully shut down the token lifecycle loop as SSHProxy terminates.

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Feb 19, 2026

Pull Request Test Coverage Report for Build 22293359321

Details

  • 56 of 90 (62.22%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 84.143%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/proxy/proxy.go 4 8 50.0%
internal/sshhandler/ca.go 1 6 16.67%
internal/sshhandler/proxy.go 5 14 35.71%
internal/sshhandler/vault.go 43 59 72.88%
Files with Coverage Reduction New Missed Lines %
internal/sshhandler/ca.go 1 53.13%
Totals Coverage Status
Change from base Build 22186679390: -0.2%
Covered Lines: 2807
Relevant Lines: 3336

💛 - Coveralls

@clement0010 clement0010 requested a review from minhtule February 20, 2026 09:45
@clement0010 clement0010 force-pushed the feat/support-token-renewal branch from 7ce2ffd to e6a78d1 Compare February 23, 2026 04:50
Comment thread internal/sshhandler/proxy.go Outdated
Comment on lines +303 to +305
if p.config.VaultClient == nil || p.config.VaultClient.authMethod == nil {
return nil
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When static token is used, we don't need to perform login because it's already configured on the client.

While a static token can be renewed, we are not supporting it right now because it requires a different setup. We might want support in the future, but it's out of the scope of this PR for now.

Base automatically changed from feat/support-vault-approle-authn-method to master February 25, 2026 04:10
Comment thread internal/sshhandler/vault.go
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 51.56250% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.28%. Comparing base (49d06a7) to head (2656c20).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
internal/sshhandler/vault.go 63.82% 14 Missing and 3 partials ⚠️
internal/sshhandler/ca.go 20.00% 12 Missing ⚠️
internal/sshhandler/proxy.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   82.87%   82.28%   -0.60%     
==========================================
  Files          35       35              
  Lines        2482     2534      +52     
==========================================
+ Hits         2057     2085      +28     
- Misses        321      341      +20     
- Partials      104      108       +4     
Flag Coverage Δ
integration 48.94% <4.68%> (-1.06%) ⬇️
unit 78.21% <51.56%> (-0.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
internal/sshhandler/proxy.go 82.30% <0.00%> (-1.49%) ⬇️
internal/sshhandler/ca.go 49.26% <20.00%> (-1.13%) ⬇️
internal/sshhandler/vault.go 64.54% <63.82%> (+5.12%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@clement0010 clement0010 force-pushed the feat/support-token-renewal branch from 3706ca9 to 33bfc4e Compare March 4, 2026 07:53
Copy link
Copy Markdown
Contributor

@minhtule minhtule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix & merge!

Comment thread internal/sshhandler/ca.go Outdated
Comment thread internal/sshhandler/vault.go Outdated
Comment thread internal/sshhandler/vault.go Outdated
Comment thread internal/sshhandler/vault.go Outdated
Comment on lines +166 to +177
for {
select {
case <-ctx.Done():
return
default:
if err := vc.watchTokenLifecycle(ctx, secret); err != nil {
vc.logger.Error("Failed to watch Vault token lifecycle, will retry later", zap.Error(err))
}

secret = vc.loginWithRetry(ctx)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently when watchTokenLifecycle exits because the context is finished, we still try to loginWithRetry. I think we could rewrite this to avoid that

for {
    if err := vc.watchTokenLifecycle(ctx, secret); err != nil {
        vc.logger.Error("Failed to watch Vault token lifecycle, will retry later", zap.Error(err))
    }

    select {
    case <-ctx.Done():
        return
    default:
    }

    secret = vc.loginWithRetry(ctx)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your example, loginWithRetry returns nil secret when context is finished. Then calling watchTokenLifecycle(ctx, nilSecret) will return ErrLifetimeWatcherMissingSecret and log it as an error which could be misleading. I checked with Claude and it proposed we could do something like:

for {
      if err := vc.watchTokenLifecycle(ctx, secret); err != nil {
          if ctx.Err() != nil {
              return
          }
          vc.logger.Error("Failed to watch Vault token lifecycle, will retry later", zap.Error(err))
      }

      secret = vc.loginWithRetry(ctx)
      if ctx.Err() != nil {
          return
      }
  }

@clement0010 clement0010 force-pushed the feat/support-token-renewal branch from 44b847b to bf93cfe Compare March 5, 2026 04:21
Comment thread internal/sshhandler/ca.go Outdated
Comment thread internal/sshhandler/vault.go Outdated
Comment thread internal/sshhandler/vault.go Outdated
@clement0010 clement0010 force-pushed the feat/support-token-renewal branch from cb1ae08 to 2656c20 Compare March 5, 2026 05:27
@clement0010 clement0010 merged commit b1e3d26 into master Mar 5, 2026
14 checks passed
@clement0010 clement0010 deleted the feat/support-token-renewal branch March 5, 2026 06:03
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.

3 participants