Add ADR 0026: OAuth2/OIDC Provider (Keystone as IdP)

[gtema]

Documents Keystone acting as an OAuth2 Authorization Server / OpenID
Connect Provider for external relying parties, complementing the
existing OIDC/SAML consumer-side federation stack (ADR 0006/0007/
0008/0013/0020). Covers issuer model, a new signed-JWT token track,
OAuth2Client registration, claims mapping via the existing Unified
Mapping Engine interpolation primitive, grant types, and the
login/consent flow.

[gtema]

General Review Summary

This is an excellent and comprehensive ADR that addresses a significant product gap. The architectural decisions are well-justified, security considerations are thorough, and the reuse of existing subsystems (key-repository, mapping engine, revocation events) is commendable.

Overall Assessment: Ready for approval with minor clarifications

Strengths

• ✅ Comprehensive scope covering crypto, client model, grant types, and integration
• ✅ Strong threat model addressing malicious RPs, network attackers, and key compromise
• ✅ Excellent reuse of existing reviewed subsystems
• ✅ Clear decision table summarizing key architectural choices
• ✅ Proper RFC references and code examples
• ✅ Follows established ADR patterns from the codebase

Key Clarifications Needed

The main point that needs resolution is the relationship between ADR 0020's oauth2:client index and this new ADR. As I've analyzed:

ADR 0020's index:oauth2:client:<client_id> was intended for Keystone acting as RP/SP (Relying Party/Service Provider) - i.e., Keystone's own client credentials when connecting to external OIDC providers (like Okta, Keycloak). This is the incoming federation direction.

ADR 0026's OAuth2Client is for Keystone acting as OP/IdP (OpenID Provider/Identity Provider) - i.e., external relying parties (like Grafana, ArgoCD) registered with Keystone. This is the outgoing token issuance direction.

These are completely different concepts and the ADR 0020 reference is indeed a stale forward-reference with no backing implementation.

Action needed: Either update ADR 0020 to remove/annotate the stale oauth2:client index, or add a concrete action item in this ADR to do so.

[gtema]

Inline Suggestions

1. Title - Suggested Clarification

The title "OAuth2 / OpenID Connect Provider (Keystone as IdP)" is good, but consider adding "Keystone as OP" for completeness since OIDC Provider is synonymous with OAuth2 Authorization Server + OpenID Connect Provider.

Suggestion: "OAuth2 / OpenID Connect Provider (Keystone as IdP/OP)"

---

2. §1 Context - Define CADF

First mention of CADF should include its expansion.

Current: "a CADF audit event is emitted (ADR 0023)"
Suggestion: "a Cloud Audit Data Federation (CADF) audit event is emitted (ADR 0023)"

---

3. §3 Token Crypto - Justify 5-minute Lifetime

The 5-minute default for id_token/access_token seems short compared to typical OIDC implementations (usually 1 hour for id_token).

Suggestion: Add justification: "Lifetimes: access_token/id_token default to 5 minutes (short because neither is individually revocable once issued—revocation acts on the refresh token and underlying user/role state, per §7)."

---

4. §4 Client Model - Clarify Issuer URL Format

The global issuer uses /v4/oauth2 while domain-scoped uses /v4/oauth2/{domain_id}.

Question: Could /v4/oauth2 (global) create routing ambiguity? Consider using /v4/oauth2/global for explicit clarity.

---

5. §4 - Resolving ADR 0020 Reference

The current text correctly identifies this as a stale forward-reference, but should be more explicit about the distinction:

Current: "ADR 0020 §8's keyspace table lists index:oauth2:client:<client_id> → {domain_id, provider_id} with no backing struct anywhere in the codebase."

