feat(aws): consolidate credential handling into shared module #224
Loading…
Reference in a new issue
No description provided.
Delete branch "swampbot/issue-1804-aws-credential-consolidation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Closes #1804 — consolidates duplicated AWS credential handling across five independent implementations into a single canonical source.
Before: The S3 datastore had all three credential features (preflight, IMDS skip, SSO error hints). The vault had a hand-copied error hint. The three codegen enrichments, the s3-bootstrap workflow, and 289 generated models had none. One expired SSO token produced three different behaviors.
After: A canonical source at
codegen/shared/awsCredentials.tsgenerates credential utilities into every consumer:deno task generate:aws-credentialslibGenerator.tssource_profilechains and readssso-sessionblocks forsso_regionformatAwsCredentialHinttakes acontextparameter ("Datastore"/"Vault"/"Model") so each consumer provides its own labelNew behaviors:
withRetry, and SSO-aware region resolutionFiles changed
codegen/shared/awsCredentials.ts— canonical credential utilities generator (single source of truth)codegen/scripts/generate-aws-credentials.ts— task scriptcodegen/aws/libGenerator.ts— template updated with credential code + region parser SSO fixcodegen/aws/enrichments/(4 files) — IMDS skipdatastore/s3/.../s3_client.ts— imports from generated module, ~80 lines removedvault/aws-sm/.../aws_sm_errors.ts— imports from generated module, ~80 lines removedvault/aws-sm/.../aws_sm.ts— IMDS skip + preflight addedworkflows/s3-bootstrap/.../provisioner.ts— IMDS skip addedmodel/aws/*/(280 services × 2 files) — regenerated with credential utilitiesCLAUDE.md— documented new task and canonical sourceTest plan
deno check main.ts)generate:awsrun produces 0 diffsgenerate:aws-credentialsrun produces 0 diffs🤖 Generated with Claude Code
Adversarial Review
Medium
s3_client.ts:509-527 — Non-timeout errors from preflightCredentials are mislabeled as TimeoutError
The catch block in the S3Client preflightCredentials method wraps any non-S3OperationError as a new S3OperationError with hardcoded name "TimeoutError". If the shared preflightCredentials function ever throws a non-timeout error (e.g., a TypeError from a bad argument, or a future code path change that throws before the probe starts), it would be misclassified as a timeout.
In practice this is currently safe because headBucket always wraps errors via run() then wrapError() then S3OperationError, and the only other rejection source is the timeout Promise. But the assumption is implicit and fragile — a future change to the shared preflightCredentials function could introduce a non-timeout rejection path that gets mislabeled.
Suggested fix: Derive the error name from the actual error rather than hardcoding "TimeoutError". For example, check err.message for the "Credential preflight timed out" substring to distinguish timeouts from other failures.
aws_sm.ts:272-282 — runPreflight() is unreachable through the typed public API
runPreflight() is defined on AwsSmVaultProvider, but createProvider() (line 748) returns VaultProvider & VaultDeleteProvider, and neither interface includes runPreflight. External callers that receive the provider through the typed API cannot call it without a cast. If the swamp framework uses convention-based runtime discovery (e.g., checking for the method name at runtime), this is fine. If it relies on the type system, the method is dead code and the preflightCredentials import is unused.
Suggested fix: Either add runPreflight to an interface (perhaps a new VaultPreflightProvider) and update createProvider's return type, or document that the framework calls it by convention.
Low
Breaking change to formatAwsCredentialHint re-exported signature
formatAwsCredentialHint gained a required third parameter (context: string). Both s3_client.ts and aws_sm_errors.ts re-export this function. Any downstream consumer that imported it with the old 2-argument signature will get a TypeScript compilation error. This is caught at compile time (strict mode), so it will not produce runtime bugs, but it is a breaking API change on exported symbols.
Enrichment files inline the IMDS check instead of importing disableImdsIfOffEc2()
The four enrichment files (bedrock-knowledgebase/methods.ts:16-22, cfn-stackset/methods.ts:15-21, rds-dbcluster/enrich.ts:25-31, rds-dbcluster/list.enrich.ts:12-18) each inline the 7-line IMDS environment check rather than importing the shared function. This is because disableImdsIfOffEc2() is generated as non-exported in the model aws.ts and there is no shared enrichment-accessible module. The behavior is correct (the inline code is identical), but it means future changes to the IMDS logic need to be applied in 4+ additional places beyond the canonical source.
Verdict
PASS — This is a well-structured consolidation. The single-source-of-truth pattern for credential utilities is sound, the codegen template produces correct output, all call sites are updated, and the test coverage is thorough (including regression guards for edge cases like BucketRegionMismatch and _CredentialsProviderError cause-chain stripping). The medium findings are worth addressing but neither represents a production failure path today.
Code Review
Blocking Issues
runPreflight()is new functionality with no tests (vault/aws-sm/extensions/vaults/aws_sm.ts).The
runPreflight()method is newly added in this PR (confirmed by diff). Neitheraws_sm_test.tsnoraws_sm_errors_test.tscontains any test for it. The method's logic is not trivial: it probes viaDescribeSecretCommandwith a sentinel secret ID and swallowsResourceNotFoundExceptionwhile propagating everything else. If the swallow logic is misconfigured (wrong error name, wrong error shape from a new SDK version),runPreflight()could silently succeed when credentials are invalid.CLAUDE.md explicitly requires: "New functionality in vault/ or datastore/ extensions should have corresponding tests." A mock-server test (using
Deno.serve({ port: 0 })) that covers at least (a) success onResourceNotFoundException, (b) propagation of a credential error likeAccessDeniedException, and (c) the 3 000 ms timeout frompreflightCredentialsis needed.Suggestions
Typo in
codegen/scripts/generate-aws-credentials.tsline 4:codgenDirshould becodegenDir. The variable is used correctly on line 5 but the misspelling makes it inconsistent with thecodegenterminology used everywhere else.Enrichment files still inline the IMDS disable guard (
bedrock-knowledgebase/methods.ts,cfn-stackset/methods.ts,rds-dbcluster/enrich.ts,rds-dbcluster/list.enrich.ts). These files added the 5-lineAWS_EC2_METADATA_DISABLEDcheck inline rather than callingdisableImdsIfOffEc2(). Given that enrichment files can't import fromcodegen/shared/at runtime, anddisableImdsIfOffEc2is non-exported in the generated_lib/aws.ts, this appears to be an unavoidable architectural constraint. A brief comment acknowledging the intentional duplication (e.g.// disableImdsIfOffEc2 inlined — enrichments can't import from shared runtime modules) would help future readers not chase a phantom refactor.withMockServerins3_client_test.tsleaksAWS_EC2_METADATA_DISABLED:S3Client's constructor callsdisableImdsIfOffEc2(), which setsAWS_EC2_METADATA_DISABLED=trueif absent.withMockServersaves and restoresAWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEYbut notAWS_EC2_METADATA_DISABLED. CLAUDE.md requires restoring all env vars infinally. In practice this leak is benign (all tests benefit from IMDS being off) and pre-exists this PR, but it's worth a note for completeness.Missing inline
sanitizeResourcescomment on the first test (s3_client_test.tslines 93–113,"getObject: surfaces ETag from GetObject response"): it hassanitizeResources: falsewith no inline comment explaining why, unlike the immediately following test which includes the SDK connection-pool rationale. Minor inconsistency with the project convention.Adversarial Review
Critical / High
No critical or high severity issues found.
Medium
datastore/s3/extensions/datastores/_lib/s3_client.ts:517— Timeout detection via string matching is fragileThe
preflightCredentialswrapping logic detects timeouts by checkingmsg.includes("Credential preflight timed out"). If the message text inawsCredentials.tsis ever changed (it's a code-generated template), this string match silently breaks and the error gets wrapped with the wrongname(the probe error's.nameinstead of"TimeoutError").Breaking example: Someone edits
awsCredentials.tsto say"Credential check timed out after…"— the S3 client now wraps the timeout as a generic error instead ofTimeoutError, and callers that match onerror.name === "TimeoutError"misclassify it.Suggested fix: Rather than string-matching the message, use a sentinel — either a custom error class (
class PreflightTimeoutError extends Error) inaws_credentials.tsthat the S3 client caninstanceof-check, or set.name = "TimeoutError"on the error thrown inpreflightCredentialsso the catch site can checkerr.name === "TimeoutError"instead of parsing the message.codegen/aws/libGenerator.ts:95(generated output) —source_profilecycle doesn't followsource_profilethrough the region chain, only through the same profile-level region lookupThe
resolveRegionfunction'ssource_profileloop setscurrent = section.get("source_profile") ?? "". Whensource_profileis absent,currentbecomes"", which is falsy, so thewhile (current && !visited.has(current))loop exits. However, an empty string""would also matchvisited.has("")asfalseon first pass — but thewhilecheck oncurrentbeing falsy catches it first, so no infinite loop. This is correct but the empty-string fallback is a subtle implicit sentinel.Suggested fix: Use
undefinedinstead of""for the no-profile case:current = section.get("source_profile")— theMap.getreturnsundefinedwhen absent, andwhile (current && ...)handlesundefinedidentically to"". The?? ""is dead weight.Low
codegen/aws/enrichments/rds-dbcluster/enrich.ts:25-32— IMDS disable insidetryblock that catches broadlyThe IMDS-disable block is placed inside the existing
trythat catches(err: unknown)at line 46. IfDeno.env.getorDeno.env.setthrew (e.g., a permissions error in a sandboxed Deno), it would be silently swallowed by the generic catch, and the caller would get back the unenrichedstatewith no indication that the credential setup failed. In practice,Deno.env.get/setdon't throw under normal Deno permissions, so this is theoretical.codegen/shared/awsCredentials.ts:123— Escaped unicode in generated templateThe template uses
\\u2014(literal backslash-u-2014) for the em-dash in the generated preflight timeout message. This is intentional (it becomes—in the generated output, which JavaScript interprets as—at runtime). Correct, but worth noting that if someone naively reads the template they might mistake it for a double-escape bug. No action needed.codegen/scripts/generate-aws-credentials.ts— NoincludePreflightpassed explicitlyThe script calls
generateAwsCredentialSource({ exported: true, includeFileHeader: true })without specifyingincludePreflight. This defaults totrue(line 20 inawsCredentials.ts:opts.includePreflight !== false). The hand-written extensions (S3 datastore, vault, workflows) getpreflightCredentialsincluded, which is correct. But the default-true convention is inverted from theexporteddefault (also true viaopts.exported !== false), so both happen to work, but it's easy to misreadincludePreflightas "opt-in" when it's actually "opt-out." No bug, just a readability note.Verdict
PASS — This is a clean consolidation refactor. The credential utilities are correctly extracted into a single canonical source (
codegen/shared/awsCredentials.ts) and distributed to hand-written extensions via codegen script and to generated models vialibGenerator.ts. All call sites have been updated to the new 3-argumentformatAwsCredentialHintsignature. Re-exports froms3_client.tsandaws_sm_errors.tspreserve backward compatibility for existing importers. ThepreflightCredentialswrapping ins3_client.tscorrectly handles both timeout and probe-failure paths. Tests have been updated to match the new signatures. The enrichment files correctly inline the IMDS-disable logic with an explanatory comment. The medium-severity string-matching concern (item 1) is a maintainability risk but not a correctness bug in the current state.Code Review
This PR consolidates AWS credential utilities (
classifyAwsCredentialError,deriveAwsErrorCode,formatAwsCredentialHint,disableImdsIfOffEc2,preflightCredentials) into a single canonical source (codegen/shared/awsCredentials.ts) and distributes generated copies to hand-written extension directories. It also upgradesresolveRegioninlibGenerator.tswith SSO session andsource_profilechain support.Blocking Issues
None.
Suggestions
s3_client_test.ts— first test missingsanitizeResources: falseexplanation comment. The test "getObject: surfaces ETag from GetObject response" (line 93) hassanitizeResources: falsebut the block comment above it explains the test's purpose (TOCTOU fix), not why resource sanitization must be suppressed. All other tests in the file have the inline comment// The SDK's NodeHttpHandler keeps an HTTP agent pool alive across requests; resource sanitization would flag those as leaks…inside the test object. CLAUDE.md requires the comment to explain why. A one-liner like the pattern used in the other tests would close the gap.IMDS-disable logic is inlined in four enrichment files.
bedrock-knowledgebase/methods.ts,cfn-stackset/methods.ts,rds-dbcluster/enrich.ts, andrds-dbcluster/list.enrich.tseach contain identical 8-line IMDS disable blocks (each with a comment explaining why). The comment is correct — enrichments run at codegen time and can't share the generatedaws_credentials.ts. A tinycodegen/shared/awsEnrichUtils.tsexporting a callabledisableImdsIfOffEc2(distinct from the string-template version inawsCredentials.ts) would let all four files call a single import rather than duplicate the logic. Not required now, but worth tracking if a fifth enrichment lands.codegen/deno.jsongenerate:aws-credentialstask — no--allow-env. Other codegen run tasks include--allow-env. The new task omits it:The script and its transitive imports appear not to read any env vars (copyright header is a static string, the credential source is a template), so this is likely intentional. If
generateCopyrightHeader()ever gains an env-var read the task will fail with an opaque permission error. Worth a quick check / doc comment confirming the omission is deliberate.