Epic: Phase 2 — Client-Server Mode with Password Auth + Signed Grants by helixclaw · Pull Request #89 · helixclaw/2keychains

helixclaw · 2026-02-26T00:55:38Z

Fixes #49

Epic: Phase 2 — Client-Server Mode with Password Auth + Signed Grants

Overview

Implement client-server mode as specified in docs/encrypted-storage-spec.md (Section 4). The server centralizes policy, approval orchestration, and audit. The client runs commands locally and verifies cryptographically signed grants issued by the server.

Depends on: Phase 1 (Epic #48) — encrypted store + local unlock + approval workflow

Goals

Server API routes: Full Fastify API for secrets, requests, grants, and injection
Server-side approval workflow: Server orchestrates notification-based approvals via Discord/channels
Signed grants (JWT): Server issues JWS-signed grant tokens; client verifies signature before injection
Session tokens with TTL: Server issues session tokens after password auth, with configurable TTL
Client local unlock: Client uses the same encrypted store + local unlock from Phase 1
Wire RemoteService: Connect the existing RemoteService HTTP client to the new server API

Key Design Decisions

Signed grants: jose library for JWS (Ed25519 or HMAC). Server signs, client verifies.
Server auth flow: Client authenticates with server.authToken → receives session JWT with TTL → uses session token for subsequent requests
Command binding: Approval requests include normalized command hash; grant is bound to that hash (configurable strictness)
Grant persistence on server: ~/.2kc/server-grants.json (or SQLite in future)
Client store: Each client maintains its own encrypted store (no sync)

Out of Scope

WebAuthn (Phase 3)
Store synchronization between clients
Database-backed persistence (JSON files sufficient for now)

Spec Reference

docs/encrypted-storage-spec.md — Sections 4 (Variant B: Client-Server Mode), 5 (Notifications)

This PR was created automatically by iloom.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Wire RemoteService to server API with session token auth, add client-side JWS grant verification via GrantVerifier, and implement local injection flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR implements Phase 2 of the 2keychains system: client-server mode with password authentication and cryptographically signed grants. The implementation adds JWT-based session authentication, Ed25519-signed grant tokens, command binding for additional security, and server-side approval workflow orchestration.

Changes:

Server API routes for secrets, requests, grants, and injection with JWT session authentication
Signed grant tokens using Ed25519 keypairs, with command hash binding for verification
Client-server split: server orchestrates approvals, clients verify signed grants locally before injection
Request/grant persistence to survive server restarts

Reviewed changes

Copilot reviewed 37 out of 38 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
package.json	Add jose library dependency for JWT/JWS operations
src/server/routes.ts	New API routes for secrets, requests, grants, and injection
src/server/auth.ts	Session JWT authentication with login endpoint and bearer token validation
src/server/app.ts	Server initialization with session secret generation and service integration
src/server/index.ts	Wire service resolution into server startup
src/core/signed-grant.ts	JWS signing/verification using jose library (appears unused)
src/core/grant.ts	Grant signing implementation using raw Node.js crypto
src/core/grant-verifier.ts	Client-side JWS verification with public key fetching
src/core/key-manager.ts	Ed25519 keypair generation and persistence
src/core/service.ts	Service layer changes for async workflow, command binding, and grant status
src/core/request.ts	Request persistence with file-based storage
src/core/remote-service.ts	Client HTTP implementation with session auth and local injection
src/core/config.ts	New config options for bindCommand and sessionTtlMs
src/core/command-hash.ts	Command normalization and SHA-256 hashing for binding
src/core/workflow.ts	Forward command/commandHash to notification channels
src/cli/secrets.ts, src/cli/request.ts, src/cli/config.ts	CLI updates for async service resolution
src/channels/discord.ts	Display bound command in Discord approval requests
src/tests/*	Comprehensive test coverage for new functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-26T15:50:17Z

src/core/remote-service.ts

  requests: Service['requests'] = {
    create: (secretUuids: string[], reason: string, taskRef: string, duration?: number) =>
      this.request<AccessRequest>('POST', '/api/requests', {
        secretUuids,
        reason,
        taskRef,
        duration,
      }),
  }


The RemoteService.requests.create() method does not accept or forward the 'command' parameter (line 157), even though the Service interface expects it as the 5th parameter. This breaks the entire command binding feature for client mode - clients can never send the command to the server for hashing and binding.

Copilot · 2026-02-26T15:50:18Z

src/server/routes.ts

+    if (status < 500) {
+      error.message = mapStatusToMessage(status)
+    }


The handleError function overwrites the original error message with a generic message for all 4xx errors (line 27). This loses important diagnostic information. For example, "Command does not match the approved command hash" becomes "Access denied", making debugging impossible. Consider preserving the original error message or only genericizing it for certain error types.

Copilot · 2026-02-26T15:50:18Z

src/server/app.ts

+  // sessionSecret is regenerated per server instance — sessions intentionally
+  // invalidate on server restart (ephemeral by design).
+  const sessionSecret = randomBytes(32)


The sessionSecret is regenerated on every server restart (line 17), which invalidates all active session tokens. While the comment says this is intentional, it creates a poor user experience. Consider persisting the session secret to a file (with proper permissions) so sessions survive restarts, or document this behavior clearly for operators.

Copilot · 2026-02-26T15:50:18Z

src/core/service.ts

+      this.runWorkflow(request).catch((err: unknown) => {
+        console.error('runWorkflow failed unexpectedly:', err)
+        request.status = 'denied'
+        this.deps.requestLog.save()
+      })


The error handler in the fire-and-forget workflow (lines 137-141) sets request.status = 'denied' on any error, but this is misleading. If the error occurs after approval (e.g., in createGrant), the status should be 'error' not 'denied'. The catch block should check if the error happened before or after approval to set the correct status.

Copilot · 2026-02-26T15:50:19Z

src/core/grant-verifier.ts

+    if (!raw.grantId || !raw.requestId || !raw.secretUuids) {
+      throw new Error('Grant is missing required fields')
+    }
+
+    if (!raw.expiresAt) {
+      throw new Error('Grant is missing expiry claim')
+    }
+
+    const payload = raw as unknown as GrantJWSPayload
+


The GrantVerifier expects the JWS payload to have fields 'grantId', 'requestId', 'secretUuids', and 'expiresAt' (lines 71-77), but the server's signGrant function in grant.ts signs a completely different structure (the raw grant object with fields like 'id', 'used', 'revokedAt', etc.). The client will always fail to verify grants because it's looking for fields that don't exist in the signed payload.

Suggested change

if (!raw.grantId || !raw.requestId || !raw.secretUuids) {

throw new Error('Grant is missing required fields')

}

if (!raw.expiresAt) {

throw new Error('Grant is missing expiry claim')

}

const payload = raw as unknown as GrantJWSPayload

// Accept both "grantId" (new schema) and "id" (raw grant object) as the identifier.

const grantId =

typeof raw.grantId === 'string'

? raw.grantId

: typeof raw.id === 'string'

? (raw.id as string)

: undefined

if (!grantId) {

throw new Error('Grant is missing required field: grantId')

}

const requestId = typeof raw.requestId === 'string' ? (raw.requestId as string) : undefined

if (!requestId) {

throw new Error('Grant is missing required field: requestId')

}

const secretUuidsRaw = raw.secretUuids

const secretUuids =

Array.isArray(secretUuidsRaw) && secretUuidsRaw.every((v) => typeof v === 'string')

? (secretUuidsRaw as string[])

: undefined

if (!secretUuids) {

throw new Error('Grant is missing required field: secretUuids')

}

const expiresAt =

typeof raw.expiresAt === 'string' ? (raw.expiresAt as string) : undefined

if (!expiresAt) {

throw new Error('Grant is missing expiry claim')

}

const commandHash =

typeof raw.commandHash === 'string' ? (raw.commandHash as string) : undefined

const payload: GrantJWSPayload = {

grantId,

requestId,

secretUuids,

expiresAt,

...(commandHash !== undefined ? { commandHash } : {}),

}

src/core/key-manager.ts

Copilot · 2026-02-26T15:50:20Z

src/core/grant-verifier.ts

+      response = await fetch(`${this.baseUrl}/api/keys/public`, {
+        headers: { Authorization: `Bearer ${this.authToken}` },
+        signal: AbortSignal.timeout(30_000),
+      })


The GrantVerifier tries to fetch the server's public key from /api/keys/public (line 29), but this route does not exist in the server implementation. Add the missing route to expose the server's public key so clients can verify grant signatures.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-02-26T15:50:21Z

src/core/command-hash.ts

+import { createHash } from 'node:crypto'
+
+export function normalizeCommand(cmd: string): string {
+  const normalized = cmd.trim().replace(/\s+/g, ' ').toLowerCase()


The normalizeCommand function converts commands to lowercase (line 4), which could break case-sensitive commands or file paths. For example, "cat MyFile.txt" becomes "cat myfile.txt", which would fail but still match the hash. Consider removing the lowercase transformation or documenting this limitation clearly.

Suggested change

const normalized = cmd.trim().replace(/\s+/g, ' ').toLowerCase()

const normalized = cmd.trim().replace(/\s+/g, ' ')

@copilot code review[agent] Is there any reason that we would want to normalize this or should we just remove normalization altogether? This is probably more likely used in the audit logs. We aren't executing the lowercase command, but maybe there's some sort of vulnerability if we allow them to run the lowercase command and then make an uppercase to access something else?

Copilot · 2026-02-26T15:50:21Z

src/core/grant.ts

+function signGrant(grant: AccessGrant, privateKey: KeyObject): string {
+  const header = Buffer.from(JSON.stringify({ alg: 'EdDSA' })).toString('base64url')
+  // Omit the jws field so the signature doesn't cover itself
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  const { jws: _, ...grantWithoutJws } = grant
+  const payload = Buffer.from(JSON.stringify(grantWithoutJws)).toString('base64url')
+  const signingInput = `${header}.${payload}`
+  const sigBytes = sign(null, Buffer.from(signingInput), privateKey)
+  return `${signingInput}.${sigBytes.toString('base64url')}`
+}


The signGrant function in grant.ts signs the entire grant object (including fields like used, revokedAt, grantedAt, expiresAt, secretUuids, id, requestId), but the client-side GrantVerifier expects a JWS with standard JWT claims (jti, sub, iat, exp). The payload structures are incompatible: the server signs a different structure than what the client expects to verify. This will cause verification failures.

Copilot · 2026-02-26T15:50:21Z

src/server/routes.ts

+      async (request, reply) => {
+        const { secretUuids, reason, taskRef, duration } = request.body
+        const result = await service.requests
+          .create(secretUuids, reason, taskRef, duration)
+          .catch(handleError)


The POST /api/requests route does not accept or forward a 'command' field from the request body, but the service.requests.create() method expects it as the 5th parameter. When bindCommand is enabled on the server, command binding will never work because the command is never passed from the API route to the service layer.

helixclaw · 2026-02-27T06:49:22Z

iloom Session Summary

Key Themes:

Parallel swarm execution across 7 issues with strict dependency ordering unlocked significant throughput after Add server API routes for secrets, requests, grants, and inject #59 unblocked the critical path.
Rebase conflicts were systematic and predictable — parallel branches evolving the same core types (AccessGrant, GrantManager, service.ts) require careful interface reconciliation.
A late architectural shift from jose/CryptoKey to node:crypto/KeyObject in key-manager.ts during Server-side approval workflow orchestration #64's rebase is the most significant technical risk to validate.

Session Details (click to expand)

Key Insights

GrantManager.createGrant() became async as part of Implement signed grants with JWS (jose library) #61's JWS work, returning Promise<{ grant: AccessGrant; jws: string | null }>. Any branch written against the old synchronous signature (returning AccessGrant directly) will fail type-checking and require test updates during rebase.
createServer() requires session config (sessionSecret, sessionTtlMs) after Add session token auth with TTL for server access #60 was merged. Callers that only pass { authToken } will fail to compile.
grants.validate() was replaced by grants.getStatus() (returning { status, grant?, jws? }) as part of Server-side approval workflow orchestration #64's async approval workflow. Any code referencing the old validate API needs updating.
POST /api/requests is fire-and-forget — it returns { requestId, status: 'pending' } immediately; approval happens asynchronously in the background. Clients must poll GET /api/grants/:requestId.
Integration tests use opts.sessionTtlMs on createServer() to force fast session expiry. This parameter was added specifically for testability — it's not exposed in normal server config.

Decisions Made

node:crypto/KeyObject over jose/CryptoKey for key management: During Server-side approval workflow orchestration #64's rebase, key-manager.ts was converted from the jose-based approach (Implement signed grants with JWS (jose library) #61) to node:crypto. Rationale: Server-side approval workflow orchestration #64's implementation used node:crypto's sign() which requires KeyObject, and the merge agent chose consistency over preserving the original jose approach. This is a meaningful divergence from Implement signed grants with JWS (jose library) #61's design intent.
Parallel wave execution after Add server API routes for secrets, requests, grants, and inject #59: Once Add server API routes for secrets, requests, grants, and inject #59 completed, Add session token auth with TTL for server access #60 and Implement signed grants with JWS (jose library) #61 were spawned simultaneously. After Implement signed grants with JWS (jose library) #61, three issues (Add optional command binding for grant approval requests #62, Wire RemoteService to server API and add client-side grant verification #63, Server-side approval workflow orchestration #64) ran in parallel. This maximized throughput but increased the frequency and complexity of rebase conflicts.

Challenges Resolved

Heavy rebase on Server-side approval workflow orchestration #64 (11 conflict files): The most conflict-prone merge in the epic. The core tension was between the epic branch's accumulated changes (jose/CryptoKey, commandHash, GrantVerifier patterns from Add server API routes for secrets, requests, grants, and inject #59–Wire RemoteService to server API and add client-side grant verification #63) and Server-side approval workflow orchestration #64's independent implementation of the same modules. Resolution required understanding both sides' intent across grant.ts, service.ts, remote-service.ts, app.ts, routes.ts, key-manager.ts, and 5 test files.
app.ts conflicts on Add session token auth with TTL for server access #60 rebase: Both Add server API routes for secrets, requests, grants, and inject #59 and Add session token auth with TTL for server access #60 independently modified the server bootstrap — Add server API routes for secrets, requests, grants, and inject #59 introduced routePlugin/Service imports, Add session token auth with TTL for server access #60 added bearerAuthPlugin changes. Resolution required merging all imports and updating createServer to forward the new session config parameters.
package.json version conflict on Implement signed grants with JWS (jose library) #61 rebase: Both the epic and child had added jose but at different versions (^6.1.3 vs ^6.0.0). Resolved by taking the higher version.

Lessons Learned

The service.ts / grant.ts / app.ts triad is the highest-conflict zone in this codebase. Any parallel branches touching Phase 2 features will almost certainly conflict in these files. Sequential or clearly partitioned work ownership would reduce friction.
key-manager.ts now uses node:crypto DER format for key persistence. If a key file exists in the old JWK format (from pre-rebase Implement signed grants with JWS (jose library) #61 work), the module will detect the format mismatch and regenerate the keypair rather than failing.
The jose library is still a runtime dependency (used in signed-grant.ts for JWS signing/verification), but key-manager.ts now uses node:crypto for key generation and storage. These two subsystems use different key representations internally.

Copilot · 2026-02-28T19:33:06Z

@NoahCardoza I've opened a new pull request, #90, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-02-28T19:33:23Z

@NoahCardoza I've opened a new pull request, #91, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…d bypass Co-authored-by: NoahCardoza <10343470+NoahCardoza@users.noreply.github.com>

…g key Co-authored-by: NoahCardoza <10343470+NoahCardoza@users.noreply.github.com>

Use single backslashes instead of triple backslashes for escaping backticks and dollar signs in the node -e inline script. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Remove case normalization from `normalizeCommand` to close command-bypass vulnerability

Add GET /api/keys/public route for grant signature verification

fixes #59

5b5ce28

helixclaw force-pushed the feat/issue-49__phase-2-client-server branch from 48e96bc to 5b5ce28 Compare February 26, 2026 01:38

helixclaw and others added 7 commits February 25, 2026 18:11

fixes #60

3652f94

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fixes #61

3dd96d2

fixes #62

dce72b9

fixes #63

5ed88ae

Wire RemoteService to server API with session token auth, add client-side JWS grant verification via GrantVerifier, and implement local injection flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fixes #64

fa38363

fixes #65

f6531f0

Fixes #49

98f0e94

NoahCardoza requested a review from Copilot February 26, 2026 15:43

Copilot started reviewing on behalf of NoahCardoza February 26, 2026 15:44 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

helixclaw marked this pull request as ready for review February 27, 2026 06:48

NoahCardoza and others added 5 commits February 27, 2026 12:45

fix unlock command

e348e1b

create inject command

cacd430

move to tsup

ed748ff

remove discord webhook in favor of bot message to prime reactions

c9e2669

Initial plan

1415acf

Copilot AI mentioned this pull request Feb 28, 2026

Remove case normalization from normalizeCommand to close command-bypass vulnerability #90

Merged

Initial plan

1d28bf9

Copilot AI mentioned this pull request Feb 28, 2026

Add GET /api/keys/public route for grant signature verification #91

Merged

NoahCardoza and others added 6 commits February 28, 2026 11:33

Update src/core/key-manager.ts

9426560

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Remove toLowerCase from normalizeCommand to prevent case-based comman…

357b74e

…d bypass Co-authored-by: NoahCardoza <10343470+NoahCardoza@users.noreply.github.com>

Add GET /api/keys/public route so clients can fetch the server signin…

80333b0

…g key Co-authored-by: NoahCardoza <10343470+NoahCardoza@users.noreply.github.com>

remove old server entry file

2453d55

add ability to config directory

dee6771

implement zod for config parsing

4a12096

fix: correct template literal escaping in CI coverage badge step

2a4a179

Use single backslashes instead of triple backslashes for escaping backticks and dollar signs in the node -e inline script. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

NoahCardoza force-pushed the feat/issue-49__phase-2-client-server branch from 218993f to 2a4a179 Compare February 28, 2026 20:25

NoahCardoza added 6 commits February 28, 2026 12:26

Merge pull request #90 from helixclaw/copilot/sub-pr-89

ac6241d

Remove case normalization from `normalizeCommand` to close command-bypass vulnerability

Merge pull request #91 from helixclaw/copilot/sub-pr-89-again

46704a4

Add GET /api/keys/public route for grant signature verification

improve coverage testing

f12a0e3

add route to get signed grant jws token

ed79d05

move Pull-in frequency into config file

0d529c6

fix client mode and add comprehensive integration tests

790f0d1

NoahCardoza merged commit 887a0b6 into main Feb 28, 2026
1 check passed

-    if (!raw.grantId || !raw.requestId || !raw.secretUuids) {
-      throw new Error('Grant is missing required fields')
-    }
-    if (!raw.expiresAt) {
-      throw new Error('Grant is missing expiry claim')
-    }
-    const payload = raw as unknown as GrantJWSPayload
+    // Accept both "grantId" (new schema) and "id" (raw grant object) as the identifier.
+    const grantId =
+      typeof raw.grantId === 'string'
+        ? raw.grantId
+        : typeof raw.id === 'string'
+          ? (raw.id as string)
+          : undefined
+    if (!grantId) {
+      throw new Error('Grant is missing required field: grantId')
+    }
+    const requestId = typeof raw.requestId === 'string' ? (raw.requestId as string) : undefined
+    if (!requestId) {
+      throw new Error('Grant is missing required field: requestId')
+    }
+    const secretUuidsRaw = raw.secretUuids
+    const secretUuids =
+      Array.isArray(secretUuidsRaw) && secretUuidsRaw.every((v) => typeof v === 'string')
+        ? (secretUuidsRaw as string[])
+        : undefined
+    if (!secretUuids) {
+      throw new Error('Grant is missing required field: secretUuids')
+    }
+    const expiresAt =
+      typeof raw.expiresAt === 'string' ? (raw.expiresAt as string) : undefined
+    if (!expiresAt) {
+      throw new Error('Grant is missing expiry claim')
+    }
+    const commandHash =
+      typeof raw.commandHash === 'string' ? (raw.commandHash as string) : undefined
+    const payload: GrantJWSPayload = {
+      grantId,
+      requestId,
+      secretUuids,
+      expiresAt,
+      ...(commandHash !== undefined ? { commandHash } : {}),
+    }

	const normalized = cmd.trim().replace(/\s+/g, ' ').toLowerCase()
	const normalized = cmd.trim().replace(/\s+/g, ' ')

Conversation

helixclaw commented Feb 26, 2026