Implement OIDC based authentication and authorization #83
Conversation
WalkthroughThis pull request introduces a new API resource—ExporterAccessPolicy—while enhancing multiple API types. Several new structs and fields are added for authentication, client, exporter, and lease configurations. Deep copy methods are updated, and numerous CRDs and sample YAMLs are modified. Changes are also applied to Helm charts, dependency updates in go.mod, and internal packages for authentication, authorization, OIDC, and controller logic. Removed files include the mock server and token-related code, marking a shift towards an integrated OIDC-based security model. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ControllerService
participant OIDCService
participant OIDCSigner
Note over Client,ControllerService: OIDC Authentication Flow
Client->>ControllerService: Send request with token
ControllerService->>OIDCService: Authenticate token using Authn, Authz, Attr
OIDCService->>OIDCSigner: Validate token / generate new JWT
OIDCSigner-->>OIDCService: Return token status & details
OIDCService-->>ControllerService: Relay authentication result
ControllerService-->>Client: Respond with authorized access
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (5)
✨ 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
oidc-design
branch
6 times, most recently
from
6f037fa to
13591b6
Compare
NickCao
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: 11
🔭 Outside diff range comments (3)
internal/service/controller_service.go (1)
525-527: Correct variable assignment for EndTimeIn the
GetLeasemethod, whenlease.Status.EndTimeis not nil, the code incorrectly assignsbeginTimeinstead ofendTime. This results inendTimeremainingnileven when it should have a value. Please assign the value toendTimeinstead.Apply this diff to fix the assignment:
var endTime *timestamppb.Timestamp if lease.Status.EndTime != nil { - beginTime = timestamppb.New(lease.Status.EndTime.Time) + endTime = timestamppb.New(lease.Status.EndTime.Time) } var exporterUuid *stringapi/v1alpha1/exporter_types.go (1)
46-47: Fix incorrect type reference.The condition type constants are using
LeaseConditionTypeinstead ofExporterConditionType.- ExporterConditionTypeRegistered LeaseConditionType = "Registered" - ExporterConditionTypeOnline LeaseConditionType = "Online" + ExporterConditionTypeRegistered ExporterConditionType = "Registered" + ExporterConditionTypeOnline ExporterConditionType = "Online"internal/service/router_service.go (1)
52-78: Enhance JWT validation security.While the JWT validation is good, consider these security improvements:
- Use environment variables for all JWT configuration
- Add token expiry time validation
- Consider adding token replay protection
func (s *RouterService) authenticate(ctx context.Context) (string, error) { token, err := authentication.BearerTokenFromContext(ctx) if err != nil { return "", err } + // Load configuration from environment + key := os.Getenv("ROUTER_KEY") + if key == "" { + return "", status.Errorf(codes.Internal, "router key not configured") + } + + maxTokenAge := 5 * time.Minute // Consider making this configurable + parsed, err := jwt.ParseWithClaims( token, &jwt.RegisteredClaims{}, - func(t *jwt.Token) (interface{}, error) { return []byte(os.Getenv("ROUTER_KEY")), nil }, + func(t *jwt.Token) (interface{}, error) { return []byte(key), nil }, jwt.WithIssuer("https://jumpstarter.dev/stream"), jwt.WithAudience("https://jumpstarter.dev/router"), jwt.WithIssuedAt(), jwt.WithExpirationRequired(), + jwt.WithLeeway(30 * time.Second), // Allow for clock skew jwt.WithValidMethods([]string{ jwt.SigningMethodHS256.Name, jwt.SigningMethodHS384.Name, jwt.SigningMethodHS512.Name, }), ) if err != nil || !parsed.Valid { return "", status.Errorf(codes.InvalidArgument, "invalid jwt token") } + // Validate token age + if claims, ok := parsed.Claims.(*jwt.RegisteredClaims); ok { + if time.Since(claims.IssuedAt.Time) > maxTokenAge { + return "", status.Errorf(codes.InvalidArgument, "token too old") + } + } return parsed.Claims.GetSubject() }
🧹 Nitpick comments (18)
internal/authorization/metadata.go (2)
15-19: Document that metadata keys should be lower-caseTo prevent potential issues with metadata key retrieval due to case sensitivity, it's important to note that gRPC metadata keys are expected to be lower-case. Adding comments to the
MetadataAttributesGetterConfigfields will enhance code clarity and maintainability.Apply this diff to add comments:
type MetadataAttributesGetterConfig struct { + // NamespaceKey is the metadata key for the namespace (expected in lower-case). NamespaceKey string + // ResourceKey is the metadata key for the resource (expected in lower-case). ResourceKey string + // NameKey is the metadata key for the name (expected in lower-case). NameKey string }
63-72: Normalize metadata keys to lower-case inmdGetfunctionSince gRPC metadata keys are normalized to lower-case, ensuring that the keys used in
mdGetare also lower-case will prevent potential mismatches due to case differences.Apply this diff to normalize keys in
mdGet:+import "strings" func mdGet(md metadata.MD, k string) (string, error) { + k = strings.ToLower(k) v := md.Get(k) if len(v) < 1 { return "", status.Errorf(codes.InvalidArgument, "missing metadata: %s", k) } if len(v) > 1 { return "", status.Errorf(codes.InvalidArgument, "multiple metadata: %s", k) } return v[0], nil }Ensure the
stringspackage is imported.internal/authentication/bearer.go (1)
58-59: Trim whitespace from the extracted tokenWhen extracting the token from the authorization header, leading or trailing whitespaces may cause token validation to fail. It's advisable to trim any whitespace from the token to ensure accurate authentication.
Apply this diff to trim the token:
token := authorization[7:] +token = strings.TrimSpace(token)Ensure the
stringspackage is imported.internal/authorization/basic.go (1)
24-53: Handle unknown resource types explicitlyIn the
Authorizemethod, thedefaultcase returns aDecisionDenywith the reason "invalid object kind". It might be better to log or handle unknown resource types to aid in debugging and provide clearer error messaging.Consider updating the default case:
default: + // Log the unknown resource type + // log.Printf("Unknown resource type: %s", attributes.GetResource()) return authorizer.DecisionDeny, "unknown resource type", nilinternal/oidc/token.go (1)
28-30: Provide more specific authentication failure messages.Returning a generic error message "failed to authenticate token" may not provide sufficient context. Consider returning more detailed error messages or using structured error handling to assist in debugging authentication issues.
internal/oidc/config.go (1)
25-34: Avoid mutating the input configuration object.The function modifies the
authenticationConfigurationby appending toJWT, which alters the original input. To prevent unexpected side effects, consider creating a copy of the configuration before making modifications.Apply this diff to create a copy:
func LoadAuthenticationConfiguration( ctx context.Context, scheme *runtime.Scheme, configuration []byte, signer *Signer, certificateAuthority string, ) (authenticator.Token, error) { + var authenticationConfigurationCopy jumpstarterdevv1alpha1.AuthenticationConfiguration + if err := deepcopy.Copy(&authenticationConfigurationCopy, &authenticationConfiguration); err != nil { + return nil, err + } + authenticationConfiguration = authenticationConfigurationCopy authenticationConfiguration.JWT = append(authenticationConfiguration.JWT, apiserverv1beta1.JWTAuthenticator{ // existing code... }) // rest of the function... }internal/controller/lease_controller.go (1)
353-359: Select exporter based on policy priorityCurrently, the first approved exporter is selected without considering the
Policy.Priority. To ensure that the exporter with the highest priority is chosen, please sort theapprovedExportersslice based on thePolicy.Prioritybefore selecting the exporter.Apply this diff to sort the approved exporters by priority:
+ // Sort approved exporters by priority in descending order + sort.SliceStable(approvedExporters, func(i, j int) bool { + return approvedExporters[i].Policy.Priority > approvedExporters[j].Policy.Priority + }) selected := approvedExporters[0] lease.Status.Priority = selected.Policy.Priority lease.Status.SpotAccess = selected.Policy.SpotAccess lease.Status.ExporterRef = &corev1.LocalObjectReference{ Name: selected.Exporter.Name, }Remember to import the
sortpackage at the top of the file:import ( "context" "fmt" "slices" + "sort" "time"internal/config/config.go (2)
14-21: Consider adding parameter validation.The function takes several critical parameters but lacks validation. Consider adding checks for nil values of
client,scheme,signer, and emptycertificateAuthoritystring.func LoadConfiguration( ctx context.Context, client client.Reader, scheme *runtime.Scheme, key client.ObjectKey, signer *oidc.Signer, certificateAuthority string, ) (authenticator.Token, error) { + if client == nil { + return nil, fmt.Errorf("client cannot be nil") + } + if scheme == nil { + return nil, fmt.Errorf("scheme cannot be nil") + } + if signer == nil { + return nil, fmt.Errorf("signer cannot be nil") + } + if certificateAuthority == "" { + return nil, fmt.Errorf("certificateAuthority cannot be empty") + }
32-41: Consider adding context timeout.The OIDC configuration loading might take time. Consider adding a timeout to prevent long-running operations.
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() authenticator, err := oidc.LoadAuthenticationConfiguration( ctx, scheme, []byte(rawAuthenticationConfiguration), signer, certificateAuthority, )api/v1alpha1/client_types.go (1)
29-29: Consider adding field validation.The optional Username field might benefit from validation rules using kubebuilder markers.
+// +kubebuilder:validation:Pattern=^[a-zA-Z0-9_-]+$ +// +kubebuilder:validation:MinLength=1 +// +kubebuilder:validation:MaxLength=32 Username *string `json:"username,omitempty"`api/v1alpha1/exporter_types.go (1)
29-29: Consider adding field validation and documentation.The optional Username field should have the same validation rules as in ClientSpec for consistency.
+// Username is the identity of the exporter used for OIDC authentication. +// +kubebuilder:validation:Pattern=^[a-zA-Z0-9_-]+$ +// +kubebuilder:validation:MinLength=1 +// +kubebuilder:validation:MaxLength=32 Username *string `json:"username,omitempty"`api/v1alpha1/exporteraccesspolicy_types.go (2)
26-28: Add validation and documentation for ClientSelector.Consider adding:
- Validation markers to ensure valid label selectors
- Documentation about the expected label format and usage
type From struct { + // ClientSelector is a label selector that determines which clients this policy applies to. + // +kubebuilder:validation:Required ClientSelector metav1.LabelSelector `json:"clientSelector,omitempty"` }
30-35: Add validation constraints and documentation for Policy fields.The Policy struct needs validation markers and clear documentation for its fields:
- Priority should have min/max values
- MaximumDuration should have reasonable bounds
type Policy struct { + // Priority determines the order in which policies are evaluated. + // Higher numbers indicate higher priority. + // +kubebuilder:validation:Minimum=-100 + // +kubebuilder:validation:Maximum=100 Priority int `json:"priority,omitempty"` + // From defines the clients this policy applies to From []From `json:"from,omitempty"` + // MaximumDuration specifies the maximum time a lease can be held + // +kubebuilder:validation:Format=duration MaximumDuration *metav1.Duration `json:"maximumDuration,omitempty"` + // SpotAccess indicates if this is a preemptible lease SpotAccess bool `json:"spotAccess,omitempty"` }api/v1alpha1/lease_types.go (1)
44-45: Add documentation for new status fields.The new Priority and SpotAccess fields need documentation to explain their relationship with ExporterAccessPolicy.
+ // Priority reflects the priority level of the policy that granted this lease Priority int `json:"priority,omitempty"` + // SpotAccess indicates whether this lease was granted as a preemptible lease SpotAccess bool `json:"spotAccess,omitempty"`internal/controller/client_controller.go (1)
116-137: Improve error handling and logging in secretForClient.The error handling could be more descriptive and include logging for better debugging.
-func (r *ClientReconciler) secretForClient(client *jumpstarterdevv1alpha1.Client) (*corev1.Secret, error) { +func (r *ClientReconciler) secretForClient(client *jumpstarterdevv1alpha1.Client) (*corev1.Secret, error) { + if r.Signer == nil { + return nil, fmt.Errorf("OIDC signer not initialized") + } + token, err := r.Signer.Token(authorization.ClientAuthorizedUsername(client, r.Signer.Prefix())) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to generate token: %w", err) } secret := &corev1.Secret{config/samples/dex.yaml (1)
1-1: Consider using a configurable issuer URL.The hardcoded IP address
10.239.206.8might not be portable across different environments.Consider using environment variables or configuration maps:
-issuer: http://10.239.206.8:5556/dex +issuer: ${DEX_ISSUER_URL}deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml (1)
41-43: New 'username' Field in Exporter SpecThe addition of the
usernameproperty as a string in thespecsection enriches the Exporter CRD schema. Consider adding a brief description for this field (e.g., its purpose or any validation constraints) to improve clarity and future maintainability.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml (1)
92-156: Policies Field StructureThe
policiesarray is comprehensively defined, including nested fields such asfromwith its ownclientSelectorlabel setup. This design supports complex policy definitions. It may be beneficial to further clarify the expected format of fields likemaximumDuration(for example, specifying ISO 8601 duration) and to outline any bounds or valid ranges forpriorityif applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
PROJECT(1 hunks)api/v1alpha1/authenticationconfiguration_types.go(1 hunks)api/v1alpha1/client_types.go(1 hunks)api/v1alpha1/exporter_types.go(1 hunks)api/v1alpha1/exporteraccesspolicy_types.go(1 hunks)api/v1alpha1/lease_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(7 hunks)cmd/main.go(6 hunks)cmd/mock/main.go(0 hunks)config/samples/dex.yaml(1 hunks)config/samples/kustomization.yaml(1 hunks)config/samples/v1alpha1_client.yaml(1 hunks)config/samples/v1alpha1_exporteraccesspolicy.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_clients.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_leases.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role.yaml(1 hunks)go.mod(3 hunks)internal/authentication/bearer.go(2 hunks)internal/authentication/types.go(1 hunks)internal/authorization/basic.go(1 hunks)internal/authorization/metadata.go(1 hunks)internal/authorization/types.go(1 hunks)internal/config/config.go(1 hunks)internal/controller/client_controller.go(2 hunks)internal/controller/exporter_controller.go(2 hunks)internal/controller/lease_controller.go(3 hunks)internal/controller/token.go(0 hunks)internal/oidc/config.go(1 hunks)internal/oidc/op.go(1 hunks)internal/oidc/token.go(1 hunks)internal/service/controller_service.go(4 hunks)internal/service/oidc_service.go(1 hunks)internal/service/router_service.go(2 hunks)
💤 Files with no reviewable changes (2)
- cmd/mock/main.go
- internal/controller/token.go
✅ Files skipped from review due to trivial changes (1)
- config/samples/v1alpha1_client.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml
[error] 10-10: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (28)
internal/oidc/token.go (2)
47-49: Verify the correctness of resource name in authorization checks.The check
attrs.GetResource() != "Client"assumes the resource name is "Client". Kubernetes resource names are typically lowercase and plural, such as "clients". Ensure that the resource name matches the actual Kubernetes resource naming conventions.
83-85: Verify the correctness of resource name in authorization checks.Similarly, the check
attrs.GetResource() != "Exporter"assumes the resource name is "Exporter". Confirm that this matches the Kubernetes resource naming conventions, which are usually lowercase and plural, like "exporters".internal/oidc/config.go (1)
62-89: Ensure proper handling when no JWT authenticators are configured.If
authenticationConfiguration.JWTis empty, the function may return an authenticator that denies all tokens. Verify that at least one JWT authenticator is provided or handle the scenario where none are configured to prevent unintended authentication failures.internal/authentication/types.go (1)
9-11: LGTM! Well-designed interface following Kubernetes patterns.The
ContextAuthenticatorinterface is well-defined, focused, and follows Kubernetes authentication patterns.internal/authorization/types.go (1)
10-12: LGTM! Well-designed interface following Kubernetes patterns.The
ContextAttributesGetterinterface is well-defined, focused, and follows Kubernetes authorization patterns.api/v1alpha1/authenticationconfiguration_types.go (1)
8-15: LGTM! Well-designed type following Kubernetes CRD patterns.The
AuthenticationConfigurationtype is well-defined, properly documented, and follows Kubernetes CRD patterns.internal/config/config.go (2)
22-25: LGTM!Good error handling for ConfigMap retrieval.
27-30: LGTM!Clear error message for missing authentication section.
PROJECT (1)
31-37: LGTM!The new
ExporterAccessPolicyresource is properly configured with appropriate CRD version and namespace settings.internal/controller/exporter_controller.go (4)
32-33: LGTM! New imports for authorization and OIDC.The imports align with the OIDC alternative design implementation.
40-40: LGTM! Added Signer field for OIDC token generation.The
Signerfield is correctly typed as*oidc.Signerfor handling token generation.
46-46: LGTM! Added RBAC permission for ExporterAccessPolicy.The new RBAC permission correctly grants read-only access (get, list, watch) to the
exporteraccesspoliciesresource.
157-158: LGTM! Updated token generation using OIDC Signer.The token generation now uses the OIDC signer with proper authorization username formatting.
Let's verify the token generation by checking the
authorizationpackage implementation:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the ExporterAuthorizedUsername function implementation ast-grep --pattern 'func ExporterAuthorizedUsername($_, $_) string { $$$ }'Length of output: 545
Token generation has been verified.
TheExporterAuthorizedUsernamefunction ininternal/authorization/basic.gocorrectly formats the username by appending the prefix and exporter details when no custom username is specified, and by returning the provided username otherwise. This aligns with the updated token generation approach using the OIDC signer.api/v1alpha1/zz_generated.deepcopy.go (1)
30-555: LGTM! Auto-generated deep copy methods for new types.The deep copy methods are correctly generated for:
- AuthenticationConfiguration with JWT field
- ExporterAccessPolicy with its spec and status
- From with ClientSelector
- Policy with From and MaximumDuration fields
config/samples/kustomization.yaml (1)
6-6: LGTM! Added ExporterAccessPolicy sample resource.The new resource is correctly added to the kustomization configuration.
config/samples/dex.yaml (2)
12-14: Review static client credentials for production use.The static client configuration uses a simple "secret" as the client secret, which is not secure for production environments.
Please ensure this is only used for development/testing purposes. For production, consider:
- Using a more secure secret generation method
- Storing secrets in Kubernetes secrets
- Implementing client credential rotation
19-26: Secure the static password configuration.The configuration includes:
- Password values visible in comments
- Static user IDs
- Fixed bcrypt hash for both users
For production environments:
- Remove password comments
- Use different strong passwords for each user
- Consider implementing an external user management system
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role.yaml (1)
49-56: New RBAC rule for ExporterAccessPolicy.
A new rule for theexporteraccesspoliciesresource has been added with read-only verbs (get,list,watch). Verify that this read-only access is aligned with your intended management strategy for ExporterAccessPolicy resources. If these objects are expected to be created or modified by controllers or users, you might need to include additional verbs (such ascreate,update, ordelete).config/samples/v1alpha1_exporteraccesspolicy.yaml (1)
1-31: New sample YAML for ExporterAccessPolicy.
This new sample file is well-structured and clearly defines anExporterAccessPolicywith an exporter selector and a list of policies (with varying priorities and constraints). Please ensure that the field names (e.g.,exporterSelectorandpolicies) and their values match the definitions in your CRD and API types for consistency.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml (2)
9-11: Review Helm Templating in Metadata Labels.
The conditional block that injectsdeployment.timestampbased on.Values.global.timestampis correctly set up. However, YAML linting reported an error (syntax error: could not find expected ':') on these lines. This is likely due to the Helm templating syntax interfering with YAML parsing. Consider wrapping the templated expression in quotes or using thetoYamlfunction to ensure that the rendered YAML is syntactically correct.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: syntax error: could not find expected ':'
(syntax)
12-40: Authentication Configuration in ConfigMap.
The multi-lineauthenticationfield correctly embeds anAuthenticationConfigurationusing the JWT configuration. Verify that the issuer URL, audience, and certificate authority are current and secured for your OIDC alternative design. The mapping for theusername(using the "sub" claim) is appropriately defined.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_clients.yaml (1)
41-44: New 'username' Field in Client CRD.
The addition of theusernamefield under thespecsection is clear and helps extend the Client resource schema. Please make sure that the corresponding API type and any validation logic (if applicable) reflect this change consistently.go.mod (3)
1-4: Go Version Update.
The Go version has been changed togo 1.22.0(downgraded from 1.22.3). Please confirm that this downgrade is intentional and that it remains compatible with your codebase and dependency requirements.
5-26: Updated and New Dependencies.
New dependencies—includingfilippo.io/keygen,github.com/go-jose/go-jose/v4, andgitproxy.zycloud.tk/zitadel/oidc/v3—have been added to support the OIDC alternative design. Several dependencies (such as the testing librariesginkgoandgomega) have also been updated. Ensure that these changes have been tested thoroughly to avoid compatibility issues.
28-130: Indirect Dependencies Verification.
The indirect dependencies have been updated alongside your core module requirements. It is advisable to double-check that there are no version conflicts between these newly added or updated modules.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml (2)
1-7: New ExporterAccessPolicy CRD DefinitionThis entirely new CRD is well-structured and adheres to Kubernetes API conventions. The metadata, versioning, and annotations are correctly defined.
43-91: ExporterSelector Field DefinitionThe
exporterSelectorfield is defined using bothmatchExpressionsandmatchLabels, offering flexible resource selection capabilities. The structure follows best practices for Kubernetes label selectors. If there are any specific constraints or usage expectations for theusernameor other labels on Exporters, consider elaborating that in an additional description.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_leases.yaml (1)
210-213: Addition of 'priority' and 'spotAccess' in Lease CRD StatusThe new properties
priority(integer) andspotAccess(boolean) in thestatusschema effectively extend the Lease CRD to support enhanced status reporting. Please verify that these fields are mirrored in the correspondingLeaseStatusstruct (inapi/v1alpha1/lease_types.go) and that any business logic processing these fields is updated accordingly.
NickCao
force-pushed
the
oidc-design
branch
3 times, most recently
from
d69bcec to
2c0f06a
Compare
|
Summary of changes for reviewer: AuthenticationConfigurationBorrowed from https://kubernetes.io/docs/reference/access-authn-authz/authentication/#using-authentication-configuration Allows you to specify multiple upstream OIDC providers and how to map upstream usernames to jumpstarter usernames. e.g. ClientSpec/ExporterSpecA new optional field ExporterAccessPolicyA new CRD ExporterAccessPolicy is added to control which Client can lease which Exporter for how long. If no ExporterAccessPolicy is present, all access is allowed. LeaseStatusTwo new fields |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/oidc/op.go (2)
37-46:⚠️ Potential issueUse cryptographically secure key generation.
This code uses math/rand seeded by sha256, which is deterministic. For production-grade security, switch to crypto/rand to ensure truly unpredictable keys.
93-106:⚠️ Potential issueRevise handling for external subjects and reduce token lifetime.
- Returning a placeholder token for external OIDC providers is insufficient and may lead to security gaps; consider returning an explicit error if not supported.
- The 365-day token lifetime is too long. Short token lifetimes (e.g., 1 hour) plus key rotation mitigates token misuse.
🧹 Nitpick comments (15)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml (1)
16-39: OIDC JWT Configuration (Commented Block).The commented-out JWT configuration block provides a useful template for enabling OIDC. When you’re ready to activate OIDC, remember to update the issuer URL, audiences, and certificate authority accordingly, and then uncomment this section. Also, double-check the indentation of the certificate block to ensure it remains valid YAML.
internal/authorization/basic.go (3)
16-18: Add input validation for the prefix parameter.Consider validating that the prefix parameter is not empty to prevent potential issues with username generation.
func NewBasicAuthorizer(reader client.Reader, prefix string) *BasicAuthorizer { + if prefix == "" { + prefix = "internal:" + } return &BasicAuthorizer{reader: reader, prefix: prefix} }
20-54: Improve code structure and reduce duplication.The authorization logic for Exporter and Client is very similar. Consider refactoring to reduce duplication and improve readability.
func (b *BasicAuthorizer) Authorize( ctx context.Context, attributes authorizer.Attributes, ) (authorizer.Decision, string, error) { + resourceType := attributes.GetResource() + if resourceType != "Exporter" && resourceType != "Client" { + return authorizer.DecisionDeny, "invalid object kind", nil + } + + key := client.ObjectKey{ + Namespace: attributes.GetNamespace(), + Name: attributes.GetName(), + } + + var username string + switch resourceType { + case "Exporter": + var e jumpstarterdevv1alpha1.Exporter + if err := b.reader.Get(ctx, key, &e); err != nil { + return authorizer.DecisionDeny, "failed to get exporter", err + } + username = ExporterAuthorizedUsername(&e, b.prefix) + case "Client": + var c jumpstarterdevv1alpha1.Client + if err := b.reader.Get(ctx, key, &c); err != nil { + return authorizer.DecisionDeny, "failed to get client", err + } + username = ClientAuthorizedUsername(&c, b.prefix) + } + + if username == attributes.GetUser().GetName() { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "", nil }
56-70: Reduce duplication in username generation logic.Both helper functions share similar logic for generating usernames. Consider extracting the common logic into a shared helper function.
+func generateDefaultUsername(prefix, kind, namespace, name string, uid types.UID) string { + return prefix + kind + ":" + namespace + ":" + name + ":" + string(uid) +} + func ClientAuthorizedUsername(c *jumpstarterdevv1alpha1.Client, prefix string) string { if c.Spec.Username == nil { - return prefix + "client:" + c.Namespace + ":" + c.Name + ":" + string(c.UID) + return generateDefaultUsername(prefix, "client", c.Namespace, c.Name, c.UID) } else { return *c.Spec.Username } } func ExporterAuthorizedUsername(e *jumpstarterdevv1alpha1.Exporter, prefix string) string { if e.Spec.Username == nil { - return prefix + "exporter:" + e.Namespace + ":" + e.Name + ":" + string(e.UID) + return generateDefaultUsername(prefix, "exporter", e.Namespace, e.Name, e.UID) } else { return *e.Spec.Username } }cmd/main.go (5)
138-142: Consider cert rotation or secure storage.
Creating a self-signed certificate for the OIDC provider is fine for local/demo usage, but for production, consider implementing certificate rotation and secure storage of the private key to adhere to best security practices.
144-153: Ensure the OIDC seed is securely handled.
Passing the signing seed via the “CONTROLLER_KEY” environment variable is convenient but can be risky if the environment is not locked down. For stronger security, ensure it’s loaded from a secure storage service or secrets manager to prevent accidental disclosure.
177-177: Use consistent naming or grouping for signers.
Injecting the same OIDC signer in the ExporterReconciler is consistent with the approach in this codebase. Still, if different reconciliation logic eventually needs distinct signers or issuance policies, consider a more modular approach.
208-215: Check advanced authorization needs.
Using a basic authorizer and metadata attributes is a good step. In the future, consider more granular RBAC or policy-based checks if your system grows in complexity, to avoid large condition blocks.
228-234: Multiple OIDC providers & additional endpoints.
Creating the “OIDCService” improves flexibility. As this expands, consider how multiple upstream OIDC providers might be supported (e.g., additional routes, dynamic issuer). Also ensure proper concurrency handling if any asynchronous calls are introduced.internal/oidc/token.go (1)
18-33: Gracefully handle failed token authentication.
Your “VerifyOIDCToken” method correctly checks the user’s context for authentication. Verify thorough logging and consider differentiating credentials misconfiguration from actual invalid tokens for better diagnostics.internal/controller/suite_test.go (1)
125-127: Consider using test-specific constants for OIDC configuration.The hardcoded values for OIDC configuration should be moved to test constants for better maintainability.
+const ( + testOIDCIssuer = "https://example.com" + testOIDCAudience = "dummy" + testOIDCPrefix = "dummy:" +) + func createExporters(ctx context.Context, exporters ...*jumpstarterdevv1alpha1.Exporter) { // ... - signer, err := oidc.NewSignerFromSeed([]byte{}, "https://example.com", "dummy", "dummy:") + signer, err := oidc.NewSignerFromSeed([]byte{}, testOIDCIssuer, testOIDCAudience, testOIDCPrefix)internal/controller/lease_controller.go (2)
276-281: Consider adding error handling for policy list retrieval.While the error is properly wrapped and returned, consider adding a specific error type or constant for policy-related errors to help with error handling in upstream code.
+const ( + ErrListPolicies = "failed to list exporter access policies" +) var policies jumpstarterdevv1alpha1.ExporterAccessPolicyList if err := r.List(ctx, &policies, client.InNamespace(lease.Namespace), ); err != nil { - return fmt.Errorf("reconcileStatusExporterRef: failed to list exporter access policies: %w", err) + return fmt.Errorf("%s: %w", ErrListPolicies, err) }
297-337: Consider breaking down the nested policy evaluation logic.The deeply nested policy evaluation logic could be extracted into separate methods for better readability and maintainability.
+func (r *LeaseReconciler) evaluatePolicy( + policy jumpstarterdevv1alpha1.ExporterAccessPolicy, + exporter jumpstarterdevv1alpha1.Exporter, + jclient jumpstarterdevv1alpha1.Client, + lease *jumpstarterdevv1alpha1.Lease, +) ([]struct { + Exporter jumpstarterdevv1alpha1.Exporter + Policy jumpstarterdevv1alpha1.Policy +}, error) { + var approved []struct { + Exporter jumpstarterdevv1alpha1.Exporter + Policy jumpstarterdevv1alpha1.Policy + } + + exporterSelector, err := metav1.LabelSelectorAsSelector(&policy.Spec.ExporterSelector) + if err != nil { + return nil, fmt.Errorf("failed to convert exporter selector: %w", err) + } + + if !exporterSelector.Matches(labels.Set(exporter.Labels)) { + return nil, nil + } + + for _, p := range policy.Spec.Policies { + for _, from := range p.From { + clientSelector, err := metav1.LabelSelectorAsSelector(&from.ClientSelector) + if err != nil { + return nil, fmt.Errorf("failed to convert client selector: %w", err) + } + + if !clientSelector.Matches(labels.Set(jclient.Labels)) { + continue + } + + if p.MaximumDuration != nil && lease.Spec.Duration.Duration > p.MaximumDuration.Duration { + continue + } + + approved = append(approved, struct { + Exporter jumpstarterdevv1alpha1.Exporter + Policy jumpstarterdevv1alpha1.Policy + }{ + Exporter: exporter, + Policy: p, + }) + } + } + + return approved, nil +}internal/controller/lease_controller_test.go (1)
339-341: Consider using test constants for OIDC configuration.Extract the OIDC test configuration into constants for better maintainability.
+const ( + testOIDCIssuer = "https://example.com" + testOIDCPrefix = "dummy:" + testOIDCName = "dummy" +) -signer, err := oidc.NewSignerFromSeed([]byte{}, "https://example.com", "dummy", "dummy:") +signer, err := oidc.NewSignerFromSeed([]byte{}, testOIDCIssuer, testOIDCName, testOIDCPrefix)go.mod (1)
3-3: Consider pinning to Go 1.22.0 for better compatibility.The Go version 1.22.3 is very recent. Consider using 1.22.0 for better compatibility with existing tools and CI systems.
-go 1.22.3 +go 1.22.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
PROJECT(1 hunks)api/v1alpha1/authenticationconfiguration_types.go(1 hunks)api/v1alpha1/client_types.go(1 hunks)api/v1alpha1/exporter_types.go(1 hunks)api/v1alpha1/exporteraccesspolicy_types.go(1 hunks)api/v1alpha1/lease_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(7 hunks)cmd/main.go(6 hunks)cmd/mock/main.go(0 hunks)config/samples/dex.yaml(1 hunks)config/samples/kustomization.yaml(1 hunks)config/samples/v1alpha1_client.yaml(1 hunks)config/samples/v1alpha1_exporteraccesspolicy.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_clients.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_leases.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role.yaml(1 hunks)go.mod(3 hunks)internal/authentication/bearer.go(2 hunks)internal/authentication/types.go(1 hunks)internal/authorization/basic.go(1 hunks)internal/authorization/metadata.go(1 hunks)internal/authorization/types.go(1 hunks)internal/config/config.go(1 hunks)internal/controller/client_controller.go(2 hunks)internal/controller/client_controller_test.go(2 hunks)internal/controller/exporter_controller.go(2 hunks)internal/controller/exporter_controller_test.go(2 hunks)internal/controller/lease_controller.go(3 hunks)internal/controller/lease_controller_test.go(2 hunks)internal/controller/suite_test.go(2 hunks)internal/controller/token.go(0 hunks)internal/oidc/config.go(1 hunks)internal/oidc/op.go(1 hunks)internal/oidc/token.go(1 hunks)internal/service/controller_service.go(4 hunks)internal/service/oidc_service.go(1 hunks)internal/service/router_service.go(2 hunks)
💤 Files with no reviewable changes (2)
- cmd/mock/main.go
- internal/controller/token.go
🚧 Files skipped from review as they are similar to previous changes (24)
- config/samples/v1alpha1_client.yaml
- config/samples/kustomization.yaml
- api/v1alpha1/authenticationconfiguration_types.go
- api/v1alpha1/client_types.go
- internal/authorization/types.go
- internal/authentication/types.go
- internal/controller/client_controller.go
- api/v1alpha1/exporter_types.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_clients.yaml
- config/samples/v1alpha1_exporteraccesspolicy.yaml
- api/v1alpha1/lease_types.go
- PROJECT
- internal/service/router_service.go
- internal/service/oidc_service.go
- config/samples/dex.yaml
- internal/oidc/config.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_leases.yaml
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role.yaml
- internal/config/config.go
- internal/authorization/metadata.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yaml
- internal/authentication/bearer.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml
- api/v1alpha1/exporteraccesspolicy_types.go
🧰 Additional context used
🪛 YAMLlint (1.35.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml
[error] 10-10: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (27)
api/v1alpha1/zz_generated.deepcopy.go (6)
27-27: LGTM! Import for JWT authenticator types.The import of
k8s.io/apiserver/pkg/apis/apiserver/v1beta1is correctly added to support JWT authenticator types.
30-59: LGTM! Deep copy implementation for AuthenticationConfiguration.The deep copy functions for
AuthenticationConfigurationcorrectly handle the JWT authenticator slice, ensuring proper deep copying of each element.
66-66: LGTM! Updated deep copy calls for Client and Exporter specs.The changes correctly use
DeepCopyIntoforSpecfields instead of direct assignment, ensuring proper deep copying of nested structures.Also applies to: 192-192
123-127: LGTM! Deep copy implementation for Username field.The deep copy functions for
ClientSpecandExporterSpeccorrectly handle the new optionalUsernamefield by:
- Checking if the field is non-nil
- Allocating new string pointer
- Copying the string value
Also applies to: 346-350
214-310: LGTM! Deep copy implementation for ExporterAccessPolicy.The deep copy functions for
ExporterAccessPolicyand related types correctly handle:
- Object metadata
- Spec with exporter selector and policies
- Empty status struct
402-416: LGTM! Deep copy implementation for From and Policy types.The deep copy functions correctly handle:
Fromtype with client selectorPolicytype with:
- Slice of
Fromobjects- Optional maximum duration field
Also applies to: 530-555
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml (3)
9-11: YAML Templating and Indentation.The conditional directive for the timestamp ({{ if .Values.global.timestamp }}) might trigger YAML lint errors because of unexpected whitespace. Consider using Helm’s trimmed delimiters (e.g. {{- if .Values.global.timestamp }} and {{- end }}) to ensure proper indentation and cleaner rendered output.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: syntax error: could not find expected ':'
(syntax)
12-15: Multi-line Authentication Configuration Block.Ensure that the multi-line string under the "authentication" key correctly represents your intended AuthenticationConfiguration YAML. After Helm renders the template, the embedded configuration should be valid YAML. It might be useful to verify the output with helm template.
40-40: Trailing Incomplete Line.Line 40 appears to be an incomplete or unintended trailing line. This could lead to YAML parsing errors. Consider removing it or completing the intended configuration if something was meant to be declared.
internal/authorization/basic.go (1)
72-72: LGTM! Good practice for interface compliance.The compile-time interface compliance check is a good practice to catch potential interface implementation issues early.
cmd/main.go (3)
20-29: Imports look appropriate.
All recently added imports (context, encoding/pem, net, apiserverinstall, authentication, authorization, config, oidc) are relevant to the new OIDC functionality and do not appear to introduce duplication or conflicts.Also applies to: 43-47
155-172: Validate external dependencies and configuration data.
Loading OIDC configuration via “LoadConfiguration” is a good approach. However, ensure that any external calls within this configuration process are robustly error-checked and that unexpected/missing fields are handled gracefully.
185-185: Maintain uniform design for all reconcilers.
Similarly, extending the same signer to the ClientReconciler is coherent. Just ensure separate domain boundaries are enforced if future updates require different trust contexts or specialized signers.internal/oidc/token.go (2)
35-69: Verify the case-sensitivity for resource checks.
The code expects attrs.GetResource() == "Client". Some Kubernetes-based resource names are lowercase or plural. Confirm that your resource naming here matches the actual CRD naming to avoid mismatch errors in production.
71-105: Enforce consistent naming for exporters.
Same concern regarding resource naming: the logic checks "Exporter" but your CRD might be “exporters” or a different pattern. Confirm correctness.internal/controller/exporter_controller.go (3)
40-40: LGTM: Signer field addition.The addition of the OIDC Signer field is correctly implemented.
46-46: LGTM: RBAC permission for ExporterAccessPolicy.The RBAC permission is correctly scoped to read-only operations (get;list;watch) for ExporterAccessPolicy resources.
157-158: Verify token generation with proper prefix.The token generation uses the authorized username with the signer's prefix, but let's verify the prefix format matches the PR objectives.
internal/controller/client_controller_test.go (1)
82-83: Use shared test constants for OIDC configuration.The hardcoded OIDC configuration values should use the same test constants suggested for suite_test.go.
internal/controller/exporter_controller_test.go (1)
82-83: Use shared test constants for OIDC configuration.The hardcoded OIDC configuration values should use the same test constants suggested for suite_test.go.
internal/controller/lease_controller.go (3)
271-274: LGTM! Well-structured policy data type.The struct definition for approved exporters with policy information is clean and well-organized.
283-296: LGTM! Sensible default policy handling.The code correctly handles the case when no policies exist by approving all available exporters with default policy values.
353-358: LGTM! Clear policy application.The code correctly applies the selected policy's priority and spot access settings to the lease status.
internal/service/controller_service.go (3)
64-67: LGTM! Well-structured authentication and authorization fields.The ControllerService struct properly integrates authentication and authorization components.
70-78: LGTM! Clean authentication method implementation.The authentication methods are well-implemented using the OIDC verifier.
Also applies to: 80-88
209-211: LGTM! Proper authentication check in ListExporters.The ListExporters method correctly verifies client authentication before proceeding.
go.mod (1)
6-8: Verify OIDC package versions for security.The OIDC-related packages are critical for security. Let's verify their versions for any known vulnerabilities.
Also applies to: 14-14
| authentication: | | ||
| apiVersion: jumpstarter.dev/v1alpha1 | ||
| kind: AuthenticationConfiguration | ||
| # jwt: |
There was a problem hiding this comment.
@NickCao can you write some docs on this config, and add the helm values to be able to inject this into place?
May be just a value that allows to pum in the whole authentication configuration?
Summary by CodeRabbit
dex.yamlconfiguration file for Dex server setup.