Support MFA-protected AWS profiles via mfaToken on @initAws - #1044
Support MFA-protected AWS profiles via mfaToken on @initAws#1044theoephraim wants to merge 1 commit into
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
varlock-website | 78afc83 | Commit Preview URL Branch Preview URL |
Sep 04 2026, 08:26 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: |
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.
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.
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 ｜ 𝕏
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.There was a problem hiding this comment.
Real, and it still applies after the TTL refactor. Tracking as a follow-up in #1047 rather than growing this PR further.
Worth flagging for whoever picks it up: a 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.
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 ｜ 𝕏
Rebuilt on current main with the non-AWS changes removed - the @cache policy fix, the getOrSet TTL callback, and the parser grammar build output now live in #1068. The duplicate varlock changeset went with them. DEPENDS ON #1068. The session cache passes getOrSet a TTL callback, which that PR adds, so this does not typecheck until #1068 is in main: plugin.ts: Argument of type '(session: StoredSession) => number' is not assignable to parameter of type 'string | number' That is the only outstanding error. Rebase on main once #1068 lands. Parked pending a real-world test - see #1047 for the known open gap in cross-process coordination on the failure path.
60afaff to
78afc83
Compare
|
Rebased onto current main and cleaned up in 78afc83. The diff is now the MFA feature only: the Closing for now rather than merging. The original request was walked back before this was built, and the preview package was never tested, so there is no confirmed user for it. The branch To pick it back up:
|
1 participant
Note
The non-AWS fixes in here (the
@cachepolicy bug, thegetOrSetTTL callback, and the parser grammar build output) have been split out into #1068 so they can land on their own. This PR is now just the MFA feature, and will be rebased once #1068 merges.Holding it pending a real-world test: the original requester walked the request back before this was built and has not tested the preview package. See #1047 for the known open gap.
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