Suggestion: "ADR 0020 §8's keyspace table lists index:oauth2:client:<client_id> → {domain_id, provider_id} with no backing struct anywhere in the codebase. Importantly, ADR 0020's oauth2:client index was intended for Keystone-as-RP (tracking Keystone's own client credentials at external IdPs), whereas this ADR's OAuth2Client is for Keystone-as-OP (tracking external relying parties registered with Keystone). These are distinct concepts. Per interview with the ADR author, the ADR 0020 entry is a stale forward-reference that should be removed or annotated as superseded the next time ADR 0020 is revised."

---

6. §6 Scopes - Clarify Validation Timing

The ADR mentions scope validation but doesn't specify when it occurs.

Suggestion: Add: "An OAuth2Client.allowed_scopes allowlist bounds which scopes a client may request, validated both at /authorize time and at token issuance (defense in depth)."

---

7. §7 Login/Consent - Clarify UI Implementation

The ADR mentions "server-rendered HTML login form" but doesn't specify the technology.

Suggestion: Add: "...serves a minimal, server-rendered HTML login form (using Rust templating crate, implementation detail TBD)..."

---

8. Editorial - Consistent Version References

The ADR mixes "v1" and "Version 1". Consider standardizing on one format.

Examples to update:

• "Client registration in v1" → "Client registration in Version 1"
• "Grant types (v1)" → "Grant types (Version 1)"
• "Administrative API (v1)" → "Administrative API (Version 1)"

---

9. §8 Integration - Rate Limiting Clarification

The ADR mentions "per-client" rate limiting but doesn't specify the identifier for public clients.

Suggestion: Clarify: "/authorize, /token, and /device_authorization are subject to the same per-source-IP and per-client (client_id for confidential clients, IP-only for public clients) token-bucket limiting..."

[gtema]

🎯 Final Assessment: Ready for Approval

Status: Approve with minor clarifications

This ADR is substantively excellent and addresses a critical product gap. The architectural decisions are well-justified, security considerations are thorough, and the reuse of existing subsystems demonstrates good engineering practice.

What's Great

• ✅ Comprehensive coverage of all aspects (crypto, client model, grant types, flows, integration)
• ✅ Strong threat model with specific mitigations
• ✅ Excellent reuse of reviewed subsystems (key-repository, mapping engine, revocation)
• ✅ Clear decision table and RFC references
• ✅ Follows established ADR patterns

What Needs Attention

The clarifications I've suggested are all non-blocking and editorial in nature:

1. Critical: Clarify the ADR 0020 oauth2:client index distinction (RP vs OP) - this is the most important point
2. Important: Define CADF on first use
3. Helpful: Justify the 5-minute token lifetime default
4. Minor: Standardize version references (v1 vs Version 1)
5. Minor: Clarify issuer URL format, UI implementation, rate limiting details

Recommendation

Approve this PR once the author addresses the clarifications, particularly the ADR 0020 reference explanation. The ADR is ready for merge from a technical and architectural standpoint.

---

To the author: Thank you for this excellent ADR! The clarifications are minor and mostly editorial. The one substantive point is making the RP vs OP distinction crystal clear in the ADR 0020 reference section.

[gtema]

📝 Inline Comment Suggestions

Below are specific line-by-line suggestions for improvements. Each can be addressed with a simple text change.

---

Line 1: Title

Current: # 26. OAuth2 / OpenID Connect Provider (Keystone as IdP)
Suggestion: Consider adding OP for completeness: # 26. OAuth2 / OpenID Connect Provider (Keystone as IdP/OP)
Reason: OIDC Provider = Authorization Server + OP, so including both terms is more precise.

---

Line 412: Define CADF Acronym

Current: immediately, and a CADF audit event is emitted (ADR 0023).
Suggestion: immediately, and a Cloud Audit Data Federation (CADF) audit event is emitted (ADR 0023).
Reason: First use of acronym should be expanded.

---

Line 195-196: Justify Token Lifetime

Current:

