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 implements extensive authentication and authorization enhancements. New optional Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant AR as ApplicationReconciler
participant AC as Auth Configs
participant RG as Resource Generator
participant K8s as Kubernetes Cluster
App->>AR: Trigger reconciliation
AR->>AR: Retrieve authentication configs
AR->>AC: GetAuthConfigsForApplication
AR->>RG: Pass app details & authConfigs
RG->>RG: Generate RequestAuthentication
RG->>RG: Generate AuthorizationPolicy (JWT/default deny)
RG->>K8s: Apply generated resources
K8s-->>RG: Resource status
RG-->>AR: Return reconciliation outcome
AR-->>App: Reconciliation complete
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.
pkg/reconciliation/reconciliation.go
Outdated
| 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
internal/controllers/application.go
Outdated
| 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"
internal/controllers/application.go
Outdated
| 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.
pkg/reconciliation/reconciliation.go
Outdated
|
|
||
| type IdentityProvider string | ||
|
|
||
| type IdentityProviderInfo struct { |
There was a problem hiding this comment.
This struct is not used in the reconciliation object, so imo it doesn't belong here.
There was a problem hiding this comment.
I agree, but where does it belong?😅
There was a problem hiding this comment.
top of the file where its used :)
internal/controllers/application.go
Outdated
|
|
||
| func getIdentityProviderInfoWithAuthenticationEnabled(ctx context.Context, application *skiperatorv1alpha1.Application, k8sClient client.Client) ([]IdentityProviderInfo, error) { | ||
| var providerInfo []IdentityProviderInfo | ||
| if util.IsIDPortenAuthenticationEnabled(application) { |
There was a problem hiding this comment.
IDPorten/MaskinPorten could implement an interface and I think we could get away with some duplicate code, but thats not a requirement to merge this pr. Only if you want to :P
There was a problem hiding this comment.
Refactored into using interface for IDPorten/Maskinporten to implement. Placed it in pkg/jwtAuth
pkg/resourcegenerator/istio/authorizationpolicy/authorization_policy.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
config/crd/skiperator.kartverket.no_applications.yaml (1)
556-622: 🛠️ Refactor suggestionConsolidate duplicate authentication schema definitions.
The authentication schema is duplicated between IDPorten and MaskinPorten sections. This violates the DRY principle and could lead to maintenance issues.
As suggested in a previous review, consider extracting the common schema to a shared definition:
+ # 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"Also applies to: 826-892
🧹 Nitpick comments (2)
pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go (1)
30-35: Confirm rationale for skipping AuthorizationPolicy on AllowAll.
Allowing all traffic may inadvertently leave the application exposed. Ensure that it’s a deliberate choice aligned with your security posture.tests/application/idporten/application.yaml (1)
69-71: Consider using unique test values for secrets.The secrets use identical test values which might not catch configuration errors when testing different authentication scenarios.
Consider using unique test values:
- IDPORTEN_CLIENT_ID: dGVzdC1jbGllbnQtaWQtaWRwb3J0ZW4= #test-client-id-idporten + IDPORTEN_CLIENT_ID: dGVzdC1jbGllbnQtaWQtaWRwb3J0ZW4tMQ== #test-client-id-idporten-1Also applies to: 79-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
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(5 hunks)pkg/jwtAuth/config.go(1 hunks)pkg/reconciliation/application.go(3 hunks)pkg/reconciliation/reconciliation.go(4 hunks)pkg/resourcegenerator/istio/authorizationpolicy/authorization_policy.go(0 hunks)pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go(1 hunks)pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go(1 hunks)pkg/resourcegenerator/istio/requestauthentication/request_authentication.go(1 hunks)tests/application/idporten/application.yaml(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/resourcegenerator/istio/authorizationpolicy/authorization_policy.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/reconciliation/reconciliation.go
- pkg/reconciliation/application.go
- api/v1alpha1/istiotypes/zz_generated.deepcopy.go
- api/v1alpha1/istiotypes/jwt_authentication.go
- internal/controllers/application.go
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/application/idporten/application.yaml
[error] 81-81: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run Pharos Security Scan
- GitHub Check: Build and run tests
- GitHub Check: Analyze (go)
- GitHub Check: Build and run tests
🔇 Additional comments (9)
pkg/resourcegenerator/istio/requestauthentication/request_authentication.go (4)
33-35: Add warning log for missing auth config.
This debug log might overlook the security implications of enabling unauthenticated requests. Consider logging a warning instead to raise visibility and help maintainers quickly detect misconfigurations.
67-70: Review potential security implications of ForwardOriginalToken.
Forwarding the original token may unintentionally expose sensitive data downstream, especially if subsequent services are not configured to handle it securely. Confirm that this behavior aligns with your threat model and compliance requirements.
74-83: Assess PII leakage from OutputClaimToHeaders.
Copying claims to headers can inadvertently expose personally identifiable information or sensitive data. Ensure that any sensitive claims are properly sanitized or omitted, and consider encrypting or hashing sensitive fields if required.
85-87: Validate JWT configuration values.
This code sets IssuerURI, JwksURI, and ClientID directly without validating their correctness or security. Ensure that URIs use HTTPS and that essential properties (e.g., ClientID) are not empty, to prevent misconfiguration or accidental leaks.pkg/jwtAuth/config.go (1)
47-84: Validate secret data to prevent empty or malformed provider URIs.
When retrieving secret data for ID Porten or Maskinporten, consider verifying non-empty string values for the IssuerURI, JWKS URI, and ClientID. Empty or poorly formed values could undermine the JWT validation process and create security gaps.pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go (1)
71-79: Verify traffic sources in the default rule.
The current rule only allows traffic from the "istio-gateways" namespace. If local traffic or other namespaces must reach the application, you might need additional rules or an expanded list of allowed namespaces.pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (1)
17-61: LGTM! The Generate function is well-structured and handles edge cases appropriately.The function correctly:
- Handles type assertions with proper error handling
- Respects the
allowAllsetting to skip policy generation- Collects allowed paths from authorization settings
- Generates JWT validation policy only when auth configs exist
tests/application/idporten/application.yaml (2)
30-36: LGTM! The authentication configuration is well-structured.The test case properly demonstrates the authentication configuration with:
- Enabled flag
- Secret name reference
- Token location in cookie
- Claim to header mapping
47-61: LGTM! The authorization settings configuration is comprehensive.The test case correctly demonstrates:
- Allow list for health and info endpoints
- Integration with authentication settings
- Proper claim mapping
pkg/jwtAuth/config.go
Outdated
| return nil, err | ||
| } | ||
| if maskinPortenClient == nil { | ||
| err := fmt.Errorf("IDPortenClient: '%s' not found", namespacedName.String()) |
There was a problem hiding this comment.
Correct the error message for Maskinporten clients.
The error message here references "IDPortenClient" instead of "MaskinPortenClient," which can cause confusion or mislead debugging efforts.
Apply this diff to fix the message:
- err := fmt.Errorf("IDPortenClient: '%s' not found", namespacedName.String())
+ err := fmt.Errorf("MaskinPortenClient: '%s' not found", namespacedName.String())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err := fmt.Errorf("IDPortenClient: '%s' not found", namespacedName.String()) | |
| err := fmt.Errorf("MaskinPortenClient: '%s' not found", namespacedName.String()) |
| func getJwtValidationAuthPolicy(namespacedName types.NamespacedName, applicationName string, authConfigs []jwtAuth.AuthConfig, allowPaths []string, denyPaths []string) *securityv1.AuthorizationPolicy { | ||
| var authPolicyRules []*securityv1api.Rule | ||
|
|
||
| notPaths := allowPaths | ||
| notPaths = append(allowPaths, denyPaths...) | ||
| for _, authConfig := range authConfigs { | ||
| authPolicyRules = append(authPolicyRules, &securityv1api.Rule{ | ||
| To: []*securityv1api.Rule_To{ | ||
| { | ||
| Operation: &securityv1api.Operation{ | ||
| NotPaths: notPaths, | ||
| }, | ||
| }, | ||
| }, | ||
| When: []*securityv1api.Condition{ | ||
| { | ||
| Key: "request.auth.claims[iss]", | ||
| Values: []string{authConfig.ProviderURIs.IssuerURI}, | ||
| }, | ||
| }, | ||
| From: default_deny.GetGeneralFromRule(), | ||
| }) | ||
| } | ||
| return &securityv1.AuthorizationPolicy{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: namespacedName.Namespace, | ||
| Name: namespacedName.Name, | ||
| }, | ||
| Spec: securityv1api.AuthorizationPolicy{ | ||
| Action: securityv1api.AuthorizationPolicy_ALLOW, | ||
| Rules: authPolicyRules, | ||
| Selector: &typev1beta1.WorkloadSelector{ | ||
| MatchLabels: util.GetPodAppSelector(applicationName), | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential path slice modification issue.
The current implementation modifies the allowPaths slice in place when creating notPaths. This could lead to unexpected behavior if the allowPaths slice is used elsewhere after this function call.
Apply this diff to create a new slice instead:
-var notPaths := allowPaths
-notPaths = append(allowPaths, denyPaths...)
+notPaths := make([]string, len(allowPaths))
+copy(notPaths, allowPaths)
+notPaths = append(notPaths, denyPaths...)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getJwtValidationAuthPolicy(namespacedName types.NamespacedName, applicationName string, authConfigs []jwtAuth.AuthConfig, allowPaths []string, denyPaths []string) *securityv1.AuthorizationPolicy { | |
| var authPolicyRules []*securityv1api.Rule | |
| notPaths := allowPaths | |
| notPaths = append(allowPaths, denyPaths...) | |
| for _, authConfig := range authConfigs { | |
| authPolicyRules = append(authPolicyRules, &securityv1api.Rule{ | |
| To: []*securityv1api.Rule_To{ | |
| { | |
| Operation: &securityv1api.Operation{ | |
| NotPaths: notPaths, | |
| }, | |
| }, | |
| }, | |
| When: []*securityv1api.Condition{ | |
| { | |
| Key: "request.auth.claims[iss]", | |
| Values: []string{authConfig.ProviderURIs.IssuerURI}, | |
| }, | |
| }, | |
| From: default_deny.GetGeneralFromRule(), | |
| }) | |
| } | |
| return &securityv1.AuthorizationPolicy{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Namespace: namespacedName.Namespace, | |
| Name: namespacedName.Name, | |
| }, | |
| Spec: securityv1api.AuthorizationPolicy{ | |
| Action: securityv1api.AuthorizationPolicy_ALLOW, | |
| Rules: authPolicyRules, | |
| Selector: &typev1beta1.WorkloadSelector{ | |
| MatchLabels: util.GetPodAppSelector(applicationName), | |
| }, | |
| }, | |
| } | |
| } | |
| func getJwtValidationAuthPolicy(namespacedName types.NamespacedName, applicationName string, authConfigs []jwtAuth.AuthConfig, allowPaths []string, denyPaths []string) *securityv1.AuthorizationPolicy { | |
| var authPolicyRules []*securityv1api.Rule | |
| notPaths := make([]string, len(allowPaths)) | |
| copy(notPaths, allowPaths) | |
| notPaths = append(notPaths, denyPaths...) | |
| for _, authConfig := range authConfigs { | |
| authPolicyRules = append(authPolicyRules, &securityv1api.Rule{ | |
| To: []*securityv1api.Rule_To{ | |
| { | |
| Operation: &securityv1api.Operation{ | |
| NotPaths: notPaths, | |
| }, | |
| }, | |
| }, | |
| When: []*securityv1api.Condition{ | |
| { | |
| Key: "request.auth.claims[iss]", | |
| Values: []string{authConfig.ProviderURIs.IssuerURI}, | |
| }, | |
| }, | |
| From: default_deny.GetGeneralFromRule(), | |
| }) | |
| } | |
| return &securityv1.AuthorizationPolicy{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Namespace: namespacedName.Namespace, | |
| Name: namespacedName.Name, | |
| }, | |
| Spec: securityv1api.AuthorizationPolicy{ | |
| Action: securityv1api.AuthorizationPolicy_ALLOW, | |
| Rules: authPolicyRules, | |
| Selector: &typev1beta1.WorkloadSelector{ | |
| MatchLabels: util.GetPodAppSelector(applicationName), | |
| }, | |
| }, | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/resourcegenerator/istio/authorizationpolicy/common.go (1)
9-17: Consider improvements to the function implementation.While the function works correctly, consider the following improvements:
- The function name could be more descriptive (e.g.,
CreateGatewaySourceRule)- The hardcoded namespace might limit flexibility across different environments
- Missing documentation explaining the function's purpose and usage
Here's a suggested improvement:
+// CreateGatewaySourceRule returns an authorization rule that allows traffic from Istio gateway namespaces. +// The rule is used as a base configuration for authorization policies. -func GetGeneralFromRule() []*v1.Rule_From { +func CreateGatewaySourceRule(gatewayNamespace string) []*v1.Rule_From { return []*v1.Rule_From{ { Source: &v1.Source{ - Namespaces: []string{"istio-gateways"}, + Namespaces: []string{gatewayNamespace}, }, }, } }pkg/jwtAuth/idporten.go (1)
74-91: Handle missing secret data keys to prevent misconfiguration.
In addition to retrieving the secret, ensure that the necessary keys (issuer, JWKS URI, client ID) are present. Doing so will prevent silent misconfigurations if the secret is missing a key or is incorrectly populated.pkg/jwtAuth/identityProvider.go (1)
24-30: Ensure interface methods can propagate typed errors.
The interface looks clean. However, you might enhance maintainability by returning typed or custom errors for missing or invalid secrets, improving observability and troubleshooting in upstream logic.pkg/jwtAuth/authConfig.go (1)
40-57: Simplify nested nil checks.The nested nil checks can be simplified to improve readability.
Apply this diff to simplify the code:
func (authConfigs *AuthConfigs) GetAllowedPaths(authorizationSettings *skiperatorv1alpha1.AuthorizationSettings) []string { var allowPaths []string - if authorizationSettings != nil { - if authorizationSettings.AllowList != nil { - if len(authorizationSettings.AllowList) > 0 { - allowPaths = authorizationSettings.AllowList - } - } + if authorizationSettings != nil && authorizationSettings.AllowList != nil && len(authorizationSettings.AllowList) > 0 { + allowPaths = authorizationSettings.AllowList } if authConfigs != nil { for _, config := range *authConfigs {pkg/util/helperfunctions.go (1)
187-189: Simplify bool comparison.The comparison to bool constant can be simplified.
Apply this diff to simplify the code:
- return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled == true + return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled🧰 Tools
🪛 golangci-lint (1.62.2)
188-188: S1002: should omit comparison to bool constant, can be simplified to
application.Spec.Maskinporten.Authentication.Enabled(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/jwtAuth/authConfig.go(1 hunks)pkg/jwtAuth/identityProvider.go(1 hunks)pkg/jwtAuth/idporten.go(1 hunks)pkg/jwtAuth/maskinporten.go(1 hunks)pkg/resourcegenerator/istio/authorizationpolicy/common.go(1 hunks)pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go(1 hunks)pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go(1 hunks)pkg/util/helperfunctions.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/util/helperfunctions.go
188-188: 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 (4)
- GitHub Check: Run Pharos Security Scan
- GitHub Check: Analyze (go)
- GitHub Check: Build and run tests
- GitHub Check: Build and run tests
🔇 Additional comments (10)
pkg/resourcegenerator/istio/authorizationpolicy/common.go (2)
1-4: LGTM! Clean package structure.The package name and import are well-organized and follow Go conventions.
5-7: LGTM! Well-defined constant.The constant name is descriptive and its value aligns with the PR objectives for default path denial.
pkg/jwtAuth/idporten.go (1)
24-62: Validate presence of required fields in retrieved secrets.
While the overall retrieval logic is robust, consider validating that secrets contain the keys needed (for example, “IssuerURI”, “JwksURI”, “ClientID”) before returning successfully, to avoid unexpected empty-string configurations and runtime errors.pkg/jwtAuth/maskinporten.go (1)
74-91: Guard against missing data fields in the returned secret.
Similar to IDPortenOps, ensure that the secret has all required fields (issuer, JWKS URI, client ID). Consider returning a more explicit error if expected fields are missing, to avoid subtle misconfigurations.pkg/jwtAuth/authConfig.go (2)
10-15: LGTM! Well-structured type definitions.The types are well-defined and follow Go conventions.
17-38: LGTM! Robust error handling and nil checks.The function has proper error handling and nil checks, making it safe and reliable.
pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (2)
17-54: LGTM! Well-structured function with proper error handling.The function has proper type assertion, error handling, and logging.
59-60: Fix potential path slice modification issue.The current implementation modifies the
allowPathsslice in place when creatingnotPaths. This could lead to unexpected behavior if theallowPathsslice is used elsewhere after this function call.Apply this diff to create a new slice instead:
-notPaths := allowPaths -notPaths = append(allowPaths, denyPaths...) +notPaths := make([]string, len(allowPaths)) +copy(notPaths, allowPaths) +notPaths = append(notPaths, denyPaths...)pkg/util/helperfunctions.go (2)
70-79: LGTM! Well-structured function with proper error handling.The function has proper error handling and follows Go conventions.
81-81: Fix 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) {
pkg/jwtAuth/authConfig.go
Outdated
| @@ -0,0 +1,53 @@ | |||
| package jwtAuth | |||
There was a problem hiding this comment.
| package jwtAuth | |
| package jwt_auth |
same for folder name
pkg/jwtAuth/identityProvider.go
Outdated
| ClientID string | ||
| } | ||
|
|
||
| type ProviderOps interface { |
There was a problem hiding this comment.
nice job on the interface, but i think that you should move this to the digdirator directory, and rename it Digdirator (If we implement azure, maybe we rename it to provider)
pkg/jwtAuth/idporten.go
Outdated
| return ID_PORTEN | ||
| } | ||
|
|
||
| func (i *IDPortenOps) GetSecret(k8sClient client.Client, ctx context.Context, application skiperatorv1alpha1.Application) (*v1.Secret, error) { |
There was a problem hiding this comment.
move the contents of this file to digdirator/idporten.go
pkg/jwtAuth/maskinporten.go
Outdated
| return MASKINPORTEN | ||
| } | ||
|
|
||
| func (i *MaskinportenOps) GetSecret(k8sClient client.Client, ctx context.Context, application skiperatorv1alpha1.Application) (*v1.Secret, error) { |
There was a problem hiding this comment.
move the contents of this file to digdirator/maskinporten.go
pkg/jwtAuth/authConfig.go
Outdated
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| type AuthConfigs []AuthConfig |
There was a problem hiding this comment.
i think the authconfigs can belong in reconciliation. the functions can be moved back to the application controller
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pkg/reconciliation/reconciliation.go (1)
89-102:⚠️ Potential issueFix potential slice modification issue.
The current implementation modifies the
allowPathsslice in place. This could lead to unexpected behavior if the slice is used elsewhere.Apply this diff to create a new slice instead:
- var allowPaths []string + allowPaths := make([]string, 0) if authorizationSettings != nil && authorizationSettings.AllowList != nil && len(authorizationSettings.AllowList) > 0 { - allowPaths = authorizationSettings.AllowList + allowPaths = append(allowPaths, authorizationSettings.AllowList...) }internal/controllers/application.go (1)
195-198:⚠️ Potential issueImprove 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) }
🧹 Nitpick comments (8)
api/v1alpha1/digdirator/digdirator.go (1)
21-32: Add documentation for interface methods.The
DigdiratorProviderinterface methods lack documentation. Consider adding godoc comments to explain:
- Purpose and expected behavior of each method
- Parameter and return value descriptions
- Error conditions and handling
pkg/reconciliation/reconciliation.go (1)
22-27: Consider using struct instead of slice type.The
AuthConfigstype is defined as a slice which limits extensibility. Consider using a struct that contains the slice:-type AuthConfigs []AuthConfig +type AuthConfigs struct { + Configs []AuthConfig +}This allows adding more fields in the future without breaking changes.
api/v1alpha1/digdirator/maskinporten.go (1)
87-95: Improve error handling in HandleDigdiratorClientError.Consider wrapping errors with more context and using sentinel errors for specific cases.
Apply this diff to improve error handling:
+var ( + ErrMaskinportenClientNotFound = fmt.Errorf("MaskinportenClient not found") +) func (i *Maskinporten) HandleDigdiratorClientError(digdiratorClients DigdiratorClients) error { if digdiratorClients.MaskinPortenClient.Error != nil { - return fmt.Errorf("failed to get MaskinportenClient: %w", digdiratorClients.MaskinPortenClient.Error) + return fmt.Errorf("failed to get MaskinportenClient: %w", digdiratorClients.MaskinPortenClient.Error) } if digdiratorClients.MaskinPortenClient.Client == nil { - return fmt.Errorf("MaskinportenClient not found") + return ErrMaskinportenClientNotFound } return nil }pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (1)
16-55: Add more detailed logging for debugging.Consider adding more debug logs to help troubleshoot issues:
- Log when skipping due to
AllowAll- Log the number of auth configs found
- Log the allowed paths being used
pkg/resourcegenerator/istio/requestauthentication/request_authentication.go (2)
33-39: Improve error handling for missing auth config.The current debug log might miss important security implications when no authentication is configured. Consider using a warning log to make it more visible.
if authConfig == nil { - ctxLog.Debug("No RequestAuthentication to generate. No auth config provided for", "application", application.Name) + ctxLog.Warn("No authentication configuration provided - the application will accept unauthenticated requests", "application", application.Name) } else {
71-73: Consider additional cookie security settings.When using cookies for JWT storage, consider adding secure cookie attributes.
if authentication.TokenLocation == "cookie" { - jwtRule.FromCookies = []string{"BearerToken"} + jwtRule.FromCookies = []string{"BearerToken"} + // TODO: Add support for secure cookie attributes in the Authentication struct + // jwtRule.CookieAttributes = &v1beta1.CookieAttributes{ + // HttpOnly: true, + // Secure: true, + // SameSite: "Strict", + // } }pkg/util/helperfunctions.go (1)
188-189: Simplify boolean expression.The comparison to
trueis redundant.- return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled == true + return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled🧰 Tools
🪛 golangci-lint (1.62.2)
188-188: S1002: should omit comparison to bool constant, can be simplified to
application.Spec.Maskinporten.Authentication.Enabled(gosimple)
api/v1alpha1/digdirator/idporten.go (1)
96-98: Consider separating authentication state checks.The IsEnabled method combines multiple conditions which could be separated for better testability and reuse.
+func (i *IDPorten) IsAuthenticationEnabled() bool { + return i.Authentication != nil && i.Authentication.Enabled +} + func (i *IDPorten) IsEnabled() bool { - return i != nil && i.Enabled && i.Authentication != nil && i.Authentication.Enabled + return i != nil && i.Enabled && i.IsAuthenticationEnabled() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/v1alpha1/application_types.go(1 hunks)api/v1alpha1/digdirator/digdirator.go(1 hunks)api/v1alpha1/digdirator/idporten.go(2 hunks)api/v1alpha1/digdirator/maskinporten.go(2 hunks)internal/controllers/application.go(6 hunks)pkg/reconciliation/application.go(2 hunks)pkg/reconciliation/reconciliation.go(5 hunks)pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go(1 hunks)pkg/resourcegenerator/istio/requestauthentication/request_authentication.go(1 hunks)pkg/util/helperfunctions.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/v1alpha1/application_types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/reconciliation/application.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/util/helperfunctions.go
188-188: 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: Analyze (go)
- GitHub Check: Build and run tests
- GitHub Check: Build and run tests
🔇 Additional comments (4)
api/v1alpha1/digdirator/maskinporten.go (1)
36-39:⚠️ Potential issueFix potential nil pointer dereference.
The
IsEnabledmethod accessesi.Authenticationwithout checking if it's nil first.Apply this diff to fix the issue:
func (i *Maskinporten) IsEnabled() bool { - return i != nil && i.Enabled && i.Authentication != nil && i.Authentication.Enabled + return i != nil && i.Enabled && i.Authentication != nil && i.Authentication.EnabledLikely an incorrect or invalid review comment.
pkg/resourcegenerator/istio/requestauthentication/request_authentication.go (1)
67-90: Validate JWT configuration values.The JWT configuration values (Issuer, JWKS URI, and Audience) are critical security parameters that should be validated.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if the URIs use HTTPS and are properly formatted # Search for URI validation in the codebase rg -A 5 'func.*ValidateURI|func.*validateURI|func.*validateURL|func.*ValidateURL' # Search for any existing JWT validation ast-grep --pattern 'func $name($_) { $$$ if !strings.HasPrefix($uri, "https://") { $$$ } $$$ }'Length of output: 193
JWT configuration parameters lack validation in the codebase
Our investigation did not uncover any logic that validates the critical JWT configuration values—namely, the Issuer, JWKS URI, and Audience—before they are assigned. In particular, there is no check to ensure that the URIs use a secure "https://" scheme. This omission could lead to a security risk if misconfigured URIs are inadvertently accepted.
- The function
getJWTRulesimply assigns the values fromproviderURIswithout verifying they are properly formatted or secure.- Searches for functions or patterns like
ValidateURI,validateURL, or checks usingstrings.HasPrefix(..., "https://")returned no results.- It is recommended to introduce explicit validation of these JWT parameters (Issuer, JWKS URI, and Audience) to ensure they meet expected security standards.
pkg/util/helperfunctions.go (1)
70-79: LGTM! Well-structured error handling.The function properly handles both "not found" and other error cases, with clear error messages.
api/v1alpha1/digdirator/idporten.go (1)
85-87: LGTM! Well-documented field.The Authentication field is properly documented with JSON tags and clear purpose.
| type DigdiratorURIs struct { | ||
| Name DigdiratorName | ||
| IssuerURI string | ||
| JwksURI string | ||
| ClientID string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for URIs and ClientID.
The DigdiratorURIs struct contains critical security-related fields. Consider adding validation to ensure:
IssuerURIandJwksURIare valid HTTPS URLsClientIDfollows the expected format for the identity provider
| func (i *Maskinporten) GetIgnoredPaths() []string { | ||
| var ignoredPaths []string | ||
| if i.IsEnabled() { | ||
| if i.Authentication.IgnorePaths != nil { | ||
| ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...) | ||
| } | ||
| } | ||
| return ignoredPaths | ||
| } |
There was a problem hiding this comment.
Fix potential nil pointer dereference in GetIgnoredPaths.
The method accesses i.Authentication.IgnorePaths without checking if i.Authentication is nil.
Apply this diff to fix the issue:
func (i *Maskinporten) GetIgnoredPaths() []string {
var ignoredPaths []string
- if i.IsEnabled() {
+ if i.IsEnabled() && i.Authentication != nil {
if i.Authentication.IgnorePaths != nil {
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...)
}
}
return ignoredPaths
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (i *Maskinporten) GetIgnoredPaths() []string { | |
| var ignoredPaths []string | |
| if i.IsEnabled() { | |
| if i.Authentication.IgnorePaths != nil { | |
| ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...) | |
| } | |
| } | |
| return ignoredPaths | |
| } | |
| func (i *Maskinporten) GetIgnoredPaths() []string { | |
| var ignoredPaths []string | |
| if i.IsEnabled() && i.Authentication != nil { | |
| if i.Authentication.IgnorePaths != nil { | |
| ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...) | |
| } | |
| } | |
| return ignoredPaths | |
| } |
| func getJwtValidationAuthPolicy(namespacedName types.NamespacedName, applicationName string, authConfigs []reconciliation.AuthConfig, allowPaths []string, denyPaths []string) *securityv1.AuthorizationPolicy { | ||
| var authPolicyRules []*securityv1api.Rule | ||
|
|
||
| notPaths := allowPaths | ||
| notPaths = append(allowPaths, denyPaths...) | ||
| for _, authConfig := range authConfigs { | ||
| authPolicyRules = append(authPolicyRules, &securityv1api.Rule{ | ||
| To: []*securityv1api.Rule_To{ | ||
| { | ||
| Operation: &securityv1api.Operation{ | ||
| NotPaths: notPaths, | ||
| }, | ||
| }, | ||
| }, | ||
| When: []*securityv1api.Condition{ | ||
| { | ||
| Key: "request.auth.claims[iss]", | ||
| Values: []string{authConfig.ProviderURIs.IssuerURI}, | ||
| }, | ||
| }, | ||
| From: authorizationpolicy.GetGeneralFromRule(), | ||
| }) | ||
| } | ||
| return &securityv1.AuthorizationPolicy{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: namespacedName.Namespace, | ||
| Name: namespacedName.Name, | ||
| }, | ||
| Spec: securityv1api.AuthorizationPolicy{ | ||
| Action: securityv1api.AuthorizationPolicy_ALLOW, | ||
| Rules: authPolicyRules, | ||
| Selector: &typev1beta1.WorkloadSelector{ | ||
| MatchLabels: util.GetPodAppSelector(applicationName), | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix slice modification and verify default deny paths.
The function has two issues:
-
Modifies
allowPathsslice in place -
Security implications of default deny paths need verification
-
Apply this diff to fix slice modification:
- notPaths := allowPaths
- notPaths = append(allowPaths, denyPaths...)
+ notPaths := make([]string, len(allowPaths))
+ copy(notPaths, allowPaths)
+ notPaths = append(notPaths, denyPaths...)- Run this script to verify default deny paths:
✅ Verification successful
🏁 Script executed:
#!/bin/bash
# Description: Verify that default deny paths are consistently defined and used.
# Search for DefaultDenyPath constant
rg -A 2 'DefaultDenyPath\s*='
# Search for deny path usage in authorization policies
ast-grep --pattern 'NotPaths: $$$'Length of output: 277
Slice Modification Fix & Default Deny Paths Consistency Verified
-
Slice Modification: Update the creation of
notPathsas shown below to avoid modifying theallowPathsslice in place:- notPaths := allowPaths - notPaths = append(allowPaths, denyPaths...) + notPaths := make([]string, len(allowPaths)) + copy(notPaths, allowPaths) + notPaths = append(notPaths, denyPaths...)
-
Default Deny Paths: The constant
DefaultDenyPathis defined as"/actuator*"inpkg/resourcegenerator/istio/authorizationpolicy/common.goand is used consistently across the codebase in constructing authorization policies.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getJwtValidationAuthPolicy(namespacedName types.NamespacedName, applicationName string, authConfigs []reconciliation.AuthConfig, allowPaths []string, denyPaths []string) *securityv1.AuthorizationPolicy { | |
| var authPolicyRules []*securityv1api.Rule | |
| notPaths := allowPaths | |
| notPaths = append(allowPaths, denyPaths...) | |
| for _, authConfig := range authConfigs { | |
| authPolicyRules = append(authPolicyRules, &securityv1api.Rule{ | |
| To: []*securityv1api.Rule_To{ | |
| { | |
| Operation: &securityv1api.Operation{ | |
| NotPaths: notPaths, | |
| }, | |
| }, | |
| }, | |
| When: []*securityv1api.Condition{ | |
| { | |
| Key: "request.auth.claims[iss]", | |
| Values: []string{authConfig.ProviderURIs.IssuerURI}, | |
| }, | |
| }, | |
| From: authorizationpolicy.GetGeneralFromRule(), | |
| }) | |
| } | |
| return &securityv1.AuthorizationPolicy{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Namespace: namespacedName.Namespace, | |
| Name: namespacedName.Name, | |
| }, | |
| Spec: securityv1api.AuthorizationPolicy{ | |
| Action: securityv1api.AuthorizationPolicy_ALLOW, | |
| Rules: authPolicyRules, | |
| Selector: &typev1beta1.WorkloadSelector{ | |
| MatchLabels: util.GetPodAppSelector(applicationName), | |
| }, | |
| }, | |
| } | |
| } | |
| func getJwtValidationAuthPolicy(namespacedName types.NamespacedName, applicationName string, authConfigs []reconciliation.AuthConfig, allowPaths []string, denyPaths []string) *securityv1.AuthorizationPolicy { | |
| var authPolicyRules []*securityv1api.Rule | |
| notPaths := make([]string, len(allowPaths)) | |
| copy(notPaths, allowPaths) | |
| notPaths = append(notPaths, denyPaths...) | |
| for _, authConfig := range authConfigs { | |
| authPolicyRules = append(authPolicyRules, &securityv1api.Rule{ | |
| To: []*securityv1api.Rule_To{ | |
| { | |
| Operation: &securityv1api.Operation{ | |
| NotPaths: notPaths, | |
| }, | |
| }, | |
| }, | |
| When: []*securityv1api.Condition{ | |
| { | |
| Key: "request.auth.claims[iss]", | |
| Values: []string{authConfig.ProviderURIs.IssuerURI}, | |
| }, | |
| }, | |
| From: authorizationpolicy.GetGeneralFromRule(), | |
| }) | |
| } | |
| return &securityv1.AuthorizationPolicy{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Namespace: namespacedName.Namespace, | |
| Name: namespacedName.Name, | |
| }, | |
| Spec: securityv1api.AuthorizationPolicy{ | |
| Action: securityv1api.AuthorizationPolicy_ALLOW, | |
| Rules: authPolicyRules, | |
| Selector: &typev1beta1.WorkloadSelector{ | |
| MatchLabels: util.GetPodAppSelector(applicationName), | |
| }, | |
| }, | |
| } | |
| } |
internal/controllers/application.go
Outdated
| func (r *ApplicationReconciler) getAuthConfig(ctx context.Context, application skiperatorv1alpha1.Application, digdiratorProvider digdirator.DigdiratorProvider) (*AuthConfig, error) { | ||
| secret, err := r.getAuthConfigSecret(ctx, application, digdiratorProvider) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get auth config secret for %s: %w", digdiratorProvider.GetDigdiratorName(), err) | ||
| } | ||
| return &AuthConfig{ | ||
| NotPaths: digdiratorProvider.GetIgnoredPaths(), | ||
| ProviderURIs: digdirator.DigdiratorURIs{ | ||
| Name: digdiratorProvider.GetDigdiratorName(), | ||
| IssuerURI: string(secret.Data[digdiratorProvider.GetIssuerKey()]), | ||
| JwksURI: string(secret.Data[digdiratorProvider.GetJwksKey()]), | ||
| ClientID: string(secret.Data[digdiratorProvider.GetClientIDKey()]), | ||
| }, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for retrieved URIs.
The URIs retrieved from secrets should be validated before use in authentication configuration.
+func validateURI(uri string) error {
+ if !strings.HasPrefix(uri, "https://") {
+ return fmt.Errorf("URI must use HTTPS: %s", uri)
+ }
+ _, err := url.Parse(uri)
+ return err
+}
+
func (r *ApplicationReconciler) getAuthConfig(ctx context.Context, application skiperatorv1alpha1.Application, digdiratorProvider digdirator.DigdiratorProvider) (*AuthConfig, error) {
secret, err := r.getAuthConfigSecret(ctx, application, digdiratorProvider)
if err != nil {
return nil, fmt.Errorf("failed to get auth config secret for %s: %w", digdiratorProvider.GetDigdiratorName(), err)
}
+ issuerURI := string(secret.Data[digdiratorProvider.GetIssuerKey()])
+ jwksURI := string(secret.Data[digdiratorProvider.GetJwksKey()])
+
+ if err := validateURI(issuerURI); err != nil {
+ return nil, fmt.Errorf("invalid issuer URI: %w", err)
+ }
+ if err := validateURI(jwksURI); err != nil {
+ return nil, fmt.Errorf("invalid JWKS URI: %w", err)
+ }
+
return &AuthConfig{
NotPaths: digdiratorProvider.GetIgnoredPaths(),
ProviderURIs: digdirator.DigdiratorURIs{
Name: digdiratorProvider.GetDigdiratorName(),
- IssuerURI: string(secret.Data[digdiratorProvider.GetIssuerKey()]),
- JwksURI: string(secret.Data[digdiratorProvider.GetJwksKey()]),
+ IssuerURI: issuerURI,
+ JwksURI: jwksURI,
ClientID: string(secret.Data[digdiratorProvider.GetClientIDKey()]),
},
}, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *ApplicationReconciler) getAuthConfig(ctx context.Context, application skiperatorv1alpha1.Application, digdiratorProvider digdirator.DigdiratorProvider) (*AuthConfig, error) { | |
| secret, err := r.getAuthConfigSecret(ctx, application, digdiratorProvider) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get auth config secret for %s: %w", digdiratorProvider.GetDigdiratorName(), err) | |
| } | |
| return &AuthConfig{ | |
| NotPaths: digdiratorProvider.GetIgnoredPaths(), | |
| ProviderURIs: digdirator.DigdiratorURIs{ | |
| Name: digdiratorProvider.GetDigdiratorName(), | |
| IssuerURI: string(secret.Data[digdiratorProvider.GetIssuerKey()]), | |
| JwksURI: string(secret.Data[digdiratorProvider.GetJwksKey()]), | |
| ClientID: string(secret.Data[digdiratorProvider.GetClientIDKey()]), | |
| }, | |
| }, nil | |
| } | |
| import ( | |
| "fmt" | |
| "net/url" | |
| "strings" | |
| // other imports | |
| ) | |
| func validateURI(uri string) error { | |
| if !strings.HasPrefix(uri, "https://") { | |
| return fmt.Errorf("URI must use HTTPS: %s", uri) | |
| } | |
| _, err := url.Parse(uri) | |
| return err | |
| } | |
| func (r *ApplicationReconciler) getAuthConfig(ctx context.Context, application skiperatorv1alpha1.Application, digdiratorProvider digdirator.DigdiratorProvider) (*AuthConfig, error) { | |
| secret, err := r.getAuthConfigSecret(ctx, application, digdiratorProvider) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get auth config secret for %s: %w", digdiratorProvider.GetDigdiratorName(), err) | |
| } | |
| issuerURI := string(secret.Data[digdiratorProvider.GetIssuerKey()]) | |
| jwksURI := string(secret.Data[digdiratorProvider.GetJwksKey()]) | |
| if err := validateURI(issuerURI); err != nil { | |
| return nil, fmt.Errorf("invalid issuer URI: %w", err) | |
| } | |
| if err := validateURI(jwksURI); err != nil { | |
| return nil, fmt.Errorf("invalid JWKS URI: %w", err) | |
| } | |
| return &AuthConfig{ | |
| NotPaths: digdiratorProvider.GetIgnoredPaths(), | |
| ProviderURIs: digdirator.DigdiratorURIs{ | |
| Name: digdiratorProvider.GetDigdiratorName(), | |
| IssuerURI: issuerURI, | |
| JwksURI: jwksURI, | |
| ClientID: string(secret.Data[digdiratorProvider.GetClientIDKey()]), | |
| }, | |
| }, nil | |
| } |
Co-authored-by: Lars Sørensen <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
internal/controllers/application.go (2)
195-198:⚠️ Potential issueImprove error handling for auth config retrieval.
The error from
getAuthConfigsForApplicationis only logged but not propagated, which could lead to silent failures in authentication setup.Apply this diff to properly handle the error:
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) }
448-463: 🛠️ Refactor suggestionAdd URI validation for authentication endpoints.
The URIs retrieved from secrets should be validated before use in authentication configuration.
Apply this diff to add URI validation:
+func validateURI(uri string) error { + if !strings.HasPrefix(uri, "https://") { + return fmt.Errorf("URI must use HTTPS: %s", uri) + } + _, err := url.Parse(uri) + return err +} func (r *ApplicationReconciler) getAuthConfig(ctx context.Context, application skiperatorv1alpha1.Application, digdiratorProvider digdirator.DigdiratorProvider) (*AuthConfig, error) { secret, err := r.getAuthConfigSecret(ctx, application, digdiratorProvider) if err != nil { return nil, fmt.Errorf("failed to get auth config secret for %s: %w", digdiratorProvider.GetDigdiratorName(), err) } + issuerURI := string(secret.Data[digdiratorProvider.GetIssuerKey()]) + jwksURI := string(secret.Data[digdiratorProvider.GetJwksKey()]) + + if err := validateURI(issuerURI); err != nil { + return nil, fmt.Errorf("invalid issuer URI: %w", err) + } + if err := validateURI(jwksURI); err != nil { + return nil, fmt.Errorf("invalid JWKS URI: %w", err) + } + return &AuthConfig{ Spec: digdiratorProvider.GetAuthSpec(), NotPaths: digdiratorProvider.GetIgnoredPaths(), ProviderURIs: digdirator.DigdiratorURIs{ Name: digdiratorProvider.GetDigdiratorName(), - IssuerURI: string(secret.Data[digdiratorProvider.GetIssuerKey()]), - JwksURI: string(secret.Data[digdiratorProvider.GetJwksKey()]), + IssuerURI: issuerURI, + JwksURI: jwksURI, ClientID: string(secret.Data[digdiratorProvider.GetClientIDKey()]), }, }, nil
🧹 Nitpick comments (7)
api/v1alpha1/digdirator/digdirator.go (1)
18-27: Add documentation for interface methods.The
DigdiratorProviderinterface methods lack documentation explaining their purpose, parameters, and return values. This documentation is crucial for maintainability and proper usage.pkg/reconciliation/reconciliation.go (2)
25-29: Add documentation for AuthConfig struct.The struct fields lack documentation explaining their purpose and usage.
91-104: Simplify GetAllowedPaths method.The method can be simplified by extracting the allowlist check into a separate function.
Apply this diff to improve readability:
+func getAllowListPaths(authorizationSettings *v1alpha1.AuthorizationSettings) []string { + if authorizationSettings != nil && authorizationSettings.AllowList != nil { + return authorizationSettings.AllowList + } + return nil +} func (authConfigs *AuthConfigs) GetAllowedPaths(authorizationSettings *v1alpha1.AuthorizationSettings) []string { - var allowPaths []string - if authorizationSettings != nil && authorizationSettings.AllowList != nil && len(authorizationSettings.AllowList) > 0 { - allowPaths = authorizationSettings.AllowList - } + allowPaths := getAllowListPaths(authorizationSettings) if authConfigs != nil { for _, config := range *authConfigs { if config.NotPaths != nil { allowPaths = append(allowPaths, config.NotPaths...) } } } return allowPaths }pkg/resourcegenerator/istio/requestauthentication/request_authentication.go (1)
64-66: Extract hardcoded cookie name into a constant.The cookie name "BearerToken" should be defined as a constant for better maintainability.
Apply this diff:
+const ( + DefaultBearerTokenCookie = "BearerToken" +) if authConfig.Spec.TokenLocation == "cookie" { - jwtRule.FromCookies = []string{"BearerToken"} + jwtRule.FromCookies = []string{DefaultBearerTokenCookie} }pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (1)
39-52: Simplify condition and improve readability.The condition
len(*authConfigs) > 0could be simplified, and the resource addition could be more concise.Apply this diff to improve the code:
- if len(*authConfigs) > 0 { - r.AddResource( - getJwtValidationAuthPolicy( - types.NamespacedName{ - Namespace: application.Namespace, - Name: application.Name + "-jwt-auth", - }, - application.Name, - *authConfigs, - allowedPaths, - authorizationpolicy.DefaultDenyPath, - ), - ) - } + if len(*authConfigs) == 0 { + return nil + } + namespacedName := types.NamespacedName{ + Namespace: application.Namespace, + Name: application.Name + "-jwt-auth", + } + r.AddResource(getJwtValidationAuthPolicy( + namespacedName, + application.Name, + *authConfigs, + allowedPaths, + authorizationpolicy.DefaultDenyPath, + ))api/v1alpha1/digdirator/idporten.go (1)
83-85: Add validation tag for Authentication field.The
Authenticationfield lacks validation tags that could help catch configuration issues early.Apply this diff to add validation:
// Authentication specifies how incoming JWT's should be validated. +// +kubebuilder:validation:Optional Authentication *istiotypes.Authentication `json:"authentication,omitempty"`pkg/util/helperfunctions.go (1)
187-189: Simplify boolean expression.The boolean comparison can be simplified.
Apply this diff to simplify the code:
- return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled == true + return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled🧰 Tools
🪛 golangci-lint (1.62.2)
188-188: S1002: should omit comparison to bool constant, can be simplified to
application.Spec.Maskinporten.Authentication.Enabled(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/v1alpha1/digdirator/digdirator.go(1 hunks)api/v1alpha1/digdirator/idporten.go(2 hunks)api/v1alpha1/digdirator/maskinporten.go(2 hunks)internal/controllers/application.go(6 hunks)pkg/reconciliation/reconciliation.go(5 hunks)pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go(1 hunks)pkg/resourcegenerator/istio/requestauthentication/request_authentication.go(1 hunks)pkg/util/helperfunctions.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/util/helperfunctions.go
188-188: 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: Analyze (go)
- GitHub Check: Build and run tests
🔇 Additional comments (4)
api/v1alpha1/digdirator/digdirator.go (1)
11-16: Add validation for URIs and ClientID.The
DigdiratorURIsstruct contains critical security-related fields that require validation.api/v1alpha1/digdirator/maskinporten.go (1)
45-53: Fix potential nil pointer dereference in GetIgnoredPaths.The method accesses
i.Authentication.IgnorePathswithout checking ifi.Authenticationis nil.pkg/resourcegenerator/istio/requestauthentication/request_authentication.go (1)
31-33: Add warning log for missing auth config.When no auth config is provided, the current debug log might miss important security implications.
pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (1)
60-61: Fix slice modification issue.The current implementation modifies the
allowPathsslice in place, which could lead to unexpected behavior if the slice is used elsewhere.Apply this diff to fix the slice modification:
- notPaths := allowPaths - notPaths = append(allowPaths, denyPath) + notPaths := make([]string, len(allowPaths)) + copy(notPaths, allowPaths) + notPaths = append(notPaths, denyPath)
| func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication { | ||
| return *i.Authentication | ||
| } |
There was a problem hiding this comment.
Fix potential nil pointer dereference in GetAuthSpec.
The method doesn't check if Authentication is nil before dereferencing it.
Apply this diff to fix the issue:
func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication {
+ if i.Authentication == nil {
+ return istiotypes.Authentication{}
+ }
return *i.Authentication
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication { | |
| return *i.Authentication | |
| } | |
| func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication { | |
| if i.Authentication == nil { | |
| return istiotypes.Authentication{} | |
| } | |
| return *i.Authentication | |
| } |
| func (i *Maskinporten) GetProvidedSecretName() *string { | ||
| return i.Authentication.SecretName | ||
| } |
There was a problem hiding this comment.
Fix potential nil pointer dereference in GetProvidedSecretName.
The method doesn't check if Authentication is nil before accessing its fields.
Apply this diff to fix the issue:
func (i *Maskinporten) GetProvidedSecretName() *string {
+ if i.Authentication == nil {
+ return nil
+ }
return i.Authentication.SecretName
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (i *Maskinporten) GetProvidedSecretName() *string { | |
| return i.Authentication.SecretName | |
| } | |
| func (i *Maskinporten) GetProvidedSecretName() *string { | |
| if i.Authentication == nil { | |
| return nil | |
| } | |
| return i.Authentication.SecretName | |
| } |
| jwtRule.Issuer = authConfig.ProviderURIs.IssuerURI | ||
| jwtRule.JwksUri = authConfig.ProviderURIs.JwksURI | ||
| jwtRule.Audiences = []string{authConfig.ProviderURIs.ClientID} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add validation for JWT configuration values.
The JWT configuration values (Issuer, JWKS URI, and Audience) are critical security parameters and should be validated.
🌐 Web query:
What are the requirements for valid JWT issuer and JWKS URIs?
💡 Result:
JWT issuers and JWKS URIs have specific technical requirements to ensure secure token validation in modern authentication systems. Below are the key specifications:
JWT Issuer Requirements
-
HTTPS URL format
Must usehttps://scheme with valid domain/host components[1][4]. Example:
issuer: "https://mycompany.corp" -
Exact string matching
Theissclaim must match exactly with registered issuers (case-sensitive)[4][6].
https://example.com≠https://example.com/secure -
Structured components
May contain:- Port numbers (
https://company:8443) - Path components (
https://auth.company/oidc)
But must exclude: - Query parameters (
?key=value) - Fragments (
#section)[1]
- Port numbers (
JWKS URI Requirements
| Property | Specification |
|---|---|
| Protocol | Must use HTTPS[1][9] |
| Content Type | Must return valid JSON Web Key Set (RFC 7517)[2][8] |
| Key Structure | Requires keys array containing valid JWKs with: - kty (key type) - use (key usage) - alg (algorithm)[5][8] |
| Security | Must provide current signing keys Should implement key rotation[3][4] |
Security Best Practices
- Dynamic key resolution: Retrieve JWKS from issuer's well-known endpoint (e.g.,
/.well-known/jwks.json)[2][9] - Certificate validation: Verify TLS certificates when fetching JWKS[4][6]
- Caching: Implement JWKS caching with TTL (e.g., 5 minutes)[9]
- Claim verification: Always validate:
iss (issuer) aud (audience) exp (expiration)
Example configuration from Contour API[9]:
jwtProviders:
- name: provider-1
issuer: https://example.com
remoteJWKS:
uri: https://example.com/.well-known/jwks.json
timeout: 1s
cacheDuration: 5mThese requirements help prevent token forgery and ensure proper cryptographic verification of JWT signatures[3][4][6].
Citations:
- 1: https://docs.tetrate.io/service-bridge/refs/onboarding/config/install/v1alpha1/jwt_issuer
- 2: https://auth0.com/docs/secure/tokens/json-web-tokens/json-web-key-sets
- 3: https://www.cryptr.co/documentation/how-to-validate-jwt
- 4: https://curity.io/resources/learn/jwt-best-practices/
- 5: https://auth0.com/docs/secure/tokens/json-web-tokens/json-web-key-set-properties
- 6: https://www.criipto.com/blog/jwt-validation-guide
- 7: https://fusionauth.io/articles/tokens/jwt-components-explained
- 8: https://datatracker.ietf.org/doc/html/rfc7517
- 9: https://projectcontour.io/docs/1.30/config/jwt-verification/
- 10: https://trino.io/docs/current/security/jwt.html
Action Required: Implement Validation Logic for JWT Configuration Values
The current implementation directly assigns configuration values without validating that they meet the security specifications necessary for JWT issuers and JWKS URIs. Based on the web query findings, you should enhance the code to validate the following:
-
Issuer URI:
- Must be an HTTPS URL.
- Should not include query parameters or fragments.
- The URL must exactly match the expected registered issuer.
-
JWKS URI:
- Must use HTTPS.
- The endpoint should return a valid JSON Web Key Set (as defined in RFC 7517) including a valid
keysarray.
-
Audience Value:
- Verify that it is non-empty and correctly formatted for its intended security context.
For example, before assigning these configuration values, consider adding helper functions that:
- Parse and validate the URL structure for both issuer and JWKS URIs.
- Check that the keys in the JWKS response include the required fields.
- Ensure that the audience string is valid as per your application's security requirements.
These validations are critical to prevent token forgery and ensure robust authentication.
| authConfigs := r.GetAuthConfigs() | ||
| allowedPaths := authConfigs.GetAllowedPaths(application.Spec.AuthorizationSettings) | ||
| if authConfigs == nil { | ||
| ctxLog.Debug("No auth configs provided for application. Skipping generating JWT-auth AuthorizationPolicy", "application", application.Name) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Fix potential nil pointer dereference.
The GetAllowedPaths is called before checking if authConfigs is nil, which could lead to a panic.
Apply this diff to fix the order of operations:
- authConfigs := r.GetAuthConfigs()
- allowedPaths := authConfigs.GetAllowedPaths(application.Spec.AuthorizationSettings)
- if authConfigs == nil {
+ authConfigs := r.GetAuthConfigs()
+ if authConfigs == nil {
ctxLog.Debug("No auth configs provided for application. Skipping generating JWT-auth AuthorizationPolicy", "application", application.Name)
return nil
}
+ allowedPaths := authConfigs.GetAllowedPaths(application.Spec.AuthorizationSettings)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| authConfigs := r.GetAuthConfigs() | |
| allowedPaths := authConfigs.GetAllowedPaths(application.Spec.AuthorizationSettings) | |
| if authConfigs == nil { | |
| ctxLog.Debug("No auth configs provided for application. Skipping generating JWT-auth AuthorizationPolicy", "application", application.Name) | |
| return nil | |
| } | |
| authConfigs := r.GetAuthConfigs() | |
| if authConfigs == nil { | |
| ctxLog.Debug("No auth configs provided for application. Skipping generating JWT-auth AuthorizationPolicy", "application", application.Name) | |
| return nil | |
| } | |
| allowedPaths := authConfigs.GetAllowedPaths(application.Spec.AuthorizationSettings) |
| func (i *IDPorten) GetIgnoredPaths() []string { | ||
| var ignoredPaths []string | ||
| if i.IsEnabled() { | ||
| if i.Authentication.IgnorePaths != nil { | ||
| ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...) | ||
| } | ||
| } | ||
| return ignoredPaths | ||
| } |
There was a problem hiding this comment.
Improve error handling in GetIgnoredPaths.
The function doesn't handle the case where Authentication is nil but IsEnabled() returns true.
Apply this diff to add proper nil checks:
func (i *IDPorten) GetIgnoredPaths() []string {
var ignoredPaths []string
if i.IsEnabled() {
+ if i.Authentication == nil {
+ return ignoredPaths
+ }
if i.Authentication.IgnorePaths != nil {
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...)
}
}
return ignoredPaths
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (i *IDPorten) GetIgnoredPaths() []string { | |
| var ignoredPaths []string | |
| if i.IsEnabled() { | |
| if i.Authentication.IgnorePaths != nil { | |
| ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...) | |
| } | |
| } | |
| return ignoredPaths | |
| } | |
| func (i *IDPorten) GetIgnoredPaths() []string { | |
| var ignoredPaths []string | |
| if i.IsEnabled() { | |
| if i.Authentication == nil { | |
| return ignoredPaths | |
| } | |
| if i.Authentication.IgnorePaths != nil { | |
| ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...) | |
| } | |
| } | |
| return ignoredPaths | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
api/v1alpha1/digdirator/maskinporten.go (2)
46-48:⚠️ Potential issueFix potential nil pointer dereference.
The function directly dereferences
i.Authenticationwithout checking for nil, which could cause a panic.Apply this diff to fix the issue:
func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication { + if i.Authentication == nil { + return istiotypes.Authentication{} + } return *i.Authentication }
54-56:⚠️ Potential issueFix potential nil pointer dereference.
The function directly accesses
i.Authentication.SecretNamewithout checking for nil, which could cause a panic.Apply this diff to fix the issue:
func (i *Maskinporten) GetProvidedSecretName() *string { + if i.Authentication == nil { + return nil + } return i.Authentication.SecretName }internal/controllers/application.go (1)
448-464: 🛠️ Refactor suggestionAdd validation for URIs retrieved from secrets.
The URIs retrieved from secrets should be validated to ensure they are valid HTTPS URLs.
Apply this diff to add URI validation:
+func validateURI(uri string) error { + if !strings.HasPrefix(uri, "https://") { + return fmt.Errorf("URI must use HTTPS: %s", uri) + } + _, err := url.Parse(uri) + return err +} + func (r *ApplicationReconciler) getAuthConfig(ctx context.Context, application skiperatorv1alpha1.Application, digdiratorProvider digdirator.DigdiratorProvider) (*AuthConfig, error) { secret, err := r.getAuthConfigSecret(ctx, application, digdiratorProvider) if err != nil { return nil, fmt.Errorf("failed to get auth config secret for %s: %w", digdiratorProvider.GetDigdiratorName(), err) } + issuerURI := string(secret.Data[digdiratorProvider.GetIssuerKey()]) + jwksURI := string(secret.Data[digdiratorProvider.GetJwksKey()]) + + if err := validateURI(issuerURI); err != nil { + return nil, fmt.Errorf("invalid issuer URI: %w", err) + } + if err := validateURI(jwksURI); err != nil { + return nil, fmt.Errorf("invalid JWKS URI: %w", err) + } + return &AuthConfig{ Spec: digdiratorProvider.GetAuthSpec(), Paths: digdiratorProvider.GetPaths(), IgnorePaths: digdiratorProvider.GetIgnoredPaths(), ProviderURIs: digdirator.DigdiratorURIs{ Name: digdiratorProvider.GetDigdiratorName(), - IssuerURI: string(secret.Data[digdiratorProvider.GetIssuerKey()]), - JwksURI: string(secret.Data[digdiratorProvider.GetJwksKey()]), + IssuerURI: issuerURI, + JwksURI: jwksURI, ClientID: string(secret.Data[digdiratorProvider.GetClientIDKey()]), }, }, nil }
🧹 Nitpick comments (5)
pkg/reconciliation/reconciliation.go (2)
23-30: Add documentation for new types.Consider adding documentation for
AuthConfigsandAuthConfigtypes to explain their purpose and usage.Apply this diff to add documentation:
+// AuthConfigs represents a collection of authentication configurations type AuthConfigs []AuthConfig +// AuthConfig represents a single authentication configuration with paths and provider URIs type AuthConfig struct { Spec istiotypes.Authentication Paths []string IgnorePaths []string ProviderURIs digdirator.DigdiratorURIs }
92-105: Optimize slice allocation.The function could be more efficient by pre-allocating the slice with an estimated capacity.
Apply this diff to optimize slice allocation:
func (authConfigs *AuthConfigs) GetAllowedPaths(authorizationSettings *v1alpha1.AuthorizationSettings) []string { - var allowPaths []string + // Pre-allocate slice with estimated capacity + var capacity int + if authorizationSettings != nil && authorizationSettings.AllowList != nil { + capacity = len(authorizationSettings.AllowList) + } + if authConfigs != nil { + for _, config := range *authConfigs { + if config.IgnorePaths != nil { + capacity += len(config.IgnorePaths) + } + } + } + allowPaths := make([]string, 0, capacity) if authorizationSettings != nil && authorizationSettings.AllowList != nil && len(authorizationSettings.AllowList) > 0 { allowPaths = authorizationSettings.AllowList } if authConfigs != nil { for _, config := range *authConfigs { if config.IgnorePaths != nil { allowPaths = append(allowPaths, config.IgnorePaths...) } } } return allowPaths }pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (1)
16-54: Enhance logging for better observability.Consider adding more detailed logging to help with debugging and monitoring.
Apply this diff to enhance logging:
func Generate(r reconciliation.Reconciliation) error { ctxLog := r.GetLogger() application, ok := r.GetSKIPObject().(*skiperatorv1alpha1.Application) if !ok { err := fmt.Errorf("failed to cast resource to application") ctxLog.Error(err, "Failed to generate JWT-auth AuthorizationPolicy") return err } ctxLog.Debug("Attempting to generate JWT-auth AuthorizationPolicy for application", "application", application.Name) if application.Spec.AuthorizationSettings != nil { // Do not create an AuthorizationPolicy if allowAll is set to true if application.Spec.AuthorizationSettings.AllowAll == true { + ctxLog.Debug("AllowAll is set to true, skipping JWT-auth AuthorizationPolicy", "application", application.Name) return nil } } authConfigs := r.GetAuthConfigs() if authConfigs == nil { ctxLog.Debug("No auth configs provided for application. Skipping generating JWT-auth AuthorizationPolicy", "application", application.Name) return nil } if len(*authConfigs) > 0 { + ctxLog.Debug("Generating JWT-auth AuthorizationPolicy", "application", application.Name, "authConfigsCount", len(*authConfigs)) r.AddResource( getJwtValidationAuthPolicy( types.NamespacedName{ Namespace: application.Namespace, Name: application.Name + "-jwt-auth", }, application.Name, *authConfigs, application.Spec.AuthorizationSettings, authorizationpolicy.DefaultDenyPath, ), ) } ctxLog.Debug("Finished generating JWT-auth AuthorizationPolicy for application", "application", application.Name) return nil }pkg/util/helperfunctions.go (1)
193-194: Simplify boolean comparison.The comparison to
trueis redundant and can be simplified.Apply this diff to simplify the boolean expression:
- return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled == true + return application.Spec.Maskinporten != nil && application.Spec.Maskinporten.Authentication != nil && application.Spec.Maskinporten.Authentication.Enabled🧰 Tools
🪛 golangci-lint (1.62.2)
193-193: S1002: should omit comparison to bool constant, can be simplified to
application.Spec.Maskinporten.Authentication.Enabled(gosimple)
config/crd/skiperator.kartverket.no_applications.yaml (1)
556-633: Extract common authentication schema to avoid duplication.The authentication blocks in IDPorten and Maskinporten are identical. Consider extracting the common schema to a shared definition.
Apply this diff to extract the common schema:
+ # 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 + forwardOriginalToken: + default: true + description: If set to `true`, the original token will be kept for the upstream request. + type: boolean + ignorePaths: + description: IgnorePaths specifies paths that do not require an authenticated JWT. + items: + type: string + maxItems: 50 + type: array + x-kubernetes-list-type: set + outputClaimToHeaders: + description: List of operations to copy claims to HTTP headers. + 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 + paths: + description: Paths specifies paths that require an authenticated JWT. + items: + type: string + maxItems: 50 + type: array + x-kubernetes-list-type: set + secretName: + description: The name of the kubernetes Secret containing OAuth2-credentials. + type: string + tokenLocation: + default: header + description: Where to find the JWT in the incoming request. + enum: + - header + - cookie + type: string + 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"Also applies to: 836-912
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/v1alpha1/digdirator/digdirator.go(1 hunks)api/v1alpha1/digdirator/idporten.go(2 hunks)api/v1alpha1/digdirator/maskinporten.go(2 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(6 hunks)pkg/reconciliation/reconciliation.go(5 hunks)pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go(1 hunks)pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go(1 hunks)pkg/util/helperfunctions.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/v1alpha1/istiotypes/jwt_authentication.go
- pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go
- api/v1alpha1/digdirator/digdirator.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/util/helperfunctions.go
193-193: 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: Analyze (go)
- GitHub Check: Build and run tests
🔇 Additional comments (5)
api/v1alpha1/digdirator/maskinporten.go (3)
58-66: LGTM!The function correctly handles nil checks and safely accesses paths.
68-76: LGTM!The function correctly handles nil checks and safely accesses ignored paths.
78-88: LGTM!The getter functions are correctly implemented and return the appropriate keys.
api/v1alpha1/istiotypes/zz_generated.deepcopy.go (1)
9-54: LGTM!The auto-generated deepcopy functions correctly handle nil checks and deep copying of all fields in the Authentication struct.
pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (1)
56-90:❓ Verification inconclusive
Add validation for paths and URIs.
Consider adding validation for paths and URIs to ensure they are well-formed.
Run this script to check for path validation in similar code:
🏁 Script executed:
#!/bin/bash # Description: Check for path validation patterns in the codebase # Search for path validation patterns rg -A 5 'func.*ValidatePath|path\..*Valid|path\..*Check' # Search for URI validation patterns rg -A 5 'func.*ValidateURI|url\..*Valid|url\..*Check'Length of output: 114
Action Required: Validate Paths and URIs
The
getJwtValidationAuthPolicyfunction correctly constructs the AuthorizationPolicy but does not perform any explicit validation on the inputs—specifically, the path patterns (authConfig.PathsandauthConfig.IgnorePaths) and the client ID URI (authConfig.ProviderURIs.ClientID). Given that our repository search (using regex for typical validation functions) returned no hints of such validations, please review whether these values are assumed to be pre-validated or if additional checks should be introduced at this stage. This would help guard against misconfigurations due to malformed paths or URIs.
- Confirm if validation is handled upstream; if not, consider adding validation logic in or before this function.
| func (i *IDPorten) GetAuthSpec() istiotypes.Authentication { | ||
| return *i.Authentication | ||
| } |
There was a problem hiding this comment.
Add nil check to prevent panic in GetAuthSpec.
The function could panic if Authentication is nil. Add a nil check before dereferencing.
Apply this diff to add proper nil checks:
func (i *IDPorten) GetAuthSpec() istiotypes.Authentication {
+ if i == nil || i.Authentication == nil {
+ return istiotypes.Authentication{}
+ }
return *i.Authentication
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (i *IDPorten) GetAuthSpec() istiotypes.Authentication { | |
| return *i.Authentication | |
| } | |
| func (i *IDPorten) GetAuthSpec() istiotypes.Authentication { | |
| if i == nil || i.Authentication == nil { | |
| return istiotypes.Authentication{} | |
| } | |
| return *i.Authentication | |
| } |
| func (i *IDPorten) GetProvidedSecretName() *string { | ||
| return i.Authentication.SecretName | ||
| } |
There was a problem hiding this comment.
Add nil check to prevent panic in GetProvidedSecretName.
The function could panic if Authentication is nil. Add a nil check before accessing the field.
Apply this diff to add proper nil checks:
func (i *IDPorten) GetProvidedSecretName() *string {
+ if i == nil || i.Authentication == nil {
+ return nil
+ }
return i.Authentication.SecretName
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (i *IDPorten) GetProvidedSecretName() *string { | |
| return i.Authentication.SecretName | |
| } | |
| func (i *IDPorten) GetProvidedSecretName() *string { | |
| if i == nil || i.Authentication == nil { | |
| return nil | |
| } | |
| return i.Authentication.SecretName | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (3)
33-38: Optimize auth config checks.The current implementation can be simplified and made more efficient.
Apply this diff to optimize the auth config checks:
authConfigs := r.GetAuthConfigs() - if authConfigs == nil { + if authConfigs == nil || len(*authConfigs) == 0 { ctxLog.Debug("No auth configs provided for application. Skipping generating JWT-auth AuthorizationPolicy", "application", application.Name) return nil } - if len(*authConfigs) > 0 { - r.AddResource(
74-77: Simplify pointer syntax.The pointer syntax can be simplified by using the address operator directly.
Apply this diff to simplify the pointer syntax:
authPolicyRules = append(authPolicyRules, &securityv1api.Rule{ To: []*securityv1api.Rule_To{ - &ruleTo, + { + Operation: ruleTo.Operation, + }, },
56-104: Add input validation.The function should validate its input parameters to ensure robustness.
Add validation for the input parameters:
func getJwtValidationAuthPolicy(namespacedName types.NamespacedName, applicationName string, authConfigs []reconciliation.AuthConfig, authorizationSettings *skiperatorv1alpha1.AuthorizationSettings, denyPath string) *securityv1.AuthorizationPolicy { + if namespacedName.Name == "" || namespacedName.Namespace == "" { + return nil + } + if applicationName == "" { + return nil + } + if len(authConfigs) == 0 { + return nil + }pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go (2)
34-37: Remove redundant condition.The condition at line 35 is redundant with the check at line 27.
Apply this diff to simplify the code:
var allowedPaths []string - if application.Spec.AuthorizationSettings != nil { - allowedPaths = append(allowedPaths, application.Spec.AuthorizationSettings.AllowList...) - } + if application.Spec.AuthorizationSettings != nil && len(application.Spec.AuthorizationSettings.AllowList) > 0 { + allowedPaths = application.Spec.AuthorizationSettings.AllowList + }
87-113: Add input validation and documentation.The function would benefit from input validation and documentation explaining its purpose and parameters.
Add validation and documentation:
+// getAuthPolicy creates an AuthorizationPolicy resource with the specified action and paths. +// Parameters: +// - namespacedName: The namespace and name for the resource +// - applicationName: The name of the application +// - action: The authorization action (ALLOW or DENY) +// - paths: The paths to apply the action to +// - notPaths: The paths to exclude from the action func getAuthPolicy(namespacedName types.NamespacedName, applicationName string, action v1beta1.AuthorizationPolicy_Action, paths []string, notPaths []string) *securityv1.AuthorizationPolicy { + if namespacedName.Name == "" || namespacedName.Namespace == "" { + return nil + } + if applicationName == "" { + return nil + } + if len(paths) == 0 && len(notPaths) == 0 { + return nil + }pkg/reconciliation/reconciliation.go (1)
92-98: Consider pre-allocating the slice for better performance.For better performance with large configurations, consider pre-allocating the
ignoredPathsslice with an estimated capacity:- var ignoredPaths []string + totalPaths := 0 + for _, config := range *a { + totalPaths += len(config.IgnorePaths) + } + ignoredPaths := make([]string, 0, totalPaths)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/reconciliation/reconciliation.go(5 hunks)pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go(1 hunks)pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and run tests
- GitHub Check: Analyze (go)
- GitHub Check: Build and run tests
🔇 Additional comments (3)
pkg/resourcegenerator/istio/authorizationpolicy/jwt_auth/authorization_policy.go (1)
1-14: LGTM!The package structure and imports are well-organized, using appropriate dependencies for Istio security and Kubernetes types.
pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go (1)
1-15: LGTM!The package structure and imports are well-organized, using appropriate dependencies for Istio security and Kubernetes types.
pkg/reconciliation/reconciliation.go (1)
23-30: Consider adding validation for URIs and ClientID.The
AuthConfigstruct contains security-critical fields fromdigdirator.DigdiratorURIs. Consider adding validation to ensure:
- URIs are valid HTTPS URLs
- ClientID follows the expected format for the identity provider
Additionally, based on the past review discussion, consider moving this struct closer to where it's used.
| @@ -28,6 +39,7 @@ type Reconciliation interface { | |||
| AddResource(client.Object) | |||
| GetIdentityConfigMap() *corev1.ConfigMap | |||
| GetRestConfig() *rest.Config | |||
| GetAuthConfigs() *AuthConfigs | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider nil pointer handling for authConfigs.
The GetAuthConfigs() method returns a pointer to AuthConfigs. Ensure that callers handle the nil case appropriately, as authConfigs field in baseReconciliation could be nil.
Also applies to: 53-53
🏁 Script executed:
#!/bin/bash
# Search for GetAuthConfigs() usage to verify nil checks
rg -A 5 'GetAuthConfigs\(\)'Length of output: 2966
Ensure proper nil pointer handling for authConfigs.
The GetAuthConfigs() method returns a pointer that could be nil. While modules like request authentication and JWT auth already check for nil before use, the default deny authorization case in pkg/resourcegenerator/istio/authorizationpolicy/default_deny/authorization_policy.go dereferences the result (via GetIgnoredPaths()) without a nil guard. Please update this usage (and verify similar scenarios, e.g., at lines 53-53 in pkg/reconciliation/reconciliation.go) to handle a possible nil value safely.
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
New Features
Improvements