Skip to content

feat: support Vault auth methods for SPIRE server - #19

Open
Manuthor wants to merge 10 commits into
developfrom
spire
Open

feat: support Vault auth methods for SPIRE server#19
Manuthor wants to merge 10 commits into
developfrom
spire

Conversation

@Manuthor

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings July 28, 2026 04:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a Vault-compatible authentication surface (AppRole + Kubernetes) and token self-service endpoints to support SPIRE integrations, backed by new database tables and shared client/server DTOs.

Changes:

  • Added Vault-compatible HTTP endpoints for AppRole login + admin CRUD, Kubernetes login + admin CRUD, and token self-service (lookup/renew/revoke).
  • Implemented X-Vault-Token extraction middleware and added DB trait + backend implementations to persist/lookup/renew/revoke Vault tokens and roles.
  • Extended the shared auth_client DTO layer and updated the OpenAPI specification and changelog for the new API.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
server/src/server/endpoints/vault_token.rs Adds Vault token self-service endpoints (lookup/renew/revoke).
server/src/server/endpoints/vault_k8s.rs Adds Kubernetes JWT login + role CRUD endpoints and JWKS/JWT helpers.
server/src/server/endpoints/vault_approle.rs Adds AppRole login + role/secret-id CRUD and shared token issuance helper.
server/src/server/endpoints/mod.rs Exposes new Vault endpoint modules and handlers.
server/src/server/auth_verifier.rs Registers Vault scopes and applies the new middleware stacks.
server/src/middleware/vault_token_extract.rs Implements X-Vault-Token auth middleware and claims injection.
server/src/middleware/mod.rs Re-exports the new Vault token middleware/types.
server/src/database/trait.rs Adds Vault-related DB structs + trait methods.
server/src/database/mod.rs Re-exports Vault DB types from the trait module.
server/src/database/impls/sqlite.rs Adds Vault tables and implements Vault DB operations (SQLite).
server/src/database/impls/postgres.rs Adds Vault tables and implements Vault DB operations (PostgreSQL).
server/src/database/impls/mysql.rs Adds Vault tables and implements Vault DB operations (MySQL).
server/documentation/openapi.yaml Documents new Vault endpoints and wire types in the OpenAPI contract.
server/Cargo.toml Adds dependencies needed for token issuance / IDs (rand, uuid).
client/src/lib.rs Re-exports new Vault DTOs from the client crate.
client/src/dto/vault.rs Adds Vault-compatible wire DTOs (AppRole/K8s/token responses).
client/src/dto/mod.rs Exposes the new vault DTO module.
CHANGELOG/spire-vault-compatible-api.md Adds release notes for the new Vault-compatible API.
Cargo.toml Adds workspace dependencies (rand, uuid).
Cargo.lock Locks new transitive dependencies.
Comments suppressed due to low confidence (1)

server/src/server/endpoints/vault_k8s.rs:80

  • This bound-namespace check allocates a new String for "*" on every request. It can be allocation-free by checking with iter().any(...) instead.
    if !bound_namespaces.contains(&"*".to_string()) && !bound_namespaces.contains(&ns) {
        return Err(AuthError::Forbidden(format!(
            "namespace '{ns}' not in bound_service_account_namespaces"
        )));
    }

Comment thread server/documentation/openapi.yaml
Comment thread server/src/server/endpoints/vault_k8s.rs Outdated
Comment thread server/src/server/endpoints/vault_k8s.rs Outdated
Comment thread server/src/server/endpoints/vault_token.rs Outdated
Comment thread server/src/database/impls/sqlite.rs
Comment thread server/src/database/impls/postgres.rs
Comment thread server/src/database/impls/mysql.rs
Comment thread server/src/database/trait.rs Outdated
Comment thread client/src/dto/vault.rs Outdated
Comment thread CHANGELOG/spire-vault-compatible-api.md Outdated
Copilot AI review requested due to automatic review settings July 28, 2026 05:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 22 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (3)

server/documentation/openapi.yaml:1769

  • /v1/auth/token/renew-self returns 403 on missing/invalid tokens (via VaultTokenExtract), but the OpenAPI spec documents a 401 response.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • /v1/auth/token/revoke-self returns 403 on missing/invalid tokens (via VaultTokenExtract), but the OpenAPI spec documents a 401 response.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:644

  • The server implementation checks for exact namespace matches (or "*"), but the OpenAPI description claims glob-pattern matching. The docs should match the actual behavior.
          description: Allowed namespaces (glob patterns; `["*"]` for any).

Comment thread server/documentation/openapi.yaml Outdated
Comment thread server/documentation/openapi.yaml Outdated
Comment thread server/documentation/openapi.yaml Outdated
Comment thread server/documentation/openapi.yaml Outdated
Comment thread server/src/server/endpoints/vault_k8s.rs Outdated
Comment thread server/src/middleware/app_token_extract.rs Outdated
Comment thread server/src/database/impls/sqlite.rs
Comment thread server/src/database/impls/postgres.rs
Comment thread server/src/server/endpoints/vault_approle.rs Outdated
Copilot AI review requested due to automatic review settings July 28, 2026 05:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (15)

server/src/server/endpoints/vault_k8s.rs:182

  • validate_k8s_jwt uses the JWT header's alg directly (Validation::new(alg)), which can allow algorithm-confusion cases (e.g. attacker supplies HS256 and the decoder treats the JWKS public key material as an HMAC secret). For Kubernetes SA tokens we should only accept asymmetric algorithms we expect (RS256/ES256) and reject everything else before attempting verification.
    let header = decode_header(token).map_err(|e| format!("invalid JWT header: {e}"))?;
    let alg = header.alg;

server/documentation/openapi.yaml:631

  • K8sRoleRequest.jwks_url is documented with an /.well-known/openid-configuration example, but the server expects a JWKS document with a top-level keys array (see fetch_jwks parsing into JwksResponse { keys: ... }). The OpenAPI example should point to the JWKS endpoint (e.g. .../jwks.json) to avoid client misconfiguration.
        jwks_url:
          type: string
          format: uri
          description: URL of the Kubernetes API server JWKS endpoint.
          example: https://kubernetes.default.svc/.well-known/openid-configuration

server/documentation/openapi.yaml:638

  • The OpenAPI description says bound_service_account_names supports glob patterns, but the implementation only does exact string matching (plus the special "*" wildcard). The schema text should match the actual semantics to prevent operators from assuming patterns like spire-* will work.
          default: ['*']
          description: Allowed service account names (glob patterns; `["*"]` for any).
          example: [spire-agent]

client/src/dto/vault.rs:113

  • The doc comment says bound_service_account_namespaces supports glob patterns, but the server-side check is exact-match only (with "*" as the only wildcard). Align this comment with the server implementation to avoid documenting unsupported behavior.
    /// Allowed namespaces (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_namespaces: Vec<String>,

server/src/database/impls/sqlite.rs:161

  • vault_approle_roles.token_policies is stored/parsed as a comma-separated string, but the table default is '[]'. If any row is ever inserted without explicitly setting token_policies, the runtime split-on-comma logic will treat "[]" as a real policy name. The default should be the empty string to match the storage format.
                token_ttl_secs      INTEGER NOT NULL DEFAULT 3600,
                bind_secret_id      INTEGER NOT NULL DEFAULT 1,
                token_policies      TEXT    NOT NULL DEFAULT '[]'
            )

server/src/database/impls/postgres.rs:142

  • vault_approle_roles.token_policies is stored/parsed as a comma-separated string, but the table default is '[]'. If any row is inserted without explicitly setting token_policies, splitting will yield a bogus policy name "[]". Use an empty-string default to match the actual storage format.
                secret_id_ttl_secs  BIGINT  NOT NULL DEFAULT 0,
                token_ttl_secs      BIGINT  NOT NULL DEFAULT 3600,
                bind_secret_id      BOOLEAN NOT NULL DEFAULT TRUE,
                token_policies      TEXT    NOT NULL DEFAULT '[]'
            )

server/documentation/openapi.yaml:1752

  • The token self-service endpoints return HTTP 403 on missing/invalid tokens (see VaultTokenExtract middleware’s HttpResponse::Forbidden()), but the OpenAPI spec lists only a 401 response. Document 403 Forbidden here to match actual behavior and Vault client expectations.
                  ttl: 3540
                  creation_time: 1722070800
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1769

  • /v1/auth/token/renew-self returns HTTP 403 on missing/invalid tokens (middleware) and on non-renewable tokens (AuthError::Forbidden). The OpenAPI spec currently lists only 401; update it to include 403 Forbidden.
                $ref: '#/components/schemas/VaultAuthResponse'
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • /v1/auth/token/revoke-self returns HTTP 403 on missing/invalid tokens via VaultTokenExtract, but the OpenAPI spec lists only 401. Update the spec to include 403 Forbidden to match actual runtime behavior.
        '204':
          description: Token revoked.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/src/server/endpoints/vault_token.rs:32

  • When the token disappears/expires between the middleware lookup and this handler’s re-read, the code returns AuthError::Session("token not found"), which maps to HTTP 401. Vault clients expect invalid/expired tokens to yield HTTP 403 ("permission denied"), and your middleware already returns 403 for missing/invalid tokens.
    // Re-read from DB to get the freshest TTL
    let token = database
        .vault_lookup_token(&claims.token_hash)
        .await?
        .ok_or_else(|| AuthError::Session("token not found".to_string()))?;

server/documentation/openapi.yaml:645

  • The OpenAPI description says bound_service_account_namespaces supports glob patterns, but the implementation only does exact string matching (plus the special "*" wildcard). Update the schema description to avoid documenting unsupported matching behavior.
          default: ['*']
          description: Allowed namespaces (glob patterns; `["*"]` for any).
          example: [spire]

client/src/dto/vault.rs:110

  • The doc comment says bound_service_account_names supports glob patterns, but the server-side check is exact-match only (with "*" as the only wildcard). This comment should match the actual API behavior to avoid misleading downstream users of auth_client.

This issue also appears on line 111 of the same file.

    /// Allowed service-account names (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_names: Vec<String>,

server/documentation/adr/2026-07-26-vault-compatible-auth-api-for-spire.md:95

  • The ADR schema snippet describes vault_approle_roles.token_policies as a JSON array, but the implementation stores policies as a comma-separated string (payload.token_policies.join(",")) and parses them via .split(','). The ADR should reflect the actual chosen storage format.
    token_ttl_secs      INTEGER NOT NULL,
    bind_secret_id      BOOLEAN NOT NULL,
    token_policies      TEXT NOT NULL           -- JSON array
);

server/documentation/vault_auth_api.md:253

  • This step says the server selects the JWKS key by kid, but the current implementation iterates over all JWKS keys until one verifies (it doesn’t use kid). The doc should match the real behavior to avoid over-promising key-selection logic.
2. Selects the correct key by `kid` from the JWT header.

server/src/server/auth_verifier.rs:380

  • This PR adds a new Vault-compatible /v1/auth/* surface area, but there are no integration tests covering these routes (no vault_ references in server/src/tests). Please add tests for AppRole login + secret-id consumption, K8s login (with a local JWKS server), and token lookup/renew/revoke, including expected 403/204 behaviors.
    // ── Vault-compatible auth scopes ─────────────────────────────────────────
    //
    // Scope prefixes are kept intentionally specific to avoid Actix-web's
    // FIFO matching swallowing requests before they reach the right scope.
    //

Copilot AI review requested due to automatic review settings July 28, 2026 05:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (10)

server/src/server/endpoints/vault_k8s.rs:181

  • validate_k8s_jwt uses the untrusted JWT header algorithm (Validation::new(header.alg)). If a token declares a symmetric alg (e.g. HS256) or none, the JWK-derived public key bytes can be misused as an HMAC secret / bypass signature checks (alg confusion). Restrict accepted algorithms to the expected asymmetric ones (RS256/ES256) before decoding.
    server/src/server/endpoints/vault_token.rs:32
  • If the token was revoked/expired between the middleware lookup and this handler, this currently returns AuthError::Session (401). For Vault-token authentication failures, return 403 consistently (matching VaultTokenExtract and the docs).
    server/src/server/endpoints/vault_token.rs:83
  • If the token is revoked/expired immediately after renewal, this currently returns AuthError::Generic (HTTP 500). That’s a client-visible auth failure and should be a 403 (consistent with VaultTokenExtract).
    server/src/server/endpoints/vault_token.rs:115
  • Same as lookup-self: missing vault token claims currently maps to AuthError::Session (401). For Vault token auth failures, return 403 consistently with VaultTokenExtract.
    server/documentation/openapi.yaml:1752
  • These Vault token self-service endpoints return 403 Forbidden for missing/invalid tokens (see VaultTokenExtract and vault_auth_api.md), but the OpenAPI spec documents a 401 response here. Update the spec so clients don’t bake in the wrong error handling.
                  creation_time: 1722070800
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1769

  • Same as lookup-self: implementation returns 403 on auth failure, but OpenAPI documents 401 here. Align the contract with the actual status code.
              schema:
                $ref: '#/components/schemas/VaultAuthResponse'
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • Same as the other Vault token endpoints: OpenAPI documents 401, but the middleware and server docs describe 403 for missing/invalid tokens. Align the spec.
        '204':
          description: Token revoked.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:631

  • jwks_url is described/used as a JWKS endpoint, but the example points at /.well-known/openid-configuration (which is not a JWKS document). This example should use a JWKS URL (e.g. .../jwks.json) to avoid confusing integrators.
          type: string
          format: uri
          description: URL of the Kubernetes API server JWKS endpoint.
          example: https://kubernetes.default.svc/.well-known/openid-configuration

server/src/server/endpoints/vault_token.rs:27

  • AuthError::Session maps to HTTP 401 in this codebase, but Vault token self-service errors for missing/invalid X-Vault-Token are expected to be 403 (and the VaultTokenExtract middleware already returns 403). Use AuthError::Forbidden here so the handler can’t accidentally return 401 if middleware ordering changes.

This issue also appears in the following locations of the same file:

  • line 29
  • line 79
  • line 112
    server/src/server/endpoints/vault_approle.rs:166
  • OpenAPI marks the /v1/auth/approle/role/{name}/secret-id request body as optional (required: false), but the handler uses Json<AppRoleSecretIdRequest>, which rejects missing bodies with a 400. Accept an optional JSON body and default ttl/num_uses to 0 when absent to match the documented contract.

Comment thread server/src/server/endpoints/vault_k8s.rs Outdated
Copilot AI review requested due to automatic review settings July 28, 2026 05:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (16)

server/src/database/impls/sqlite.rs:161

  • vault_approle_roles.token_policies is stored/parsed as a comma-separated string (see vault_approle_create_role joining with ,), but the SQLite schema default is '[]'. If a row is ever created without an explicit value (or migrated from an earlier schema), this will parse as a literal "[]" policy name.
                role_id             TEXT    NOT NULL UNIQUE,
                secret_id_ttl_secs  INTEGER NOT NULL DEFAULT 0,
                token_ttl_secs      INTEGER NOT NULL DEFAULT 3600,
                bind_secret_id      INTEGER NOT NULL DEFAULT 1,
                token_policies      TEXT    NOT NULL DEFAULT '[]'
            )

server/src/database/impls/postgres.rs:142

  • vault_approle_roles.token_policies is treated as comma-separated throughout the code, but the PostgreSQL schema default is '[]', which would later parse as a literal policy name []. The default should be an empty string to match the actual storage format.
                role_id             TEXT    NOT NULL UNIQUE,
                secret_id_ttl_secs  BIGINT  NOT NULL DEFAULT 0,
                token_ttl_secs      BIGINT  NOT NULL DEFAULT 3600,
                bind_secret_id      BOOLEAN NOT NULL DEFAULT TRUE,
                token_policies      TEXT    NOT NULL DEFAULT '[]'
            )

server/src/server/endpoints/vault_k8s.rs:75

  • Same issue as bound_sa_names: unwrap_or_default() hides malformed JSON in bound_sa_namespaces and turns it into an empty allow-list. This should surface as a server-side configuration error.
    server/src/server/endpoints/vault_k8s.rs:224
  • Same as above: clearing required_spec_claims in the RS256 fallback disables requiring exp and may accept non-expiring JWTs. Keep exp required.
    server/documentation/openapi.yaml:1752
  • The Vault token self-service endpoints return 403 Forbidden on missing/invalid X-Vault-Token (see VaultTokenExtract middleware), not 401 Unauthorized. The OpenAPI spec should match the actual behavior so clients can handle errors correctly.
                  creation_time: 1722070800
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1769

  • /v1/auth/token/renew-self returns 403 Forbidden on missing/invalid X-Vault-Token (middleware), not 401. OpenAPI should reflect the real status code.
                $ref: '#/components/schemas/VaultAuthResponse'
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • /v1/auth/token/revoke-self returns 403 Forbidden on missing/invalid X-Vault-Token (middleware), not 401. Update the OpenAPI responses accordingly.
        '204':
          description: Token revoked.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:631

  • jwks_url is described as a JWKS endpoint, but the example points to /.well-known/openid-configuration (which is not a JWKS document). This is likely to misconfigure clients/admins.
        jwks_url:
          type: string
          format: uri
          description: URL of the Kubernetes API server JWKS endpoint.
          example: https://kubernetes.default.svc/.well-known/openid-configuration

server/documentation/openapi.yaml:638

  • The implementation only supports exact matches plus the "*" wildcard for bound service-account names/namespaces; it does not implement glob/pattern matching. The OpenAPI description should not claim "glob patterns" unless the server actually supports them.
          default: ['*']
          description: Allowed service account names (glob patterns; `["*"]` for any).
          example: [spire-agent]

server/documentation/openapi.yaml:645

  • Same as above for namespaces: the server enforces exact matches plus "*" wildcard, not glob patterns. Update OpenAPI wording to match behavior.
          default: ['*']
          description: Allowed namespaces (glob patterns; `["*"]` for any).
          example: [spire]

server/src/server/endpoints/vault_approle.rs:48

  • New Vault-compatible auth endpoints introduce substantial new behavior (AppRole login, secret-id consumption, token issuance). There are existing integration tests for other HTTP APIs under server/src/tests/, but no tests are added here to cover success/failure cases (invalid role_id/secret_id, secret-id reuse, TTL/renew, etc.).
    server/src/server/endpoints/vault_k8s.rs:127
  • vault_k8s_create_role will store bound_sa_names/bound_sa_namespaces as [] when the fields are omitted (serde default = empty vec). That makes all logins fail by default, even though the schema/OpenAPI defaults indicate ['*']. Also, a negative token_ttl would be treated as non-expiring (expiry=0) when issuing tokens.
    server/src/server/endpoints/vault_k8s.rs:66
  • serde_json::from_str(&role.bound_sa_names).unwrap_or_default() silently turns invalid JSON in the stored role into an empty allow-list, which then denies all logins without surfacing the configuration error. Returning a server error makes misconfiguration diagnosable.

This issue also appears on line 74 of the same file.
client/src/dto/vault.rs:113

  • The DTO docs claim bound_service_account_names / bound_service_account_namespaces accept "glob patterns", but the server currently enforces exact match plus "*" wildcard. This doc mismatch can lead users to configure patterns that will never match.
    /// URL of the Kubernetes JWKS endpoint for verifying service-account JWTs.
    pub jwks_url: String,
    /// Allowed service-account names (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_names: Vec<String>,
    /// Allowed namespaces (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_namespaces: Vec<String>,

server/documentation/vault_auth_api.md:257

  • This flow description says the server "Selects the correct key by kid", but the current implementation in validate_k8s_jwt iterates over all keys and tries to validate until one works. Either update the docs or implement kid-based key selection.
1. Fetches the JWKS from the role's `jwks_url`.
2. Selects the correct key by `kid` from the JWT header.
3. Validates the JWT signature and expiry.
4. Checks `sub` (`system:serviceaccount:<namespace>:<name>`) against the role's allow-lists.

server/documentation/adr/2026-07-26-vault-compatible-auth-api-for-spire.md:96

  • The ADR's schema block says token_policies is a JSON array, but the implementation stores policies as a comma-separated string (CSV) and splits on , in the handlers. The ADR should match the actual storage format to avoid future migration/interop mistakes.
CREATE TABLE vault_approle_roles (
    name                TEXT PRIMARY KEY,
    role_id             TEXT UNIQUE NOT NULL,  -- stable UUID SPIRE stores as approle_id
    secret_id_ttl_secs  INTEGER NOT NULL,       -- 0 = no expiry
    token_ttl_secs      INTEGER NOT NULL,
    bind_secret_id      BOOLEAN NOT NULL,
    token_policies      TEXT NOT NULL           -- JSON array
);

Comment thread server/src/server/endpoints/vault_k8s.rs Outdated
Comment thread server/src/server/endpoints/approle.rs
Copilot AI review requested due to automatic review settings July 28, 2026 06:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (14)

server/documentation/openapi.yaml:1751

  • The Vault token self-service endpoints return 403 on missing/invalid/expired tokens (see VaultTokenExtract middleware and vault_auth_api.md), but the OpenAPI spec documents a 401 here. This makes the published contract inconsistent with runtime behavior.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1769

  • /v1/auth/token/renew-self is documented as returning 401, but the server responds with 403 when the Vault token is missing/invalid/expired (handled in VaultTokenExtract). OpenAPI should match the actual behavior.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • /v1/auth/token/revoke-self is documented as returning 401, but the middleware returns 403 for missing/invalid/expired Vault tokens. This mismatch can break client codegen and troubleshooting.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:631

  • jwks_url is described as a JWKS endpoint, but the example points to the OIDC discovery document (.../openid-configuration). The example should be a JWKS URL (e.g. .../jwks.json) to match what fetch_jwks() expects ({ "keys": [...] }).
          example: https://kubernetes.default.svc/.well-known/openid-configuration

server/documentation/openapi.yaml:637

  • The OpenAPI schema says bound_service_account_names supports glob patterns, but the implementation only supports exact matches plus a literal "*" wildcard. The description should reflect the actual matching behavior.
          description: Allowed service account names (glob patterns; `["*"]` for any).

server/documentation/vault_auth_api.md:370

  • The schema snippet documents vault_approle_roles.token_policies as "comma-separated", but it is persisted as a JSON array string across backends. This should be updated to avoid incorrect expectations when debugging.
    token_ttl_secs      INTEGER NOT NULL,
    bind_secret_id      BOOLEAN NOT NULL,
    token_policies      TEXT NOT NULL        -- comma-separated
);

server/src/server/endpoints/vault_k8s.rs:133

  • If bound_service_account_names / bound_service_account_namespaces are omitted, serde defaults them to empty arrays, and the login check will reject all service accounts/namespaces (because the only wildcard supported is a literal "*"). This contradicts the DB defaults (["*"]) and the OpenAPI defaults, and is likely an accidental deny-all configuration. Also, negative token_ttl currently becomes a non-expiring token (expiry=0) via issue_vault_token().
    server/src/server/endpoints/vault_approle.rs:109
  • Negative TTL inputs are currently accepted for AppRole role creation; because issue_vault_token() treats lease_duration_secs <= 0 as expiry = 0 (never expires), a negative token_ttl can unintentionally mint non-expiring Vault tokens. These TTL fields should be validated as >= 0 (and the meaning of 0 should be explicitly documented).
    server/src/server/endpoints/vault_approle.rs:50
  • This PR introduces a substantial new Vault-compatible auth surface (AppRole login + admin CRUD, token issuance/renew/revoke). There are extensive integration tests under server/src/tests/, but none cover these new endpoints yet. Adding end-to-end tests (SQLite backend) for: (1) AppRole role create → get role-id → secret-id generate → login consumes secret-id, (2) token lookup/renew/revoke behavior and status codes, and (3) Kubernetes role default bindings/wildcard behavior would reduce regression risk.
    server/documentation/openapi.yaml:1823
  • The server accepts both POST and PUT for /v1/auth/approle/login (for Vault/SPIRE SDK compatibility), but the OpenAPI spec only documents POST. This violates the project rule of keeping endpoints and openapi.yaml in sync.
        '403':
          description: Invalid role_id or secret_id.

server/documentation/openapi.yaml:644

  • The OpenAPI schema says bound_service_account_namespaces supports glob patterns, but the server checks exact matches plus "*" only. Documenting globs here is misleading for operators.
          description: Allowed namespaces (glob patterns; `["*"]` for any).

server/documentation/vault_auth_api.md:256

  • This section says the server selects the correct key by kid, but validate_k8s_jwt() currently iterates over all keys and does not use kid for selection. The doc should match the actual validation behavior (or the code should be updated to use kid).

This issue also appears on line 367 of the same file.

1. Fetches the JWKS from the role's `jwks_url`.
2. Selects the correct key by `kid` from the JWT header.
3. Validates the JWT signature and expiry.
4. Checks `sub` (`system:serviceaccount:<namespace>:<name>`) against the role's allow-lists.

server/documentation/vault_auth_api.md:356

  • The schema snippet documents vault_tokens.policies as "comma-separated", but all DB backends store it as a JSON array string (see serde_json::to_string(&token.policies) in vault_issue_token and serde_json::from_str on lookup). This doc mismatch could mislead operators inspecting the DB.
    token_hash          BLOB PRIMARY KEY,    -- SHA-256(raw "hvs.<base64>" token)
    entity              TEXT NOT NULL,       -- AppRole role name or K8s service-account name
    policies            TEXT NOT NULL,       -- comma-separated
    expiry              INTEGER NOT NULL,    -- Unix timestamp; 0 = never

server/documentation/adr/2026-07-26-vault-compatible-auth-api-for-spire.md:83

  • This ADR's schema snippet says vault_tokens.policies is comma-separated, but the implementation persists policies as a JSON array string in all DB backends. The ADR should match the actual schema/encoding to prevent confusion for future maintainers.
    token_hash          BLOB PRIMARY KEY,   -- SHA-256(raw "hvs.<base64>" token)
    entity              TEXT NOT NULL,      -- AppRole role name / K8s SA name
    policies            TEXT NOT NULL,      -- comma-separated
    expiry              INTEGER NOT NULL,   -- unix timestamp, 0 = never

Comment thread server/src/server/endpoints/kubernetes.rs
Copilot AI review requested due to automatic review settings July 28, 2026 06:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (16)

server/src/server/endpoints/vault_k8s.rs:252

  • Same issue in the RS256 fallback loop: a single malformed/unexpected JWK causes an early return (due to ?), instead of being skipped. This can break login if the JWKS contains any key we don't understand.
    server/src/server/endpoints/vault_k8s.rs:87
  • bound_sa_namespaces is parsed with unwrap_or_default(). If the stored JSON is invalid, the namespace allow-list becomes empty and login behavior changes silently. Prefer returning an operator-actionable error instead of defaulting.
    server/src/server/endpoints/vault_token.rs:67
  • vault_token_renew_self returns 401 when claims are missing, and 500 when the token row can't be re-read after renew. Both cases should be treated as a forbidden/invalid token (403), especially since revocation can race with renewal between these two DB calls.
    server/src/server/endpoints/vault_token.rs:111
  • vault_token_revoke_self maps missing VaultTokenClaims to AuthError::Session (401). Since this endpoint is token-authenticated, missing/invalid token should be 403 to align with VaultTokenExtract and Vault client expectations.
    server/documentation/openapi.yaml:631
  • K8sRoleRequest in OpenAPI is out of sync with the implemented API/client DTO: it uses an OpenID configuration URL example (not a JWKS), and it omits the expected_issuer and bound_audiences fields that vault_k8s_create_role accepts and persists.
        jwks_url:
          type: string
          format: uri
          description: URL of the Kubernetes API server JWKS endpoint.
          example: https://kubernetes.default.svc/.well-known/openid-configuration

server/documentation/openapi.yaml:1752

  • OpenAPI documents 401 Unauthorized for /v1/auth/token/lookup-self, but the server returns 403 Forbidden for missing/invalid/revoked X-Vault-Token (see VaultTokenExtract). This mismatch can break generated clients and contradicts Vault semantics.
                  ttl: 3540
                  creation_time: 1722070800
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1769

  • OpenAPI documents 401 Unauthorized for /v1/auth/token/renew-self, but the implementation returns 403 Forbidden when the token is missing/expired/not-renewable. The contract should reflect the actual status code.
              schema:
                $ref: '#/components/schemas/VaultAuthResponse'
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • OpenAPI documents 401 Unauthorized for /v1/auth/token/revoke-self, but VaultTokenExtract returns 403 Forbidden when X-Vault-Token is missing/invalid. This should be reflected in the OpenAPI spec.
      responses:
        '204':
          description: Token revoked.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/vault_auth_api.md:370

  • The schema snippet documents vault_approle_roles.token_policies as "comma-separated", but the DB layer stores a JSON array string (via serde_json::to_string(&role.token_policies)). This should be corrected for accuracy.
    secret_id_ttl_secs  INTEGER NOT NULL,
    token_ttl_secs      INTEGER NOT NULL,
    bind_secret_id      BOOLEAN NOT NULL,
    token_policies      TEXT NOT NULL        -- comma-separated
);

server/src/server/endpoints/vault_k8s.rs:229

  • validate_k8s_jwt returns an error as soon as it encounters a single malformed/unexpected JWK in the JWKS, because the JWK parse uses ? inside the loop. That makes Kubernetes login fragile (one bad key DoS'es all logins) even though the code otherwise intends to skip non-matching keys.

This issue also appears on line 248 of the same file.
server/src/server/endpoints/vault_k8s.rs:62

  • bound_audiences is parsed with unwrap_or_default(). If the stored JSON is corrupted or manually edited, this silently disables audience enforcement (treats it as an empty list), weakening auth without surfacing an operator-visible error.
    server/src/server/endpoints/vault_k8s.rs:77
  • bound_sa_names is parsed with unwrap_or_default(). If the stored JSON is invalid, the allow-list becomes empty and the subsequent check denies all logins (or could behave unexpectedly if the check changes). It's safer to fail fast with a clear server-side error so operators can fix the role config.

This issue also appears on line 84 of the same file.
server/src/server/endpoints/vault_token.rs:33

  • These token endpoints are behind VaultTokenExtract (which returns 403 on missing/invalid token), but the handlers map missing claims / missing DB row to AuthError::Session (401). This can happen if the token is revoked between the middleware lookup and the handler’s DB re-read, and should return 403 to match Vault semantics and the middleware behavior.

This issue also appears on line 107 of the same file.
server/documentation/vault_auth_api.md:357

  • The database schema snippet says vault_tokens.policies is "comma-separated", but the implementations store a JSON array string (they use serde_json::to_string(&token.policies) on insert). The doc should match the actual persisted format to avoid operator confusion.

This issue also appears on line 366 of the same file.

CREATE TABLE vault_tokens (
    token_hash          BLOB PRIMARY KEY,    -- SHA-256(raw "hvs.<base64>" token)
    entity              TEXT NOT NULL,       -- AppRole role name or K8s service-account name
    policies            TEXT NOT NULL,       -- comma-separated
    expiry              INTEGER NOT NULL,    -- Unix timestamp; 0 = never

server/documentation/vault_auth_api.md:388

  • vault_k8s_roles in the schema snippet is missing the expected_issuer and bound_audiences columns that are created by all DB backends and used by vault_k8s_login/vault_k8s_create_role. The doc should include these columns so operators can inspect/maintain the table correctly.
-- Kubernetes roles
CREATE TABLE vault_k8s_roles (
    name                TEXT PRIMARY KEY,
    jwks_url            TEXT NOT NULL,
    bound_sa_names      TEXT NOT NULL,       -- JSON array
    bound_sa_namespaces TEXT NOT NULL,       -- JSON array
    token_ttl_secs      INTEGER NOT NULL
);

server/src/server/auth_verifier.rs:386

  • New Vault auth/token scopes are registered here, but there are no corresponding integration tests under server/src/tests/ exercising AppRole login, Kubernetes login (JWT validation + bound SA checks), and token self-service (lookup/renew/revoke). Given the server already has a comprehensive HTTPS test harness, these flows should be covered to prevent regressions in auth semantics.
    // ── Vault-compatible auth scopes ─────────────────────────────────────────
    //
    // Scope prefixes are kept intentionally specific to avoid Actix-web's
    // FIFO matching swallowing requests before they reach the right scope.
    //
    // /v1/auth/approle/login  — unauthenticated AppRole login (registered first, most specific)
    // /v1/auth/kubernetes/login — unauthenticated K8s login
    // /v1/auth/approle        — AppRole admin CRUD (CookieAuthSameServer + AdminAuth)
    // /v1/auth/kubernetes     — K8s admin CRUD
    // /v1/auth/token          — token self-service (VaultTokenExtract middleware)

Copilot AI review requested due to automatic review settings July 28, 2026 06:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (17)

server/documentation/openapi.yaml:1751

  • These endpoints return HTTP 403 when the X-Vault-Token is missing/invalid (the AppTokenExtract middleware uses HttpResponse::Forbidden()), but the OpenAPI spec documents 401 Unauthorized here. This makes the contract inaccurate for clients.
                  ttl: 3540
                  creation_time: 1722070800
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1769

  • /v1/auth/token/renew-self returns 403 for missing/invalid X-Vault-Token (middleware), but the OpenAPI spec lists only 401 Unauthorized. The documented status code should match the implementation.
              schema:
                $ref: '#/components/schemas/AppAuthResponse'
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • /v1/auth/token/revoke-self returns 403 for missing/invalid X-Vault-Token (middleware), but the OpenAPI spec lists only 401 Unauthorized. This is a contract mismatch.
        '204':
          description: Token revoked.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:632

  • jwks_url is documented (and used in code) as a JWKS endpoint returning { "keys": [...] }, but the example points to /.well-known/openid-configuration (OIDC discovery), which would fail fetch_jwks parsing. The example should be a JWKS URL.
          type: string
          format: uri
          description: URL of the Kubernetes API server JWKS endpoint.
          example: https://kubernetes.default.svc/.well-known/openid-configuration
        bound_service_account_names:

server/documentation/openapi.yaml:638

  • The implementation only supports exact matches plus the special wildcard "*" for bound_service_account_names (it uses Vec::contains), but the schema description says “glob patterns”. This description should match the actual matching semantics.
          type: array
          items:
            type: string
          default: ['*']
          description: Allowed service account names (glob patterns; `["*"]` for any).
          example: [spire-agent]

client/src/dto/app_auth.rs:116

  • The doc comment says namespaces support glob patterns, but the server code only supports exact matches plus "*". Keeping this accurate helps avoid misconfiguration.
    #[serde(default)]
    pub bound_service_account_names: Vec<String>,
    /// Allowed namespaces (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_namespaces: Vec<String>,
    /// TTL for issued tokens in seconds.
    #[serde(default = "default_token_ttl")]
    pub token_ttl: i64,

server/documentation/app_auth_api.md:370

  • The DB schema example says approle_roles.token_policies is “comma-separated”, but the implementation stores JSON (serde_json::to_string(&role.token_policies)). The doc should match the actual storage format.
    secret_id_ttl_secs  INTEGER NOT NULL,
    token_ttl_secs      INTEGER NOT NULL,
    bind_secret_id      BOOLEAN NOT NULL,
    token_policies      TEXT NOT NULL        -- comma-separated
);

server/src/server/endpoints/auth_token.rs:79

  • auth_token_renew_self maps a missing token after renewal to AuthError::Generic, which becomes HTTP 500. A token that is revoked/expired concurrently should be treated as a client-side auth failure (403/401), not an internal error.
    server/documentation/openapi.yaml:1791
  • The server accepts both POST and PUT for /v1/auth/approle/login (see #[route("", method = "POST", method = "PUT")] in server/src/server/endpoints/approle.rs), but the OpenAPI spec documents only POST. This can break client generation / validation for SPIRE which may use PUT.
  /v1/auth/approle/login:
    post:
      tags: [AppRole]
      summary: Login with AppRole credentials
      description: |

server/documentation/openapi.yaml:644

  • The implementation only supports exact matches plus the special wildcard "*" for bound_service_account_namespaces (it uses Vec::contains), but the schema description says “glob patterns”. This description should match the actual matching semantics.
        bound_service_account_namespaces:
          type: array
          items:
            type: string
          default: ['*']
          description: Allowed namespaces (glob patterns; `["*"]` for any).
          example: [spire]

server/documentation/openapi.yaml:649

  • K8sRoleRequest in Rust includes expected_issuer and bound_audiences, and k8s_create_role persists both, but the OpenAPI schema omits them. This makes the API contract incomplete.
        token_ttl:
          type: integer
          default: 3600
          description: Issued app token TTL in seconds.

client/src/dto/app_auth.rs:108

  • The doc comments describe bound_service_account_names/bound_service_account_namespaces as “glob patterns”, but the server-side check only supports exact matches plus "*" (wildcard). The docs should reflect the actual matching semantics so clients don’t assume globbing support.

This issue also appears on line 109 of the same file.

pub struct K8sRoleRequest {
    /// URL of the Kubernetes JWKS endpoint for verifying service-account JWTs.
    pub jwks_url: String,
    /// Allowed service-account names (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_names: Vec<String>,
    /// Allowed namespaces (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_namespaces: Vec<String>,

server/documentation/app_auth_api.md:355

  • The DB schema example says app_tokens.policies is “comma-separated”, but the implementation stores JSON (serde_json::to_string(&token.policies)). This doc mismatch can confuse operators inspecting the DB.

This issue also appears on line 366 of the same file.

CREATE TABLE app_tokens (
    token_hash          BLOB PRIMARY KEY,    -- SHA-256(raw "hvs.<base64>" token)
    entity              TEXT NOT NULL,       -- AppRole role name or K8s service-account name
    policies            TEXT NOT NULL,       -- comma-separated
    expiry              INTEGER NOT NULL,    -- Unix timestamp; 0 = never
    renewable           BOOLEAN NOT NULL,
    lease_duration_secs INTEGER NOT NULL,
    created_at          INTEGER NOT NULL
);

server/documentation/adr/2026-07-26-app-auth-api-for-spire.md:82

  • The ADR’s schema sketch says app_tokens.policies is “comma-separated”, but the code stores JSON arrays in this column. This should be corrected so the ADR remains an accurate design reference.
CREATE TABLE app_tokens (
    token_hash          BLOB PRIMARY KEY,   -- SHA-256(raw "hvs.<base64>" token)
    entity              TEXT NOT NULL,      -- AppRole role name / K8s SA name
    policies            TEXT NOT NULL,      -- comma-separated
    expiry              INTEGER NOT NULL,   -- unix timestamp, 0 = never

server/src/database/impls/sqlite.rs:144

  • app_tokens.policies is stored as JSON (see serde_json::to_string(&token.policies)), but the table DDL sets DEFAULT '' which is not valid JSON. Using DEFAULT '[]' keeps the schema consistent with how the column is parsed/stored.
            CREATE TABLE IF NOT EXISTS app_tokens (
                token_hash      BLOB    NOT NULL PRIMARY KEY,
                entity          TEXT    NOT NULL,
                policies        TEXT    NOT NULL DEFAULT '',
                expiry          INTEGER NOT NULL,
                renewable       INTEGER NOT NULL DEFAULT 0,
                lease_duration_secs INTEGER NOT NULL DEFAULT 3600,
                created_at      INTEGER NOT NULL

server/src/database/impls/postgres.rs:122

  • app_tokens.policies is stored as JSON (see serde_json::to_string(&token.policies)), but the table DDL sets DEFAULT '' which is not valid JSON. Using DEFAULT '[]' keeps the schema consistent with how the column is parsed/stored.
            CREATE TABLE IF NOT EXISTS app_tokens (
                token_hash          BYTEA   NOT NULL PRIMARY KEY,
                entity              TEXT    NOT NULL,
                policies            TEXT    NOT NULL DEFAULT '',
                expiry              BIGINT  NOT NULL,
                renewable           BOOLEAN NOT NULL DEFAULT FALSE,
                lease_duration_secs BIGINT  NOT NULL DEFAULT 3600,
                created_at          BIGINT  NOT NULL

server/src/server/auth_verifier.rs:385

  • New /v1/auth/* endpoints and the AppTokenExtract middleware introduce substantial new auth and persistence behavior, but there are no integration tests covering AppRole login, secret-id consumption semantics, K8s JWT validation, or token self-service. The repo already has extensive endpoint/integration tests under server/src/tests/, so this is a noticeable gap for a security-sensitive feature.
    // ── AppRole-compatible auth scopes ─────────────────────────────────────────
    //
    // Scope prefixes are kept intentionally specific to avoid Actix-web's
    // FIFO matching swallowing requests before they reach the right scope.
    //
    // /v1/auth/approle/login  — unauthenticated AppRole login (registered first, most specific)
    // /v1/auth/kubernetes/login — unauthenticated K8s login
    // /v1/auth/approle        — AppRole admin CRUD (CookieAuthSameServer + AdminAuth)
    // /v1/auth/kubernetes     — K8s admin CRUD
    // /v1/auth/token          — token self-service (AppTokenExtract middleware)

Copilot AI review requested due to automatic review settings July 28, 2026 09:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (11)

server/documentation/openapi.yaml:1752

  • The app-token endpoints currently document 401 Unauthorized, but the implementation (AppTokenExtract middleware) returns 403 Forbidden for missing/invalid/expired X-Vault-Token. The OpenAPI spec should match the actual status codes so client integrations don't mis-handle auth failures.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1769

  • /auth/token/renew-self documents 401 Unauthorized, but the server returns 403 Forbidden when the token is missing/invalid/not-renewable (via AppTokenExtract + renew_app_token mapping). Update the OpenAPI responses accordingly.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • /auth/token/revoke-self documents 401 Unauthorized, but the server returns 403 Forbidden when X-Vault-Token is missing/invalid (AppTokenExtract). Update the OpenAPI responses to match.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:632

  • K8sRoleRequest.jwks_url is described as a JWKS endpoint, but the example points to /.well-known/openid-configuration (OIDC discovery), which will not return a { keys: [...] } JWKS and would fail at runtime. Update the example to a JWKS JSON URL.
        jwks_url:
          type: string
          format: uri
          description: URL of the Kubernetes API server JWKS endpoint.
          example: https://kubernetes.default.svc/.well-known/openid-configuration
        bound_service_account_names:

server/documentation/openapi.yaml:645

  • The OpenAPI descriptions claim bound_service_account_names / bound_service_account_namespaces are "glob patterns", but the implementation only supports exact matches plus the special wildcard value "*". Either implement glob matching or adjust the documentation to avoid misleading operators.
        bound_service_account_names:
          type: array
          items:
            type: string
          default: ['*']
          description: Allowed service account names (glob patterns; `["*"]` for any).
          example: [spire-agent]
        bound_service_account_namespaces:
          type: array
          items:
            type: string
          default: ['*']
          description: Allowed namespaces (glob patterns; `["*"]` for any).
          example: [spire]

server/src/server/endpoints/kubernetes.rs:281

  • build_validation clears validation.required_spec_claims, which makes exp no longer required even though validate_exp = true. This can allow acceptance of service-account JWTs without an expiry (depending on jsonwebtoken behavior), weakening replay resistance. Prefer requiring at least exp (and typically sub) for these tokens.
    server/documentation/adr/2026-07-26-app-auth-api-for-spire.md:58
  • This ADR describes the new endpoints under a /v1/auth/ prefix, but the server routes and OpenAPI in this PR are registered under /auth/... (no /v1). The ADR should match the actual API surface to avoid deployment/configuration errors (e.g., SPIRE/KMS pointing at the wrong path).
### Decision 1 — New `/v1/auth/` scope, unauthenticated login + admin-gated CRUD

Three new endpoint modules under `server/src/server/endpoints/`:

| Module | Routes | Auth |
|--------|--------|------|
| `approle.rs` | `POST/PUT /v1/auth/approle/login` | none (credential is the body) |
| `approle.rs` | `POST /v1/auth/approle/role/{name}`, `GET .../role-id`, `POST .../secret-id`, `POST .../secret-id/destroy`, `DELETE /role/{name}`, `GET /role?list=true` | `CookieAuthSameServer` + `AdminAuth` |
| `kubernetes.rs` | `POST /v1/auth/kubernetes/login` | none (credential is the K8s SA JWT) |
| `kubernetes.rs` | `POST /v1/auth/kubernetes/role/{name}`, `DELETE /role/{name}` | `CookieAuthSameServer` + `AdminAuth` |
| `auth_token.rs` | `GET /v1/auth/token/lookup-self`, `POST /v1/auth/token/renew-self`, `POST /v1/auth/token/revoke-self` | `app_token_extract` middleware |

server/documentation/app_auth_api.md:370

  • The DB schema snippet documents app_tokens.policies and approle_roles.token_policies as "comma-separated", but the implementations store JSON (they serde_json::to_string a Vec<String> and parse JSON on reads). This mismatch will confuse operators inspecting DB state and future maintainers.
CREATE TABLE app_tokens (
    token_hash          BLOB PRIMARY KEY,    -- SHA-256(raw "hvs.<base64>" token)
    entity              TEXT NOT NULL,       -- AppRole role name or K8s service-account name
    policies            TEXT NOT NULL,       -- comma-separated
    expiry              INTEGER NOT NULL,    -- Unix timestamp; 0 = never
    renewable           BOOLEAN NOT NULL,
    lease_duration_secs INTEGER NOT NULL,
    created_at          INTEGER NOT NULL
);

-- AppRole roles
CREATE TABLE approle_roles (
    name                TEXT PRIMARY KEY,
    role_id             TEXT UNIQUE NOT NULL,
    secret_id_ttl_secs  INTEGER NOT NULL,
    token_ttl_secs      INTEGER NOT NULL,
    bind_secret_id      BOOLEAN NOT NULL,
    token_policies      TEXT NOT NULL        -- comma-separated
);

client/src/dto/app_auth.rs:33

  • AppRoleRoleRequest.bind_secret_id is documented as allowing login with role_id only when set to false, but AppRoleLoginRequest currently requires secret_id: String (non-optional). As-is, role_id-only login cannot work (serde will reject bodies without secret_id). Either make secret_id optional end-to-end or remove/adjust the bind_secret_id semantics/documentation.
/// Request body for `POST /auth/approle/login`.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct AppRoleLoginRequest {
    pub role_id: String,
    pub secret_id: String,
}

/// Request body for `POST /auth/approle/role/{name}` (create/update).
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct AppRoleRoleRequest {
    /// TTL for secret IDs in seconds (0 = no expiry).
    #[serde(default)]
    pub secret_id_ttl: i64,
    /// TTL for issued tokens in seconds.
    #[serde(default = "default_token_ttl")]
    pub token_ttl: i64,
    /// List of policies to attach to issued tokens.
    #[serde(default)]
    pub token_policies: Vec<String>,
    /// Whether `secret_id` is required for login. Defaults to `true`; set to
    /// `false` to allow login with `role_id` only (not recommended for production).
    #[serde(default = "default_true")]
    pub bind_secret_id: bool,
}

client/src/dto/app_auth.rs:113

  • K8sRoleRequest docs say the bound service-account names/namespaces support "glob patterns", but the server implementation currently only supports exact matches plus the special wildcard value "*". Update these doc comments (or implement actual glob matching) so callers configure the role correctly.
pub struct K8sRoleRequest {
    /// URL of the Kubernetes JWKS endpoint for verifying service-account JWTs.
    pub jwks_url: String,
    /// Allowed service-account names (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_names: Vec<String>,
    /// Allowed namespaces (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_namespaces: Vec<String>,

server/documentation/adr/2026-07-26-app-auth-api-for-spire.md:87

  • In the ADR's schema sketch, app_tokens.policies is documented as "comma-separated", but the code stores JSON (it writes serde_json::to_string(Vec<String>) and reads it back as JSON). Adjust the ADR so it matches the implemented storage format.
    token_hash          BLOB PRIMARY KEY,   -- SHA-256(raw "hvs.<base64>" token)
    entity              TEXT NOT NULL,      -- AppRole role name / K8s SA name
    policies            TEXT NOT NULL,      -- comma-separated
    expiry              INTEGER NOT NULL,   -- unix timestamp, 0 = never
    renewable           BOOLEAN NOT NULL,
    lease_duration_secs INTEGER NOT NULL,
    created_at          INTEGER NOT NULL

Comment thread server/src/server/endpoints/kubernetes.rs Outdated
Comment thread server/src/server/endpoints/mod.rs
Copilot AI review requested due to automatic review settings July 28, 2026 10:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 37 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (18)

server/documentation/openapi.yaml:637

  • The OpenAPI description says bound_service_account_names supports glob patterns, but the implementation only supports exact matches plus the special wildcard value "*". The docs should reflect the actual matching semantics to avoid unexpected auth failures.
          description: Allowed service account names (glob patterns; `["*"]` for any).

server/documentation/openapi.yaml:631

  • K8sRoleRequest.jwks_url example points to /.well-known/openid-configuration, but the server expects a JWKS document with a top-level keys array. Using the OpenID configuration URL here will fail at runtime.
          example: https://kubernetes.default.svc/.well-known/openid-configuration

server/documentation/openapi.yaml:1751

  • The token self-service endpoints return 403 Forbidden on missing/invalid tokens (see AppTokenExtract and app_auth_api.md), but the OpenAPI spec documents 401 Unauthorized. This mismatch will confuse clients and generated SDKs.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1769

  • /auth/token/renew-self documents a 401 response, but the implementation responds with 403 Forbidden for missing/invalid/non-renewable tokens (via middleware + handler mapping). The OpenAPI responses should match actual behavior.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/openapi.yaml:1783

  • /auth/token/revoke-self documents a 401 response, but the middleware returns 403 Forbidden for missing/invalid tokens. Update the OpenAPI responses to match runtime behavior.
        '401':
          $ref: '#/components/responses/Unauthorized'

server/documentation/adr/2026-07-26-app-auth-api-for-spire.md:58

  • The routes table uses /v1/auth/... paths, but the server registers scopes under /auth/... and the endpoint modules use /auth/... in their module docs. This inconsistency makes it hard to deploy the feature correctly (and may break SPIRE if it truly requires /v1).
| `approle.rs` | `POST/PUT /v1/auth/approle/login` | none (credential is the body) |
| `approle.rs` | `POST /v1/auth/approle/role/{name}`, `GET .../role-id`, `POST .../secret-id`, `POST .../secret-id/destroy`, `DELETE /role/{name}`, `GET /role?list=true` | `CookieAuthSameServer` + `AdminAuth` |
| `kubernetes.rs` | `POST /v1/auth/kubernetes/login` | none (credential is the K8s SA JWT) |
| `kubernetes.rs` | `POST /v1/auth/kubernetes/role/{name}`, `DELETE /role/{name}` | `CookieAuthSameServer` + `AdminAuth` |
| `auth_token.rs` | `GET /v1/auth/token/lookup-self`, `POST /v1/auth/token/renew-self`, `POST /v1/auth/token/revoke-self` | `app_token_extract` middleware |

server/documentation/adr/2026-07-26-app-auth-api-for-spire.md:66

  • This paragraph still refers to the /v1/auth/ prefix, but the rest of the docs/OpenAPI and the Actix scopes are /auth/.... The wording should be updated so readers don’t configure the wrong paths.
**Distinct scope prefix**: the `/v1/auth/` scope was deliberately kept separate from
`/login`, `/realms`, etc. to avoid an Actix-web FIFO routing conflict between the new
unauthenticated login routes and existing authenticated scopes matching a similar path

client/src/dto/app_auth.rs:113

  • This DTO doc comment says bound_service_account_namespaces supports glob patterns, but the server currently only supports exact matches plus "*". The docs should match behavior.
    /// Allowed namespaces (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_namespaces: Vec<String>,

server/documentation/openapi.yaml:649

  • K8sRoleRequest in the OpenAPI schema is missing the expected_issuer and bound_audiences fields, but the server handler (k8s_create_role) and client DTO (K8sRoleRequest) both support them. This makes the OpenAPI contract incomplete and will mislead clients/codegen.
        token_ttl:
          type: integer
          default: 3600
          description: Issued app token TTL in seconds.

server/documentation/openapi.yaml:644

  • The OpenAPI description says bound_service_account_namespaces supports glob patterns, but the implementation only supports exact matches plus the special wildcard value "*". Documenting this as globbing is misleading.
          description: Allowed namespaces (glob patterns; `["*"]` for any).

server/documentation/openapi.yaml:1790

  • The server accepts both POST and PUT for /auth/approle/login (see #[route(... method = "POST", method = "PUT")]), but the OpenAPI spec documents only post. For Vault/SPIRE compatibility and accurate codegen, the spec should also include a put operation with the same request/response schema.
  /auth/approle/login:
    post:
      tags: [AppRole]
      summary: Login with AppRole credentials

server/documentation/app_auth_api.md:255

  • This doc says Kubernetes login selects the JWKS key by kid, but validate_k8s_jwt currently tries each key in the JWKS and does not filter by kid. The step list should match the actual implementation.
1. Fetches the JWKS from the role's `jwks_url`.
2. Selects the correct key by `kid` from the JWT header.
3. Validates the JWT signature and expiry.

server/documentation/adr/2026-07-26-app-auth-api-for-spire.md:48

  • This ADR describes the feature as being under a /v1/auth/ scope, but the implementation and OpenAPI are using /auth/... (no /v1). The ADR should be aligned with the actual route prefix (or the code should be changed to match the ADR).

This issue also appears in the following locations of the same file:

  • line 54
  • line 64
### Decision 1 — New `/v1/auth/` scope, unauthenticated login + admin-gated CRUD

CHANGELOG/spire-app-auth-api.md:27

  • The changelog documents the new endpoints under /v1/auth/..., but the server registers them under /auth/... and the OpenAPI spec uses /auth/.... The changelog should match the actual public routes.
  - **Token endpoints** under `/v1/auth/token/`:
    - `GET /lookup-self` — validate a token and return entity + policies;
    - `POST /renew-self` — extend a renewable token's TTL;
    - `POST /revoke-self` — immediately invalidate a token.
  - **AppRole endpoints** under `/v1/auth/approle/`:

client/src/dto/app_auth.rs:110

  • This DTO doc comment says bound_service_account_names supports glob patterns, but the server implementation only supports exact matches plus the special wildcard value "*". Keeping the client docs accurate helps prevent misconfiguration.

This issue also appears on line 111 of the same file.

    /// Allowed service-account names (glob patterns, `["*"]` for any).
    #[serde(default)]
    pub bound_service_account_names: Vec<String>,

server/src/server/endpoints/mod.rs:82

  • lease_duration_secs can be negative (it comes from request bodies / DB values). With the current logic, a negative value produces expiry = 0 (non-expiring) but preserves a negative lease_duration_secs, which later yields a negative ttl in lookup responses. Clamp the lease duration to >= 0 before computing expiry and storing the record.
    server/src/server/auth_verifier.rs:385
  • These scopes are registered under /auth/..., but the ADR and changelog in this PR describe /v1/auth/... (and SPIRE/HashiCorp Vault clients typically call /v1/auth/...). Please reconcile the public route prefix across code + docs (either change the scopes to /v1/auth/... or update all docs/consumers to /auth/...).
    server/src/server/auth_verifier.rs:391
  • New unauthenticated login endpoints and token self-service endpoints are introduced here, but there are no integration tests covering: (1) AppRole login success/failure and secret_id consumption; (2) Kubernetes login JWT validation + bound SA/namespace/audience enforcement; (3) token lookup/renew/revoke behavior. The server crate already has an integration test harness, so adding tests would prevent regressions in this security-sensitive surface.

Comment thread server/src/server/endpoints/kubernetes.rs
Manuthor added a commit that referenced this pull request Jul 28, 2026
…corrections

Security fixes:
- kubernetes.rs: fail closed on corrupt bound_audiences/bound_sa_names/bound_sa_namespaces
  JSON in the DB (previously unwrap_or_default() silently disabled checks — CWE-754)
- kubernetes.rs: require 'exp' claim in required_spec_claims so JWTs without an expiry
  are rejected; previously validate_exp=true was set but exp was not required (RFC 7519 §4.1.4)
- kubernetes.rs: disable reqwest redirect following in fetch_jwks to prevent downgrade
  from https:// to plaintext HTTP via redirect; add error_for_status() for non-2xx responses
- kubernetes.rs: normalize empty bound_sa_names/bound_sa_namespaces to ["*"] at
  create-role time to match the documented default (omitting = allow all)
- kubernetes.rs: reject negative token_ttl values in k8s_create_role
- approle.rs: reject negative secret_id_ttl/token_ttl values in approle_create_role
- app_token_extract.rs: replace raw DB error string in HTTP 500 with generic message to
  avoid leaking internal details (CWE-209 information exposure)

OpenAPI corrections:
- token self-service (lookup-self, renew-self, revoke-self): change 401 to 403 to match
  actual middleware behaviour (AppTokenExtract returns HTTP 403, not 401)
- renew-self: document that auth.client_token echoes the presented token per Vault spec §3.6
- K8sRoleRequest.jwks_url: fix example from OIDC discovery URL to JWKS document URL;
  add note that redirects are not followed
- K8sRoleRequest.bound_service_account_names/namespaces: replace 'glob patterns' with
  accurate 'exact match or * wildcard' description

Test fixes:
- app_auth_tests: update assertions to expect client_token/id echoed per Vault spec
- auth_token.rs: implement token echo in lookup-self and renew-self per Vault API spec

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 28, 2026 14:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (18)

server/documentation/openapi.yaml:1774

  • The /auth/token/lookup-self example shows data.id: '', but the endpoint echoes back the token presented in X-Vault-Token. The example should reflect that to match actual behavior.
                data:
                  id: ''
                  entity_id: spire-server

server/documentation/app_auth_api.md:337

  • The text says id is always empty, but the implementation returns id as the raw token from X-Vault-Token (to match Vault token lookup-self expectations). This sentence should be updated to avoid contradicting the server behavior.
`id` is always an empty string — the raw token is never echoed back.

server/documentation/app_auth_api.md:364

  • The text says client_token is always empty, but the implementation returns the presented token in auth.client_token (Vault renew-self behavior). This should be updated to match reality.
`client_token` is always empty — the raw token is never echoed back.

CHANGELOG/spire-app-auth-api.md:15

  • This changelog path prefix (/v1/auth/approle/) does not match the implemented routes (/auth/approle/...).
  - **AppRole endpoints** under `/v1/auth/approle/`:

CHANGELOG/spire-app-auth-api.md:24

  • This changelog path prefix (/v1/auth/kubernetes/) does not match the implemented routes (/auth/kubernetes/...).
  - **Kubernetes auth endpoints** under `/v1/auth/kubernetes/`:

server/src/server/endpoints/auth_token.rs:44

  • Use the shared APP_TOKEN_HEADER constant here instead of a string literal to keep the header name consistent across middleware and endpoints.
    server/src/server/endpoints/auth_token.rs:106
  • Use the shared APP_TOKEN_HEADER constant here instead of a string literal to keep the header name consistent across middleware and endpoints.
    server/documentation/app_auth_api.md:405
  • The DB schema comment says approle_roles.token_policies is "comma-separated", but it is stored as a JSON array string in all backends (via serde_json::to_string(&role.token_policies)).
    token_policies      TEXT NOT NULL        -- comma-separated

server/documentation/openapi.yaml:723

  • AppTokenData.id is documented as always empty, but the server implementation returns data.id as the raw token from the X-Vault-Token header (per Vault lookup-self behavior and the integration tests). This mismatch will mislead generated clients and consumers.

This issue also appears on line 1772 of the same file.

      properties:
        id:
          type: string
          description: Token ID — always empty string (never echoed for security).
        entity_id:

server/documentation/app_auth_api.md:327

  • This lookup-self response example sets data.id to an empty string, but the server actually echoes back the presented token in data.id (and the integration tests assert that behavior).

This issue also appears on line 337 of the same file.

    "id": "",

server/documentation/app_auth_api.md:355

  • This renew-self response example sets auth.client_token to an empty string, but the server echoes back the presented token in auth.client_token (and tests assert this).

This issue also appears on line 364 of the same file.

    "client_token": "",

CHANGELOG/spire-app-auth-api.md:11

  • This changelog still documents the app-auth endpoints under /v1/auth/..., but the implementation and OpenAPI use /auth/... (no /v1 prefix). This will mislead users following the changelog.

This issue also appears in the following locations of the same file:

  • line 15
  • line 24
  - **Token endpoints** under `/v1/auth/token/`:

server/src/server/endpoints/kubernetes.rs:150

  • This comment says an explicit empty array is semantically different (deny-all), but with the current DTO type (Vec<String>) the server cannot distinguish an omitted field from an explicitly empty array — both deserialize to an empty vec and are normalized to ["*"]. The comment should be corrected to match the actual behavior.
    server/src/server/endpoints/auth_token.rs:9
  • This module hard-codes the X-Vault-Token header name in multiple places. Since the header name is already defined as APP_TOKEN_HEADER in the app-token middleware, using the shared constant avoids drift if the header name ever changes.

This issue also appears in the following locations of the same file:

  • line 41
  • line 103
    server/documentation/app_auth_api.md:391
  • The DB schema comment says app_tokens.policies is "comma-separated", but the database implementations persist it as a JSON array string via serde_json::to_string(&token.policies). The docs should match the actual storage format.

This issue also appears on line 405 of the same file.

    policies            TEXT NOT NULL,       -- comma-separated

server/documentation/adr/2026-07-26-app-auth-api-for-spire.md:89

  • The ADR's schema snippet says app_tokens.policies is "comma-separated", but the code persists policies as a JSON array string (see the issue_app_token implementations that bind serde_json::to_string(&token.policies)). Update the ADR to avoid confusing operators inspecting the DB.
    policies            TEXT NOT NULL,      -- comma-separated

server/src/server/endpoints/approle.rs:196

  • ttl is accepted as any integer; negative values currently fall back to the role default (same behavior as 0), which is surprising and inconsistent with the negative-TTL validation applied in approle_create_role. It would be clearer to reject ttl < 0 with 400 BadRequest.
    server/src/server/endpoints/approle.rs:207
  • num_uses currently treats any non-positive value (including negatives) as unlimited (-1). Given the API docs define 0 = unlimited, accepting negative numbers as a synonym is unexpected input tolerance and makes it harder to detect misconfiguration. Consider rejecting num_uses < 0 with 400 BadRequest.

@HatemMn HatemMn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One diagnosability gap in the new auth-scope registration: the unimplemented cert-auth method fails closed (safe) but silently, with a bare 404 instead of a structured error.

Comment thread server/src/server/auth_verifier.rs Outdated

@HatemMn HatemMn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One consistency gap: Kubernetes login error messages distinguish role-not-found from JWT-validation-failure, unlike AppRole's uniform error — a low-value but easy-to-fix role-name enumeration oracle.

Comment thread server/src/server/endpoints/kubernetes.rs Outdated

@HatemMn HatemMn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings from the SPIRE-branch static review, re-posted after confirming they hadn't actually gone out in an earlier pass: an unimplemented cert-auth method that fails closed but silently (bare 404, no structured error), and a Kubernetes login error message that leaks role existence unlike AppRole's uniform error.

Comment thread server/src/server/endpoints/kubernetes.rs Outdated
Comment thread server/src/server/auth_verifier.rs Outdated
@HatemMn

HatemMn commented Jul 28, 2026

Copy link
Copy Markdown

Note: my last review duplicates two findings (cert-auth 404, k8s role-existence leak) already posted earlier in this thread — my tooling gave me a stale read that made it look like they hadn't gone out. Apologies for the noise; the content is the same finding, not a new one.

@HatemMn HatemMn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2 review — live-confirmed Content-Type asymmetry between the K8s and AppRole login handlers.

Comment thread server/src/server/endpoints/kubernetes.rs Outdated
Manuthor added a commit that referenced this pull request Jul 29, 2026
- k8s_login: Content-Type-agnostic body parsing (Bytes + from_slice) to match
  approle_login and SPIRE's Go client (ADR Decision 1).
- k8s_login: uniform generic error for all auth failures (role-not-found, JWT,
  binding) to remove the role-name enumeration oracle (CWE-204); specifics logged.
- auth_verifier: structured default_service so unsupported auth methods (e.g.
  cert_auth) fail closed with an {"errors":[...]} envelope, not a bare 404.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Manuthor and others added 9 commits August 1, 2026 05:58
- Update admin-ui pnpm dependency hashes for linux-x86_64 and
  linux-aarch64 after pnpm-lock.yaml drift
- Flush React 19 scheduler setImmediate callbacks in vitest afterEach
  to prevent 'window is not defined' race on CI Linux environments
After clicking Renew, a toast notification 'Token renewed — new lease
7200s' appeared alongside the TTL display '7200s', causing
getByText('7200s') to resolve to 2 elements in Playwright strict mode.
Changed to getByText('7200s', { exact: true }).
Previously the server only used RUST_LOG env var via log_init(None).
Now a [log] section in the TOML config controls the minimum log level,
defaulting to "info". Target-qualified filters like
"info,auth_verifier=debug" are also supported.

Configs updated:
- auth_verifier.toml: [log] level = "info"
- auth_verifier.dev.toml: [log] level = "debug"
- auth_verifier.spire.toml: [log] level = "info"
  (replaces old comment saying log level was not configurable)
…dcoded roles from default config

- Rename auth_verifier.service → cosmian_auth_verifier.service for naming
  consistency with cosmian_kms.service
- Update SyslogIdentifier to cosmian_auth_verifier so journalctl output is
  uniformly prefixed cosmian_*
- Update cargo-deb unit-name to match new service file name
- Comment out roles = [...] in auth_verifier.toml and auth_verifier.spire.toml:
  the default sample config should not prescribe domain-specific role names;
  operators define their own list matching their OPA policy

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dow-is-not-defined in CI

React 19 + scheduler@0.27 chains multiple setImmediate callbacks via
performWorkUntilDeadline → schedulePerformWorkUntilDeadline. After the
antd v5 → v6 upgrade a single flush round was insufficient; draining three
rounds covers all known cases.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The no-await-in-loop rule is not enabled in this ESLint config so the
disable directive was flagged as unused with --max-warnings 0.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…service name

The service was renamed from auth_verifier to cosmian_auth_verifier for
naming consistency with cosmian_kms. Update all systemctl/journalctl/
systemd-analyze references in the packaging test workflow accordingly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

3 participants