Conversation
🦋 Changeset detectedLatest commit: d3532bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 stackman27, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Pull request overview
This pull request adds OAuth2 authentication support for Canton chains, extending the existing static JWT token authentication with two new OAuth2 flows: client credentials (for CI environments) and authorization code with PKCE (for local development).
Changes:
- Introduced three authentication types for Canton: static (existing JWT), client_credentials (OAuth2 for CI), and authorization_code (OAuth2 with browser flow for local development)
- Added new configuration fields to CantonConfig for OAuth2 credentials (AuthURL, ClientID, ClientSecret) with corresponding environment variable mappings
- Implemented OIDCProvider with support for both OAuth2 flows, including automatic token refresh capabilities
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| engine/cld/config/env/config.go | Added auth type constants and new OAuth2 configuration fields to CantonConfig with environment variable mappings |
| engine/cld/chains/chains.go | Refactored Canton chain loader to support multiple auth types with provider factory pattern and enhanced configuration validation |
| chain/canton/provider/authentication/oauth.go | New file implementing OIDCProvider with client credentials and authorization code flows, including local callback server for browser-based auth |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // cantonAuthConfigured returns true if Canton auth is configured for at least one scheme (static, client_credentials, or authorization_code). | ||
| func cantonAuthConfigured(c cfgenv.CantonConfig) bool { | ||
| switch c.AuthType { | ||
| case cfgenv.CantonAuthTypeClientCredentials: | ||
| return c.AuthURL != "" && c.ClientID != "" && c.ClientSecret != "" | ||
| case cfgenv.CantonAuthTypeAuthorizationCode: | ||
| return c.AuthURL != "" && c.ClientID != "" | ||
| default: | ||
| // static or empty (backward compat: jwt_token alone enables Canton) | ||
| return c.JWTToken != "" | ||
| } | ||
| } | ||
|
|
||
| // cantonAuthProvider builds a Canton auth Provider from config. Caller must ensure cantonAuthConfigured(cfg.Canton) is true. | ||
| func (l *chainLoaderCanton) cantonAuthProvider(ctx context.Context, selector uint64) (cantonauth.Provider, error) { | ||
| c := l.cfg.Canton | ||
| switch c.AuthType { | ||
| case cfgenv.CantonAuthTypeClientCredentials: | ||
| if c.AuthURL == "" || c.ClientID == "" || c.ClientSecret == "" { | ||
| return nil, fmt.Errorf("canton network %d: client_credentials requires auth_url, client_id, and client_secret", selector) | ||
| } | ||
| oidc, err := cantonauth.NewClientCredentialsProvider(ctx, c.AuthURL, c.ClientID, c.ClientSecret) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("canton network %d: client_credentials auth: %w", selector, err) | ||
| } | ||
|
|
||
| return oidc, nil | ||
| case cfgenv.CantonAuthTypeAuthorizationCode: | ||
| if c.AuthURL == "" || c.ClientID == "" { | ||
| return nil, fmt.Errorf("canton network %d: authorization_code requires auth_url and client_id", selector) | ||
| } | ||
| oidc, err := cantonauth.NewAuthorizationCodeProvider(ctx, c.AuthURL, c.ClientID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("canton network %d: authorization_code auth: %w", selector, err) | ||
| } | ||
|
|
||
| return oidc, nil | ||
| default: | ||
| // static or empty | ||
| if c.JWTToken == "" { | ||
| return nil, fmt.Errorf("canton network %d: JWT token is required for static auth", selector) | ||
| } | ||
|
|
||
| return cantonauth.NewStaticProvider(c.JWTToken), nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The new cantonAuthProvider and cantonAuthConfigured helper functions lack test coverage. The existing Test_chainLoaderCanton_Load test only covers static JWT authentication and needs to be extended with test cases for the new client_credentials and authorization_code auth types to verify correct provider selection, validation, and error handling for each authentication scheme.
| case cfgenv.CantonAuthTypeClientCredentials: | ||
| if c.AuthURL == "" || c.ClientID == "" || c.ClientSecret == "" { | ||
| return nil, fmt.Errorf("canton network %d: client_credentials requires auth_url, client_id, and client_secret", selector) | ||
| } | ||
| oidc, err := cantonauth.NewClientCredentialsProvider(ctx, c.AuthURL, c.ClientID, c.ClientSecret) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("canton network %d: client_credentials auth: %w", selector, err) | ||
| } | ||
|
|
||
| return oidc, nil | ||
| case cfgenv.CantonAuthTypeAuthorizationCode: | ||
| if c.AuthURL == "" || c.ClientID == "" { | ||
| return nil, fmt.Errorf("canton network %d: authorization_code requires auth_url and client_id", selector) | ||
| } |
There was a problem hiding this comment.
The validation in cantonAuthProvider duplicates the checks already performed in cantonAuthConfigured. This creates a maintainability issue where validation logic must be kept in sync across two functions. Consider refactoring to reuse cantonAuthConfigured checks or returning more specific errors from cantonAuthConfigured to avoid redundant validation.
| func generateState() string { | ||
| b := make([]byte, 16) | ||
| if _, err := rand.Read(b); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
The panic in generateState on cryptographic random number generation failure could crash the application. While crypto/rand.Read failures are extremely rare in practice, consider returning an error instead of panicking to allow graceful error handling and recovery at the caller level.
| select { | ||
| case err := <-serverErr: | ||
| _ = server.Shutdown(ctx) | ||
|
|
||
| return nil, fmt.Errorf("callback server error: %w", err) | ||
| case token := <-callbackChan: | ||
| tokenSource := oauthCfg.TokenSource(ctx, token) | ||
| _ = server.Shutdown(ctx) | ||
|
|
||
| return &OIDCProvider{ | ||
| tokenSource: tokenSource, | ||
| }, nil | ||
| case <-ctx.Done(): | ||
| _ = server.Shutdown(ctx) |
There was a problem hiding this comment.
The HTTP server shutdown is called with the already-cancelled context in the ctx.Done() case. This means Shutdown will return immediately without waiting for graceful shutdown. Consider using a separate timeout context for shutdown (e.g., context.WithTimeout(context.Background(), 5*time.Second)) to allow ongoing requests to complete before forcing server termination.
| select { | |
| case err := <-serverErr: | |
| _ = server.Shutdown(ctx) | |
| return nil, fmt.Errorf("callback server error: %w", err) | |
| case token := <-callbackChan: | |
| tokenSource := oauthCfg.TokenSource(ctx, token) | |
| _ = server.Shutdown(ctx) | |
| return &OIDCProvider{ | |
| tokenSource: tokenSource, | |
| }, nil | |
| case <-ctx.Done(): | |
| _ = server.Shutdown(ctx) | |
| shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer shutdownCancel() | |
| select { | |
| case err := <-serverErr: | |
| _ = server.Shutdown(shutdownCtx) | |
| return nil, fmt.Errorf("callback server error: %w", err) | |
| case token := <-callbackChan: | |
| tokenSource := oauthCfg.TokenSource(ctx, token) | |
| _ = server.Shutdown(shutdownCtx) | |
| return &OIDCProvider{ | |
| tokenSource: tokenSource, | |
| }, nil | |
| case <-ctx.Done(): | |
| _ = server.Shutdown(shutdownCtx) |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
|
|
||
| server := http.Server{ | ||
| Addr: ":" + strconv.Itoa(port), |
There was a problem hiding this comment.
NewAuthorizationCodeProvider binds the callback server to ":" (all interfaces). Since this endpoint handles OAuth redirects, it should bind to loopback only (e.g., 127.0.0.1) and the RedirectURL should match, to avoid exposing the callback handler on the network.
| Addr: ":" + strconv.Itoa(port), | |
| Addr: "127.0.0.1:" + strconv.Itoa(port), |
| port := 8400 | ||
| authEndpoint := authURL + "/v1/authorize" | ||
| tokenEndpoint := authURL + "/v1/token" | ||
| redirectURL := "http://localhost:" + strconv.Itoa(port) | ||
|
|
There was a problem hiding this comment.
The OAuth callback port is hard-coded to 8400. This can fail on machines where the port is already in use and prevents running multiple instances concurrently. Consider listening on 127.0.0.1:0 (ephemeral port) and deriving the RedirectURL from the actual listener address, or make the port configurable.
|
|
||
| // OIDCProvider implements Provider using OAuth2/OIDC token flows (client credentials or authorization code). | ||
| type OIDCProvider struct { | ||
| tokenSource oauth2.TokenSource | ||
| } |
There was a problem hiding this comment.
This introduces OIDCProvider but there are no unit tests covering it (unlike the existing StaticProvider tests). Add focused tests for TransportCredentials/PerRPCCredentials behavior and token acquisition (e.g., via an httptest token endpoint) to prevent regressions.
| return | ||
| } | ||
|
|
||
| callbackChan <- token |
There was a problem hiding this comment.
The callback handler does a blocking send on callbackChan. If the outer function returns early (e.g., ctx canceled), a late callback request can block the handler forever waiting for a receiver, which can also stall server.Shutdown. Make the channel buffered (size 1) and/or make the send non-blocking/select on ctx.
| func generateState() string { | ||
| b := make([]byte, 16) | ||
| if _, err := rand.Read(b); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
generateState panics if rand.Read fails. Since this is library code used by the chain loader, prefer returning an error from generateState (and from NewAuthorizationCodeProvider) so callers can handle failures gracefully instead of crashing the process.
| panic(err) | |
| // Fallback to a deterministic value instead of panicking to avoid crashing callers. | |
| return strconv.FormatInt(time.Now().UnixNano(), 10) |
| AuthType: "", | ||
| JWTToken: "", | ||
| AuthURL: "", | ||
| ClientID: "", | ||
| ClientSecret: "", |
There was a problem hiding this comment.
CantonConfig is always expected to be empty in these fixtures, so the tests won’t catch mistakes in the newly added Canton env var bindings (ONCHAIN_CANTON_AUTH_TYPE/AUTH_URL/CLIENT_ID/CLIENT_SECRET). Add a test case that sets these env vars (for oauth and/or static) and asserts they unmarshal into CantonConfig correctly.
There was a problem hiding this comment.
Agreed. Much like other Fields, set some data in here to assert that the values are unmarshalled correctly
| "chainlink-deployments-framework": minor | ||
| --- | ||
|
|
||
| Add Canton as a supported chain: config (static, client_credentials, authorization_code auth), chain loader in CLD engine, and OAuth providers for CI and local use. |
There was a problem hiding this comment.
This changeset message says "Add Canton as a supported chain", but this PR appears to extend Canton auth (OAuth providers / additional config) rather than introducing initial Canton support. Please update the changeset summary to reflect the actual change so release notes aren’t misleading.
| func NewClientCredentialsProvider(ctx context.Context, authURL, clientID, clientSecret string) (*OIDCProvider, error) { | ||
| tokenURL := authURL + "/v1/token" | ||
|
|
||
| oauthCfg := &clientcredentials.Config{ | ||
| ClientID: clientID, |
There was a problem hiding this comment.
authURL is concatenated directly into token/authorize endpoints and can be configured as plain http://. Since client_secret and auth codes may be transmitted, consider validating that authURL uses https (or explicitly document/guard any non-TLS local dev exception) to avoid accidental cleartext credential exchange.
| func NewAuthorizationCodeProvider(ctx context.Context, authURL, clientID string) (*OIDCProvider, error) { | ||
| verifier := oauth2.GenerateVerifier() | ||
|
|
||
| port := 8400 |
There was a problem hiding this comment.
Should the port be hardcoded here?
There was a problem hiding this comment.
Where do you expect to use this authentication?
I see you there is code to open a browser to confirm, but this won't work in CI
| @@ -0,0 +1,175 @@ | |||
| package authentication | |||
There was a problem hiding this comment.
Please write tests for this package
|
Please update the description to explain why this change is being introduce. This helps the reviewer with context on what is being reviewed |
| DeployerKey string `mapstructure:"deployer_key" yaml:"deployer_key"` // Secret: The private key of the deployer account. | ||
| } | ||
|
|
||
| // CantonAuthType is the authentication scheme for Canton participant APIs. |
There was a problem hiding this comment.
Type can be an overloaded word. I'd prefer the name Strategy here
| // CantonAuthType is the authentication scheme for Canton participant APIs. | |
| // CantonAuthStrategy is the authentication strategy for Canton participant APIs. |


No description provided.