Skip to content

feat(platform): pluggable credential resolvers — remove the 1Password default - #3841

Open
jamaynor wants to merge 2 commits into
mvanhorn:mainfrom
jamaynor:feat/pluggable-credential-resolvers
Open

feat(platform): pluggable credential resolvers — remove the 1Password default#3841
jamaynor wants to merge 2 commits into
mvanhorn:mainfrom
jamaynor:feat/pluggable-credential-resolvers

Conversation

@jamaynor

Copy link
Copy Markdown

What

Every printed CLI currently ships a 1Password dependency whether or not its operator uses 1Password:

  • platform_profile.go.tmpl emits an OnePasswordResolver that shells out to 1password-pp-cli secrets read --reveal
  • ValidateCredentialReference rejects any reference not beginning with op://
  • platform_cli.go.tmpl's platformResolverFactory returns that resolver as the only option

The CredentialResolver interface was already the right abstraction — it was just bypassed by a hardcoded validator and a single hardcoded implementation. This puts a registry behind it:

  • RegisterCredentialResolver(scheme, resolver) — scheme and resolver register together, so a scheme the validator accepts but no resolver handles cannot exist
  • ValidateCredentialReference is registry-driven; with nothing registered every reference is rejected with an error saying so
  • RegistryResolver dispatches by scheme and is what the generated CLI uses; no vendor is privileged
  • New spec field auth.credential_resolvers: [file|bitwarden|onepassword], defaulting to [file] — the one resolver that depends on nothing installed. 1Password remains available as an explicit opt-in
  • Unknown resolver names are a spec validation error naming the catalog, not a silent skip

Notes for review

  • CredentialScheme.Validate exists because a filesystem path is not a sequence of opaque identifiers: file:///etc/key splits to a leading empty component, so the generic component rules would reject every absolute path
  • Test fixtures now come from a credentialRefFixture template function instead of hardcoded op:// strings — hardcoding one vendor's syntax in the test templates is part of how the original assumption stayed invisible
  • Subprocess-backed resolvers share runCredentialCommand, which enforces: no credential value ever enters argv, and provider stderr never enters an error (a secret manager may echo the resolved value while reporting an unrelated failure)
  • Second commit is an unrelated pre-existing fix: the read-only-dir save test fails under root (CAP_DAC_OVERRIDE bypasses the DAC check); it now skips as root, mirroring its existing Windows skip. Happy to split it out if preferred

Behaviour change

A reprint of a CLI that used op:// references rejects them unless its spec opts into onepassword. In practice nothing is affected today: registerPlatformSource is never called by any printed CLI, so this path is dead code everywhere — which is also why the hard dependency went unnoticed.

Verification

  • internal/spec suite green including 9 new tests
  • Full internal/generator suite run to completion (-timeout 45m): no failures attributable to this change (one pre-existing root-environment failure, fixed in the second commit)
  • Three end-to-end prints of a real spec — default, [bitwarden, file], [onepassword] — each passing the generator's own build/vet/test/bundle gates; the default print contains zero 1Password references

🤖 Generated with Claude Code

root and others added 2 commits July 29, 2026 00:06
…word default

Every printed CLI shipped a 1Password dependency whether or not its operator
used 1Password. platform_profile.go.tmpl emitted an OnePasswordResolver that
shells out to `1password-pp-cli secrets read --reveal`;
ValidateCredentialReference rejected any reference not beginning with op://; and
platform_cli.go.tmpl's platformResolverFactory returned that resolver as the
only option. The net effect was that the entire client-profile mechanism was
unusable without 1Password installed, and every SourceProfile in every printed
CLI was implicitly a 1Password profile.

The CredentialResolver interface already existed and was already the right
abstraction — it was simply bypassed by a hardcoded validator and a single
hardcoded implementation. This change puts a registry behind it.

- CredentialScheme declares a reference syntax; RegisterCredentialResolver takes
  a scheme AND its resolver together, so a scheme the validator accepts but no
  resolver handles cannot exist. That combination is a reference that passes
  validation and then fails at use, which is the worst time to find out.
- ValidateCredentialReference is registry-driven. No scheme is built in. With
  none registered, every reference is rejected and the error says exactly that —
  correct for a CLI never configured to reach a secret manager, and strictly
  better than silently binding to whichever vendor was hardcoded.
