fix: source aware api-key flag + redact api key value from logging#1819
fix: source aware api-key flag + redact api key value from logging#1819nborges-aws wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #1819 +/- ##
============================================
+ Coverage 94.84% 94.88% +0.03%
============================================
Files 143 144 +1
Lines 7120 7227 +107
============================================
+ Hits 6753 6857 +104
- Misses 367 370 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| "JSON key containing the API key in the secret", | ||
| z.string().optional(), | ||
| ), | ||
| flag("tags", "tags as key=value (repeatable) or JSON object", z.string().optional()), |
There was a problem hiding this comment.
This advertises repeatable key=value tags or a JSON object, but z.string() plus parseJsonFlag only accepts one JSON value. For example, --tags env=prod currently fails as invalid JSON. Could we implement and test both documented forms, including validating that the JSON form is a string map?
| flag("api-key", "the API key value (inline, file://path, or -)", z.string().optional(), { | ||
| sensitive: true, | ||
| }), | ||
| flag("api-key-secret-arn", "existing Secrets Manager secret ARN", z.string().optional()), |
There was a problem hiding this comment.
In the doc discussion, the preferred external-secret representation was one structured-reference flag rather than separate ARN and JSON-key flags. Could we use something like --api-key-secret-reference '{"secretId":"...","jsonKey":"..."}' for create/update, mutually exclusive with --api-key? We should also have a successful external-reference request-mapping test, I think the current tests only cover invalid combinations.
Update should first retrieve the provider and preserve its existing apiKeySecretSource. A MANAGED provider should accept only --api-key, while an EXTERNAL provider should accept only the structured secret reference. I think we should reject mismatched input locally and test both directions instead of deriving the source mode from the supplied flags.
| // - "file://path" reads the file at path | ||
| // - "-" reads stdin to EOF | ||
| // - anything else is returned as-is (inline value encoded as UTF-8) | ||
| export async function readSource( |
There was a problem hiding this comment.
I already implemented a version of source-aware input on another branch and I think it has some features we can use. Can we use that implementation (pasted below) as the shared behavior? The shared reader should remain byte-oriented so Runtime payloads are not decoded, while text-only credentials use strict UTF-8 decoding:
import { readFile } from "node:fs/promises";
import { addAbortSignal } from "node:stream";
import { buffer } from "node:stream/consumers";
export async function readSource(
source: string,
stdin?: NodeJS.ReadStream,
signal?: AbortSignal,
): Promise<Uint8Array> {
signal?.throwIfAborted();
if (source.startsWith("file://")) {
const path = source.slice("file://".length);
try {
return await readFile(path, { signal });
} catch (error) {
if (error instanceof Error && error.name === "AbortError") throw error;
throw new TypeError(`unable to read source file: ${path}`);
}
}
if (source !== "-") {
return new TextEncoder().encode(source);
}
if (!stdin) {
throw new TypeError("stdin is not available for this source");
}
return buffer(signal ? addAbortSignal(signal, stdin) : stdin);
}
export async function readSourceText(
source: string,
stdin?: NodeJS.ReadStream,
signal?: AbortSignal,
): Promise<string> {
const bytes = await readSource(source, stdin, signal);
try {
return new TextDecoder("utf-8", { fatal: true }).decode(bytes);
} catch {
throw new TypeError("source must contain valid UTF-8");
}
}Runtime can import these functions while retaining its request-specific dual-stdin validation. Identity can use readSourceText for --api-key. What do you think? I can also make the change in my Runtime invoke PR so not a problem really just a suggestion.
Description
Expand api-key-credential-provider create/update operations with full source-aware flag surface.
Changes:
--api-key: accepts inline values, file://path, or stdin so credentials never appear in shell history--api-key-secret-arn+--api-key-secret-json-keyfor existing Secrets Manager entries--api-keyand the pair of external secret cannot be mixed; the secret pair requires both values--tagson create (JSON object)--api-keymarked sensitive so values are redacted from debug logsType of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.