feat: elastic config command group + credential-safe project create#216
Conversation
✅MegaLinter analysis: Success
See detailed reports in MegaLinter artifacts MegaLinter is graciously provided by OX Security |
5803480 to
246cf21
Compare
| * `~/.elasticrc.yml` (authored by us — we don't pick among the legacy | ||
| * discovery candidates when creating from scratch). | ||
| */ | ||
| function resolveConfigPath (options: Record<string, string | number | boolean>): string { |
There was a problem hiding this comment.
this function and the defaultConfigPath in credentials.ts seem to me like they could be a shared function? They seem to have the same order and fallback
maybe we could extract to writer.ts or a new config/paths.ts?
There was a problem hiding this comment.
Good catch — consolidated both into a single resolveConfigPath(explicit?) in writer.ts, used by both commands.ts and cloud/credentials.ts now.
| const writeResult = await writeConfig(configPath, next, { restrictPermissions: hasInlineSecrets(next) }) | ||
| const storeAvailable = await store.isAvailable() | ||
|
|
||
| const redacted = redactCredentials(body, '(saved to keychain)') |
There was a problem hiding this comment.
nit; marker says '(saved to keychain)' even when no keychain is available and secrets are written inline.
There was a problem hiding this comment.
Fixed — the marker is now dynamic: (saved to keychain) / (saved to secret_service) etc. when a store actually wrote the secret, (saved inline to config) when it landed in the YAML. Added a test asserting the inline-fallback case.
| * values behind. Duplicates the check in `commands.ts` to keep this module | ||
| * self-contained (the set of "secret field paths" is small and stable). | ||
| */ | ||
| function hasInlineSecrets (config: RawConfig): boolean { |
There was a problem hiding this comment.
hasInlineSecrets is implemented three times, here in credentials.ts, in commands.ts , and in loader.ts. All three walk the config differently but answer the same question. Could we extract a single hasInlineSecrets(config) into writer.ts and import it in all three places? Reduces the risk of them drifting apart when new secret fields are added? 🤔
There was a problem hiding this comment.
I missed your code comment about duplication and that this was a conscious design decision. The comment only talks about commader.ts, just to check, is the other copy in credentials.ts along the same design decision?
There was a problem hiding this comment.
Agreed, done. Extracted to hasInlineSecrets(config) + SECRET_AUTH_FIELDS in writer.ts and deleted the three local copies. The writer version walks every service block dynamically (instead of a hard-coded list) so new blocks don't need an update here.
|
small conflict after merging my PR sorry! |
246cf21 to
fe06dda
Compare
JoshMock
left a comment
There was a problem hiding this comment.
LGTM on the whole. Just one question in there.
Also, if we haven't done this already, it'd be nice to add some end-to-end tests for SecretStore that actually run on each OS to validate that it interacts with each store backend correctly. But that can happen later.
| if (isCredentialCommand(def.name)) { | ||
| (cmd as Command) | ||
| .option('--save-as <name>', 'store returned credentials in the OS keychain and upsert a context of this name') | ||
| .option('--credentials-file <path>', 'write credentials to a standalone YAML config fragment at this path (0600)') | ||
| .option('--config-file <path>', 'override the config file written by --save-as (defaults to ~/.elasticrc.yml)') | ||
| .option('--force', 'overwrite an existing context (--save-as) or file (--credentials-file)') | ||
| } |
There was a problem hiding this comment.
Will these options also show up in the JSON schema for agents that are calling this? Or will they only work as CLI args? This is really the only module that manually applies extra options like this (other than global args, obviously), so it's mostly a question of whether these make more sense as CLI args only or would benefit from being added to the JSON schema.
There was a problem hiding this comment.
Good question! These are CLI-only — they won't show up in the JSON schema. The schema (derived from the Zod input in defineCommand) describes the API request body, while these options control local credential storage, which is really a CLI-layer concern. An agent calling this would manage auth differently and wouldn't need the keychain integration. --wait on project create follows the same pattern for the same reason.
That said, if you think there's a case where having them in the schema would be useful, happy to open a follow-up issue to track it!
New flag-driven command tree for creating and maintaining the config file without hand-editing YAML: elastic config context list/add/edit/remove elastic config current-context get/set `edit` supports both `--set key=value` patch mode and an `$EDITOR` round-trip. Secrets are written to the OS keychain when available (macOS `security`, Linux `secret-tool`/`pass`, Windows Credential Manager) via a new `SecretStore` abstraction that shells out to the same tools the existing read-side resolvers use. The YAML holds a `$(keychain:...)` resolver expression rather than the raw secret, mirroring the read side. When no keychain is available (or `--inline-secrets` is passed), values are written inline and the file is chmod'd to 0600. `loadConfig` now emits an stderr warning when a loaded config has inline secrets at looser-than-0600 permissions, pointing at chmod 0600 or the new `config context edit` migration path. Skips the config-loading preAction hook for `config` descendants so the commands tolerate the file being absent (they create it).
…#154) `serverless {es,observability,security} projects create` and `reset-credentials` gain three new flags to keep admin credentials off stdout -- critical for agent/LLM workflows where captured output persists into model context and transcripts. --save-as <name> store returned creds in the OS keychain, upsert a context bound to the new project's endpoints, and redact stdout. Next command runs as `elastic --use-context <name> ...` with zero manual wiring. --credentials-file <path> write a standalone 0600 YAML config fragment at <path> instead of mutating the main config. --force overwrite an existing context / file. `reset-credentials --save-as <ctx>` updates the named context's auth in place -- URLs preserved, only passwords rotate. Without either flag, behaviour is unchanged. Reuses the `SecretStore` abstraction from the `elastic config` commands so a single keychain namespace covers both authored and auto-generated creds.
fe06dda to
f2d3fa2
Compare
Review guidance
Please review each commit individually — they address separate issues but share a small abstraction (
SecretStore) introduced in the first:ea6c85f—elastic configcommand group (Configuration file management commands #75)246cf21—--save-as/--credentials-fileon serverless project create + reset-credentials (Credentials in stdout during project creation are problematic for agent/LLM workflows #154)Each commit builds, lints, and tests green on its own.
Summary
Closes #75 and #154 by sharing a single
SecretStoreabstraction between config authoring and serverless project creation.elastic config …(#75 — commit 1)New flag-driven command group for authoring the config without hand-editing YAML:
Secrets go to the OS keychain when available (macOS
security, Linuxsecret-tool/pass, Windows Credential Manager). The YAML then contains$(keychain:...)resolver expressions, mirroring the existing read side insrc/config/resolvers.ts. If no keychain is available (or--inline-secretsis passed), secrets are written inline and the file ischmod 0600.A new stderr warning fires at load time when a config has inline (non-resolver) secrets at looser-than-0600 permissions.
Credential-safe
serverless projects create/reset-credentials(#154 — commit 2)Scoped to those commands, three new flags:
--save-as <name>— stores admin creds in the keychain, upserts a config context bound to the new project's endpoints, and redacts stdout. Next command runs aselastic --use-context <name> ...with zero manual wiring.--credentials-file <path>— alternative that writes a standalone 0600 YAML fragment instead of mutating the main config.--force— overwrite on name/file collision.reset-credentials --save-as <ctx>updates the existing context's auth in-place (URLs preserved). Without either flag, behavior is unchanged.What a reviewer should know
SecretStoreshells out to the same OS tools the existing read-side resolvers use, behind an interface so a native binding (keytar /@napi-rs/keyring) can be dropped in later without API churn.service="elastic-cli",account="<context>:<dotted.field.path>"(e.g.prod:elasticsearch.auth.password). Easy to audit (security dump-keychain/secret-tool search service elastic-cli) and to wipe.configcommands skip theloadConfigpreAction hook (seesrc/cli.ts) — they author the file and must tolerate it being absent.projects createwithout--save-as/--credentials-filereturns the full JSON response verbatim. A global--redactfilter is deliberately deferred to a follow-up once usage patterns settle.elastic config initTUI (@inquirer/prompts), reusing the same primitives.Test plan
npm run build— clean.npm run test:lint— clean.npm run test:unit— 940 tests pass (75 new). Coverage 98.59% lines / 91.29% branches / 96.57% functions (above 90% threshold).config context add/edit/remove/listwith keychain (macOS) + inline fallback,current-context set/get, name-collision guards, $EDITOR round-trip,--inline-secretsopt-out.--save-asagainst cloud functional harness (requires real cloud creds) — unit tests cover the keychain/config/redaction flow with canned responses.