Feature/planetexpress/OIDC refresh tokens #2704
Conversation
mazlumtoprak
requested review from
juanfont,
kradalby,
nblock and
ohdearaugustin
as code owners
Pull Request Revisions
✅ AI review completed for r2 HelpReact with emojis to give feedback on AI-generated reviews:
We'd love to hear from you—reach out anytime at [email protected]. |
| // Validate that the MachineKey in the Noise session matches the one associated with the NodeKey. | ||
| if ns.machineKey != node.MachineKey { | ||
| return nil, NewHTTPError(http.StatusNotFound, "node key in request does not match the one associated with this machine key", nil) | ||
| } |
There was a problem hiding this comment.
In hscontrol/noise.go's getAndValidateNode method, consider adding logging when a mismatch between MachineKey and NodeKey is detected. This would help with debugging potential security incidents where someone is attempting to use a valid NodeKey with an incorrect MachineKey.
| if newToken.RefreshToken != "" { | ||
| session.RefreshToken = newToken.RefreshToken | ||
| } else { | ||
| log.Debug(). | ||
| Str("session_id", session.SessionID). | ||
| Msg("OIDC: No new refresh token received, keeping existing one") | ||
| } |
There was a problem hiding this comment.
In RefreshOIDCSession, there's no explicit handling for token rotation security concerns. Some OIDC providers invalidate old refresh tokens when issuing new ones, but the code doesn't check for this case when newToken.RefreshToken is empty. Consider adding a check to verify with the provider whether the old token is still valid if no new token is received.
| threshold := now.Add(5 * time.Minute) | ||
| needsRefresh := tt.session.IsActive && |
There was a problem hiding this comment.
In TestNeedsRefresh, the token refresh threshold is hardcoded to 5 minutes, but in the TokenRefreshConfig struct, there's an ExpiryThreshold configuration field. Consider using the configuration value for consistency or explicitly documenting why the test uses a different threshold than what's configurable.
| RegistrationID RegistrationID `gorm:"not null"` // For reusing HandleNodeFromAuthPath | ||
|
|
||
| // Token data | ||
| RefreshToken string `gorm:"type:text"` // TODO: Encrypt? |
There was a problem hiding this comment.
In hscontrol/types/oidc_session.go, refresh tokens are stored as plaintext with only a TODO comment about encryption. Given that refresh tokens are long-lived security credentials, consider prioritizing the implementation of encryption for these tokens before deploying to production.
* Improve map auth logic * Bugfix * Add comment, improve error message * noise: make func, get by node this commit splits the additional validation into a separate function so it can be reused if we add more endpoints in the future. It swaps the check, so we still look up by NodeKey, but before accepting the connection, we validate the known machinekey from the db against the noise connection. The reason for this is that when a node logs in or out, the node key is replaced and it will no longer be possible to look it up, breaking reauthentication. Signed-off-by: Kristoffer Dalby <[email protected]> Co-authored-by: Kristoffer Dalby <[email protected]>
mazlumtoprak
force-pushed
the
feature/planetexpress/oidc-refresh-tokens
branch
from
4d8ed21 to
a0007a7
Compare
| func (s *OIDCSession) IsExpired() bool { | ||
| return s.TokenExpiry != nil && s.TokenExpiry.Before(time.Now()) | ||
| } | ||
|
|
||
| // IsExpiringSoon checks if the session's token will expire within the given duration | ||
| func (s *OIDCSession) IsExpiringSoon(duration time.Duration) bool { | ||
| return s.TokenExpiry != nil && s.TokenExpiry.Before(time.Now().Add(duration)) | ||
| } |
There was a problem hiding this comment.
In IsExpired and IsExpiringSoon methods of OIDCSession, direct calls to time.Now() make unit testing difficult. Consider refactoring to use a testable time source (e.g., via dependency injection or a package variable that can be overridden in tests).
| err = a.state.SaveOIDCSession(existingSession) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update OIDC session: %w", err) | ||
| } | ||
|
|
||
| } else { | ||
| // Create new session | ||
| err = a.state.CreateOIDCSession(session) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create OIDC session: %w", err) | ||
| } |
There was a problem hiding this comment.
In the createOrUpdateOIDCSession function, errors from a.state.SaveOIDCSession and a.state.CreateOIDCSession are wrapped without preserving the original context. Consider adding more detailed information in the error messages to make debugging easier (e.g., include node ID, session ID).
2 participants
This issue is a small proof of concept that fixes #1531. The implementation may not be complete yet but fulfils our personal needs on
refresh_tokensFeatures:
offline_accessscope to retrieve refresh tokencheck_intervalandexpiry_threshold.session_invalidation_grace_periodCurrent Limitations:
session_invalidation_grace_period), we want him to do a reauth to enforce our compliance checks via Azure. Therefore we delete the refresh tokens on the invalidation goroutine. With this, we did not implement the retrieval of anaccess_tokenusing therefresh_tokenwhen the user connects again after thesession_invalidation_grace_periodhas passed.