spec(security): add security boundary specification and refactor model spec#1514
spec(security): add security boundary specification and refactor model spec#1514markturansky merged 6 commits intomainfrom
Conversation
Initial draft of the security specification covering: - OpenShift namespace-scoped build agent ServiceAccount - Control Plane SA as the single SRE-owned cluster identity - Per-project Vertex AI credential scoping - User SSO token propagation into runners - Integration credential lifecycle (Credential provider=*) - Dynamic MCP credential watching (sidecar vs pod mode) - Per-session ServiceAccount isolation (closing the shared-SA gap) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Move security.md to specs/security/security.spec.md. Extract HOW content (credential authorization model, RBAC runtime grant semantics, proxy authentication, design decisions) from ambient-model.spec.md into the security spec. Model spec retains WHAT (schemas, endpoints, provider enum, permission matrix, CLI mappings). Cross-references link both documents. Also moves ambient-model.spec.md from specs/sessions/ to specs/api/ and updates all references (BOOKMARKS.md, design README, devflow skill, workflows). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughWalkthroughThis PR reorganizes platform specifications by relocating the ambient data model to the ChangesSpecification Structure Reorganization & Credential Model Redesign
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
jsell-rh
left a comment
There was a problem hiding this comment.
Review: Security Boundary Specification
Overall this is a strong spec — the six identity boundaries are clearly articulated, the accounts/tokens table is comprehensive, and the per-session SA isolation gap (§2.5) is well-documented with a concrete migration path. A few items to address before merge:
Blockers
-
Broken GFM table (§5 Design Decisions). The header separator row has a double pipe (
||) which will break table rendering. Quick fix. -
Spec format doesn't match project conventions. Per
specs/index.spec.md, specs require### Requirement:blocks with RFC 2119 keywords (SHALL/MUST/SHOULD) and#### Scenario:blocks with Given/When/Then. This spec uses prose sections and narrative description throughout. This is the same gap the markdown rendering specs had before they were rewritten — seespecs/frontend/sessions/messages/markdown-rendering.spec.mdfor the target format. -
Implementation details leak into the spec. File paths (
components/manifests/base/rbac/control-plane-sa.yaml), function names (resolveCredentialIDs(),enforceCredentialRBAC(),clear_runtime_credentials()), and line numbers appear throughout. Per the spec format: "if the implementation can change without changing externally visible behavior, it does not belong in the spec." These belong in a paired workflow file, not the spec itself.
Structural Issues
-
namespacevsprojectterminology is conflated. The spec uses "project namespace", "single namespace", and "project-scoped" interchangeably without establishing the relationship. In the Ambient domain model, Project is the Ambient-level isolation boundary (API resource) and namespace is the Kubernetes implementation detail. They're 1:1 today, but the spec blurs them — e.g. §2.5 says "all runner sessions within a project namespace share access" without clarifying which boundary is being described. Recommend adding a terminology note upfront: "Each Project is realized as a single Kubernetes namespace. This spec uses 'project' for the Ambient boundary and 'namespace' only when referring to the Kubernetes primitive directly." — then use the terms consistently throughout. -
New
api/domain not registered. Theambient-model.spec.mdmove fromspecs/sessions/tospecs/api/creates a new domain without adding it tospecs/index.spec.md's domain table.BOOKMARKS.mdalso still labels it asdomain: sessions. -
§1 (Build Agent SA) feels out of place. It's a proposed (not existing) SA for OpenShift CI/CD with very implementation-specific details (exact API groups and verbs). Consider a separate spec or appendix — it reads differently from the rest of the document.
Minor
-
"Status: Draft" / "Authors" / "Last Updated" metadata — other specs don't use this header format. Inconsistent but non-blocking.
-
Cross-references use
§4anchors that depend on heading numbering staying stable — fragile if sections are reordered. Consider using descriptive anchor links instead. -
Model spec refactoring removes content and replaces it with cross-references. If someone reads the model spec standalone, they lose important credential design context. The security spec should be additive, not require readers to chase links for context that was previously inline.
Ambient Review Bot <jsell-rh.ambient-review-bot@redhat.com>
- Rewrite to Requirement:/Scenario: format with RFC 2119 keywords (SHALL/MUST/SHOULD) - Fix broken GFM table (double pipe in Design Decisions header separator) - Remove implementation details (file paths, function names) from spec - Use "Project" consistently instead of "namespace" for Ambient boundary; add terminology note - Register api/ and security/ domains in specs/index.spec.md - Fix BOOKMARKS.md domain label (sessions -> api) - Remove Draft/Authors/Last Updated metadata header to match other specs - Replace fragile §N anchors with descriptive anchor links in model spec cross-refs 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Credentials are now global resources bound to Projects via RoleBindings instead of project-scoped with a project_id FK. This eliminates duplication when the same PAT is used across multiple Projects. Adds vertex and kubeconfig to the provider enum. Splits the security spec Accounts and Tokens table into scoped subsections for independent implementation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
jsell-rh
left a comment
There was a problem hiding this comment.
Credential ownership gap after global migration
The latest commit (c37f0c90) moves credentials from project-scoped to global, but the permission matrix doesn't define who can create them:
| Role | Credentials |
|---|---|
platform:admin |
full |
project:owner |
manage bindings |
project:editor |
— |
project:viewer |
— |
Only platform:admin has credential CRUD. That means a regular user can't create their own GitHub PAT credential without an admin — which defeats the self-service model. This probably needs credential:owner and credential:viewer roles (or similar) so users can manage credentials they created and bind them to projects they own.
Security spec contradictions
The security spec was not updated to reflect the global credential model. It currently contradicts the model spec in several places:
- Requirement: Project-Scoped Credential Sharing — says "Credentials SHALL belong to a Project" and "no explicit sharing or per-credential RoleBindings needed" (the model spec now requires RoleBindings as the only access mechanism)
- Requirement: Integration Credential Isolation — says "Projects MUST NOT access each other's credentials" with a scenario "Cross-Project credential access blocked" (model spec explicitly enables cross-project sharing via RoleBindings)
- Design decisions table — says "Four-scope RBAC... no dedicated
credentialscope needed" and "Credential is Project-scoped, like a Kubernetes Secret" (model spec addscredentialas a 5th scope and says "Credential is global, not project-scoped") - Token Reader Role Grant — says "project:owner and project:editor can create/update/delete credentials" (model spec permission matrix says they can't)
- Multi-Project credential pattern — describes duplicating credentials across projects; model spec says the opposite
Stale references in security spec
- Token Reader scenario uses
GET /projects/{id}/credentials/{cred_id}/token— model spec changed toGET /credentials/{cred_id}/token - Key invariant #3 still says "Integration credentials are Project-scoped"
- Accounts table still shows
Scope: Projectfor all credential rows - K8s analogy table says "Credential | Secret | Project-scoped secret" — analogy breaks for global resources since K8s Secrets are namespace-scoped
- Add credential:owner and credential:viewer roles for self-service CRUD - Update security spec: credentials are global, bound via RoleBindings - Fix all stale references: endpoint paths, K8s analogy, named patterns, design decisions (5-scope RBAC), key invariants, accounts table scopes - Update cross-reference anchor from project-scoped to credential-access Addresses review feedback from jsell-rh on PR #1514. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
specs/security/security.spec.md— a security boundary specification defining WHO can do WHAT across the platform's six identity boundaries (CP SA, per-session SA, user SSO, Vertex credentials, integration credentials, build agent SA)specs/api/ambient-model.spec.mdto separate WHAT (schemas, endpoints, provider enum, permission matrix) from HOW (runtime authorization, credential grant semantics, proxy auth)specs/sessions/tospecs/api/with all reference updatesKey sections in security spec:
Accounts and tokens table
15-row identity matrix covering every SA, token, and credential in the platform.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/api/ambient/v1/credentials).credential:ownerandcredential:viewer.Documentation