Add JWT-validation for idporten/maskinporten #589
Conversation
…specify how incoming JWT's should be authenticated. This definition will create a RequestAuthentication that validates incoming requests
…injection of audience
…d workloads made from digdirator. Also setting up AuthorizationPolicy to enforce Bearer-token in requests
…and AuthPolicy from JWT-validation to a common AuthPolicy
WalkthroughThe pull request introduces comprehensive enhancements to authentication and authorization mechanisms within the Skiperator project. The changes primarily focus on adding JWT (JSON Web Token) validation capabilities for both IDPorten and Maskinporten identity providers. New structs, methods, and configurations have been added to support dynamic JWT authentication, allowing applications to specify token validation rules, claim forwarding, and path-based access controls. The modifications span across multiple packages, including API definitions, controllers, resource generators, and test configurations. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Controller as ApplicationController
participant AuthConfig as AuthConfigGenerator
participant Istio as IstioResourceGenerator
participant K8s as Kubernetes
App->>Controller: Reconcile Request
Controller->>AuthConfig: Get Authentication Configs
AuthConfig-->>Controller: Return Auth Configurations
Controller->>Istio: Generate RequestAuthentication
Istio->>K8s: Create/Update Istio Resources
K8s-->>Istio: Confirm Resource Creation
Controller->>Istio: Generate AuthorizationPolicy
Istio->>K8s: Create/Update Authorization Policies
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
larsore
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
tests/application/authorization-settings/patch-application-assert.yaml (1)
Line range hint
5-18: Ensure comprehensive path protection.The policy explicitly allows
/actuator/infoand/actuator/shutdown. The inclusion of/actuator/shutdownendpoint could pose a security risk if not properly protected.Consider removing the shutdown endpoint or adding additional authentication requirements:
paths: - /actuator/info - - /actuator/shutdown
🧹 Nitpick comments (19)
pkg/resourcegenerator/istio/authorizationpolicy/authorization_policy.go (4)
35-35: Single default deny pathDefining
defaultDenyPath := []string{"/actuator*"}is clear but be sure it covers all paths you want restricted. Some applications might host other sensitive endpoints.
45-66: AppendingNotPathstoallowPathsIn this logic,
NotPathsfrom each AuthConfig get appended toallowPaths. Consider whether these sets of paths should be distinct to avoid confusion in subsequent policy logic.
68-95: Conditional creation of ALLOW vs. DENY AuthorizationPolicyThe approach of creating an ALLOW policy if
allowPathsis non-empty, and a DENY policy otherwise, is consistent with the intended “default deny” strategy. Just ensure it aligns with your application’s security stance—particularly for partial matches on"/actuator*".
138-171: Potential confusion in slice append fornotPathsLine 141 sets
notPaths := allowPaths, then line 142 usesappend(allowPaths, denyPaths...). This works but can be more intuitive by appending tonotPathsdirectly (e.g.,notPaths = append(notPaths, denyPaths...)).internal/controllers/common/reconciler.go (1)
276-317: Typo in error messageIn the
MASKINPORTENbranch (line 303), the error text still references “IDPortenClient: '%s' not found.” This can cause confusion for troubleshooting. Consider correcting it to “MaskinPortenClient.”- err := fmt.Errorf("IDPortenClient: '%s' not found", namespacedName.String()) + err := fmt.Errorf("MaskinPortenClient: '%s' not found", namespacedName.String())pkg/testutil/reconciliation.go (1)
30-30: Enhance test utility with authentication scenarios.The test utility should demonstrate proper usage of the new authentication features instead of passing nil.
Consider adding test cases for authentication:
- r := reconciliation.NewApplicationReconciliation(ctx, application, log.NewLogger(), false, nil, &identityConfigMap, nil) + authConfig := &istiotypes.Authentication{ + Enabled: true, + TokenLocation: "header", + ForwardOriginalToken: true, + } + r := reconciliation.NewApplicationReconciliation(ctx, application, log.NewLogger(), false, nil, &identityConfigMap, authConfig)pkg/reconciliation/reconciliation.go (1)
58-59: Use iota for IdentityProvider constants.Consider using
iotafor theIdentityProviderconstants to ensure uniqueness and make it easier to add new providers in the future.-var ( - MASKINPORTEN IdentityProvider = "MASKINPORTEN" - ID_PORTEN IdentityProvider = "ID_PORTEN" -) +const ( + MASKINPORTEN IdentityProvider = iota + ID_PORTEN )api/v1alpha1/digdirator/idporten.go (1)
82-83: Add kubebuilder validation markers for Authentication field.Consider adding kubebuilder validation markers to enforce validation rules for the Authentication field, similar to other fields in the struct.
// Authentication specifies how incoming JWT's should be validated. +// +kubebuilder:validation:Optional Authentication *istiotypes.Authentication `json:"authentication,omitempty"`tests/application/authorization-policy/application-assert.yaml (1)
4-4: LGTM! Naming changes improve clarity.The renaming from 'authorization-policy-deny' to 'application-default-deny' better describes the policy's purpose, and the selector label update maintains consistency with the application renaming.
Note: Add a newline at the end of the file.
Also applies to: 18-18
tests/application/authorization-policy/chainsaw-test.yaml (2)
Line range hint
4-4: Update test name for consistency.The test name should be updated to match the new policy name pattern.
- name: authorization-policy + name: application-default-deny
Line range hint
20-22: Fix formatting.Remove extra newlines at the end of the file, keeping just one.
tests/application/idporten/application-assert.yaml (1)
4-4: LGTM! Consistent renaming across deployment resources.All references to 'idporten-test-client' have been properly updated to 'application', including deployment name, labels, container name, and volume/secret references.
Note: Add a newline at the end of the file.
Also applies to: 8-8, 12-12, 17-17, 21-21, 24-24
tests/application/maskinporten/application.yaml (1)
53-71: Review security of Secret resources.The Secrets contain sensitive Maskinporten credentials. Consider:
- Using external secret management (e.g., HashiCorp Vault)
- Adding metadata labels for better secret management
- Implementing secret rotation mechanisms
metadata: name: "maskinporten-application-auth-6ef1ddb7" + labels: + app.kubernetes.io/component: auth + secret-rotation: enabled🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 71-71: no new line character at the end of file
(new-line-at-end-of-file)
tests/application/maskinporten/application-maskinporten-auth-assert.yaml (1)
6-14: Enhance JWT validation configuration.The JWT validation configuration looks good but consider adding:
fromHeadersspecification for explicit JWT locationjwksUritimeout and caching settings- Rate limiting for JWKS endpoint requests
jwtRules: - audiences: - test-client-id-maskinporten forwardOriginalToken: true issuer: https://maskinporten.no/ jwksUri: https://maskinporten.no/jwk + fromHeaders: + - name: Authorization + prefix: Bearer + jwtParams: ["access_token"] outputClaimToHeaders: - claim: aud header: Audiencetests/application/idporten/application-idporten-auth-assert.yaml (2)
26-38: Enhance JWT claim validation in authorization policy.While the policy correctly validates the issuer claim, consider adding additional claim validations for enhanced security:
nbf(Not Before) claimexp(Expiration Time) claimsub(Subject) claim if applicablewhen: - key: request.auth.claims[iss] values: - https://idporten.no + - key: request.auth.claims[exp] + values: + - "*" + - key: request.auth.claims[nbf] + values: + - "*"
74-77: Remove redundant path exclusion.The
/actuator*exclusion is redundant and potentially confusing when combined with specific actuator path exclusions. Consider removing the generic exclusion since specific paths are already excluded.notPaths: - /actuator/health - /actuator/info - - /actuator*tests/application/idporten/application.yaml (1)
30-35: Consider adding security-related claim mappings.While mapping the "aud" claim is good, consider mapping additional security-relevant claims to headers for enhanced logging and debugging:
subclaim for subject identificationexpclaim for token expiration trackingoutputClaimToHeaders: - claim: "aud" header: "Audience" + - claim: "sub" + header: "X-User-ID" + - claim: "exp" + header: "X-Token-Expiry"config/crd/skiperator.kartverket.no_applications.yaml (1)
556-619: Comprehensive JWT validation configuration for IDPorten.The authentication block provides a well-structured configuration for JWT validation with appropriate security controls:
- Required
enabledflag for explicit opt-in- Flexible token location support (header/cookie)
- Path-based authentication bypass with safety check (must start with '/')
- Secure claim-to-header mapping
However, consider the following security recommendations:
- Add validation to ensure
ignorePathsdoesn't contain sensitive endpoints- Consider adding a maximum length for header names in
outputClaimToHeaderstests/application/maskinporten/application-assert.yaml (1)
4-4: LGTM! Consistent resource renaming.The renaming from
maskinporten-test-clienttoapplicationis applied consistently across all resources:
- Deployment name
- Selector labels
- Container name
- Volume mounts
- Secret references
Add newline at end of file.
Add a newline character at the end of the file to comply with YAML best practices.
secretName: "maskinporten-application-07ab8a18" +Also applies to: 8-8, 12-12, 17-17, 21-21, 24-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
api/v1alpha1/digdirator/idporten.go(2 hunks)api/v1alpha1/digdirator/maskinporten.go(2 hunks)api/v1alpha1/digdirator/zz_generated.deepcopy.go(3 hunks)api/v1alpha1/istiotypes/jwt_authentication.go(1 hunks)api/v1alpha1/istiotypes/zz_generated.deepcopy.go(1 hunks)config/crd/skiperator.kartverket.no_applications.yaml(2 hunks)internal/controllers/application.go(4 hunks)internal/controllers/common/reconciler.go(2 hunks)internal/controllers/common/util_test.go(0 hunks)pkg/reconciliation/application.go(2 hunks)pkg/reconciliation/reconciliation.go(3 hunks)pkg/resourcegenerator/istio/authorizationpolicy/authorization_policy.go(2 hunks)pkg/resourcegenerator/istio/requestauthentication/request_authentication.go(1 hunks)pkg/resourceprocessor/diffs_test.go(1 hunks)pkg/testutil/reconciliation.go(1 hunks)pkg/util/helperfunctions.go(2 hunks)tests/application/authorization-policy/application-assert.yaml(2 hunks)tests/application/authorization-policy/application.yaml(1 hunks)tests/application/authorization-policy/chainsaw-test.yaml(1 hunks)tests/application/authorization-settings/multiple-application-assert.yaml(2 hunks)tests/application/authorization-settings/multiple-application-errors.yaml(1 hunks)tests/application/authorization-settings/patch-application-assert.yaml(2 hunks)tests/application/authorization-settings/patch-application-errors.yaml(1 hunks)tests/application/idporten/application-assert.yaml(1 hunks)tests/application/idporten/application-idporten-assert.yaml(1 hunks)tests/application/idporten/application-idporten-auth-assert.yaml(1 hunks)tests/application/idporten/application.yaml(2 hunks)tests/application/idporten/chainsaw-test.yaml(1 hunks)tests/application/maskinporten/application-assert.yaml(1 hunks)tests/application/maskinporten/application-maskinporten-assert.yaml(1 hunks)tests/application/maskinporten/application-maskinporten-auth-assert.yaml(1 hunks)tests/application/maskinporten/application.yaml(1 hunks)tests/application/maskinporten/chainsaw-test.yaml(1 hunks)tests/application/subresource-status/application-resource-apply-error-assert.yaml(1 hunks)tests/application/subresource-status/application-synced-assert.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- internal/controllers/common/util_test.go
✅ Files skipped from review due to trivial changes (4)
- tests/application/authorization-policy/application.yaml
- tests/application/authorization-settings/patch-application-errors.yaml
- tests/application/idporten/application-idporten-assert.yaml
- tests/application/maskinporten/application-maskinporten-assert.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/application/authorization-policy/application-assert.yaml
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
tests/application/maskinporten/application-maskinporten-auth-assert.yaml
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
tests/application/idporten/application.yaml
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
tests/application/maskinporten/application.yaml
[error] 71-71: no new line character at the end of file
(new-line-at-end-of-file)
tests/application/idporten/application-assert.yaml
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
tests/application/maskinporten/application-assert.yaml
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (33)
pkg/resourcegenerator/istio/authorizationpolicy/authorization_policy.go (5)
9-9: Imports appear correct for v1beta1 and typesThe newly added imports for Istio’s
v1beta1package and Kubernetestypesare necessary for your new code references. Everything looks consistent here.Also applies to: 13-13
30-30: Early return whenAllowAllis trueSkipping AuthorizationPolicy generation when
AllowAllis set makes sense, but double-check if you want to handle special endpoints (e.g.,/actuator) differently in such a scenario.
37-37: Initialization ofallowPathsInitializing and then assigning
allowPathsfromAllowListis a straightforward approach. Carefully verify that none of the paths inAllowListoverlap undesirably with paths indefaultDenyPath.Also applies to: 39-41
100-126:getGeneralAuthPolicyfunction clarityThe structure for constructing
AuthorizationPolicyingetGeneralAuthPolicyis understandable. The explicit setting ofPathsandNotPathsis good, but watch for potential collisions if these lists have overlapping patterns.
128-136:getGeneralFromRulerestricts traffic sourceThis function restricts traffic to the
istio-gatewaysnamespace. Ensure this meets your requirements if there’s internal, namespace-to-namespace communication that also needs to be allowed.internal/controllers/common/reconciler.go (2)
146-197:GetAuthConfigsForApplicationLogic for retrieving and assembling
AuthConfigfrom secrets looks solid. Error handling for missing data keys is handled gracefully. Confirm that you have test coverage for cases where secrets are incomplete or incorrectly named.
199-274:getIdentityProviderInfoWithAuthenticationEnabledApproach to require either “Enabled” providers or an explicit
secretNameis sensible. The error messages guide the user to correct configuration. This effectively enforces your JWT requirements.pkg/reconciliation/application.go (1)
Line range hint
17-26: ExtendedNewApplicationReconciliationsignatureThe additional
authConfigsparameter seamlessly integrates into yourbaseReconciliationflow. This supports the new JWT validation logic without breaking existing usage.api/v1alpha1/digdirator/maskinporten.go (1)
21-23: LGTM! Well-structured authentication field addition.The Authentication field is properly documented and correctly implemented as an optional pointer field.
api/v1alpha1/istiotypes/jwt_authentication.go (1)
16-17: Define explicit default for ForwardOriginalToken.While the comment states the default is true, it's better to enforce this in the CRD validation.
Add the following kubebuilder marker to set the default value:
// If set to true, the original token will be kept for the upstream request. Default is true. +// +kubebuilder:default=true ForwardOriginalToken *bool `json:"forwardOriginalToken,omitempty"`api/v1alpha1/digdirator/zz_generated.deepcopy.go (1)
44-48: LGTM! Proper deep copy implementation.The generated code correctly handles deep copying of the Authentication field for both IDPorten and Maskinporten structs.
Also applies to: 74-78
pkg/reconciliation/reconciliation.go (1)
45-48: Consider adding validation for NotPaths.The
AuthConfigstruct contains a pointer to a slice of strings forNotPaths. Consider adding validation to ensure these paths are well-formed and don't contain potentially dangerous patterns.api/v1alpha1/istiotypes/zz_generated.deepcopy.go (1)
9-45: LGTM! The DeepCopyInto implementation looks correct.The implementation properly handles all pointer fields and slices, ensuring deep copies are made correctly.
pkg/resourcegenerator/istio/requestauthentication/request_authentication.go (1)
74-76:⚠️ Potential issueVerify cookie security settings.
When using cookies for token storage, ensure secure cookie flags are set. Consider adding validation for secure cookie configuration.
pkg/resourceprocessor/diffs_test.go (1)
73-73: LGTM: Test updated correctly for new function signature.The test has been properly updated to accommodate the new authentication configuration parameter.
internal/controllers/application.go (1)
Line range hint
198-218: LGTM: Proper integration of authentication components.The changes correctly integrate the new authentication functionality:
- Added authConfigs parameter to NewApplicationReconciliation
- Added requestauthentication.Generate to the reconciliation functions
tests/application/authorization-settings/multiple-application-errors.yaml (1)
4-5: LGTM: Policy name change is more descriptive.The renamed policy 'allow-all-default-deny' better describes its intended behavior.
tests/application/idporten/chainsaw-test.yaml (1)
17-18: LGTM! Good test coverage for JWT authentication.The addition of the auth assertion ensures that the new JWT validation configuration is properly tested.
tests/application/maskinporten/chainsaw-test.yaml (1)
17-18: LGTM! Consistent test coverage across providers.The auth assertion mirrors the idporten implementation, ensuring consistent JWT validation testing for both authentication providers.
tests/application/authorization-settings/multiple-application-assert.yaml (2)
Line range hint
23-36: Review the removal of DENY action.The removal of the DENY action changes this to an ALLOW policy by default. Ensure this aligns with the security requirements, as it might be more permissive than intended.
Run this script to check for any similar patterns in other authorization policies:
✅ Verification successful
The removal of DENY action is consistent with the policy's purpose and security model
The codebase follows a clear pattern where default deny policies (with explicit DENY action) are paired with specific allow policies (without action) for necessary paths. The security model is maintained through this dual-policy approach, where all traffic is denied by default and only specific paths are allowed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AuthorizationPolicy resources without explicit DENY action rg -l 'kind:\s*AuthorizationPolicy' | xargs yq -r 'select(.kind == "AuthorizationPolicy" and .spec.action == null) | .metadata.name'Length of output: 171
Script:
#!/bin/bash # Search for AuthorizationPolicy resources and their context rg -A 10 -B 2 'kind:\s*AuthorizationPolicy' tests/Length of output: 14485
Line range hint
4-18: Verify the security implications of the default deny policy.The policy correctly denies access to
/actuator*paths fromistio-gatewaysnamespace, but consider adding a catch-all rule to deny access from all other namespaces for enhanced security.spec: action: DENY rules: - from: - source: namespaces: - istio-gateways to: - operation: paths: - /actuator* + - to: + - operation: + paths: + - /*tests/application/authorization-settings/patch-application-assert.yaml (1)
Line range hint
24-38: LGTM: Default deny policy is properly configured.The policy correctly maintains the DENY action and properly restricts access to
/actuator*paths.tests/application/subresource-status/application-synced-assert.yaml (1)
24-24: LGTM: Consistent naming update.The status field correctly reflects the new naming convention for the AuthorizationPolicy resource.
tests/application/maskinporten/application.yaml (2)
24-29: Review the security implications of JWT claim mapping.The configuration maps the
audclaim to theAudienceheader. While this is valid, ensure that:
- The header name follows your organization's naming convention
- Downstream services are aware of this header mapping
- The claim mapping doesn't expose sensitive information
40-43: Verify completeness of allowed paths.The allowList only includes health and info endpoints. Ensure this is sufficient for your application's needs and monitoring requirements.
Run this script to check for additional actuator endpoints that might need access:
✅ Verification successful
Actuator endpoints allowList is correctly configured
The current allowList configuration with
/actuator/healthand/actuator/infoendpoints is appropriate and sufficient. This is consistent with the standard pattern used across the codebase and provides the necessary endpoints for basic monitoring and health checks while maintaining security by not exposing unnecessary endpoints.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for actuator endpoint usage in the codebase rg -g '!*.yaml' -i 'actuator/[a-z]+' -A 3Length of output: 1149
Script:
#!/bin/bash # Search for actuator configuration in properties and yaml files echo "=== Searching for actuator configuration ===" rg -g '*.{properties,yaml,yml}' '(?i)management\.endpoints|spring\.actuator' -A 3 echo -e "\n=== Searching for custom actuator endpoint implementations ===" rg -g '*.{java,kt}' '(?i)@endpoint|@webendpoint|@restcontrollerendpoint' -A 3 echo -e "\n=== Searching for other actuator-related code ===" rg -g '*.{java,kt,xml,gradle}' '(?i)actuator(?!\/health|\/info)' -A 3Length of output: 986
Script:
#!/bin/bash echo "=== Searching for actuator configuration ===" rg --debug -g '*.properties' -g '*.yaml' -g '*.yml' 'management.endpoint' || true rg --debug -g '*.properties' -g '*.yaml' -g '*.yml' 'spring.actuator' || true echo -e "\n=== Searching for custom actuator endpoint implementations ===" rg --debug -g '*.java' -g '*.kt' '@Endpoint' || true rg --debug -g '*.java' -g '*.kt' '@WebEndpoint' || true echo -e "\n=== Searching for other actuator references ===" rg -g '*.java' -g '*.kt' -g '*.xml' -g '*.gradle' 'actuator' || trueLength of output: 67022
Script:
#!/bin/bash echo "=== Searching for actuator endpoints in application files ===" fd -e yaml -e yml application tests | xargs grep -l "actuator" || true echo -e "\n=== Searching for actuator endpoint patterns ===" fd . -e yaml -e yml | xargs grep -l "/actuator/" || true echo -e "\n=== Checking test files for actuator configuration ===" fd . tests -e yaml | xargs grep -A 2 -B 2 "actuator" || trueLength of output: 9950
tests/application/maskinporten/application-maskinporten-auth-assert.yaml (1)
77-79: LGTM: Proper issuer validation.The policy correctly validates the JWT issuer claim against the expected Maskinporten issuer URL.
tests/application/idporten/application-idporten-auth-assert.yaml (2)
43-61: LGTM! Well-structured allow policy for actuator endpoints.The policy correctly allows access to essential health check endpoints while maintaining namespace-based restrictions.
10-11: Consider security implications of cookie-based tokens.Using cookies for JWT storage (
fromCookies: ["BearerToken"]) could expose the application to CSRF attacks. Consider implementing additional CSRF protection measures if not already in place.Run the following script to check for CSRF protection:
tests/application/idporten/application.yaml (4)
Line range hint
4-14: LGTM! Basic configuration is correct.The application configuration correctly enables idporten with the required scope.
46-49: LGTM! Well-structured allow list for actuator endpoints.The authorization settings correctly specify the allowed health check endpoints.
78-80: Verify if duplicate credentials are intentional.Both secrets contain identical credentials. Please verify if this is intentional or if the second secret should have different values for its specific use case.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
68-70: Verify the encoded credentials.Please ensure that the base64-encoded values correspond to the correct:
- Client ID
- Issuer URL
- JWKS URI
✅ Verification successful
Base64-encoded credentials verified successfully
All encoded values match their expected values exactly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Decode and verify the credentials # Decode and verify the values echo "Decoding IDPORTEN_CLIENT_ID..." echo "dGVzdC1jbGllbnQtaWQtaWRwb3J0ZW4=" | base64 -d echo -e "\nExpected: test-client-id-idporten" echo -e "\nDecoding IDPORTEN_ISSUER..." echo "aHR0cHM6Ly9pZHBvcnRlbi5ubw==" | base64 -d echo -e "\nExpected: https://idporten.no" echo -e "\nDecoding IDPORTEN_JWKS_URI..." echo "aHR0cHM6Ly9pZHBvcnRlbi5uby9qd2tzLmpzb24=" | base64 -d echo -e "\nExpected: https://idporten.no/jwks.json"Length of output: 922
Script:
#!/bin/bash python3 << 'EOF' import base64 def decode_and_verify(encoded, expected): decoded = base64.b64decode(encoded).decode('utf-8') print(f"Decoded: {decoded}") print(f"Expected: {expected}") print(f"Matches: {decoded == expected}\n") print("Verifying IDPORTEN_CLIENT_ID:") decode_and_verify("dGVzdC1jbGllbnQtaWQtaWRwb3J0ZW4=", "test-client-id-idporten") print("Verifying IDPORTEN_ISSUER:") decode_and_verify("aHR0cHM6Ly9pZHBvcnRlbi5ubw==", "https://idporten.no") print("Verifying IDPORTEN_JWKS_URI:") decode_and_verify("aHR0cHM6Ly9pZHBvcnRlbi5uby9qd2tzLmpzb24=", "https://idporten.no/jwks.json") EOFLength of output: 360
tests/application/subresource-status/application-resource-apply-error-assert.yaml (1)
22-22: LGTM! AuthorizationPolicy identifier update aligns with new naming convention.The change from
badport-denytobadport-default-denycorrectly reflects the new authorization policy naming scheme introduced as part of the JWT validation feature.
| type ProviderURIs struct { | ||
| Provider IdentityProvider | ||
| IssuerURI string | ||
| JwksURI string | ||
| ClientID string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for URIs and ClientID.
The ProviderURIs struct contains critical security-related fields. Consider adding validation to ensure:
IssuerURIandJwksURIare valid HTTPS URLsClientIDfollows the expected format for the identity provider
| jwtRule.Issuer = providerURIs.IssuerURI | ||
| jwtRule.JwksUri = providerURIs.JwksURI | ||
| jwtRule.Audiences = []string{providerURIs.ClientID} |
There was a problem hiding this comment.
Validate JWT configuration values.
The JWT configuration values (Issuer, JWKS URI, and Audience) are critical security parameters. Consider adding validation to ensure:
- URIs use HTTPS
- JWKS URI is properly formatted
- Audience is not empty
| authConfigs, err := r.GetAuthConfigsForApplication(ctx, application) | ||
| if err != nil { | ||
| rLog.Error(err, "can't resolve auth config") | ||
| } |
There was a problem hiding this comment.
Improve error handling for auth config retrieval.
The error from GetAuthConfigsForApplication is only logged but not propagated, which could lead to silent failures in authentication setup.
authConfigs, err := r.GetAuthConfigsForApplication(ctx, application)
if err != nil {
- rLog.Error(err, "can't resolve auth config")
+ rLog.Error(err, "failed to resolve auth config")
+ r.SetErrorState(ctx, application, err, "failed to resolve auth config", "AuthConfigError")
+ return common.RequeueWithError(err)
}Committable suggestion skipped: line range outside the PR's diff.
tests/application/authorization-settings/multiple-application-errors.yaml
Show resolved
Hide resolved
tests/application/maskinporten/application-maskinporten-auth-assert.yaml
Show resolved
Hide resolved
| authentication: | ||
| description: Authentication specifies how incoming JWT's should | ||
| be validated. | ||
| properties: | ||
| enabled: | ||
| description: |- | ||
| Whether to enable JWT validation. | ||
| If enabled, incoming JWT's will be validated against the issuer specified in the app registration and the generated audience. | ||
| type: boolean | ||
| forwardOriginalToken: | ||
| description: If set to true, the original token will be kept | ||
| for the upstream request. Default is true. | ||
| type: boolean | ||
| ignorePaths: | ||
| description: |- | ||
| IgnorePaths specifies paths that do not require an authenticated JWT. | ||
|
|
||
| The specified paths must start with '/'. | ||
| items: | ||
| type: string | ||
| type: array | ||
| x-kubernetes-list-type: set | ||
| outputClaimToHeaders: | ||
| description: "This field specifies a list of operations to | ||
| copy the claim to HTTP headers on a successfully verified | ||
| token.\nThe header specified in each operation in the list | ||
| must be unique. Nested claims of type string/int/bool is | ||
| supported as well.\n```\n\n\toutputClaimToHeaders:\n\t- | ||
| header: x-my-company-jwt-group\n\t claim: my-group\n\t- | ||
| header: x-test-environment-flag\n\t claim: test-flag\n\t- | ||
| header: x-jwt-claim-group\n\t claim: nested.key.group\n\n```" | ||
| items: | ||
| properties: | ||
| claim: | ||
| description: The claim to be copied. | ||
| type: string | ||
| header: | ||
| description: The name of the HTTP-header for which the | ||
| specified claim will be copied to. | ||
| type: string | ||
| required: | ||
| - claim | ||
| - header | ||
| type: object | ||
| type: array | ||
| secretName: | ||
| description: |- | ||
| The name of the kubernetes Secret containing OAuth2-credentials. | ||
|
|
||
| If omitted, the associated client registration in the application manifest is used for JWT validation. | ||
| type: string | ||
| tokenLocation: | ||
| description: |- | ||
| Where to find the JWT in the incoming request | ||
|
|
||
| An enum value of `header` means that the JWT is present in the `Authorization` as a Bearer Token. | ||
| An enum value of `cookie` means that the JWT is present as a cookie called `BearerToken`. | ||
| enum: | ||
| - header | ||
| - cookie | ||
| type: string | ||
| required: | ||
| - enabled | ||
| type: object |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify duplicate authentication configuration.
The Maskinporten authentication block is identical to the IDPorten block. Consider extracting the common authentication schema to a shared definition to maintain DRY principles and ensure consistent validation rules.
+ # Add at the top level of the schema
+ definitions:
+ authentication:
+ description: Authentication specifies how incoming JWT's should be validated.
+ properties:
+ enabled:
+ description: Whether to enable JWT validation.
+ type: boolean
+ # ... rest of the authentication properties
+ required:
+ - enabled
+ type: object
# In both idporten and maskinporten sections
- authentication:
- description: Authentication specifies how incoming JWT's should be validated.
- properties:
- # ... duplicate properties
+ authentication:
+ $ref: "#/definitions/authentication"Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/util/helperfunctions.go (2)
81-81:⚠️ Potential issueFix typo in function name.
The function name has a typo:
GetMaskinPortenlientshould beGetMaskinPortenClient.-func GetMaskinPortenlient(k8sClient client.Client, ctx context.Context, namespacedName types.NamespacedName) (*nais_io_v1.MaskinportenClient, error) { +func GetMaskinPortenClient(k8sClient client.Client, ctx context.Context, namespacedName types.NamespacedName) (*nais_io_v1.MaskinportenClient, error) {
92-99:⚠️ Potential issueFix potential nil pointer dereference in GetSecretData.
The function doesn't check for errors from GetSecret before accessing secret.Data, which could lead to a nil pointer dereference.
func GetSecretData(client client.Client, ctx context.Context, namespacedName types.NamespacedName, dataKeys []string) (map[string][]byte, error) { secret, err := GetSecret(client, ctx, namespacedName) + if err != nil { + return nil, fmt.Errorf("failed to get secret: %w", err) + } secretDataMap := map[string][]byte{} for _, key := range dataKeys { secretDataMap[key] = secret.Data[key] } return secretDataMap, err }
🧹 Nitpick comments (2)
pkg/util/helperfunctions.go (1)
196-198: Simplify boolean expression.The comparison with
trueis redundant and can be simplified.-func IsMaskinPortenAuthenticationEnabled(application *skiperatorv1alpha1.Application) bool { - return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled == true +func IsMaskinPortenAuthenticationEnabled(application *skiperatorv1alpha1.Application) bool { + return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled }🧰 Tools
🪛 golangci-lint (1.62.2)
197-197: S1002: should omit comparison to bool constant, can be simplified to
application.Spec.Maskinporten.Authentication.Enabled(gosimple)
internal/controllers/application.go (1)
475-550: Consider refactoring to reduce code duplication.The function has similar code blocks for both IDPorten and MaskinPorten providers. Consider extracting the common logic into a helper function.
+func getProviderInfo(application *skiperatorv1alpha1.Application, k8sClient client.Client, ctx context.Context, provider IdentityProvider) (*IdentityProviderInfo, error) { + var auth *istiotypes.Authentication + var enabled bool + var secretNamePtr *string + + switch provider { + case ID_PORTEN: + auth = application.Spec.IDPorten.Authentication + enabled = application.Spec.IDPorten.Enabled + case MASKINPORTEN: + auth = application.Spec.Maskinporten.Authentication + enabled = application.Spec.Maskinporten.Enabled + default: + return nil, fmt.Errorf("unsupported provider: %s", provider) + } + + if auth.SecretName != nil { + secretNamePtr = auth.SecretName + } else if enabled { + var err error + secretNamePtr, err = getSecretNameForIdentityProvider(k8sClient, ctx, + types.NamespacedName{ + Namespace: application.Namespace, + Name: application.Name, + }, + provider, + application.UID) + if err != nil { + return nil, fmt.Errorf("failed to get secret name for %s: %w", provider, err) + } + } else { + return nil, fmt.Errorf("JWT authentication requires either %s to be enabled or a secretName to be provided", provider) + } + + return &IdentityProviderInfo{ + Provider: provider, + SecretName: *secretNamePtr, + NotPaths: auth.IgnorePaths, + }, nil +} func getIdentityProviderInfoWithAuthenticationEnabled(ctx context.Context, application *skiperatorv1alpha1.Application, k8sClient client.Client) ([]IdentityProviderInfo, error) { var providerInfo []IdentityProviderInfo if util.IsIDPortenAuthenticationEnabled(application) { - var secretName *string - var err error - if application.Spec.IDPorten.Authentication.SecretName != nil { - secretName = application.Spec.IDPorten.Authentication.SecretName - } else if application.Spec.IDPorten.Enabled { - secretName, err = getSecretNameForIdentityProvider(k8sClient, ctx, - types.NamespacedName{ - Namespace: application.Namespace, - Name: application.Name, - }, - ID_PORTEN, - application.UID) - } else { - return nil, fmt.Errorf("JWT authentication requires either IDPorten to be enabled or a secretName to be provided") - } - if err != nil { - err := fmt.Errorf("failed to get secret name for IDPortenClient: %w", err) - return nil, err - } - - var notPaths *[]string - if application.Spec.IDPorten.Authentication.IgnorePaths != nil { - notPaths = application.Spec.IDPorten.Authentication.IgnorePaths - } else { - notPaths = nil - } - providerInfo = append(providerInfo, IdentityProviderInfo{ - Provider: ID_PORTEN, - SecretName: *secretName, - NotPaths: notPaths, - }) + info, err := getProviderInfo(application, k8sClient, ctx, ID_PORTEN) + if err != nil { + return nil, err + } + providerInfo = append(providerInfo, *info) } if util.IsMaskinPortenAuthenticationEnabled(application) { - var secretName *string - var err error - if application.Spec.Maskinporten.Authentication.SecretName != nil { - secretName = application.Spec.Maskinporten.Authentication.SecretName - } else if application.Spec.Maskinporten.Enabled { - secretName, err = getSecretNameForIdentityProvider(k8sClient, ctx, - types.NamespacedName{ - Namespace: application.Namespace, - Name: application.Name, - }, - MASKINPORTEN, - application.UID) - } else { - return nil, fmt.Errorf("JWT authentication requires either Maskinporten to be enabled or a secretName to be provided") - } - if err != nil { - err := fmt.Errorf("failed to get secret name for MaskinPortenClient: %w", err) - return nil, err - } - - var notPaths *[]string - if application.Spec.Maskinporten.Authentication.IgnorePaths != nil { - notPaths = application.Spec.Maskinporten.Authentication.IgnorePaths - } else { - notPaths = nil - } - providerInfo = append(providerInfo, IdentityProviderInfo{ - Provider: MASKINPORTEN, - SecretName: *secretName, - NotPaths: notPaths, - }) + info, err := getProviderInfo(application, k8sClient, ctx, MASKINPORTEN) + if err != nil { + return nil, err + } + providerInfo = append(providerInfo, *info) } return providerInfo, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/crd/skiperator.kartverket.no_applications.yaml(2 hunks)internal/controllers/application.go(5 hunks)pkg/util/helperfunctions.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/util/helperfunctions.go
197-197: S1002: should omit comparison to bool constant, can be simplified to application.Spec.Maskinporten.Authentication.Enabled
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and run tests
- GitHub Check: Build and run tests
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
pkg/util/helperfunctions.go (2)
70-79: LGTM! Well-structured error handling.The function follows Kubernetes client patterns with proper error handling and clear error messages.
192-194: LGTM! Safe nil checks.The function properly checks for nil pointers before accessing nested fields, preventing potential panics.
internal/controllers/application.go (1)
422-473: LGTM! Well-structured authentication configuration retrieval.The function properly handles:
- Multiple identity providers
- Secret data retrieval with proper error handling
- Clear error messages
- Edge case of no auth configs
config/crd/skiperator.kartverket.no_applications.yaml (1)
556-620: Extract common authentication schema.The authentication block is identical between IDPorten and MaskinPorten. Consider extracting it to a shared definition to maintain DRY principles and ensure consistent validation rules.
+ # Add at the top level of the schema + definitions: + authentication: + description: Authentication specifies how incoming JWT's should be validated. + properties: + enabled: + description: Whether to enable JWT validation. + type: boolean + # ... rest of the authentication properties + required: + - enabled + type: object # In both idporten and maskinporten sections - authentication: - description: Authentication specifies how incoming JWT's should be validated. - properties: - # ... duplicate properties + authentication: + $ref: "#/definitions/authentication"
| func getSecretNameForIdentityProvider(k8sClient client.Client, ctx context.Context, namespacedName types.NamespacedName, provider IdentityProvider, applicationUID types.UID) (*string, error) { | ||
| switch provider { | ||
| case ID_PORTEN: | ||
| idPortenClient, err := util.GetIdPortenClient(k8sClient, ctx, namespacedName) | ||
| if err != nil { | ||
| err := fmt.Errorf("failed to get IDPortenClient: %s", namespacedName.String()) | ||
| return nil, err | ||
| } | ||
| if idPortenClient == nil { | ||
| err := fmt.Errorf("IDPortenClient: '%s' not found", namespacedName.String()) | ||
| return nil, err | ||
| } | ||
| for _, ownerReference := range idPortenClient.OwnerReferences { | ||
| if ownerReference.UID == applicationUID { | ||
| return &idPortenClient.Spec.SecretName, nil | ||
| } | ||
| } | ||
| err = fmt.Errorf("no IDPortenClient with ownerRef to '%s' found", namespacedName.String()) | ||
| return nil, err | ||
|
|
||
| case MASKINPORTEN: | ||
| maskinPortenClient, err := util.GetMaskinPortenlient(k8sClient, ctx, namespacedName) | ||
| if err != nil { | ||
| err := fmt.Errorf("failed to get MaskinPortenClient: %s", namespacedName.String()) | ||
| return nil, err | ||
| } | ||
| if maskinPortenClient == nil { | ||
| err := fmt.Errorf("IDPortenClient: '%s' not found", namespacedName.String()) | ||
| return nil, err | ||
| } | ||
| for _, ownerReference := range maskinPortenClient.OwnerReferences { | ||
| if ownerReference.UID == applicationUID { | ||
| return &maskinPortenClient.Spec.SecretName, nil | ||
| } | ||
| } | ||
| err = fmt.Errorf("no MaskinPortenClient with ownerRef to (%s) found", namespacedName.String()) | ||
| return nil, err | ||
|
|
||
| default: | ||
| return nil, fmt.Errorf("provider: %s not supported", provider) | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor duplicate code and fix function name usage.
- The function has similar code blocks for both providers that can be extracted into a helper function.
- It uses the typo'd function name
GetMaskinPortenlient.
+func getClientAndCheckOwner[T any](k8sClient client.Client, ctx context.Context, namespacedName types.NamespacedName, applicationUID types.UID, provider IdentityProvider, getClient func(client.Client, context.Context, types.NamespacedName) (*T, error)) (*T, error) {
+ client, err := getClient(k8sClient, ctx, namespacedName)
+ if err != nil {
+ return nil, fmt.Errorf("failed to get %s: %s", provider, namespacedName.String())
+ }
+ if client == nil {
+ return nil, fmt.Errorf("%s: '%s' not found", provider, namespacedName.String())
+ }
+
+ // Use reflection to access OwnerReferences and Spec.SecretName
+ v := reflect.ValueOf(client).Elem()
+ ownerRefs := v.FieldByName("OwnerReferences").Interface().([]metav1.OwnerReference)
+ for _, ownerReference := range ownerRefs {
+ if ownerReference.UID == applicationUID {
+ secretName := v.FieldByName("Spec").FieldByName("SecretName").String()
+ return client, nil
+ }
+ }
+ return nil, fmt.Errorf("no %s with ownerRef to '%s' found", provider, namespacedName.String())
+}
func getSecretNameForIdentityProvider(k8sClient client.Client, ctx context.Context, namespacedName types.NamespacedName, provider IdentityProvider, applicationUID types.UID) (*string, error) {
switch provider {
case ID_PORTEN:
- idPortenClient, err := util.GetIdPortenClient(k8sClient, ctx, namespacedName)
- if err != nil {
- err := fmt.Errorf("failed to get IDPortenClient: %s", namespacedName.String())
- return nil, err
- }
- if idPortenClient == nil {
- err := fmt.Errorf("IDPortenClient: '%s' not found", namespacedName.String())
- return nil, err
- }
- for _, ownerReference := range idPortenClient.OwnerReferences {
- if ownerReference.UID == applicationUID {
- return &idPortenClient.Spec.SecretName, nil
- }
- }
- err = fmt.Errorf("no IDPortenClient with ownerRef to '%s' found", namespacedName.String())
- return nil, err
+ client, err := getClientAndCheckOwner(k8sClient, ctx, namespacedName, applicationUID, provider, util.GetIdPortenClient)
+ if err != nil {
+ return nil, err
+ }
+ return &client.Spec.SecretName, nil
case MASKINPORTEN:
- maskinPortenClient, err := util.GetMaskinPortenlient(k8sClient, ctx, namespacedName)
- if err != nil {
- err := fmt.Errorf("failed to get MaskinPortenClient: %s", namespacedName.String())
- return nil, err
- }
- if maskinPortenClient == nil {
- err := fmt.Errorf("IDPortenClient: '%s' not found", namespacedName.String())
- return nil, err
- }
- for _, ownerReference := range maskinPortenClient.OwnerReferences {
- if ownerReference.UID == applicationUID {
- return &maskinPortenClient.Spec.SecretName, nil
- }
- }
- err = fmt.Errorf("no MaskinPortenClient with ownerRef to (%s) found", namespacedName.String())
- return nil, err
+ client, err := getClientAndCheckOwner(k8sClient, ctx, namespacedName, applicationUID, provider, util.GetMaskinPortenClient)
+ if err != nil {
+ return nil, err
+ }
+ return &client.Spec.SecretName, nil
default:
return nil, fmt.Errorf("provider: %s not supported", provider)
}
}Committable suggestion skipped: line range outside the PR's diff.
This PR extends
idporten-spec andmaskinporten-spec to include a new block calledauthentication. It can be enabled like this:When enabled, an Istio RequestAuthentication resource is generated to validate all incoming requests with a JWT. The correct issuer-uri, jwks-uri and client-id are retrieved from the kubernetes-
Secretcreated by digdirator. I also needed to modify the generation of AuthorizationPolicy such that all paths requires the presence of a validated token with the correct issuer, except for/actuator*which by default is denied (unlessauthorizationSettingssays otherwise).The generated requestauthentication resource looks like this:
and two authorizationpolicies are generated. One that default-denies
/actuator*and one that enforces a JWT with the correct issuer:Additional parameters in
AuthenticationsecretName: If an IDPorten or MaskinPorten client is registered outside of the application-manifest, the name of a kubernetes-Secretholding issuer-uri, jwks-uri and client-id can be provided.forwardOriginalTokentokenLocation: Is the JWT located as a BearerToken-cookie or as an Authorization-header?outputClaimToHeaders: If you wish to copy claims from the JWT and add them as headers to the requests.Summary by CodeRabbit
Based on the comprehensive changes, here are the high-level release notes:
New Features
Authentication Enhancements
Authorization Improvements
Kubernetes Integration
These changes significantly improve the security and configuration flexibility for applications using ID Porten and Maskinporten authentication providers.