Customizable client authorization #84
Conversation
WalkthroughThis pull request introduces a new authorization configuration mechanism to the project. It adds new API types and their deepcopy methods, updates configuration loading to support both authentication and authorization, and refactors the main initialization flow accordingly. The PR also extends helm chart configurations by adding authorization settings and implements a new CEL-based authorizer along with its configuration loader. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant ConfigLoader as LoadConfiguration
participant AuthConfigLoader as LoadAuthorizationConfiguration
participant CEL as NewCELAuthorizer
Main->>ConfigLoader: Request configuration load
ConfigLoader->>ConfigMap: Retrieve auth & authorization data
ConfigLoader->>AuthConfigLoader: Load authorization configuration
AuthConfigLoader->>CEL: If type == "CEL", compile expression and create CELAuthorizer
CEL-->>AuthConfigLoader: Return CELAuthorizer instance
AuthConfigLoader-->>ConfigLoader: Return authorizer instance
ConfigLoader-->>Main: Return authenticator and authorizer
Possibly related PRs
Suggested reviewers
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 (
|
NickCao
force-pushed
the
cel-authorization
branch
from
174cfaf to
b153a1a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
internal/config/config.go (2)
23-23: Refactored function signature to returnauthentication.ContextAuthenticator,authorizer.Authorizer, anderror.This change correctly aligns the function's outputs with the new approach of loading both authentication and authorization from the config. Consider whether returning multiple values might be better encapsulated in a struct for improved maintainability, although it's optional.
31-31: Enhanced error message for missing authentication configuration.This message helps clarify which section is missing, but for consistency, ensure the style matches other error messages (e.g., beginning with "missing 'xyz' section").
internal/authorization/cel.go (2)
21-23:Expressionstruct straightforwardly wraps the CEL expression string.Consider renaming
Expressionfield to something likeCodeorRuleto avoid shadowing the struct name, although it's not critical.
33-47: Environment initialization for CEL is well-structured.The usage of
environment.MustBaseEnvSetand extension points for variables is appropriate. Consider logging or handling advanced environment features for debugging complex rules if needed.internal/authorization/config.go (1)
21-29: Consider adding validation for empty configuration.While the function handles decoding errors, it might be worth adding explicit validation for empty configuration bytes.
func LoadAuthorizationConfiguration( ctx context.Context, scheme *runtime.Scheme, configuration []byte, reader client.Reader, prefix string, ) (authorizer.Authorizer, error) { + if len(configuration) == 0 { + return nil, fmt.Errorf("empty authorization configuration") + } var authorizationConfiguration jumpstarterdevv1alpha1.AuthorizationConfigurationdeploy/helm/jumpstarter/values.yaml (1)
97-103: Authorization Configuration Block is Well-Structured!The new
authorizationConfigblock is clearly defined and mirrors the style of the existingauthenticationConfigsection. The multi-line YAML string is appropriately used to encapsulate the authorization configuration details. In particular, the declaration of the API version, kind, type, and CEL-based expression is consistent with the intended design.A couple of suggestions:
- Validation of the CEL Expression: Ensure that downstream components (e.g., the configuration loader and the CEL-based authorizer) validate the expression
"self.spec.username == user.username"correctly. Consider adding tests or inline documentation that specifies any constraints or expected format for these expressions.- Documentation: It may be beneficial to enhance inline comments or update the documentation for
jumpstarter-controllerparameters, explaining the purpose and usage ofauthorizationConfigto aid users in customizing client authorization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/v1alpha1/authorizationconfiguration_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)cmd/main.go(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml(1 hunks)deploy/helm/jumpstarter/values.yaml(1 hunks)internal/authorization/cel.go(1 hunks)internal/authorization/config.go(1 hunks)internal/config/config.go(2 hunks)
🔇 Additional comments (23)
internal/config/config.go (7)
7-8: Imports for new authentication and authorization functionality look valid.No issues observed. The addition of these imports is consistent with the new requirement to load both authentication and authorization within this module.
12-12: Import for theauthorizerinterface is appropriate.This aligns with the newly introduced return type requiring an
authorizer.Authorizer.
26-26: Updated return statement to(nil, nil, err)is correct.Returning three values here is now mandatory due to the new signature.
34-43: OIDC-based authentication loading appears well-structured.The code consistently handles decoding and error scenarios. Ensure you have sufficient test coverage for potential parsing errors from OIDC, especially if the configuration is malformed.
45-48: Authorization configuration check is properly mirrored from the authentication check.This symmetry helps maintain consistency. Keep the error message uniform with the authentication check for better clarity.
50-59: CallsLoadAuthorizationConfigurationto parse the new authorization config.The approach is consistent with the separation of concerns: OIDC config is in
oidc, while authorization config is inauthorization. Looks good.
61-61: Returns a bearer token authenticator and the loaded authorizer.This final return effectively ties together authentication and authorization. Good job linking the dependencies.
internal/authorization/cel.go (6)
1-2: Package declaration for the new authorization logic is appropriately structured.No concerns at this top level.
3-5: Import statements are aligned with CEL-based authorization.The external dependencies (
cel-go,k8s.io/apiserver, etc.) are relevant and correctly scoped for this implementation.Also applies to: 7-14
16-20:CELAuthorizerstruct introduces a clear, minimal set of fields.Storing the compiled CEL program is efficient. The
readerandprefixfields look well-defined for resource fetching and user prefixing.
25-27: Getter method simply returns the expression string.Clean and concise.
29-31:ReturnTypes()indicates a boolean return type for CEL expressions.This is properly aligned with typical authorization decisions.
49-50: Compilation of the CEL expression and creation ofCELAuthorizer.Compiling the expression once avoids repeated overhead at runtime. Error handling is clear. Good job.
Also applies to: 51-56, 58-63
api/v1alpha1/authorizationconfiguration_types.go (6)
1-2: New packagev1alpha1introduced for authorization configuration is well-scoped.This versioned approach aligns with Kubernetes API best practices.
3-5: Imports are minimal and fit the CRD style.No further concerns.
7-8: Deepcopy generation annotation is standard for CRD definitions.This is necessary to ensure proper CRD behavior in Kubernetes.
9-14:AuthorizationConfigurationstructure is well-defined.The
Typefield and a pointer toCELConfigurationallow flexible extension for future authorization types.
16-18:CELConfigurationstruct uses a singleExpressionfor CEL rules.Straightforward and sufficiently descriptive.
20-22:init()properly registersAuthorizationConfigurationwith the SchemeBuilder.Ensures the Kubernetes API can recognize and manage these objects.
internal/authorization/config.go (1)
14-20: LGTM! Well-structured function signature with comprehensive parameters.The function signature is well-designed, accepting all necessary dependencies for configuration loading and authorization setup.
cmd/main.go (1)
154-154: LGTM! Clean separation of authentication and authorization concerns.The refactoring to separate authentication and authorization is a good practice, improving modularity and maintainability.
api/v1alpha1/zz_generated.deepcopy.go (1)
61-104: LGTM! Generated deepcopy methods are consistent with existing patterns.The auto-generated deepcopy methods for
AuthorizationConfigurationandCELConfigurationfollow the same patterns as other types in the codebase.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml (1)
14-14: LGTM! Consistent configuration structure.The authorization configuration follows the same pattern as the existing authentication configuration, maintaining consistency in the ConfigMap structure.
| func (b *CELAuthorizer) Authorize( | ||
| ctx context.Context, | ||
| attributes authorizer.Attributes, | ||
| ) (authorizer.Decision, string, error) { | ||
| var self map[string]interface{} | ||
| var err error | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Authorize() method handles resource fetching and CEL evaluation logically.
- The logic for retrieving
ExporterorClientis correct, but consider gracefully handling the case wherespecdoesn't exist before using.(map[string]any), to avoid potential runtime panics. - The CEL evaluation and final decision returning
DecisionAlloworDecisionDenyis consistent with standard authorizer patterns.
Apply a safer approach for type assertions on self["spec"] to prevent panics, for example:
- self["spec"].(map[string]any)["username"] = e.Username(b.prefix)
+ spec, ok := self["spec"].(map[string]any)
+ if !ok {
+ return authorizer.DecisionDeny, "missing spec in resource", fmt.Errorf("spec field not found or invalid")
+ }
+ spec["username"] = e.Username(b.prefix)Also applies to: 72-85, 87-99, 100-101, 103-116, 118-127
| switch authorizationConfiguration.Type { | ||
| case "Basic": | ||
| return NewBasicAuthorizer(reader, prefix), nil | ||
| case "CEL": | ||
| if authorizationConfiguration.CEL == nil { | ||
| return nil, fmt.Errorf("CEL authorizer configuration missing") | ||
| } | ||
| return NewCELAuthorizer(reader, prefix, authorizationConfiguration.CEL.Expression) | ||
| default: | ||
| return nil, fmt.Errorf("unsupported authorizer type: %s", authorizationConfiguration.Type) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify CEL expression validity during initialization.
For the CEL authorizer type, it would be beneficial to validate the expression syntax during initialization to fail fast on invalid expressions.
🏁 Script executed:
#!/bin/bash
# Description: Search for CEL expression validation in the codebase.
# Test: Look for CEL expression validation in NewCELAuthorizer.
ast-grep --pattern $'func NewCELAuthorizer($_, $_, $expr string) ($_, error) {
$$$
}'
# Test: Look for CEL expression validation in cel.go.
rg -A 5 'NewCELAuthorizer'Length of output: 966
CEL expression validation: Additional check required during initialization
The current implementation calls NewCELAuthorizer with the provided CEL expression without performing any syntax checks. The verification confirms that neither the caller in internal/authorization/config.go nor the implementation in internal/authorization/cel.go includes an explicit validation step for the expression. To improve reliability and fail fast on misconfigurations, please add a validation routine (for example, leveraging the CEL parser) within NewCELAuthorizer before extending the environment.
- File:
internal/authorization/cel.go- Suggestion: Add a step to validate the
expressionparameter, ensuring it adheres to the expected CEL syntax.
- Suggestion: Add a step to validate the
- File:
internal/authorization/config.go- Observation: The switch case for the "CEL" type currently only checks for missing configuration (
authorizationConfiguration.CEL == nil).
- Observation: The switch case for the "CEL" type currently only checks for missing configuration (
Summary by CodeRabbit
New Features
Refactor