Lifetimes: `access_token`/`id_token` default to 5 minutes (short, because
neither is individually revocable once issued � revocation acts on the

Suggestion:

Lifetimes: `access_token`/`id_token` default to 5 minutes (short, because
neither is individually revocable once issued � revocation acts on the
refresh token and on the underlying Keystone user/role state, per �7).

Reason: The current text is cut off mid-sentence. Complete the justification.

---

Line 235-238: Clarify ADR 0020 Reference (CRITICAL)

Current:

ADR 0020 �8's keyspace table lists `index:oauth2:client:<client_id>` �
`{"domain_id": "...", "provider_id": "..."}` with no backing struct anywhere in
the codebase. Per interview with the ADR author, this ADR does **not** claim
that index � it is flagged here as a stale forward-reference in ADR 0020 that
should be removed or annotated as superseded the next time ADR 0020 is revised.

Suggestion:

ADR 0020 �8's keyspace table lists `index:oauth2:client:<client_id>` �
`{"domain_id": "...", "provider_id": "..."}` with no backing struct anywhere in
the codebase. **Importantly, ADR 0020's `oauth2:client` index was intended for
Keystone-as-RP (tracking Keystone's own client credentials at external IdPs),
whereas this ADR's `OAuth2Client` is for Keystone-as-OP (tracking external
relying parties registered with Keystone). These are distinct concepts.** Per
interview with the ADR author, the ADR 0020 entry is a stale forward-reference
that should be removed or annotated as superseded the next time ADR 0020 is
revised. This ADR's own storage coordinate (SQL table `oauth2_client`, or the
FjallDB key `data:oauth2_client:<domain_id>:<client_id>`) is independent of
that entry.

Reason: This is the most important clarification - makes the RP vs OP distinction crystal clear.

---

Line 220: Clarify Scope Validation

Current: pub allowed_scopes: Vec<String>, // allowlist this client may request (�6)
Suggestion: pub allowed_scopes: Vec<String>, // allowlist this client may request, validated at /authorize and token issuance (�6)
Reason: Clarifies when validation occurs.

---

Line 326: Clarify Scope Validation Timing

Current: An OAuth2Client.allowed_scopes allowlist bounds which of these a given
Suggestion: An OAuth2Client.allowed_scopesallowlist bounds which of these a given client may request, **validated both at/authorize time and at token issuance** (defense in depth against a scope smuggled in a step where the two checks disagree).
Reason: Explicitly states validation timing.

---

Line 269: Clarify Issuer URL

Current: GET /v4/oauth2/{domain_id}/.well-known/openid-configuration
Question: Should the global issuer use /v4/oauth2/.well-known/... or /v4/oauth2/global/.well-known/... to avoid ambiguity with domain-scoped paths?
Reason: Potential routing ambiguity between /v4/oauth2 (global) and /v4/oauth2/{domain_id} (domain-scoped).

---

Line 350-355: Clarify UI Implementation

Current: serves a minimal, server-rendered HTML login form (username/password,...
Suggestion: Add: (using Rust templating crate, implementation detail TBD)
Reason: Clarifies the technology stack for the minimal UI.

---

Multiple Lines: Standardize Version References

Issue: The ADR mixes "v1" and "Version 1" inconsistently.
Suggestions:

• Line 63: "Client registration in v1" → "Client registration in Version 1"
• Line 109: "Grant types (v1)" → "Grant types (Version 1)"
• Line 113: "Administrative API (v1)" → "Administrative API (Version 1)"
  Reason: Consistency in terminology.

---

Line 405: Clarify Rate Limiting for Public Clients

Current: "per-source-IP and per-client token-bucket limiting"
Suggestion: "per-source-IP and per-client (client_id for confidential clients, IP-only for public clients) token-bucket limiting"
Reason: Clarifies how public clients (no client_secret) are rate-limited.

---

✅ Summary

All of these suggestions are non-blocking and editorial in nature. The most important one is Line 235-238 (ADR 0020 reference clarification) as it addresses the RP vs OP distinction you correctly identified.

The ADR is technically sound and ready for approval once these clarifications are addressed.