- RegistryResolver dispatches by scheme and is what platformResolverFactory now
  returns. No manager is privileged.
- auth.credential_resolvers selects which resolvers compile in. Omitted, it
  defaults to [file] — the one resolver that depends on nothing installed.
  1Password remains available as an explicit opt-in for anyone who does use it.
- Three resolvers ship: file (file://), bitwarden (bws://), onepassword
  (op://). Adding a fourth is a template plus a catalog entry; the validator,
  registry and dispatcher do not change.

Two things fell out of building it that are worth keeping:

CredentialScheme.Validate exists because a filesystem path is not a sequence of
opaque identifiers. "file:///etc/key" splits to a leading EMPTY component, so
the generic component rules rejected every absolute path — that is, every valid
file reference. Path-shaped schemes supply their own rules rather than
contorting the generic ones.

Test fixtures are now generated via a credentialRefFixture template function
rather than hardcoding op:// strings. Hardcoding one vendor's syntax in the test
templates is part of how the original assumption got baked in and stayed
invisible: the conformance tests could only pass under 1Password.

Subprocess-backed resolvers share runCredentialCommand, which enforces the two
rules that were previously restated per vendor: no credential value ever enters
argv, and the provider's stderr never enters an error (a secret manager may echo
the resolved value while reporting an unrelated failure, and error strings reach
logs and run receipts).

Behaviour change: a reprint of an existing CLI that used op:// references will
reject them unless its spec opts into `onepassword`. In practice nothing is
affected today — registerPlatformSource is never called by any printed CLI, so
this entire path is dead code everywhere, which is also why the 1Password
dependency went unnoticed.

Verified by printing a real spec three ways: default (emits resolver_file.go
only, zero 1Password references anywhere in the tree), credential_resolvers:
[bitwarden, file], and [onepassword]. All three pass the generator's own build,
vet, test and bundle gates. An unknown resolver name is rejected at spec
validation with the catalog listed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PieLzEBrxVUbR4HauaTG3u
TestConfigSaveFailureLeavesOriginalParseable makes save() fail by chmod-ing the
config directory to 0500 and asserting the write is refused. Root ignores that:
CAP_DAC_OVERRIDE bypasses the DAC check, so the temp file is created anyway,
save() returns nil, and the test fails for a reason unrelated to the code it
covers.

Verified rather than assumed, in both directions: as uid 0 a write into a 0500
directory succeeds, and as a normal user the same write is refused. So the
premise holds everywhere except root, and the skip is environment-specific
rather than a blanket disable — the assertion still runs in CI and on developer
machines.

This mirrors the Windows skip already at the top of the same test: the mechanism
the test depends on does not exist in this environment. The alternative —
loosening the assertion to tolerate a nil error — would delete the coverage for
everyone, which is the wrong trade for an environment quirk.

Pre-existing on main; unrelated to the credential-resolver change in the
preceding commit, and kept separate for that reason.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PieLzEBrxVUbR4HauaTG3u
@mergify

mergify Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 2 of 2 protections blocking · waiting on 👀 reviews, 🤖 CI and 🙋 you

Protection Waiting on
🔴 require-ready-label-and-ci 👀 reviews, 🤖 CI and 🙋 you
🔴 🚦 Auto-queue 🙋 you

🔴 require-ready-label-and-ci

Waiting for

  • #review-threads-unresolved = 0
  • check-success = build-and-test
  • any of:
    • label = ready-to-merge
    • title ~= ^chore\(main\): release
This rule is failing.
  • #review-threads-unresolved = 0
  • check-success = build-and-test
  • any of:
    • label = ready-to-merge
    • all of:
      • head = release-please--branches--main
      • title ~= ^chore\(main\): release
  • #changes-requested-reviews-by = 0
  • check-success = generated-test
  • check-success = go-lint
  • check-success = golden
  • check-success = pr-title
  • check-success = test
  • any of:
    • -files ~= ^(\.github/workflows/|\.github/scripts/|scripts/|\.github/CODEOWNERS$)
    • approved-reviews-by = mvanhorn
    • approved-reviews-by = tmchow
    • author = mvanhorn
    • author = tmchow
  • any of:
    • check-success = Greptile Review
    • check-neutral = Greptile Review
    • check-skipped = Greptile Review
    • head ~= ^mergify/merge-queue/
    • label = queued
    • all of:
      • head = release-please--branches--main
      • title ~= ^chore\(main\): release

🔴 🚦 Auto-queue

Waiting for

  • label = ready-to-merge
This rule is failing.

When all merge protections are satisfied and these conditions match, this pull request will be queued automatically.

  • label = ready-to-merge

Comment on lines +115 to +121
// Surrounding whitespace is stripped so a file written with a trailing
// newline — which every editor and every `echo` produces — works.
value := strings.TrimSpace(string(data))
if value == "" {
return nil, fmt.Errorf("credential file %s is empty", path)
}
return []byte(value), nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 File resolver mutates credentials

When a file-backed credential legitimately begins or ends with whitespace, strings.TrimSpace removes those significant bytes, causing the generated CLI to authenticate with a different value and reject an otherwise valid credential.

Suggested change
// Surrounding whitespace is stripped so a file written with a trailing
// newline — which every editor and every `echo` produces — works.
value := strings.TrimSpace(string(data))
if value == "" {
return nil, fmt.Errorf("credential file %s is empty", path)
}
return []byte(value), nil
// Remove one editor-added line ending without changing other credential
// bytes, since leading or trailing whitespace may be significant.
value := strings.TrimSuffix(string(data), "\n")
value = strings.TrimSuffix(value, "\r")
if value == "" {
return nil, fmt.Errorf("credential file %s is empty", path)
}
return []byte(value), nil

Fix in Codex Fix in Claude Code Fix in Cursor Fix in Conductor

@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds spec-selectable credential resolvers and makes file-backed credentials the vendor-neutral default.

  • Introduces registry-driven credential reference validation and dispatch in generated CLIs.
  • Adds file, Bitwarden, and opt-in 1Password resolver templates plus shared subprocess handling.
  • Adds resolver catalog validation and resolver-aware generated test fixtures.
  • Documents auth.credential_resolvers and updates generated conformance tests.
  • Skips a POSIX permission test when running as root.

Confidence Score: 3/5

The file resolver’s credential mutation should be fixed before merging because it can make valid file-backed credentials fail authentication.

The new default resolver applies strings.TrimSpace to credential contents, changing valid leading or trailing whitespace bytes before they reach the authentication path.

Files Needing Attention: internal/generator/templates/platform_resolver_file.go.tmpl

Important Files Changed

Filename Overview
internal/generator/templates/platform_profile.go.tmpl Replaces hardcoded 1Password validation and resolution with a synchronized scheme-and-resolver registry.
internal/generator/templates/platform_resolver_file.go.tmpl Adds the default file resolver, but its blanket whitespace trimming can alter valid credential values.
internal/generator/templates/platform_resolver_bitwarden.go.tmpl Adds ID-based and project/key-based Bitwarden Secrets Manager resolution with duplicate-key rejection.
internal/generator/templates/platform_resolver_onepassword.go.tmpl Moves 1Password support into an explicitly selected resolver template.
internal/spec/credential_resolvers.go Defines the resolver catalog, default selection, template mapping, validation, and synthetic fixture references.
internal/generator/generator.go Emits only the credential resolver templates selected by the effective auth configuration.
internal/spec/spec.go Adds the resolver selection field to AuthConfig and validates catalog names during spec validation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    S[auth.credential_resolvers] --> T[Selected resolver templates]
    T --> I[Resolver init registration]
    R[Credential reference] --> V[Registry validation]
    V --> D[RegistryResolver dispatch]
    I --> V
    D --> F[File resolver]
    D --> B[Bitwarden resolver]
    D --> O[1Password resolver]
    F --> C[Resolved credential bytes]
    B --> C
    O --> C
Loading

Fix All in Codex Fix All in Claude Code Fix All in Cursor Fix All in Conductor

Reviews (1): Last reviewed commit: "test(generator): skip the read-only-dir ..." | Re-trigger Greptile

jamaynor pushed a commit to jamaynor/cli-printing-press that referenced this pull request Jul 29, 2026
…resolvers

Merged to the fork's main so local prints and installs carry the fix while
upstream PR mvanhorn#3841 awaits review. The upstream merge
remains the maintainer's call.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PieLzEBrxVUbR4HauaTG3u
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant