Skip to content

feat(policy): Implement legacy Policy API (/v3/policies) - #1127

Open
ShJ-code wants to merge 1 commit into
openstack-experimental:mainfrom
ShJ-code:feat/policies-1035-clean
Open

feat(policy): Implement legacy Policy API (/v3/policies)#1127
ShJ-code wants to merge 1 commit into
openstack-experimental:mainfrom
ShJ-code:feat/policies-1035-clean

Conversation

@ShJ-code

Copy link
Copy Markdown
Collaborator

Summary

Implements the deprecated /v3/policies document store, closing one row of the
Python API compatibility matrix (#938). Fixes #1035.

Tempest's test_create_update_delete_policy and test_list_policies pass
against keystone-py but fail against keystone-rs because the endpoint simply
doesn't exist. This adds it as a new policy_store domain.

Naming: the API stores opaque policy documents that remote services fetch
and interpret; it is unrelated to this service's own OPA-based authorization.
Hence policy_store rather than policycore::policy is already the OPA
enforcement client and [api_policy] its config section.

Routes mirror python keystone exactly: GET/POST on the collection,
GET/PATCH/DELETE on the member, and no PUT. Authorization mirrors
keystone/common/policies/policy.py: list/show are admin-or-system-reader,
create/update/delete are admin-only.

Compatibility details carried over from upstream:

  • blob is accepted as a string only, and returned as the stored JSON value
    (rows written by older python releases hold an object, so the response type
    is an unconstrained JSON value rather than string).
  • type is required but may be empty, matching upstream's maxLength-only
    schema.
  • id on create is an unconstrained additional property: accepted in any JSON
    shape and discarded. The service layer is the sole source of the identifier.
  • A PATCH id must equal the path; an empty patch document is a 400.
  • Unknown properties round-trip through extra and are merged on PATCH
    (supplied keys win, omitted stored keys survive). Note this differs from e.g.
    RegionUpdate.extra, which this codebase overwrites wholesale.
  • Additional-property names are normalized (:/-_) on create only,
    matching upstream's _normalize_dict; PATCH does not normalize.

Second commit: shared pagination fixes

fix(api): Fix pagination limit, filters, and rows fixes three defects in the
shared v3/v4 pagination helpers that every paginated collection inherits. They
are bundled here because /v3/policies is the endpoint that surfaced all
three, and the list handler cannot be correct without them.

  1. PaginationQuery::limit defaulted to Some(20), so a request that omits
    ?limit still arrived with one. Config::resolve_list_limit only consults
    configured values when the request carries none, so every per-provider and
    global list_limit was dead config, contrary to the precedence chain ADR
    0029 documents. Now None, matching python keystone's unset
    [DEFAULT] list_limit.
  2. The paginators read query.limit — the raw client value — while the handler
    had fetched resolve_list_limit(...) + 1 rows. A request for limit=100
    against a max_list_limit of 10 returned all 11 fetched rows, exceeding the
    cap, and emitted no next link. Both paginators now resolve the effective
    limit themselves.
  3. build_pagination_link rebuilt the href from pagination controls alone,
    dropping every resource filter, so following the link from
    GET /v3/policies?type=application/json silently widened page two to the
    unfiltered collection. Non-pagination query parameters now carry over
    verbatim.

Separately, a list endpoint that re-checks each item against the per-item read
policy (I8) cannot over-fetch limit + 1 rows just once: if any are filtered
out, the surviving count can fall to limit or below while more visible rows
remain, which the paginator reads as "no next page". collect_authorized_page
keeps pulling batches until limit + 1 authorized items are collected or the
backend is exhausted, bounded at limit * AUTHORIZED_PAGE_SCAN_FACTOR (2) raw
rows per request so a caller with sparse visibility cannot force a full scan.
identity/credential/list adopts it too.

BEHAVIOUR CHANGE — GET /v3/credentials. It previously dropped entries
the caller could not read and returned a short page, ending pagination early.
It now refills from subsequent rows up to the page size. A caller who could
see 3 of 20 credentials used to receive 3 items and no next link; it now
receives a full page and continues paging. Callers relying on the old short
pages will see more entries per response.

When the scan budget stops the refill before the page fills, the page is short
but next is emitted anyway, keyed on the last authorized item, so the caller
keeps paging instead of treating it as the end of the collection. If no
authorized item was found at all, no link is emitted — the only available
marker would name an object the caller may not read — and the truncation is
logged.

Test plan

All green on this branch, rebased onto main (e2f52af0) with no conflicts.

Gate Result
cargo check --workspace clean
cargo clippy (changed crates) no new warnings
cargo fmt --check clean
cargo test -p openstack-keystone --lib policy 138 passed
cargo test — api-types, core, core-types, config, policy-store-driver-sql 967 passed (incl. doctests)
opa test policy/policy/ 15/15; full opa test policy/ clean
cargo nextest run -p test_integration 228/228 passed
cargo nextest run -p test_integration --profile raft 334/334 passed
policy_store integration (CRUD) 11/11 passed
test_apiapi_v3::policy::* (live server) 13/13 passed

New coverage: each of the five handlers has ≥3 unit tests (valid auth,
positive/negative policy, invalid auth) per AGENTS.md, plus 5 rego test files,
11 policy_store provider integration tests, and 13 live-server test_api
tests across create/show/list/update/delete.

Note on unrelated test_api failures in this environment

A full cargo nextest run -p test_api --profile ci-api run on this branch
reports ~30 failures outside api_v3::policy, all 401 Unauthorized cascading
from failed /v3/auth/tokens calls. These reproduce identically on main
without this branch's changes and are environmental, not a regression here:
tools/teardown-api.sh only removes /tmp/nextest/keystone/raft, so
start-api.sh restarts against half-stale state (12s setup instead of a 107s
cold bootstrap) and blanket-401s. Worth a separate issue.

Security review checklist

Required if this diff touches authentication, scope, delegation, tokens,
credentials, EC2, trusts, application credentials, or OPA policy input.

Delete this section entirely if it doesn't apply. See
doc/src/security.md §7 for the full context
behind each item.

  • Does any delegation/authorization decision read the scope
    (project_id, ScopeInfo) where it should read the chain
    (authentication_context(), delegation object)? (I1/I2)
    — No. Policies are global objects with no project binding; no rule reads
    scope or any delegation fact.
  • New delegation-sensitive policy rule? Is the fact it needs projected
    onto Credentials from the chain, and does the rule anchor on
    delegated_project_id + carry the scope-drift tripwire? (I2/I3)
    — N/A by construction. Writes are admin-only, reads admin or
    reader + system == "all". Nothing delegation-sensitive to anchor.
  • New scope shape or redemption path for a delegated auth? Are effective
    roles still bounded by the delegation? Added a test that a restricted
    delegation cannot exceed its roles via the new path? (I4)
    — N/A, no new scope shape or redemption path.
  • New ScopeInfo variant or auth method? Updated
    validate_scope_boundaries(), calculate_effective_roles(),
    fully_resolved(), and Credentials::try_from — all without adding a
    wildcard _ => arm? (I5, Gate J)
    — N/A, neither added.
  • New lookup by a client-derivable key (like sha256(access))? Does it
    re-assert type/shape after fetch? (I6)
    — N/A. Lookup is by server-minted UUID only; a caller-supplied id on
    create is discarded, never used as a key.
  • Does any new data reaching OPA include secrets/decrypted blobs? (I7,
    Gate I)
    — No. blob and extra are arbitrary caller-supplied data with an open
    key space, so to_policy_input() constructs the input from an
    allowlist (id, type) rather than filtering known-bad keys — a
    denylist cannot cover extra. Handlers and the SQL backend use
    skip_all spans so those fields never reach a debug log either.
  • New list/collection endpoint? Does it re-check each item with the
    per-item read policy? (I8)
    — Yes. GET /v3/policies re-enforces identity/policy/show per row.
    A Forbidden drops the item; every other error propagates, so an OPA
    outage fails the request rather than silently returning a truncated
    collection.
  • Does the change let a narrow auth method be broadened by a
    request-supplied scope? (I5)
    — No. No scope is consulted anywhere in this domain.
  • Are there negative tests proving the escape is blocked, not just
    positive tests proving the happy path works?
    — Yes: per-handler forbidden/unauth tests, rego *_test.rego deny cases,
    test_list_omits_items_denied_by_show_policy,
    test_list_propagates_non_forbidden_policy_errors,
    test_list_runs_one_show_check_per_item, and *_policy_input_contract
    tests asserting no secret or caller-controlled payload reaches OPA.
  • Does the test drive ValidatedSecurityContext::new_for_scope()
    end-to-end, not just the inner helper (calculate_effective_roles,
    validate_scope_boundaries) in isolation?
    — Yes. real_policy_decision::* tests drive decisions through a real
    opa run subprocess, and the 13 test_api tests go through the live
    server's full auth path.

Reviewer note on pagination

The pagination refactor touches ~15 list endpoints. Those edits are mechanical
(resolve the effective limit before querying; pass &Uri instead of &str) and
remove no authorization step. The one part worth reading closely is
collect_authorized_page: its internal fetch marker advances over raw
rows, authorized or not, but that value never escapes — the client-facing next
marker is always taken from an authorized item.

@ShJ-code
ShJ-code force-pushed the feat/policies-1035-clean branch from 44dd780 to 2da193b Compare July 31, 2026 03:01
@ShJ-code

Copy link
Copy Markdown
Collaborator Author

I somehow fixed the unsafe pagination pull request and included it as a commit here (514171e). Otherwise the two pagination fixes should behave the same. Additionally I brought the api's actual implementation into the other commit 2da193b, and it should be unsafe-free now.

Add the deprecated `/v3/policies` document store as a new `policy_store`
domain, closing one row of the python compatibility matrix (openstack-experimental#1035).

The API stores opaque policy documents that remote services fetch and
interpret; it is unrelated to this service's OPA-based authorization,
hence `policy_store` rather than `policy` (`core::policy` is the OPA
enforcement client, and `[api_policy]` its config section).

Routes mirror python keystone exactly: GET/POST on the collection and
GET/PATCH/DELETE on the member, with no PUT.

Compatibility details carried over from upstream: `blob` is accepted as
a string only and returned as the stored JSON value; `type` is required
but, matching upstream's `maxLength`-only schema, may be empty; `id` on
create is an unconstrained additional property that is accepted in any
JSON shape and discarded; a PATCH `id` must equal the path; an empty
patch document is a 400; extras round-trip and are merged on PATCH; and
additional-property names are normalized on create only.

Security: `list` re-checks every item with the per-item show policy and
propagates non-Forbidden policy errors (I8). OPA input is built from an
allowlist of `id`/`type` rather than by filtering, because `blob` and
`extra` are arbitrary caller-supplied data with an open key space (I7).
Handlers and the SQL backend use `skip_all` spans so those fields never
reach a debug log.

The service layer is the sole source of the policy id, assigning it
before the audited operation so the event carries the final identifier;
the backend rejects a missing id rather than minting a second one. A
malformed stored `extra` is an error rather than an empty map, since
`update` writes the decoded extras back and would otherwise destroy a
row's properties on an ordinary PATCH.

The new Rego reads `input.credentials.system`, the key `Credentials`
actually emits; several older policies compare against a `system_scope`
key that is never emitted, which is recorded as a known gap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Sihao Jiang <sihao_jiang@outlook.com>
@ShJ-code
ShJ-code force-pushed the feat/policies-1035-clean branch from a89db4e to a2cd694 Compare July 31, 2026 19:00
@ShJ-code

Copy link
Copy Markdown
Collaborator Author

I removed the previous pagination fix and the api implementation is ready for review.

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.

Implement Policy API (/v3/policies)

1 participant