-
Notifications
You must be signed in to change notification settings - Fork 3
feat(pep): integrate with real Nuts node for token introspection and DPoP #362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…DPoP Replace mock token handling with real Nuts node integration: - Token introspection via RFC 7662 endpoint - DPoP token binding validation via RFC 9449 - Support both Bearer and DPoP authorization schemes - Extract claims from introspection response based on Presentation Definition Add comprehensive e2e test with real Nuts node: - Issues X509Credential, NutsEmployeeCredential, and HealthcareProviderRoleTypeCredential - Tests Bearer token authorization flow - Tests DPoP token authorization flow with proof creation - Tests consent denial scenario via mock MITZ Clean up unused code and configuration: - Remove mock introspection from authorize.js - Remove unused NGINX proxy locations (use ngx.fetch instead) - Move Presentation Definition to test-only location - Update helm values for production deployment
On Linux (GitHub Actions), NGINX's resolver directive doesn't check /etc/hosts, only DNS. Add dnsmasq to serve /etc/hosts entries via DNS, allowing NGINX to resolve host.docker.internal added via --add-host. Changes: - Install dnsmasq in PEP container - Add entrypoint script that starts dnsmasq before nginx - Set DNS_RESOLVER default to 127.0.0.1 (dnsmasq) - Simplify harness by removing platform-specific DNS resolver logic
Replace ngx.fetch with nginx subrequests for token introspection, DPoP validation, and PDP calls. This fixes DNS resolution issues in GitHub Actions where host.docker.internal couldn't be resolved at runtime. - Add upstream definitions for nuts_node and knooppunt_pdp - Add internal proxy locations with explicit timeouts (5s/10s/10s) - Remove DNS_RESOLVER env var and custom entrypoint script - Update error codes from 503 to 502 for gateway errors - Remove sensitive debug logging (token/DPoP payload)
- Add safeDecode wrapper for decodeURIComponent to handle malformed percent-encoding gracefully instead of throwing - Validate PDP response schema (result.allow must be boolean) - Add security headers: X-Content-Type-Options, Cache-Control - Add tests for malformed input handling
The patient_bsn field in PDPContext was dead code: - PEP always sent empty string - PDP/PIP derives BSN from FHIR request via PatientID lookup The BSN is now exclusively derived by the PIP from the Patient resource, based on PatientID extracted from the FHIR request URL.
- Fix credential name: HealthcareProviderRoleTypeCredential (not Type) - Clarify PD constraint IDs are for PDP/Mitz, not PEP requirements - Fix e2e description: Knooppunt runs in-process, not as container - Remove redundant prerequisites from test comment
Resolved conflicts: - test/e2e/pdp/bgz_test.go: Keep _id param (for patient lookup) and add header - component/pdp/fhirreq.go: Remove PatientBSN copy (field removed from PDPContext)
This reverts commit 93cc43f.
…checking FHIR capability checking was incorrectly skipped for GET requests because they don't have Content-Type headers. The check should be based on whether the request targets a FHIR resource type, not the Content-Type header. Changes: - Check Resource.Type == nil instead of Content-Type != "application/fhir+json" - Make Resource.Type a pointer to distinguish "no type" from "Account" (0) - Update tests to use to.Ptr() for Resource.Type
There was a problem hiding this comment.
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 integrates the PEP (Policy Enforcement Point) with a real Nuts node for OAuth token introspection (RFC 7662) and DPoP token binding validation (RFC 9449). It replaces the mock token introspection with real credential validation and implements dynamic claim forwarding from Presentation Definitions to the PDP.
Changes:
- PEP now introspects tokens via Nuts node
/internal/auth/v2/accesstoken/introspectand validates DPoP proofs - Dynamic claim extraction: filters standard OAuth/JWT/OIDC claims, forwards all Presentation Definition-defined claims to PDP
- Comprehensive e2e tests with real X509Credential, NutsEmployeeCredential, and HealthcareProviderRoleTypeCredential issuance
- Updated PDP to accept nullable ResourceType for non-FHIR requests
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pep/nginx/js/authorize.js | Complete rewrite: token introspection via Nuts node, DPoP validation, dynamic claim forwarding (no hardcoded mappings) |
| pep/nginx/js/authorize.test.js | 56 unit tests covering token extraction, claim filtering, DPoP validation, PDP request building |
| pep/nginx/conf.d/knooppunt.conf | Added upstream for Nuts node, internal locations for introspection and DPoP validation |
| pep/nginx/nginx.conf | Added NUTS_NODE_HOST and NUTS_NODE_INTERNAL_PORT env vars |
| pep/nginx/Dockerfile | Added Nuts node connection env vars, removed hardcoded claim env vars |
| pep/README.md | Comprehensive update: claim flow diagram, PD constraint ID mapping, DPoP validation |
| test/e2e/pep/authorization_test.go | New comprehensive e2e test: starts real Nuts node + HAPI + Knooppunt, issues credentials, tests Bearer/DPoP flows |
| test/e2e/pep/testdata/ | Test certificates (UZI), Presentation Definitions for discovery and access policy |
| test/e2e/pep/certs/ | Shell scripts to generate test CA and issue UZI certificates |
| test/e2e/harness/pep.go | StartPEPContainer now returns container handle for log access |
| test/e2e/harness/entrypoint.go | Removed old StartPEP harness (no longer used) |
| component/pdp/shared.go | Changed PolicyResource.Type from value to pointer for nil support |
| component/pdp/fhirreq.go | Updated to handle nil ResourceType, moved PatientBSN assignment |
| component/pdp/capabilities.go | Added nil check for ResourceType before capability checking |
| component/pdp/capabilities_test.go | Updated all test cases to use pointer for ResourceType |
| helm/pep/values.yaml | Added Nuts node configuration, removed hardcoded claim env vars |
| helm/pep/templates/deployment.yaml | Added Nuts node env vars to deployment |
| docker-compose.yml | Updated PEP service with Nuts node connection |
| test/e2e/pdp/bgz_test.go | Updated test to reflect empty patient_bsn in context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
copilot feedback Co-authored-by: Copilot <[email protected]>
- Replace ASCII diagram with link to dataexchange-authorization-sd.puml - Add DPoP validation endpoint to requirements - Add considerations for production deployments
- Use FHIR_BASE_PATH in nginx location and njs prefix stripping - Document single-tenant limitation in README
Forward request body (e.g., FHIR _search form data) to the PDP for authorization decisions. Also fix createMockRequest helper to properly deep merge variables and sync method with request_method.
Add FHIR_UPSTREAM_PATH env var for the backend FHIR server path, separate from FHIR_BASE_PATH which is the incoming path clients use. This allows HAPI multi-tenancy (e.g., /fhir/DEFAULT) while exposing a simpler path (/fhir) to clients. FHIR_UPSTREAM_PATH defaults to /fhir (same as FHIR_BASE_PATH).
There was a problem hiding this comment.
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 27 out of 27 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add configurable PEP_HOSTNAME env var for DPoP URL construction. Prevents attackers from spoofing Host header to bypass htu binding. Falls back to Host header for backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, but after that lets get this merged in the sake of progress and future iterative improvements.
Preserve numbers and booleans from introspection response instead of converting them to strings. This ensures the PEP passes through claim values faithfully, allowing proper type handling in the PDP.
- Switch PEP e2e test from standalone Nuts container to embedded Nuts node - Add StartPEP() harness function with proper Nuts node setup - Enhance startKnooppunt to handle Nuts node readiness when enabled - Add /nuts prefix to Nuts API paths in nginx config (Knooppunt mounts Nuts APIs at /nuts) - Simplify authorization_test.go from ~536 to ~290 lines by using harness - Expose NutsPublicURL in PEPDetails for OAuth authorization server URL
Summary
Changes
PEP Authorization (
pep/nginx/js/authorize.js)/internal/auth/v2/accesstoken/introspect/internal/auth/v2/dpop/validatewhencnf.jktclaim presentE2E Tests (
test/e2e/pep/)Test Coverage
Test plan
cd pep/nginx/js && npm test)go test ./test/e2e/pep/... -v)