Skip to content

feat(plugins): add host-side secret verification - #1115

Open
allyblockcast[bot] wants to merge 2 commits into
masterfrom
blo-20738-secret-verify
Open

feat(plugins): add host-side secret verification#1115
allyblockcast[bot] wants to merge 2 commits into
masterfrom
blo-20738-secret-verify

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown

Thinking Path

  • Public webhook authentication must not expose plaintext secrets to plugin workers.
  • It also must not turn unauthenticated guesses into provider reads, audit writes, or shared resolution-budget consumption.
  • Secret versions already carry a host-only, version-bound SHA-256 verifier.
  • This PR authorizes the company/plugin/config-path binding, resolves only active version metadata, and compares fixed-size digests in constant time.
  • Alertmanager therefore authenticates secret refs without plaintext access or provider work on guesses.

Linked Issues or Issue Description

What Changed

  • Added ctx.secrets.verify(ref, presented, options) across the SDK protocol, worker RPC, host services, and server handler.
  • Added the verify-only secrets.verify-ref capability so Alertmanager cannot resolve plaintext secret values.
  • Enforced company/plugin/config-path binding authorization before verifier access.
  • Compared candidates against the stored version-bound SHA-256 verifier with timingSafeEqual; verification no longer calls the provider, updates lastResolvedAt, emits successful access events, or consumes the plaintext-resolution limiter.
  • Rejected presented credentials larger than 4096 UTF-8 bytes before database access.
  • Migrated Alertmanager secret-ref auth and updated operator docs/schema guidance.

Verification

  • pnpm exec vitest run src/__tests__/plugin-secrets-handler.test.ts in server: 11 passed.
  • pnpm typecheck in server: passed.
  • pnpm test in packages/plugins/paperclip-plugin-alertmanager: 148 passed.
  • Integration coverage drives 31 invalid verifications, proves zero access events and no lastResolvedAt update, then proves valid verification and explicit plaintext resolution still succeed.

Risks

  • Verification still performs binding and active-version metadata reads; ingress rate limiting remains useful defense in depth against generic request/DB load.
  • webhookTokenRef takes precedence when both token fields are configured; this intentionally revokes stale inline fallback credentials during migration.

Model Used

  • OpenAI gpt-5.6-sol, reasoning-enabled software engineering model with repository tools and code execution; context-window size not disclosed by the runtime.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used
  • I have linked the Paperclip issue and related PR
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have run focused tests locally and they pass
  • I have added regression coverage for the reviewed failure mode
  • I have updated relevant documentation
  • All Paperclip CI gates are green
  • Ally review is approved at the current head

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-20738

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-20738

@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

@ally please review head 511dd66 with focus on the verify-only capability boundary, company/plugin binding authorization, rate-limit isolation, and constant-time comparison.

@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 511dd66

Critical Issues (0)

Important Issues (1)

  • [gstack/review + native-codex] server/src/services/plugin-secrets-handler.ts:323verify() bypasses the resolution limiter but still calls resolveBoundSecret() for every presented credential. Each unauthenticated guess therefore performs a provider resolution, updates secret resolution metadata, and records a successful secret-access event. An attacker can make unlimited provider calls and database writes, exhaust remote-provider quotas or worker capacity, and still silence legitimate Alertmanager traffic through resource exhaustion; the new 31-attempt test codifies this unbounded behavior.
    • Add a separately budgeted verification limiter and concurrency/input-size bounds that cannot consume trusted plaintext-resolution capacity, or compare against a securely stored version-bound verifier so failed checks do not resolve plaintext or emit successful resolution audit events. Partition any public verification budget by tenant/plugin/binding and an ingress identity where available.

Suggestions (0)

Strengths

  • The new secrets.verify-ref capability is distinct from secrets.read-ref, and the host-client tests prove it does not grant plaintext resolution.
  • Invocation company scope and the company/plugin/config-path binding are enforced before provider access in the reviewed Alertmanager path.
  • Hashing both values to fixed-size SHA-256 digests before timingSafeEqual avoids length-dependent comparison behavior.

Recommended Action

  1. Address the unbounded public verification path before merge.
  2. Re-run the secret-handler integration tests with verification-specific throttling and audit assertions.
  3. This PR is authored by app/allyblockcast; the exact head must be reopened under an independent author before an Ally App approval is possible.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

Addressed the current-head finding in commit 42e7a44fd.

  • verify() now compares against the authorized active version's host-stored SHA-256 verifier and does not call resolveSecretValue().
  • Invalid and valid verification attempts no longer update lastResolvedAt or emit successful secret-access events.
  • Presented credentials over 4096 UTF-8 bytes fail before database access.
  • Focused server integration: 11 passed; server typecheck passed; Alertmanager: 148 passed.

The push should trigger current-head review automatically; I have not stacked another marker request.

@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 42e7a44

Prior Findings Dispositioned (1)

  • prior:511dd66 important 1 — fixed — server/src/services/plugin-secrets-handler.ts:342 — Verification now resolves version metadata and reads the stored verifier instead of calling resolveBoundSecret; the current integration test also demonstrates no access-event write or lastResolvedAt update across failed attempts.

Critical Issues (0)

Important Issues (1)

  • [pr-review-toolkit + gstack/review] server/src/services/plugin-secrets-handler.ts:365verify() assumes every companySecretVersions.valueSha256 hashes the secret value, but external-reference versions use that column for a metadata fingerprint (for AWS, aws_secrets_manager_v1:<external-ref>:<provider-version>). A production webhookTokenRef backed by an imported AWS secret will therefore reject the real bearer token on every delivery.
    • Distinguish value verifiers from external-reference fingerprints. Either reject unsupported external-reference versions explicitly or add a provider/verifier mechanism that securely tracks the resolved value, with managed and external-provider verification tests.

Suggestions (1)

  • [gstack/review + native-codex] server/src/services/plugin-secrets-handler.ts:329 — Add a separately budgeted verification concurrency/rate limit as defense in depth so public invalid credentials cannot amplify database work, without consuming trusted plaintext-resolution capacity.

Strengths

  • secrets.verify-ref is separated from plaintext secrets.read-ref, and invocation company scope remains host-enforced.
  • Binding authorization covers company, plugin, secret, version selector, and config path before verifier access.
  • Oversized candidates are rejected before database access, and fixed-size digests are compared with timingSafeEqual.
  • Tests cover capability isolation, missing bearer handling, repeated invalid guesses, audit suppression, and resolution-budget isolation for local encrypted secrets.

Recommended Action

  1. Fix external-reference verifier semantics before merge.
  2. Add external-provider regression coverage alongside the existing local-encrypted test.
  3. This PR is authored by app/allyblockcast; the exact head must be reopened under an independent author before an Ally App approval is possible.

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