OIDC Authentication Driver #14406
Merged
OIDC Authentication Driver #14406
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CL-Andrew
requested review from
vyzaldysanchez
and removed request for
a team
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
harry-anderson
force-pushed
the
oidc-support
branch
from
361b921 to
c15e09a
Compare
|
AER Report: CI Coreaer_workflow , commit , Clean Go Tidy & Generate , Detect Changes , Scheduled Run Frequency , GolangCI Lint (.) , Core Tests (go_core_tests) , test-scripts , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_fuzz) , Core Tests (go_core_race_tests) , lint , SonarQube Scan 1. Duplicate migration version found:go_core_race_testsSource of Error:Suggested fix: Rename one of the migration files to have a unique version number. 2. Duplicate migration version found:go_core_tests_integrationSource of Error:Suggested fix: Rename one of the migration files to have a unique version number. 3. Missing go.sum entry: test-scriptsSource of Error:Suggested fix: Run 4. Duplicate migration version found:go_core_ccip_deployment_testsSource of Error:Suggested fix: Rename one of the migration files to have a unique version number. 5. GolangCI Lint errors: GolangCI Lint (.)Source of Error: |
harry-anderson
force-pushed
the
oidc-support
branch
from
a771d47 to
ce4bdea
Compare
jmank88
reviewed
Jun 30, 2025
jmank88
reviewed
Jun 30, 2025
core/sessions/oidcauth/oidc.go
Outdated
Comment on lines
40
to
41
| RouterRateLimitterPeriod = 1 * time.Minute | ||
| RouterRateLimitterLimit = 1000 |
There was a problem hiding this comment.
Unused? Otherwise, spelling:
Suggested change
| RouterRateLimitterPeriod = 1 * time.Minute | |
| RouterRateLimitterLimit = 1000 | |
| RouterRateLimiterPeriod = 1 * time.Minute | |
| RouterRateLimiterLimit = 1000 |
And could use a https://pkg.go.dev/golang.org/x/time/rate#Limiter which has some helpers for specifying the rate and supports bursts.
jmank88
reviewed
Jun 30, 2025
core/sessions/oidcauth/oidc.go
Outdated
Comment on lines
375
to
376
| ); err != nil { | ||
| return clsessions.ErrUserSessionExpired |
There was a problem hiding this comment.
Should this confirm sql.ErrNoRows or something to rule out other kinds of errors?
There was a problem hiding this comment.
Yes we should catch that case, updated now to propagate up the error if its not the expected NoRows
jmank88
reviewed
Jun 30, 2025
core/sessions/oidcauth/oidc.go
Outdated
Comment on lines
468
to
486
| if err := sqlutil.TransactDataSource(ctx, oi.ds, nil, func(tx sqlutil.DataSource) error { | ||
| return tx.GetContext(ctx, &localAdminUser, SQLSelectUserbyEmail, user.Email) | ||
| }); err != nil { | ||
| oi.lggr.Infof("Can not change password, local user with email not found in users table: %s, err: %v", user.Email, err) | ||
| return clsessions.ErrNotSupported | ||
| } | ||
|
|
||
| // User is local admin, save new password | ||
| hashedPassword, err := utils.HashPassword(newPassword) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := sqlutil.TransactDataSource(ctx, oi.ds, nil, func(tx sqlutil.DataSource) error { | ||
| sql := "UPDATE users SET hashed_password = $1, updated_at = now() WHERE email = $2 RETURNING *" | ||
| return tx.GetContext(ctx, user, sql, hashedPassword, user.Email) | ||
| }); err != nil { | ||
| oi.lggr.Errorf("unable to set password for user: %s, err: %v", user.Email, err) | ||
| return errors.New("unable to save password") | ||
| } |
There was a problem hiding this comment.
We should be able to skip the transactions in cases with a single statement
jmank88
reviewed
Jun 30, 2025
jmank88
reviewed
Jun 30, 2025
jmank88
reviewed
Jun 30, 2025
core/sessions/oidcauth/oidc.go
Outdated
Comment on lines
658
to
661
| const constantTimeEmailLength = 256 | ||
|
|
||
| func constantTimeEmailCompare(left, right string) bool { | ||
| length := mathutil.Max(constantTimeEmailLength, len(left), len(right)) |
There was a problem hiding this comment.
nit/ this is valid if you want to declare it in the narrowest scope:
Suggested change
| const constantTimeEmailLength = 256 | |
| func constantTimeEmailCompare(left, right string) bool { | |
| length := mathutil.Max(constantTimeEmailLength, len(left), len(right)) | |
| func constantTimeEmailCompare(left, right string) bool { | |
| const constantTimeEmailLength = 256 | |
| length := mathutil.Max(constantTimeEmailLength, len(left), len(right))``` |
jmank88
reviewed
Jun 30, 2025
core/sessions/oidcauth/oidc.go
Outdated
| // User is local admin, save new password | ||
| hashedPassword, err := utils.HashPassword(newPassword) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Logged and created new user facing error in the case this is hit
jmank88
reviewed
Jun 30, 2025
core/sessions/oidcauth/oidc.go
Outdated
| ) | ||
| if err != nil { | ||
| oi.lggr.Errorf("unable to create new session in oidc_sessions table %v", err) | ||
| return "", fmt.Errorf("error creating local LDAP session: %w", err) |
There was a problem hiding this comment.
Suggested change
| return "", fmt.Errorf("error creating local LDAP session: %w", err) | |
| return "", fmt.Errorf("error creating local OIDC session: %w", err) |
|
Merged operator-ui with support for optional OIDC login button: https://github.com/smartcontractkit/operator-ui/releases/tag/v0.8.0-371c5cf - release cut |
|
Documentation PR: smartcontractkit/documentation#2790 |
jmank88
reviewed
Jul 1, 2025
core/sessions/oidcauth/oidc.go
Outdated
Comment on lines
282
to
286
| err := sqlutil.TransactDataSource(ctx, oi.ds, nil, func(tx sqlutil.DataSource) error { | ||
| return tx.GetContext(ctx, &foundUser, SQLSelectUserbyEmail, email) | ||
| }) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
Suggested change
| err := sqlutil.TransactDataSource(ctx, oi.ds, nil, func(tx sqlutil.DataSource) error { | |
| return tx.GetContext(ctx, &foundUser, SQLSelectUserbyEmail, email) | |
| }) | |
| if err != nil { | |
| if err := tx.GetContext(ctx, &foundUser, SQLSelectUserbyEmail, email); err != nil { |
jmank88
reviewed
Jul 1, 2025
core/sessions/oidcauth/oidc.go
Outdated
Comment on lines
345
to
348
| if err := sqlutil.TransactDataSource(ctx, oi.ds, nil, func(tx sqlutil.DataSource) error { | ||
| sql := "SELECT * FROM users ORDER BY email ASC;" | ||
| return tx.SelectContext(ctx, &returnUsers, sql) | ||
| }); err != nil { |
There was a problem hiding this comment.
Suggested change
| if err := sqlutil.TransactDataSource(ctx, oi.ds, nil, func(tx sqlutil.DataSource) error { | |
| sql := "SELECT * FROM users ORDER BY email ASC;" | |
| return tx.SelectContext(ctx, &returnUsers, sql) | |
| }); err != nil { | |
| if err := tx.SelectContext(ctx, &returnUsers, "SELECT * FROM users ORDER BY email ASC;"); err != nil { |
jmank88
approved these changes
Jul 1, 2025
|
jmank88
approved these changes
Jul 1, 2025
harry-anderson
approved these changes
Jul 1, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a new authentication driver for the core node session auth flow for the Operator UI. This extends the options to local auth, LDAP server authentication, and now OIDC auth. This means that SSO sign on and RBAC roles can be managed from a provider/identity server that manages and maps user group membership.
The only required config to support this new authentication method looks as follows:
See Front-end changes
smartcontractkit/operator-ui#104