Support MFA-protected AWS profiles via mfaToken on @initAws - #1044
Support MFA-protected AWS profiles via mfaToken on @initAws#1044theoephraim wants to merge 6 commits into
Conversation
The AWS SDK credential chain already handles role_arn profile assumption; the missing piece was the MFA code callback. mfaToken is kept as an unresolved resolver and only resolved when the SDK actually asks for a code, and the resulting STS session creds are cached until expiry since MFA codes are single-use. Also fixes plugin cache accessors to respect the cache mode set via the @cache root decorator: plugins install before the cache policy is applied in finishLoad, so their accessors were bound to the loader's initial store (or none). The accessor now reads the store lazily and the final store is re-propagated to plugins.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
varlock-website | 58c606c | Commit Preview URL Branch Preview URL |
Aug 28 2026, 06:53 PM |
|
The changes in this PR will be included in the next version bump.
|
📦 Bundle size
dist/ only; native binaries are versioned separately and not counted here. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
MFA session credentials cannot refresh after the first successful lookup, so long-lived processes will eventually use an expired STS session.
Reviewed changes covering the AWS MFA profile flow and the shared plugin cache-store rebinding.
- MFA profile support: Adds lazy
mfaTokenresolution through the AWS SDK credential chain, targeted authentication errors, and expiring STS credential persistence. - Integration coverage: Adds local STS and Secrets Manager fixtures covering role assumption, OTP generation, cross-load cache reuse, near-expiry rejection, and failures.
- Plugin cache lifecycle: Makes plugin cache accessors follow the graph's final cache store after
@cachepolicy resolution. - Documentation and release metadata: Documents MFA profile setup and records package bumps for the plugin feature and core cache fix.
azure/gpt-5.6-sol ｜ 𝕏
varlock
@varlock/native-helper-darwin
@varlock/native-helper-linux-arm64
@varlock/native-helper-linux-x64
@varlock/native-helper-win32-x64
@varlock/aws-secrets-plugin
commit: |
The SDK re-invokes its credential provider once creds are inside its refresh threshold, but getMfaSessionCredentials memoized its promise forever, so a dev server or watch-mode process would be handed the same session past its expiry. The memoized session is now dropped once it nears expiry. Since the SDK asks several times per request in that state and each assume burns a single-use MFA code, re-assumes are spaced by one TOTP window - an already-expired session bypasses that cooldown, since reusing it is guaranteed to fail anyway. The expiry buffer now matches the SDK's own 5 minute threshold so both sides stop trusting a session at once.
There was a problem hiding this comment.
Important
A failed near-expiry refresh can still bypass the new cooldown and immediately replay a potentially spent MFA code.
Reviewed changes since the prior Pullfrog review at 327ba96, focusing on the new long-lived session refresh behavior.
- Added expiration-aware refresh: Invalidated in-memory MFA session credentials at the AWS SDK's five-minute refresh threshold and coalesced replacement lookups.
- Added re-assumption cooldown: Reused still-valid credentials briefly after a recent role assumption to avoid bursts of single-use TOTP requests.
- Added same-instance coverage: Advanced the clock between sequential secret resolutions and verified a fresh STS assumption with a new TOTP.
- Updated documentation: Documented refresh behavior for long-running processes.
azure/gpt-5.6-sol ｜ 𝕏
| } | ||
|
|
||
| // stamped before the attempt, since a failed assume may still have spent the code | ||
| this.lastAssumeAt = Date.now(); |
There was a problem hiding this comment.
Recording the attempt here does not enforce the cooldown when a refresh rejects: the old session was already cleared, and the catch clears the promise, so the next provider invocation bypasses the mfaSessionCreds-guarded cooldown and can replay the same TOTP. Preserve enough failure state to delay a new assumption until the next window, and cover a failed first refresh followed by another provider invocation.
Technical details
# Enforce cooldown after failed refreshes
## Affected sites
- `packages/plugins/aws-secrets/src/plugin.ts:252` - cooldown logic only runs when `mfaSessionCreds` exists.
- `packages/plugins/aws-secrets/src/plugin.ts:262` - near-expiry credentials are cleared before the replacement attempt settles.
- `packages/plugins/aws-secrets/src/plugin.ts:270` - rejection clears the promise without retaining cooldown state that the next call checks.
- `packages/plugins/aws-secrets/test/mfa.test.ts:235` - the refresh test only exercises successful STS responses.
## Required outcome
- A failed role-assumption attempt that may have consumed its TOTP must not allow another assumption with the same code window.
- A subsequent attempt after the cooldown must remain possible, and already-expired credentials must not be served while waiting.
## Suggested approach
- Gate new fetches directly on `lastAssumeAt`, including when no in-memory session remains, and represent the wait or retry result explicitly rather than returning expired credentials.
- Add a fake STS failure for the first refresh and assert that immediate SDK reinvocation does not issue a second STS request.The session fetch used a plain cache get/set pair, which has no lock, so parallel varlock runs in separate processes (turborepo, several dev servers) would each assume the role with the same TOTP code. AWS rejects a code that has already been spent, so all but one would fail. Routed through the cache's getOrSet instead, which holds a cross-process lock and re-checks inside it - one process assumes and the rest read the session it cached. Since getOrSet takes its TTL before the session length is known, the producer writes the entry itself and returns undefined, so an entry expires exactly when the session stops being usable.
There was a problem hiding this comment.
Important
The new cache-lock path does not reliably share the assumed MFA session with concurrent callers, so it can still issue duplicate STS requests with one TOTP code.
Reviewed changes since the prior Pullfrog review at 064f116, focusing on the new cross-process serialization path.
- Serialized role assumption: Moved the MFA STS request into the plugin cache's
getOrSetproducer and persisted the resulting session with its actual usable lifetime. - Added lock-path coverage: Added assertions that the session lookup uses
getOrSetand that the explicit cache write receives the expected TTL. - Documented parallel loads: Explained disk-cache coordination and the limitations of memory or disabled caching.
azure/gpt-5.6-sol ｜ 𝕏
| TTL_WRITTEN_BY_PRODUCER, | ||
| async () => { | ||
| assumed = await this.assumeRoleWithMfa(); | ||
| await this.persistSession(cacheKey, assumed); |
There was a problem hiding this comment.
Returning undefined makes the producer's assumed session unavailable to other callers sharing the same in-flight InMemoryCacheStore.getOrSet promise, so each waiter reaches the direct-assume fallback and can reuse the same TOTP. A disk write failure has the same result because PluginCacheAccessor.set() discards CacheStore.set()'s failure signal before this lock is released.
Technical details
# Preserve the shared result from the locked producer
## Affected sites
- `packages/plugins/aws-secrets/src/plugin.ts:304` - the producer stores its result only in one caller's local `assumed` variable.
- `packages/plugins/aws-secrets/src/plugin.ts:306` - cache persistence is best effort, but its success is not observable through `PluginCacheAccessor.set()`.
- `packages/plugins/aws-secrets/src/plugin.ts:310` - returning `undefined` makes in-process waiters receive no credentials.
- `packages/plugins/aws-secrets/src/plugin.ts:327` - callers without the producer-local value bypass the lock and assume directly.
- `packages/plugins/aws-secrets/test/mfa.test.ts:235` - the test performs one load and checks method calls, so it cannot detect duplicate concurrent assumptions.
## Required outcome
- Every caller coordinated by `getOrSet` must receive the single produced session, or fail without issuing another STS request when that session cannot be shared safely.
- Add a concurrent-caller test that fails if more than one STS assumption occurs, including the in-memory store path.src/grammar.js is produced by peggy and gitignored, but the parser's build task only declared dist/** as output. On a cache hit turbo restored dist/ without it, so a fresh checkout that hit the cache had no grammar.js - and anything importing the parser from source (tests resolving through the ts-src condition) failed with ERR_MODULE_NOT_FOUND. This only surfaced when a downstream test task cache-missed while the parser build cache-hit. Also adds grammar.peggy to the build inputs. It sits at the package root, so the shared src/** glob never covered it and editing the grammar would not have invalidated the cache. The eslint jsonc glob now matches package-level turbo.json files, not just the root one, so they can carry comments like the root config does.
There was a problem hiding this comment.
ℹ️ No new issues in the latest commit. Two previously reported must-address findings remain open.
Reviewed changes since the prior Pullfrog review at 58c606c, focusing on the parser build cache correction.
- Corrected parser cache inputs: Added a package-level Turbo build definition that hashes the root-level
grammar.peggysource. - Restored generated parser source: Declared
src/grammar.jsas a build output so cache hits restore the source import used under thets-srccondition. - Expanded config lint coverage: Updated the JSONC file glob to include nested
turbo.jsonfiles.
azure/gpt-5.6-sol ｜ 𝕏
…after failures Two problems with the MFA session handling: The producer passed to getOrSet returned undefined so it could write the entry itself with a precise TTL. But callers coalesced onto the same in-flight producer then received nothing and each fell through to assuming directly, replaying a code the first call had just spent. The producer now returns the session so every waiter gets it, written under a lower-bound TTL that is rewritten once the real session length is known. Erring short means an entry never outlives the session it describes. A failed assume also left no session and no cooldown state, so the next provider invocation could immediately retry with the same spent code. The cooldown is now checked before starting any assume, not only when a session is still held.
There was a problem hiding this comment.
Important
A rejected shared MFA assumption can still be retried immediately by another plugin instance or process with the same TOTP code.
Reviewed changes since the prior Pullfrog review at c301d373, focusing on the updated result-sharing and failure-cooldown behavior.
- Shared successful assumptions: Returned serialized STS credentials from the cache producer so coalesced callers receive the same result, then replaced the conservative initial TTL with the session's actual usable lifetime.
- Extended failure cooldowns: Applied the re-assumption gate after failed refreshes even when no in-memory session remains.
- Expanded integration coverage: Added failed-refresh and concurrent-instance scenarios for the MFA credential path.
azure/gpt-5.6-sol ｜ 𝕏
| } | ||
|
|
||
| // a failed attempt also lands here, with no session left to fall back on | ||
| this.assertCanAssume(); |
There was a problem hiding this comment.
This gate only knows about attempts made by this AwsPluginInstance. If a shared getOrSet producer rejects, another instance or process has no lastAssumeAt value and can immediately become the next producer, replaying the same TOTP; please coordinate the failed-attempt cooldown across the shared cache path and cover concurrent callers when the producer rejects.
Technical details
# Coordinate failed assumptions across callers
## Affected sites
- `packages/plugins/aws-secrets/src/plugin.ts:325` - the cooldown gate reads instance-local state.
- `packages/plugins/aws-secrets/src/plugin.ts:352` - a rejected producer leaves no shared cache value or cooldown marker before the lock is released.
- `packages/plugins/aws-secrets/test/mfa.test.ts:330` - the failure case exercises one instance only.
- `packages/plugins/aws-secrets/test/mfa.test.ts:362` - the concurrent-instance case exercises only a successful producer.
## Required outcome
- Concurrent callers sharing either the in-memory or disk-backed cache must not issue another STS assumption in the same MFA window after the first producer rejects.
- Add a concurrent failure test that would fail if a second STS request is made.getOrSet needed a TTL up front, but for anything whose lifetime the source decides - an STS session, an OAuth token, a lease - that is only known once the producer has run. Callers had to guess a lower bound and correct it afterwards, leaving a window where the entry outlived what it described. The ttl argument now also accepts a callback given the produced value, called once and only when the producer actually produced. Returning zero or less skips the write. Existing callers pass a literal and are unaffected. The aws plugin drops its lower-bound-then-rewrite dance for a single getOrSet with an exact TTL, so an entry existing once again means the session is still usable, by construction rather than by argument.
There was a problem hiding this comment.
ℹ️ No new must-address issues in the latest commit; one API-contract clarification is inline.
Reviewed changes since the prior Pullfrog review at 91ae640:
- Derived cache lifetimes: Added callback TTLs to
PluginCacheAccessor.getOrSetand the underlying cache stores, including duration-string normalization and non-positive skip semantics. - Simplified MFA session persistence: Derived the STS cache lifetime directly from returned session credentials and removed the provisional write-then-correct flow.
- Expanded cache coverage: Added tests for value-derived TTLs, skipped writes, cache-hit callback behavior, and process-local result coalescing.
- Updated release metadata: Consolidated the core cache changes into a minor release entry describing both callback TTLs and cache-policy propagation.
azure/gpt-5.6-sol ｜ 𝕏
|
|
||
| // cache write failed (e.g. encryption unavailable) — still return the computed value | ||
| // not written (skipped, or the write failed with encryption unavailable) — the | ||
| // computed value still goes back to the caller and to anyone waiting on the lock |
There was a problem hiding this comment.
For a non-positive callback TTL, this value cannot reach processes waiting on the directory lock: after the lock is released, each waiter rechecks the empty cache and runs its own producer. Please narrow this comment and PluginCacheAccessor.getOrSet's cross-process sharing guarantee to persisted results, so callers protecting single-use work do not rely on coalescing in the new skip-write branch.
There was a problem hiding this comment.
Good catch - real, and it still applies. Tracking as a follow-up in #1047 rather than growing this PR further.
Worth noting for whoever picks it up: the naive shared "someone failed, everyone hold off" marker would be wrong for transient failures. If the first attempt never reached STS the code was not consumed, and the next caller would have succeeded - blocking it turns a network blip into a failure across a whole parallel build. The marker should only be set when AWS actually responded rejecting the code. The issue covers that, plus the concurrent-failure test you asked for.
1 participant
Adds an
mfaTokenparam to@initAwsso profiles that require multi-factor authentication (mfa_serialin~/.aws/config) work through the aws-secrets plugin. Addresses the underlying need in #1037: the SDK credential chain already handlesrole_arnprofile assumption, so with MFA wired up there is no need to shell out to awsume.Key behaviors:
getOrSet, which holds a cross-process lock: one process assumes the role and the rest reuse the session it stored. Without this they would each assume with the same spent TOTP code. Needs the disk cache (the default when the OS keychain / Secure Enclave is available); documented for@cache=memoryand--skip-cache.mfaToken+generateOtp().This also adds a small piece of core cache API that the feature needed.
getOrSettook its TTL up front, but for anything whose lifetime the source decides (an STS session, an OAuth token, a lease) that is only known once the producer has run, which forced a guess-then-correct dance with a window where the entry outlived what it described. Thettlargument now also accepts a callback given the produced value; returning zero or less skips the write. All existing callers pass a literal and are unaffected. This should be the reusable answer for any plugin caching a credential whose lifetime it does not choose.Also fixes a core plugin caching bug this uncovered: plugins install during
finishInit, before the@cacheroot decorator policy is applied infinishLoad, so plugin cache accessors stayed bound to the loader's initial store.@cache=memory/@cache=disabledwere silently ignored by plugin-level caching (all plugins withcacheTtl, not just this one). The accessor now reads the store lazily and the final store is re-propagated after the policy step.Tests run the full flow end-to-end against fake local STS / Secrets Manager endpoints (via
AWS_ENDPOINT_URL_*), covering the AssumeRole call contents, lazy resolution, session cred caching and expiry, mid-process refresh with the clock advanced, and both failure modes.Closes #1037