Skip to content

Option to encrypt the OAuth vault at rest #408

Closed

Description

@stenh0use

Problem

Every OAuth-backed MCP server mcporter manages stores its tokens in ~/.mcporter/credentials.json as plaintext JSON. Secret scanners flag that file for storing API tokens in plaintext.

Proposal

An opt-in mode where, when MCPORTER_ENCRYPTION_KEY is set, credentials.json holds AES-256-GCM ciphertext instead of JSON. That way cleartext JSON never touches the disk when enabled. Using an env var keeps things simple as where the key comes from is then up to the user, eg. macOS Keychain, 1Password CLI, lpass, etc.

Notes

I've done some work already on this to prove the idea, but wanted to understand whether this is something of interest before putting up a PR.

Approach:

  • Zero new dependencies using AES-256-GCM via node:crypto
  • Migration: if the key is set and the vault is plaintext JSON, the first write converts it. If the key is unset and the vault is encrypted, reads fail with a clear error naming the file and the env var.
  • An oauthVaultEncryption: optional | required config key, where required refuses vault writes without a valid key. This stops a cleared environment from silently re-creating the vault as plaintext.
  • A wrong key or tampered file fails decryption with a clear error, and never enters the corrupt-vault repair path, so a bad key can't cause the vault to be rewritten empty.
  • With the key unset, behavior is unchanged: the vault stays plaintext JSON and everything works exactly as today.
  • mcporter config doctor reports the vault's on-disk state as plain JSON or encrypted, without decrypting, so it stays safe to run without the key set.

