feat: SPOCP-based issuance policy evaluation with dynamic OIDC params - #380
feat: SPOCP-based issuance policy evaluation with dynamic OIDC params#380leifj wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Implements per-credential-scope issuance gating and OIDC authorization-request customization by introducing (1) dynamic parameter propagation from PAR → auth context/session and (2) SPOCP-based post-login policy evaluation of OIDC claims in the OIDC RP callback.
Changes:
- Adds
DynamicParamsto PAR requests, authorization context, and OIDC sessions; forwards them into OIDC auth initiation for template substitution. - Introduces
OIDCRequestParamsandIssuancePolicyscope configuration (plus a cross-data-source lookup helper) to drive OIDC request customization and SPOCP rules. - Adds a new
pkg/issuancepackage with an “advanced-form” S-expression parser and policy engine wrapper; integrates evaluation into the OIDC callback path.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/openid4vci/authoriziation.go | Extends PAR request model with DynamicParams. |
| pkg/model/data_sources.go | Adds per-scope OIDCRequestParams + IssuancePolicy config models and LookupScopePolicyConfig(). |
| pkg/issuance/policy.go | New SPOCP policy engine wrapper + query builder + optional rules file loading. |
| pkg/issuance/policy_test.go | Unit tests for policy engine behavior and query building. |
| pkg/issuance/parser.go | New “advanced form” S-expression parser with star-form support. |
| pkg/issuance/parser_test.go | Unit tests for the advanced S-expression parser. |
| pkg/cache/authcontext_types.go | Adds DynamicParams persistence to AuthorizationContext. |
| internal/apigw/httpserver/endpoints_oauth.go | Forwards per-scope OIDC params and stored dynamic params into OIDC auth initiation (VCI consent flow). |
| internal/apigw/auth_providers/oidcrp/session.go | Persists DynamicParams into the OIDC RP session model. |
| internal/apigw/auth_providers/oidcrp/service.go | Extends OIDC initiation to accept per-scope params + dynamic params; resolves templates into AuthCodeURL options. |
| internal/apigw/apiv1/handlers_oidcrp.go | Evaluates issuance policy after claims retrieval (pre-issuance). |
| internal/apigw/apiv1/handlers_oauth.go | Stores DynamicParams from PAR into the authorization context. |
Comments suppressed due to low confidence (1)
internal/apigw/auth_providers/oidcrp/service.go:152
- InitiateAuth’s signature change is a breaking API change; current repo call sites still use the old signature (e.g., internal/apigw/integration/oidc_integration_test.go calls InitiateAuth(ctx, "pid")). Update those callers (and any external consumers) or provide a backward-compatible wrapper to avoid build failures.
// InitiateAuth initiates an OIDC authentication flow.
// oidcParams and dynamicParams are optional: when non-nil, they customize the
// authorization request (e.g., acr_values, claims parameter, extra scopes).
func (s *Service) InitiateAuth(ctx context.Context, credentialType string, oidcParams *model.OIDCRequestParams, dynamicParams map[string]string) (*AuthRequest, error) {
s.log.Debug("Initiating OIDC auth",
"credential_type", credentialType)
// Create session with state, nonce, and PKCE verifier
session, err := s.createSession(ctx, credentialType)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
71920c4 to
42459b9
Compare
Implement conditional credential issuance using the SPOCP policy engine, allowing authentic source business systems to pass dynamic parameters through the OIDC authorization flow. New components: - pkg/issuance/policy.go: PolicyEngine wrapping spocp.AdaptiveEngine with Evaluate() for checking claims against rules - pkg/issuance/parser.go: Human-readable S-expression parser supporting star forms (wildcard, prefix, suffix, set) - Model types: OIDCRequestParams, IssuancePolicy, ScopePolicyConfig in pkg/model/data_sources.go Integration points: - OIDC RP InitiateAuth/InitiateAuthForVCI accept dynamic params and OIDC request parameters with template resolution - OIDCRPCallback evaluates issuance policy against OIDC claims - PAR handler stores DynamicParams in AuthorizationContext - Consent flow forwards dynamic params to OIDC auth initiation Closes SUNET#379
- Change QueryTemplate from map to ordered []QueryDimension slice to ensure deterministic SPOCP query dimension ordering (positional match) - Sort claim keys in default (no-template) query path - Cache PolicyEngine per scope via sync.Map to avoid per-request parsing - Merge session DynamicParams into policy evaluation claims - Add validation constraints on DynamicParams (max key=64, value=1024) - Fix CustomParams doc: keys are static, only values support templates
Move the duplicated advancedParser implementation from both pkg/issuance/parser.go and pkg/httphelpers/middleware_jwt.go into a new shared pkg/spocputil package. This eliminates 19.8% new-code duplication that was failing the SonarCloud quality gate (threshold: 3%).
Merge TestEvaluate_PrefixStarForm and TestEvaluate_SetStarForm into a single TestEvaluate_StarForms table-driven test to eliminate internal duplication flagged by SonarCloud CPD.
The spocputil parser was missing the bare * → Wildcard and trailing * → Prefix conversions inside tagged lists. This meant rules like (scope *) or (path /api/*) were parsed as plain atoms instead of star forms, causing SPOCP matching failures. Also update test imports to use the exported spocputil.ParseAdvancedSExp.
42459b9 to
142fc3c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
pkg/cache/authcontext_types.go:93
- DynamicParams is persisted to MongoDB as a map (via bson), but MongoDB rejects document keys containing '.' or starting with '$'. The current validation for map keys only enforces printability/length, so a crafted dynamic_params key can cause InsertOne/Update to fail (potential DoS) even if the rest of the request is valid. Tighten key validation to explicitly exclude '.' and '$'.
DynamicParams map[string]string `json:"dynamic_params,omitempty" bson:"dynamic_params,omitempty" validate:"omitempty,dive,keys,max=64,printascii,endkeys,max=1024,printascii"`
pkg/issuance/policy.go:143
- toStringValue falls back to fmt.Sprintf("%v") for non-primitive claim values (e.g., map/slice). For maps this produces non-deterministic output (random key iteration order), which can make SPOCP query construction unstable and cause policy matches to fail unpredictably when claims contain nested objects/arrays. Convert maps/slices deterministically (e.g., by sorting keys and serializing recursively) and cover the common integer/float variants.
func toStringValue(v any) string {
switch val := v.(type) {
case string:
return val
case bool:
if val {
return "true"
}
return "false"
case float64:
return fmt.Sprintf("%g", val)
case int:
return fmt.Sprintf("%d", val)
default:
return fmt.Sprintf("%v", val)
}
pkg/spocputil/parser.go:64
- LoadRulesFromFile uses bufio.Scanner with the default 64K token limit. A long SPOCP rule line will make scanning fail with ErrTooLong, which is hard to diagnose from the current error path and can break startup depending on rule complexity. Increase the scanner buffer/max token size to a reasonable upper bound for policy files.
scanner := bufio.NewScanner(f)
lineNum := 0
internal/apigw/apiv1/handlers_oidcrp.go:222
- Issue #379 acceptance criteria calls for an integration test that exercises the OIDC callback path with a mock OP and verifies pass/fail policy gating. This PR adds unit tests for PolicyEngine/ParseAdvancedSExp, but there’s no integration-level coverage proving that (a) dynamic_params are propagated end-to-end, and (b) a failing SPOCP policy produces the expected hard error in the callback flow.
// Evaluate issuance policy (if configured for this scope).
// This uses SPOCP rules to gate credential issuance on claim values.
// The raw OIDC claims (pre-transformation) are used for policy evaluation
// since the rules reference OIDC claim names, not mapped credential claim names.
if scopeCfg := c.cfg.APIGW.DataSources.LookupScopePolicyConfig(session.CredentialType); scopeCfg != nil && scopeCfg.IssuancePolicy != nil {
policyEngine, policyErr := issuance.GetPolicyEngine(scopeCfg.IssuancePolicy)
if policyErr != nil {
span.SetStatus(codes.Error, policyErr.Error())
return nil, fmt.Errorf("failed to initialize issuance policy engine: %w", policyErr)
}
if policyEngine != nil {
// Merge dynamic params into claims for policy evaluation.
// Dynamic params from the authentic source are available as dimensions;
// OIDC claims take precedence over dynamic params.
policyClaims := maps.Clone(authResp.Claims)
for k, v := range session.DynamicParams {
if _, exists := policyClaims[k]; !exists {
policyClaims[k] = v
}
}
if policyErr := policyEngine.Evaluate(session.CredentialType, policyClaims, scopeCfg.IssuancePolicy.QueryTemplate); policyErr != nil {
c.log.Warn("Issuance policy denied credential",
"credential_type", session.CredentialType,
"subject", authResp.IDToken.Subject,
"error", policyErr)
span.SetStatus(codes.Error, policyErr.Error())
return nil, fmt.Errorf("credential issuance denied: %w", policyErr)
}
c.log.Info("Issuance policy evaluation passed",
"credential_type", session.CredentialType,
"subject", authResp.IDToken.Subject)
}
}
…rams oauth2.AuthCodeOption values are applied by key (last-write-wins), so an operator-configured custom_params entry named e.g. "nonce", "state", or "code_challenge" would silently override the security-critical value already set by BuildAuthorizationURL, breaking PKCE/state/nonce guarantees. Reject reserved parameter names up front instead.
|
Fixed (commit 56637ed): |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
pkg/httphelpers/middleware_jwt.go:415
- File-based SPOCP rules are now loaded via spocputil.LoadRulesFromFile, but (unlike inline rules) they are no longer passed through validateRuleElement. This is a behavior regression: a misordered/malformed rule file can be accepted at startup but never match at runtime, causing unexpected denials that are hard to debug. Consider extending spocputil.LoadRulesFromFile to optionally validate each parsed element (or return elements so callers can validate before adding).
if hasFile {
if err := spocputil.LoadRulesFromFile(engine, cfg.RulesFile); err != nil {
return nil, fmt.Errorf("failed to load SPOCP rules from %s: %w", cfg.RulesFile, err)
}
internal/apigw/auth_providers/oidcrp/service.go:337
- This comment references BuildAuthorizationURL, but the code here constructs the authorization URL inside InitiateAuth via oauth2.Config.AuthCodeURL. Updating the comment avoids confusion when reasoning about which guarantees (state/nonce/PKCE) are being protected from CustomParams overrides.
// reservedOIDCParams are authorization request parameters that CustomParams
// must not be allowed to set, since oauth2.AuthCodeOption values are applied
// by key (last write wins) - letting an operator-configured custom param
// collide with one of these would silently override state/nonce/PKCE
// guarantees set earlier in BuildAuthorizationURL.
pkg/httphelpers/middleware_jwt.go:435
- The doc comment for BuildSPOCPQuery starts with “buildSPOCPQuery”, which doesn’t match the exported function name and can trigger golint/staticcheck warnings (and makes generated docs inconsistent).
// buildSPOCPQuery constructs a SPOCP query S-expression for the current HTTP
// request, service name, and JWT subject:
pkg/cache/authcontext_types.go:93
- AuthorizationContext is persisted to MongoDB, where document/map keys containing '.' or starting with '$' are not allowed. DynamicParams currently allows any printable ASCII key, so an authentic source can cause persistence failures by supplying such keys (DoS) even though validation passes. Tighten key validation to exclude '.' and '$'.
DynamicParams map[string]string `json:"dynamic_params,omitempty" bson:"dynamic_params,omitempty" validate:"omitempty,dive,keys,max=64,printascii,endkeys,max=1024,printascii"`
session.DynamicParams are supplied by the caller in the PAR request
body ("from the authentic source business system", per the doc
comment, but nothing here verifies that) - they aren't asserted by
the OIDC provider. The issuance-policy evaluation was silently
falling back to a dynamic param whenever the OP's actual ID token
claims didn't contain a given key, which let a caller forge any
policy dimension the OP didn't return, undermining "validated
against returned token" semantics and potentially authorizing
issuance it shouldn't.
Policy evaluation now uses only the OIDC-provider-asserted claims.
DynamicParams are still used (correctly) to template outgoing OIDC
request parameters in resolveOIDCRequestParams - that's a separate,
earlier use of the same data that isn't a security decision.
# Conflicts: # pkg/openid4vci/authoriziation.go
|
Pushed another fix + merged latest main: Fixed: `DynamicParams` issuance-policy bypass. The issuance policy was being evaluated against Now the policy is evaluated against the OIDC-provider-asserted claims only. Also merged latest |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
pkg/cache/authcontext_types.go:93
- DynamicParams is persisted as a BSON document (dynamic_params.), but the current key validation allows '.' and '$'. MongoDB forbids keys containing '.' and keys starting with '$', which can cause AuthContext.Save/InsertOne to fail at runtime if a caller provides such keys. Tighten key validation to explicitly exclude these characters to avoid persistence failures / DoS.
// DynamicParams holds key-value parameters provided by the authentic source business system
// at flow initiation time. These values are used for template substitution in OIDC request
// parameters (e.g., acr_values, claims) and are available during issuance policy evaluation.
DynamicParams map[string]string `json:"dynamic_params,omitempty" bson:"dynamic_params,omitempty" validate:"omitempty,dive,keys,max=64,printascii,endkeys,max=1024,printascii"`
internal/apigw/apiv1/handlers_oidcrp.go:214
- Issuance policy evaluation currently ignores session.DynamicParams entirely, so values templated into the outgoing OIDC auth request are never validated against the returned token claims. This makes it impossible to enforce the “dynamic params must be validated against returned claims” requirement described in the issue/PR. A minimal safe approach is to require that for each provided dynamic param key, the OP returns a matching claim value (hard deny on missing/mismatch), and then proceed with SPOCP evaluation using the OP-asserted claims.
// Evaluate against the OIDC-provider-asserted claims only. Do NOT
// fall back to session.DynamicParams here: those are supplied by
// the caller in the PAR request body (nominally "from the
// authentic source business system", but nothing here verifies
// that), not validated by the OP. Letting them silently satisfy a
// missing OIDC claim would let a caller forge any policy
// dimension the OP didn't actually assert, defeating the point of
// gating issuance on the returned token. DynamicParams are still
// used (legitimately) to template the outgoing OIDC request
// parameters in resolveOIDCRequestParams - this is a separate,
// later use of the same data for a security decision.
policyClaims := maps.Clone(authResp.Claims)
pkg/httphelpers/middleware_jwt.go:435
- The doc comment for the exported BuildSPOCPQuery function does not start with the function name, which can trigger golint/staticcheck warnings and makes generated docs inconsistent. Rename the comment header to match the exported identifier.
// buildSPOCPQuery constructs a SPOCP query S-expression for the current HTTP
// request, service name, and JWT subject:
pkg/openid4vci/authoriziation.go:52
- PARRequest.DynamicParams is documented as coming from an “authentic source business system”, but this struct is bound directly from the PAR caller’s request body. This is misleading (and matters for threat modeling), especially since dynamic params influence OIDC request templating and policy decisions later. Reword the comment to describe the actual source of the data.
// DynamicParams holds key-value parameters from the authentic source business system.
// These are used for template substitution in OIDC request parameters and for
// issuance policy evaluation.
|
remove jwt_issuer binary from diff |
|
Since spocp is used both for endpoint access and oidc claims, i would like to harmonize them as much as possible. The goal should be that both endpoint rules and oidc rules can be processed by the same method. |
- Remove the 8.7MB jwt_issuer binary accidentally committed at repo root in e5cb4ac (May 2026). Added /jwt_issuer to .gitignore alongside the other root-level service binaries (issuer, apigw, persistent, registry, verifier) it was missing from, so this can't slip through again. - pkg/httphelpers/middleware_jwt.go: BuildSPOCPQuery's doc comment still started with lowercase "buildSPOCPQuery" even though the function is exported — Copilot flagged this originally and it was acknowledged as a follow-up, but never actually fixed.
|
Addressed both (commit a9cafe6):
Build and |
…ments
- PARRequest.DynamicParams (pkg/openid4vci/authoriziation.go) had no key
validation at all, and AuthorizationContext.DynamicParams
(pkg/cache/authcontext_types.go) only checked length/printascii —
neither rejected keys containing "." or starting with "$", both of
which MongoDB treats specially in field names (nested-path separator
and query-operator prefix respectively). A caller-supplied key like
that would reach AuthorizationContext.Save and fail persistence for
the whole request. Applied the same "safe_key" pattern already used
for Attributes maps elsewhere (alphanumeric/underscore, starting with
a letter) to both structs, validating at the earliest point (PAR
request binding) as well as at the persistence boundary.
- pkg/cache.AuthorizationContext.Validate() builds its own minimal
*validator.Validate (deliberately, since it resolves tag names from
json rather than pkg/helpers.NewValidator's yaml-preferring
resolution), so it doesn't know pkg/helpers' "safe_key" tag.
Registered the same regex locally rather than reusing
pkg/helpers.NewValidator, which would change tag-name resolution
(and therefore validation error messages) for every other field on
this struct.
- Added TestPARRequestDynamicParams_RejectsMongoUnsafeKeys.
- Reworded both DynamicParams doc comments: they said the values come
"from the authentic source business system", but the data is bound
directly from the PAR caller's request body — nothing verifies the
caller's claim about its origin. Also corrected the comment on
AuthorizationContext.DynamicParams, which still claimed the field
feeds issuance policy evaluation after that was deliberately removed
in an earlier commit on this branch.
|
Found and fixed a real bug from Copilot's low-confidence findings (commit 3c41b98): MongoDB-unsafe DynamicParams keys: neither `PARRequest.DynamicParams` (no key validation at all) nor `AuthorizationContext.DynamicParams` (length/printascii only) rejected keys containing `.` or starting with `$` — both special to MongoDB field names (nested-path separator / query-operator prefix respectively). A caller-supplied key like that would reach `AuthorizationContext.Save` and fail persistence for the whole request. Applied the same `safe_key` pattern already used elsewhere in this codebase (alphanumeric/underscore, must start with a letter) to both structs — validated at PAR request binding (earliest point) and again at the persistence boundary. `pkg/cache`'s own validator needed the tag registered locally since it deliberately doesn't share `pkg/helpers.NewValidator` (different tag-name resolution). Added a test. Also reworded both `DynamicParams` doc comments — they said the values come "from the authentic source business system," but the data is bound directly from the PAR caller's request body with nothing verifying that claim, and one comment still said the field feeds issuance policy evaluation after that was deliberately removed in an earlier fix on this branch. (Copilot's other suggestion — evaluate policy against DynamicParams — is literally what my earlier fix removed; that's the point. And the Build, vet, and |
masv3971: "Since spocp is used both for endpoint access and oidc claims, i would like to harmonize them as much as possible. The goal should be that both endpoint rules and oidc rules can be processed by the same method." pkg/httphelpers.SafeEngine and pkg/issuance.PolicyEngine were nearly identical hand-rolled wrappers around *spocp.AdaptiveEngine + a mutex, each with its own copy of rule parsing/loading and (for httphelpers only) rule-shape validation. Extracted the shared machinery into pkg/spocputil (which already held the shared S-expression parser): - spocputil.Engine: the shared engine wrapper (QueryElement, RuleCount, ExportRules). httphelpers.SafeEngine is now a type alias for it, so every existing call site across internal/apigw and internal/verifier that references *httphelpers.SafeEngine keeps compiling unchanged. - spocputil.BuildEngine: parses inline rules + an optional rules file, validating each against a (tag, ordered dimension list) shape when one is given. Both BuildSPOCPEngine (endpoint access, tag "vc") and NewPolicyEngine (issuance policy, tag "credential") now call this instead of duplicating the loop. - spocputil.ValidateRuleElement: generalizes the old httphelpers-only validateRuleElement to an arbitrary tag/dimension list. This also closes an old deferred TODO (a Copilot comment on this same PR, acknowledged as "will add validation as a follow-up if needed") -- issuance policy rules loaded from a file now get the same shape validation inline rules already had. - spocputil.BuildTaggedQuery: generalizes the old fixed 6-argument BuildSPOCPQuery and issuance's BuildQuery into one query builder driven by an ordered dimension list. One real incompatibility surfaced while merging the two: issuance rules author wildcards as an empty dimension, e.g. "(org_id)" (TestEvaluate_WildcardRule, TestEvaluate_MultipleRules), while endpoint-access rules require an explicit "*" atom and reject empty dimensions (TestBuildSPOCPEngine_EmptyRequiredParts). Rather than silently break one domain's existing rule files to match the other, ValidateRuleElement takes a requireValue bool controlling which convention applies -- true for endpoint-access (unchanged behavior), false for issuance (unchanged behavior, now just validated when a QueryTemplate is configured instead of never validated at all). Deliberately did NOT touch pkg/trust.WalletAttestationPolicyEngine, a third, separate SPOCP usage site from an already-merged PR (SUNET#516) that masv3971's comment didn't mention. Added pkg/spocputil/engine_test.go for the new shared code directly. All existing pkg/httphelpers and pkg/issuance SPOCP tests pass unchanged. Regenerated docs/CONFIGURATION.md and updated IssuancePolicy.Rules/RulesFile's doc comments to describe the new validation.
|
Implemented the SPOCP unification (commit fe95fef): `pkg/httphelpers.SafeEngine` and `pkg/issuance.PolicyEngine` were nearly-identical hand-rolled wrappers around `*spocp.AdaptiveEngine` + a mutex, each with its own copy of rule parsing/loading, and only the endpoint side had rule-shape validation. Extracted the shared machinery into `pkg/spocputil` (which already held the shared S-expression parser):
One real incompatibility surfaced merging the two: issuance rules author wildcards as an empty dimension (e.g. `(org_id)`), while endpoint rules require an explicit `*` atom and reject empty dimensions — existing tests on both sides assert their respective convention. Rather than silently break one domain's rule files to match the other, `ValidateRuleElement` takes a `requireValue` bool so each domain keeps its existing, already-shipped rule-authoring convention while sharing everything else. Left `pkg/trust.WalletAttestationPolicyEngine` (a third, separate SPOCP site from an already-merged PR, #516) untouched since your comment named the two you meant. Added dedicated tests for the new shared code (`pkg/spocputil/engine_test.go`) and confirmed every existing `pkg/httphelpers`/`pkg/issuance` SPOCP test passes unchanged. Build, vet, and |
# Conflicts: # docs/CONFIGURATION.md
|
Rebased onto latest |
|



Summary
Implements conditional credential issuance using the SPOCP policy engine, allowing authentic source business systems to pass dynamic parameters through the OIDC authorization flow.
Closes #379
Changes
New:
pkg/issuance/— Policy engine and S-expression parserpolicy.go:PolicyEnginewrappingspocp.AdaptiveEnginewithEvaluate(scope, claims, queryTemplate)for checking OIDC claims against SPOCP rules.BuildQuery()constructs S-expression queries from claims.parser.go: Human-readable S-expression parser (ParseAdvancedSExp) supporting star forms: wildcard(*), prefix(* prefix urn:example:), suffix(* suffix @example.com), set(* set loa3 loa4).policy_test.go+parser_test.go: 27 tests covering simple match/deny, wrong scope, wildcard rules, prefix/suffix/set star forms, multiple rules, missing claims, boolean coercion, query building, and parser edge cases.Model types in
pkg/model/data_sources.goOIDCRequestParams— per-scope OIDC authorization request customization (acr_values, claims, extra_scopes, custom_params)IssuancePolicy— SPOCP rules (inline + file) with query template mappingScopePolicyConfig— combined config structLookupScopePolicyConfig()helper onDataSourcesIntegration
service.go):InitiateAuth/InitiateAuthForVCIacceptOIDCRequestParams+DynamicParams;resolveOIDCRequestParams()buildsoauth2.AuthCodeOptionlist with Go template resolution for dynamic valueshandlers_oidcrp.go): Policy evaluation block after claims retrieval — denied claims return hard errorhandlers_oauth.go): StoresDynamicParamsfromPARRequestintoAuthorizationContextendpoints_oauth.go): Forwards dynamic params fromAuthorizationContexttoInitiateAuthForVCIDynamicParamsfield added to OIDC session andAuthorizationContextConfiguration Example