feat(pkg): shared identity and certauth for session mTLS#337
feat(pkg): shared identity and certauth for session mTLS#337Agent-Hellboy wants to merge 1 commit into
Conversation
Extract session SPIFFE parsing, CSR build/validation, and safe credential file writes into pkg/identity and pkg/certauth so the gateway, adapter CLI, and runtime-api share one contract for session-bound mTLS credentials. Co-authored-by: Cursor <cursoragent@cursor.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors certificate signing request (CSR) generation, validation, and SPIFFE identity parsing by extracting these shared helpers into common packages (pkg/certauth and pkg/identity). This eliminates duplicate code across the CLI adapter, gateway, and runtime API. The review feedback highlights several key improvements: URL-escaping the namespace and session parameters in SessionSPIFFEID to prevent path-splitting bugs, strictly rejecting query parameters and fragments in SPIFFE IDs to align with the SPIFFE standard, adding corresponding test cases for these edge cases, and separating CSR parsing and signature verification in ValidateCSRPEM to preserve specific error details for debugging.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func SessionSPIFFEID(trustDomain, namespace, session string) string { | ||
| trustDomain = strings.TrimSpace(trustDomain) | ||
| namespace = strings.TrimSpace(namespace) | ||
| session = strings.TrimSpace(session) | ||
| return fmt.Sprintf("spiffe://%s/ns/%s/session/%s", trustDomain, namespace, session) | ||
| } |
There was a problem hiding this comment.
The namespace and session parameters are not URL-escaped when constructing the SPIFFE ID. If either contains special characters (such as /), it will break the path splitting logic in ParseSessionSPIFFE. Using url.PathEscape ensures that they are safely encoded, matching the url.PathUnescape calls already present in the parser.
| func SessionSPIFFEID(trustDomain, namespace, session string) string { | |
| trustDomain = strings.TrimSpace(trustDomain) | |
| namespace = strings.TrimSpace(namespace) | |
| session = strings.TrimSpace(session) | |
| return fmt.Sprintf("spiffe://%s/ns/%s/session/%s", trustDomain, namespace, session) | |
| } | |
| func SessionSPIFFEID(trustDomain, namespace, session string) string { | |
| trustDomain = strings.TrimSpace(trustDomain) | |
| namespace = strings.TrimSpace(namespace) | |
| session = strings.TrimSpace(session) | |
| return fmt.Sprintf("spiffe://%s/ns/%s/session/%s", trustDomain, url.PathEscape(namespace), url.PathEscape(session)) | |
| } |
| if !strings.EqualFold(uri.Scheme, "spiffe") || !strings.EqualFold(uri.Host, strings.TrimSpace(trustDomain)) { | ||
| return "", "", false | ||
| } |
There was a problem hiding this comment.
According to the SPIFFE standard, a SPIFFE ID must not contain query parameters or fragments. Strictly rejecting these prevents potential security bypasses or parameter injection.
if !strings.EqualFold(uri.Scheme, "spiffe") || !strings.EqualFold(uri.Host, strings.TrimSpace(trustDomain)) {
return "", "", false
}
if uri.RawQuery != "" || uri.Fragment != "" {
return "", "", false
}| { | ||
| name: "valid", | ||
| raw: "spiffe://example.org/ns/team-a/session/session-1", | ||
| trust: "example.org", | ||
| wantNS: "team-a", | ||
| wantSess: "session-1", | ||
| wantOK: true, | ||
| }, |
There was a problem hiding this comment.
Add test cases to verify that special characters in the namespace and session are correctly escaped/unescaped, and that SPIFFE IDs with query parameters or fragments are strictly rejected.
{
name: "valid",
raw: "spiffe://example.org/ns/team-a/session/session-1",
trust: "example.org",
wantNS: "team-a",
wantSess: "session-1",
wantOK: true,
},
{
name: "valid with special characters",
raw: "spiffe://example.org/ns/team%2Fa/session/session%231",
trust: "example.org",
wantNS: "team/a",
wantSess: "session#1",
wantOK: true,
},
{
name: "rejected with query",
raw: "spiffe://example.org/ns/team-a/session/session-1?foo=bar",
trust: "example.org",
wantOK: false,
},
{
name: "rejected with fragment",
raw: "spiffe://example.org/ns/team-a/session/session-1#baz",
trust: "example.org",
wantOK: false,
},| csr, err := x509.ParseCertificateRequest(block.Bytes) | ||
| if err != nil || csr.CheckSignature() != nil { | ||
| return nil, fmt.Errorf("csr is invalid or has an invalid signature") | ||
| } |
There was a problem hiding this comment.
Separating the CSR parsing and signature verification checks improves readability and preserves the underlying error details, which is highly beneficial for debugging.
| csr, err := x509.ParseCertificateRequest(block.Bytes) | |
| if err != nil || csr.CheckSignature() != nil { | |
| return nil, fmt.Errorf("csr is invalid or has an invalid signature") | |
| } | |
| csr, err := x509.ParseCertificateRequest(block.Bytes) | |
| if err != nil { | |
| return nil, fmt.Errorf("parse csr: %w", err) | |
| } | |
| if err := csr.CheckSignature(); err != nil { | |
| return nil, fmt.Errorf("csr signature verification failed: %w", err) | |
| } |
1 participant
Summary
pkg/identityfor session-bound SPIFFE ID construction/parsing and client-cert URI checks.pkg/certauthfor session CSR generation, CSR validation, and safe credential file writes.Test plan
go test ./pkg/identity/... ./pkg/certauth/...go test ./internal/cli/adapter/...go testinservices/mcp-gatewayandservices/runtime-api/internal/runtimeapiPart of the post-#331 follow-up queue (#6). Next: runtime-api workload mTLS (#5), then live data-path E2E (#1).
Made with Cursor