Activity

  1. added
    P2Normal priority bug or improvement with limited blast radius.
    clawsweeper:needs-product-decisionClawSweeper marked this issue as needing a product or behavior decision.
    clawsweeper:needs-security-reviewClawSweeper marked this issue as needing security-sensitive review.
    clawsweeper:no-new-fix-prClawSweeper does not recommend queueing a new automated fix PR for this issue.
    impact:securityThis issue is about security boundaries, credentials, authz, sandboxing, or sensitive data.
    on Oct 2, 2026
  2. clawsweeper commented on Oct 2, 2026

    @clawsweeper
    Contributor

    Codex review: this still needs some work. Reviewed October 2, 2026, 5:56 PM ET / 21:56 UTC.

    Summary
    Encryption is absent from current main and v0.14.2, so this proposal remains useful. It adds a security-sensitive storage mode rather than repairing an established encryption contract.

    Likely related people: steipete and feniix (medium-confidence routing candidates).

    Reproducibility: not applicable. as a bug reproduction: source and v0.14.2 confirm plaintext JSON storage, but encryption is a requested new mode. No runtime commands or tests were executed.

    Maintainer decision needed

    Question Recommendation
    Should MCPorter support environment-keyed OAuth vault encryption, and should its required mode protect only the shared vault or every MCPorter-managed credential store? Approve a bounded vault-only design: Sponsor optional shared-vault encryption with an explicit required-write policy and clearly documented exclusions for other credential stores.

    Why: This adds cryptographic key management, persistent-format migration, and configuration policy; existing source and repository guidance do not establish the intended protection contract.

    Next step
    Route the security-sensitive feature and configuration contract to the identified area contributors before considering implementation dispatch.

    Security
    Needs attention: This security-sensitive proposal needs an agreed threat model, key lifecycle, and credential-storage scope before implementation.

    Review details

    Best possible solution:

    If approved, provide versioned authenticated vault encryption with unchanged default behavior, explicit protection limits, atomic locked migration, and non-destructive key-failure handling.

    Do we have a high-confidence way to reproduce the issue?

    Not applicable as a bug reproduction: source and v0.14.2 confirm plaintext JSON storage, but encryption is a requested new mode. No runtime commands or tests were executed.

    Is this the best way to solve the issue?

    Unclear until the protection contract is approved; a centralized vault codec is a focused approach, but vault-only encryption cannot promise that every credential copy avoids plaintext storage.

    AGENTS.md: found and applied where relevant.

    Remaining risk / open question:

    • The protection scope is undecided: vault-only encryption leaves configured token-cache files and pre-existing legacy credential copies outside its guarantee.
    • Encrypted-format migration, key loss or rotation, mixed-key processes, and decryption failures need a contract that preserves credentials and prevents plaintext downgrade or destructive repair.

    Codex review notes: model internal, reasoning medium; reviewed against 9d3cf4f4a3b0.

    Label changes

    Label changes:

    • add P2: This is a concrete credential-storage improvement without evidence of an urgent exploit or broken core workflow.
    • add impact:security: The request concerns protection of OAuth credentials at rest and handling of encryption keys.
    • add issue-rating: 🌊 off-meta tidepool: Current issue advisory state selects this label.
    • add clawsweeper:no-new-fix-pr: Current issue advisory state selects this label.
    • add clawsweeper:needs-product-decision: Current issue advisory state selects this label.
    • add clawsweeper:needs-security-review: Current issue advisory state selects this label.

    Label justifications:

    • P2: This is a concrete credential-storage improvement without evidence of an urgent exploit or broken core workflow.
    • impact:security: The request concerns protection of OAuth credentials at rest and handling of encryption keys.
    Evidence reviewed

    Security concerns:

    • [medium] Vault encryption leaves other credential stores plaintext — src/oauth-persistence-stores.ts:660
      Configured token caches receive JSON token writes alongside the shared vault; any broader at-rest guarantee must account for these copies and legacy files.
      Confidence: 0.98

    What I checked:

    • Current vault stores JSON: The vault reader accepts JSON versions 1 and 2, and writeVault delegates directly to writeJsonFile. The node:crypto import is used for credential identifiers; no vault encryption path exists. (src/oauth-vault.ts:95, 9d3cf4f4a3b0)
    • Existing filesystem protection: JSON is serialized directly to an atomic temporary file. Newly created files default to mode 0600; existing target permissions are preserved. These protections do not encrypt the contents. (src/fs-json.ts:78, 9d3cf4f4a3b0)
    • Additional plaintext storage boundary: Configured tokenCacheDir adds DirectoryPersistence alongside the vault. DirectoryPersistence writes tokens.json as JSON, and composite token saves write to every store. Encrypting credentials.json alone would not eliminate all plaintext credential copies. (src/oauth-persistence-stores.ts:660, 9d3cf4f4a3b0)
    • Destructive repair must remain separate from decryption failure: Malformed JSON currently produces an empty repairable vault. A regression test expressly verifies that clearing a missing entry rewrites a corrupt vault empty; encrypted-format and authentication failures must never enter that recovery path. (tests/oauth-persistence.test.ts:701, 9d3cf4f4a3b0)
    • Latest release lacks encryption: The verified v0.14.2 commit contains the same JSON vault reader and writer. The requested encrypted storage mode is absent from both the latest release and reviewed main. (src/oauth-vault.ts:95, aa0f55f9bffc)
    • Area history and routing: Available file history shows repeated OAuth persistence work by Peter Steinberger and Sebastian B Otaegui. GitHub identifies steipete as the author of merged fix(daemon): share one user daemon across configurations #346 and feniix as the author of merged fix(oauth): serialize the refresh transaction across processes #306. This supports routing, not feature-introduction attribution. (src/oauth-persistence-stores.ts, 9d3cf4f4a3b0)

    Likely related people:

    • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
    • feniix: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
    How this review workflow works
    • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
    • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
    • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
    • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
    • Maintainers can also comment @clawsweeper review to request a fresh review only.
    • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
    • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
    • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
  3. alexminza commented on Oct 6, 2026

    @alexminza
    Contributor

    +1 from a consumer. The vault is the one credential store mcporter owns, and every way of using mcporter ends with it holding live refresh_tokens, plus any DCR-issued client_secret in clientInfo, for every connected server as plaintext JSON: mcporter auth, config login, SDK-driven refresh, and vault set all write the same credentials.json.

    Why file mode is not enough. The 0600 default in fs-json.ts protects the original file on its original host and nothing else. Backups, snapshots, dotfile and state sync, and machine migrations carry the content; whether they carry the mode depends on the tool, and resolveAtomicWriteTarget() then preserves whatever mode the restored file has, so a vault that comes back 0644 stays 0644. Secret scanners flag credentials.json as a plaintext token store regardless of mode. Encryption at rest is the only property that follows the file into every copy.

    Precedent from the @steipete gogcli's OAuth store already works this way: GOG_KEYRING_BACKEND=file with GOG_KEYRING_PASSWORD supplied from the environment or a secret store (docs/install.md). The proposal here is the same env-keyed shape, so anyone already provisioning that kind of secret needs nothing new.

    On the open questions from the triage review, from the consumer side:

    • Scope: the shared vault is the default store and tokenCacheDir is opt-in, so a vault-only guarantee covers the default configuration. Documenting the exclusion rather than widening the guarantee seems right.
    • Threat model: at-rest copies (backups, snapshots, sync, scanners), not same-user runtime access. That is the boundary gogcli's file keyring draws too.
    • Key lifecycle: env-supplied key, consumer-owned provisioning, plus the required mode so a cleared environment cannot silently downgrade to plaintext. The must-have is the one already called out: a decrypt/auth failure must never reach the malformed-vault repair path in readVaultState() that rewrites the vault empty, otherwise one start with a missing key destroys the tokens.
    • Migration: convert-on-first-write with the key set is sufficient; a config doctor state report that runs without the key would make rollout verifiable.

    Happy to test a PR and report back.

  4. alexminza commented on Oct 6, 2026

    @alexminza
    Contributor

    Following up with a concrete design, so the open questions from the triage review have a proposal to accept or amend. It mirrors what gogcli already ships for the same problem in the same org, and it stays inside the bounded vault-only scope the review recommended. Happy to build on the proposal author's WIP or to submit this as a PR with tests, whichever the maintainers prefer.

    Why mirror gogcli. Its file keyring has carried headless deployments with exactly this shape: a password from the environment, JWE compact tokens on disk, a lock around read-modify-write, and a hard error when no password can be obtained. The third column below points at the code for each choice, at gogcli v0.43.0 unless another tag is given. Using the same JOSE profile also means any RFC 7516 implementation, gogcli's included, can read the vault with the password.

    Proposal

    Aspect Design gogcli
    Password MCPORTER_VAULT_PASSWORD, a passphrase read verbatim from the environment. No TTY prompt: mcporter runs under agents and daemons, so a prompt would hang call. Minimum 16 characters; empty is rejected. Docs prescribe openssl rand -base64 32. GOG_KEYRING_PASSWORD read from the environment; a TTY prompt when unset; otherwise errNoTTY naming the variable. An empty value is accepted as intentional: fileKeyringPasswordFuncFrom() in internal/secrets/backend.go. Operator guidance: docs/install.md.
    On-disk format The whole vault document as one RFC 7516 compact JWE: PBES2-HS512+A256KW key management, A256GCM content encryption, 8192 PBKDF2 iterations, protected header typ: mcporter-vault+jwe, cty: application/json. Written through the existing atomic temp-file-and-rename path, still 0600 on create. Same JOSE family with a 128-bit wrap: jose.Encrypt(..., PBES2_HS256_A128KW, A256GCM, password) and os.WriteFile(..., 0600) in the 99designs/keyring file backend: file.go at v1.2.2. The iteration count and the 12-byte random salt come from jose2go pbse2_hmac_aeskw.go at v1.11.0. This design widens the wrap to 256 bits to match the content key; the typ/cty headers are new.
    Detection readVaultState() checks for a five-segment compact token before any JSON.parse. Ciphertext therefore never reaches the malformed-JSON repair branch that rewrites the vault empty. Not needed in gogcli: file-keyring items are always ciphertext. New here because plaintext vaults must keep working.
    Failure model Distinct error class with codes password_missing, password_invalid, policy_invalid, decrypt_failed, payload_invalid. The file on disk is left byte-identical in every case. A JWE with a foreign alg or typ is payload_invalid, not corrupt plaintext. Messages name the vault path and env var names, never values. A decode failure propagates as an error from ring.Get; nothing rewrites a store on error; only a post-decrypt JSON failure is classified (ErrCorruptStoredToken): getTokenNoLockOptions() in internal/secrets/token.go. The typed codes are new.
    Verified write The produced token is decrypted in memory before the rename, so a codec regression cannot persist an unreadable vault. verifiedSet() in internal/secrets/store.go: Set, Get, byte comparison.
    Migration A plaintext vault is read as today and converted on the first credential write once the password is set. No command to run. Legacy plaintext client_secret in credentials.json is still read and moves to the keyring on the next write: CredentialsStore.Read() / Write() in internal/oauthclient/credentials.go. Legacy token:<email> keys are copied on first read: docs/auth-clients.md.
    Policy MCPORTER_VAULT_ENCRYPTION=required refuses every vault operation while the password is unset, so a cleared environment cannot recreate a plaintext vault. Default optional keeps current behavior byte-for-byte when no variable is set. gogcli is required by construction: the file backend with no password and no TTY is a hard error. Its explicit opt-out is gog auth credentials set --insecure: internal/cmd/auth_credentials.go.
    Rotation MCPORTER_VAULT_PASSWORD_PREVIOUS, decrypt-only; the next write re-encrypts with the current password. None; a changed GOG_KEYRING_PASSWORD means re-authentication.
    Visibility config doctor reports absent, plaintext, encrypted (alg/enc) or unreadable from the protected header, without the password, and lists the policy and whether the password is set. gog auth keyring prints the resolved backend: internal/cmd/auth_keyring.go; credentials set prints client_secret_in_keyring.
    Scope The shared vault only. tokenCacheDir directories stay plaintext, and the docs say so. Client metadata and service-account keys stay plaintext under the data directory: docs/paths.md ("Data: OAuth client metadata, file-keyring entries, and service-account keys").
    Locking Existing withFileLock() around the vault read-modify-write; unchanged. flock on a .lock file next to the store, shared for reads, exclusive for writes, bounded by GOG_KEYRING_LOCK_TIMEOUT: internal/secrets/keyring_lock.go.

    One structural departure the table does not show: the vault stays one document rather than gogcli's one-file-per-item layout, because the version: 2 vault, its file lock, the same-URL fallback scan and vault set/clear are all document-level contracts. The other two departures, the refused empty password and the 256-bit key wrap, are marked in the gogcli column.

    Answers to the triage review

    • Scope: vault only; documented exclusion for tokenCacheDir. The headless seeding path (vault set, Add mcporter vault set <server> to seed credentials non-interactively #156) writes only the vault, so it is fully covered.
    • Threat model: copies at rest (backups, snapshots, sync, scanners, lost disks), not same-user runtime access. Same boundary as gogcli's file keyring.
    • Key lifecycle: consumer-provisioned passphrase in the environment, required mode against silent downgrade, previous-password rotation, no decrypt-back command.
    • Migration and failure: convert-on-first-write; decryption failure is a typed error that never enters the repair path; the self-check runs before anything is written.

    One question before any PR, since mcporter's AGENTS.md asks contributors to check before adding a dependency. Producing standards-compliant JWE compact is simplest with jose (pure JS, zero dependencies; 6.2.12 is a month old, so inside the release-age policy). The alternative is a hand-rolled compact serialization over node:crypto with PBKDF2 and AES-GCM, which is roughly a hundred lines but is custom crypto framing to maintain. Which do you prefer? Everything above works with either.

    Test coverage I would ship with it: codec round trip, fresh salt per write, wrong and previous password, tampered ciphertext, foreign alg/typ, non-ASCII password, whitespace around the token; read path with the file proven byte-identical after every refusal; write path for conversion, rotation and the corrupt-plaintext case; vault set/clear through an encrypted vault; doctor for all four states; and test:package so the dependency survives the npm tarball, given #406.

  5. alexminza commented on Oct 8, 2026

    @alexminza
    Contributor

    Implementation is up as #425, ready for a maintainer look.

    It follows the design posted above with one change: instead of encrypting the whole credentials.json, the five secret string values of each entry (access_token, refresh_token, client_secret, the PKCE verifier and the OAuth state) are stored in place as RFC 7516 compact JWE strings (PBES2-HS512+A256KW / A256GCM, 8192 iterations, as gogcli's keyring). The file keeps its shape, names, URLs, client IDs and timestamps stay readable, and nothing changes without MCPORTER_VAULT_PASSWORD set. The whole-file variant was dropped because the released reader rewrites a non-JSON vault empty on its first OAuth read, which I reproduced against 9d3cf4f. The config key is the one proposed here, oauthVaultEncryption: "optional" | "required", with MCPORTER_VAULT_ENCRYPTION as the environment override; required refuses every vault operation while the password is unset. The same applies to tokenCacheDir directories, generated CLIs carry the policy, stdio servers are started without the two variables, and config doctor reports sealed and plaintext counts.

    The PR went through eight ClawSweeper review rounds; every finding is closed with a regression test, and the PR body carries an eleven-step transcript of the built CLI and the released 0.14.2 sharing one state directory against a local OAuth authorization server plus MCP server (sealed writes, refresh and tool calls, refusals with unchanged hashes, directory and legacy migration, older-writer round trip, child-process isolation). The branch is synchronized with main and mergeable.

    What needs a maintainer decision is the compatibility contract, which the review asks to be recorded explicitly: a pre-encryption binary, daemon or bundled generated CLI that re-authenticates writes that server's values in plaintext until the next write by a current version re-seals them, so the password should be set only after every consumer sharing the state directory is upgraded and restarted; required is enforced only by versions that know it. The alternative, a separate encrypted store, is what breaks the released reader. Details and the gogcli comparison are in the PR description; happy to adjust anything.

  6. alexminza commented on Oct 14, 2026

    @alexminza
    Contributor

    @steipete this is a severe issue that needs a proper fix.

    It was implemented along the architectural decisions you took in gogcli - please have a look at #425 and state your vision/decision on this.

  7. alexminza commented on Oct 15, 2026

    @alexminza
    Contributor

    @stenh0use #425 implements this request, and your read of it would carry weight with the maintainers. Most of what the issue asks for is in the PR as proposed: the oauthVaultEncryption: optional | required key with required refusing writes without the secret, migration on the first write, fail-closed reads with a clear error naming the file and the variable, a wrong secret or tampered value never reaching the corrupt-vault repair path, config doctor reporting the on-disk state without decrypting, and nothing changing while the variable is unset.

    Two places depart from the proposal, both deliberate, and I would like to hear whether you agree:

    1. Ciphertext per secret value instead of the whole file. Every released reader treats a non-JSON credentials.json as corrupt, and reconcileVaultServerUrl rewrites it empty on the first OAuth read; I reproduced that against 9d3cf4f with a whole-file encrypted vault. A generated CLI bundles its own mcporter and shares the same file, so that reader keeps existing on upgraded machines. Sealing the five secret strings in place keeps the file valid for every version: an older binary sees a token it cannot use, gets a 401, and re-authenticates; nothing is destroyed. The trade is the upgrade contract the review is asking a maintainer to accept, and that older binaries can leave plaintext until a current one re-seals.
    2. JOSE (PBES2-HS512+A256KW, A256GCM) from a passphrase instead of raw AES-256-GCM with a key. Same cipher underneath; the difference is that each value is an RFC 7516 string any JOSE library opens, the format and KDF match the keyring gogcli already ships in this org, and jose was already in the dependency tree through the MCP client, so it still adds no package. The point about where the secret comes from holds unchanged: MCPORTER_VAULT_PASSWORD can come from Keychain, 1Password CLI or anything else.

    If you have time, the branch builds in a minute (git clone -b feat/408-vault-encrypt https://github.com/alexminza/mcporter && pnpm install && pnpm build && node dist/cli.js), and the PR description carries an eleven-step transcript of the branch and the released 0.14.2 sharing one vault. Anything it does not cover for your setup, or anything in the contract above you would not accept, is what I am after.

  8. stenh0use commented on Oct 15, 2026

    @stenh0use
    Author

    Thanks @alexminza. I reviewed #425 and left comments there: #425 (comment)

    Short version: I'm happy with both departures. In a Kingfisher scan, sealing clears the token hits, but the unsealed id_token is still flagged, so that needs to be sealed before merge.

  9. added a commit that references this issue on Oct 17, 2026
    aee85ef
  10. steipete commented on Oct 17, 2026

    @steipete
    Collaborator

    Implemented on main in #425: 90bb83f.

    The feature remains opt-in and preserves the plaintext default. The completed implementation adds persistent encryption markers for supporting versions, including empty stores and all credential-clearing paths. The documentation prominently requires upgrading/restarting shared-state consumers and using isolated HOME/XDG directories and token caches when downgrading: old binaries cannot enforce the new policy and can restore plaintext if allowed to share an encrypted profile.

    Proof includes 181 focused tests, pnpm check, and an installed-package compatibility test against published 0.15.0 covering migration, missing-password byte preservation, isolated rollback/return, unsafe shared-store downgrade reproduction, and re-sealing recovery for both vault and tokenCacheDir. The new marker regressions were first reproduced against the original implementation. Independent P0–P2 review is clean.

    All checks below passed at their exact candidate or merge head.

    Exact-head PR CI: https://github.com/openclaw/mcporter/actions/runs/39006239989
    Default-branch CI: https://github.com/openclaw/mcporter/actions/runs/39007272115
    No-publish release rehearsal: https://github.com/openclaw/mcporter/actions/runs/39007281796

    Thanks @alexminza for the implementation and @stenh0use for the original validation. This is merged on main; no release or version bump was created.

  11. added a commit that references this issue on Oct 17, 2026
    90bb83f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Normal priority bug or improvement with limited blast radius.clawsweeper:needs-product-decisionClawSweeper marked this issue as needing a product or behavior decision.clawsweeper:needs-security-reviewClawSweeper marked this issue as needing security-sensitive review.clawsweeper:no-new-fix-prClawSweeper does not recommend queueing a new automated fix PR for this issue.impact:securityThis issue is about security boundaries, credentials, authz, sandboxing, or sensitive data.issue-rating: 🌊 off-meta tidepoolIssue quality rating does not apply to this item.

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions