-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redo OIDC configuration #2020
Redo OIDC configuration #2020
Conversation
ec90090
to
df9a74d
Compare
WalkthroughThe recent changes introduce significant modifications to the authentication and user management systems within the codebase. Key updates include the removal of deprecated configuration options, the establishment of an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthProvider
participant Database
User->>AuthProvider: Request authentication
AuthProvider->>Database: Validate user credentials
Database-->>AuthProvider: Return user data
AuthProvider-->>User: Return authentication token
Assessment against linked issues
Possibly related PRs
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Additional comments not posted (16)
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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (4)
hscontrol/handlers.go (1)
171-173
: ReviewAuthProviderWeb
struct for extensibility.The
AuthProviderWeb
struct currently only encapsulatesserverURL
. Consider if additional fields might be needed in the future for further extensibility.hscontrol/oidc.go (1)
Line range hint
371-428
: Consider passing context parameter.The
validateNodeForOIDCCallback
method correctly handles node validation. Consider passing the context parameter to align with best practices.-func (a *AuthProviderOIDC) validateNodeForOIDCCallback( +func (a *AuthProviderOIDC) validateNodeForOIDCCallback( + ctx context.Context, writer http.ResponseWriter, state string, claims *types.OIDCClaims, expiry time.Time, ) (*key.MachinePublic, bool, error) { // retrieve nodekey from state cache machineKeyIf, machineKeyFound := a.registrationCache.Get(state) if !machineKeyFound { return nil, false, errOIDCNodeKeyMissing } var machineKey key.MachinePublic machineKey, machineKeyOK := machineKeyIf.(key.MachinePublic) if !machineKeyOK { return nil, false, errOIDCInvalidNodeState } // retrieve node information if it exist // The error is not important, because if it does not // exist, then this is a new node and we will move // on to registration. node, _ := a.db.GetNodeByMachineKey(machineKey) if node != nil { log.Trace(). Caller(). Str("node", node.Hostname). Msg("node already registered, reauthenticating") err := a.db.NodeSetExpiry(node.ID, expiry) if err != nil { return nil, true, err } log.Debug(). Str("node", node.Hostname). Time("expiresAt", expiry). Msg("successfully refreshed node") var content bytes.Buffer if err := oidcCallbackTemplate.Execute(&content, oidcCallbackTemplateConfig{ User: claims.Email, Verb: "Reauthenticated", }); err != nil { return nil, true, fmt.Errorf("rendering OIDC callback template: %w", err) } writer.Header().Set("Content-Type", "text/html; charset=utf-8") writer.WriteHeader(http.StatusOK) _, err = writer.Write(content.Bytes()) if err != nil { util.LogErr(err, "Failed to write response") } ctx := types.NotifyCtx(context.Background(), "oidc-expiry", "na") a.notifier.NotifyWithIgnore(ctx, types.StateUpdateExpire(node.ID, expiry), node.ID) return nil, true, nil } return &machineKey, false, nil }hscontrol/auth.go (2)
21-24
: Consider naming parameters inAuthProvider
interface.The
AuthProvider
interface methods could benefit from named parameters for clarity, as suggested by static analysis tools.- RegisterHandler(http.ResponseWriter, *http.Request) - AuthURL(key.MachinePublic) string + RegisterHandler(w http.ResponseWriter, r *http.Request) + AuthURL(machineKey key.MachinePublic) stringTools
golangci-lint
22-22: interface method RegisterHandler must have named param for type http.ResponseWriter
(inamedparam)
23-23: interface method AuthURL must have named param for type key.MachinePublic
(inamedparam)
182-182
: Pass context tohandleNodeLogOut
.Consider passing the context parameter to
handleNodeLogOut
to maintain consistency and enable potential cancellation or timeout handling.- func (h *Headscale) handleNodeLogOut(writer http.ResponseWriter, node types.Node) { + func (h *Headscale) handleNodeLogOut(ctx context.Context, writer http.ResponseWriter, node types.Node) {Tools
golangci-lint
182-182: Function
handleNodeLogOut
should pass the context parameter(contextcheck)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (26)
- flake.nix (1 hunks)
- hscontrol/app.go (5 hunks)
- hscontrol/auth.go (11 hunks)
- hscontrol/db/db.go (3 hunks)
- hscontrol/db/node.go (4 hunks)
- hscontrol/db/preauth_keys.go (3 hunks)
- hscontrol/db/routes.go (1 hunks)
- hscontrol/db/users.go (6 hunks)
- hscontrol/db/users_test.go (2 hunks)
- hscontrol/grpcv1.go (3 hunks)
- hscontrol/handlers.go (3 hunks)
- hscontrol/mapper/mapper.go (3 hunks)
- hscontrol/mapper/mapper_test.go (4 hunks)
- hscontrol/mapper/tail.go (1 hunks)
- hscontrol/oidc.go (11 hunks)
- hscontrol/policy/acls.go (3 hunks)
- hscontrol/policy/acls_test.go (3 hunks)
- hscontrol/suite_test.go (1 hunks)
- hscontrol/types/config.go (13 hunks)
- hscontrol/types/config_test.go (4 hunks)
- hscontrol/types/node.go (2 hunks)
- hscontrol/types/node_test.go (4 hunks)
- hscontrol/types/users.go (5 hunks)
- hscontrol/util/dns.go (2 hunks)
- hscontrol/util/dns_test.go (1 hunks)
- integration/auth_oidc_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
- hscontrol/types/config_test.go
Additional context used
golangci-lint
hscontrol/types/users.go
62-62: Comment should end in a period
(godot)
53-53: fmt.Sprintf can be replaced with faster strconv.FormatUint
(perfsprint)
58-58: ST1016: methods on the same type should have the same receiver name (seen 1x "n", 7x "u")
(stylecheck)
hscontrol/oidc.go
234-234: Function
validateNodeForOIDCCallback
should pass the context parameter(contextcheck)
hscontrol/auth.go
22-22: interface method RegisterHandler must have named param for type http.ResponseWriter
(inamedparam)
23-23: interface method AuthURL must have named param for type key.MachinePublic
(inamedparam)
182-182: Function
handleNodeLogOut
should pass the context parameter(contextcheck)
hscontrol/app.go
157-157: Magic number: 30, in detected
(gomnd)
Additional comments not posted (59)
hscontrol/suite_test.go (1)
49-49
: Verify the default behaviour ofOIDCConfig
.The
OIDC
field is now initialized with an empty configuration. Ensure that the default values, especially forStripEmaildomain
, align with the application's requirements.hscontrol/db/users_test.go (1)
23-23
: Verify consistent usage ofGetUserByName
.The method
GetUser
has been renamed toGetUserByName
. Ensure that this change is consistently applied throughout the codebase.Also applies to: 76-76, 79-79
Verification successful
Consistent Usage of
GetUserByName
VerifiedThe method
GetUserByName
is used consistently across the codebase, with no instances of the old method nameGetUser
found. The renaming has been applied correctly in all relevant files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of the GetUserByName method across the codebase. # Test: Search for GetUserByName method usage. Expect: Consistent usage throughout the codebase. rg --type go 'GetUserByName'Length of output: 1278
hscontrol/mapper/tail.go (1)
79-79
: Verify the correctness ofGetFQDN
with reduced parameters.The
cfg
parameter has been removed from theGetFQDN
function call. Ensure that the function operates correctly with onlycfg.BaseDomain
.Verification successful
The
GetFQDN
function operates correctly with the reduced parameter set. The function is defined to accept a singlestring
parameter, which matches the usage intail.go
. No issues were found with this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of GetFQDN with the reduced parameter set. # Test: Search for the GetFQDN function definition. Expect: Confirm it operates correctly with the new parameter set. rg --type go 'func GetFQDN' -A 5Length of output: 34
Script:
#!/bin/bash # Use ast-grep to search for the GetFQDN function definition and its parameters. ast-grep --lang go --pattern 'func GetFQDN($_) { $$$ }'Length of output: 57
Script:
#!/bin/bash # Use rg to search for any mention of GetFQDN in the codebase. rg 'GetFQDN' --type go -A 10Length of output: 2345
hscontrol/types/users.go (5)
21-43
: Enhancements to theUser
struct look good.The additional fields for OIDC support and user information management are well-integrated.
58-59
: TheDisplayNameOrUsername
method is well-implemented.The method provides a clear fallback mechanism for user identification.
Tools
golangci-lint
58-58: ST1016: methods on the same type should have the same receiver name (seen 1x "n", 7x "u")
(stylecheck)
62-64
: TheprofilePicURL
method is correctly implemented.It simply returns the
ProfilePicURL
field, as expected.Tools
golangci-lint
62-62: Comment should end in a period
(godot)
123-132
: TheFromClaim
method is effectively synchronising user data.The method correctly updates user fields based on OIDC claims.
106-109
: TheProto
method is correctly implemented.The method accurately converts a
User
to its protobuf representation.flake.nix (1)
34-34
: ThevendorHash
update is appropriate.This change reflects updates to dependencies or vendor packages.
hscontrol/db/users.go (5)
52-54
: The use ofGetUserByName
improves clarity.Renaming the function to specify retrieval by name enhances code readability.
93-101
: TheRenameUser
function changes are consistent and clear.The renaming aligns with the new naming convention for user retrieval.
118-124
: TheGetUserByName
function is well-implemented.The function correctly retrieves a user by name and handles errors.
136-152
: TheGetUserByOIDCIdentifier
function effectively extends functionality.This addition enhances user retrieval capabilities using OIDC identifiers.
176-178
: TheListNodesByUser
function changes are consistent and clear.Using
GetUserByName
aligns with the naming convention and improves readability.hscontrol/db/preauth_keys.go (2)
47-49
: TheCreatePreAuthKey
function changes enhance clarity.Using
GetUserByName
improves the readability of user retrieval logic.
109-111
: TheListPreAuthKeys
function changes are consistent and clear.Using
GetUserByName
aligns with the naming convention and improves readability.hscontrol/util/dns.go (2)
Line range hint
13-34
: ReviewCheckForFQDNRules
for completeness.The
CheckForFQDNRules
function checks for FQDN compliance but does not normalize inputs. Ensure that inputs are pre-normalized or that the function's limitations are documented.
Line range hint
1-1
: Verify impact of removed normalization functions.The removal of
NormalizeToFQDNRulesConfigFromViper
andNormalizeToFQDNRules
may affect parts of the codebase that expected user names to be normalized. Ensure that all dependencies on these functions are updated accordingly.Verification successful
No Impact from Removed Normalisation Functions
The functions
NormalizeToFQDNRulesConfigFromViper
andNormalizeToFQDNRules
have been removed without any remaining references or dependencies in the codebase. Their removal does not affect other parts of the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of removed normalization functions in the codebase. # Test: Search for references to the removed functions. Expect: No references found. rg --type go 'NormalizeToFQDNRulesConfigFromViper|NormalizeToFQDNRules'Length of output: 73
Script:
#!/bin/bash # Description: Verify the removal of the normalization functions and check for any indirect references. # Check for any comments or documentation that might reference the removed functions. rg --type go 'NormalizeToFQDNRulesConfigFromViper|NormalizeToFQDNRules' -g '*.md' -g '*.txt' -g '*.go' -g '*.yaml' -g '*.yml' # Verify the removal of the functions by searching for their definitions in the codebase. ast-grep --lang go --pattern 'func NormalizeToFQDNRulesConfigFromViper($_) { $$$ }' ast-grep --lang go --pattern 'func NormalizeToFQDNRules($_) { $$$ }'Length of output: 284
hscontrol/handlers.go (2)
181-186
: Ensure URL safety inAuthURL
.The
AuthURL
method constructs a URL usingfmt.Sprintf
. Ensure thatmKey.String()
is URL-safe or consider using a URL encoding function.
Line range hint
193-216
: ReviewRegisterHandler
for security vulnerabilities.The
RegisterHandler
method processes user input from the URL. Ensure that all inputs are properly sanitised to prevent XSS or injection attacks.integration/auth_oidc_test.go (1)
Line range hint
1-1
: Verify impact of removedHEADSCALE_OIDC_STRIP_EMAIL_DOMAIN
.The removal of
HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN
from tests indicates a change in logic. Ensure that the new logic is covered by tests and that the removal does not affect test validity.hscontrol/types/node_test.go (1)
167-172
: Ensure comprehensive FQDN validation.The test case for overly long usernames is a good addition. Ensure that all edge cases for FQDN validation are covered, such as invalid characters or empty domains.
hscontrol/mapper/mapper_test.go (2)
32-34
: LGTM! Improved user profile initialization.The addition of the
Model
field in theUser
struct enhances the robustness of the user profile representation in tests. This aligns with GORM's conventions.
79-79
: LGTM! Simplified DNS configuration.The use of an empty
Routes
map in the DNS configuration response simplifies the test setup and aligns with a more flexible DNS handling approach.hscontrol/types/node.go (1)
Line range hint
396-411
: LGTM! Simplified FQDN generation.The
GetFQDN
method now constructs the FQDN using onlyGivenName
andbaseDomain
, reducing complexity. Ensure that this change aligns with all intended use cases.hscontrol/oidc.go (9)
50-59
: LGTM! Enhanced modularity withAuthProviderOIDC
.The introduction of the
AuthProviderOIDC
struct encapsulates OIDC functionality, improving modularity and clarity.
62-100
: LGTM! Centralised OIDC setup.The
NewAuthProviderOIDC
constructor effectively initializes theAuthProviderOIDC
instance, centralising OIDC setup and enhancing modularity.
102-106
: LGTM! Correct construction of authentication URL.The
AuthURL
method constructs the URL using the server URL and machine key, ensuring correct URL formation.
109-114
: LGTM! Correct token expiration logic.The
determineTokenExpiration
method correctly handles expiration based on configuration, ensuring appropriate token validity.
Line range hint
120-167
: LGTM! Robust OIDC registration handling.The
RegisterHandler
method effectively manages state and redirects for OIDC registration, ensuring robust error handling.
Line range hint
190-269
: LGTM! Effective OIDC callback handling.The
OIDCCallback
method processes the callback, validates parameters, and manages node registration with comprehensive error handling.Tools
golangci-lint
234-234: Function
validateNodeForOIDCCallback
should pass the context parameter(contextcheck)
285-296
: LGTM! Correct ID token retrieval.The
getIDTokenForOIDCCallback
method exchanges the code for a token and extracts the ID token, ensuring proper error handling.
302-311
: LGTM! Correct ID token verification.The
verifyIDTokenForOIDCCallback
method uses the OIDC provider's verifier to ensure the ID token's validity and integrity.
434-462
: LGTM! Effective user management.The
createOrUpdateUserFromClaim
method handles user creation and updates based on OIDC claims, with proper error handling and legacy support.hscontrol/mapper/mapper.go (2)
97-100
: LGTM! Improved user identification.The
generateUserProfiles
function now uses user IDs instead of names for identification, enhancing robustness and reducing the risk of conflicts.
Line range hint
116-122
: LGTM! Simplified DNS configuration logic.The
generateDNSConfig
function has been simplified by removing MagicDNS-related logic, reducing complexity. Ensure that this change aligns with all DNS configuration requirements.hscontrol/db/db.go (2)
406-417
: Review new migration entry fortypes.User
.The migration entry for
types.User
seems straightforward. Ensure that this migration aligns with the current schema and does not introduce any conflicts.
261-261
: Verify removal of normalization logic.The removal of the normalization step for
GivenName
may affect how node names are stored. Ensure that this change does not introduce any inconsistencies or issues in other parts of the system that rely on normalized names.hscontrol/db/routes.go (1)
647-647
: Ensure consistency withUsername()
usage.The change from
node.User.Name
tonode.User.Username()
may affect user identity validation. Verify thatUsername()
is consistently used across the codebase to prevent any mismatches or inconsistencies.Verification successful
Consistent Usage of
Username()
VerifiedThe method
Username()
is consistently used across the codebase for accessing user identities internally. The references toclaims.Username
andu.Name
are part of handling external user data, such as from an OIDC provider, and do not conflict with the internal usage ofUsername()
. Thus, the change tonode.User.Username()
is consistent with the established pattern.
hscontrol/oidc.go
: Handles external claims.hscontrol/types/users.go
: Maps external claims to internal structures.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `Username()` for user identity validation. # Test: Search for all usages of `Username()` to ensure consistency. rg --type go 'Username()'Length of output: 1644
Script:
#!/bin/bash # Check if `claims.Username` and `u.Name` are related to the same user identity context as `Username()`. rg --type go 'claims.Username' -A 3 rg --type go 'u.Name =' -A 3Length of output: 843
hscontrol/grpcv1.go (3)
43-43
: Improved clarity withGetUserByName
.The change to
GetUserByName
improves code clarity by explicitly indicating that the user is fetched by name. This enhances readability and aligns with best practices.
72-72
: Improved clarity withGetUserByName
.This change enhances the clarity of the
RenameUser
function by making it explicit that the user is retrieved by their new name.
754-754
: Improved clarity withGetUserByName
.The use of
GetUserByName
inDebugCreateNode
clarifies the intention of fetching a user by name, improving code maintainability.hscontrol/auth.go (2)
476-476
: StreamlinedAuthURL
handling.The refactoring of
AuthURL
handling to useh.authProvider.AuthURL(machineKey)
simplifies the logic and improves maintainability.
701-701
: StreamlinedAuthURL
handling.This change enhances the handling of authentication URLs by centralising the logic in the
AuthProvider
interface, promoting modularity.hscontrol/db/node.go (3)
620-639
: Refactoring approved.The refactoring of
generateGivenName
improves readability and error handling by directly checking the length ofsuppliedName
and using a specific error for long hostnames.
340-340
: LGTM! VerifyGetUserByName
implementation.The change to use
GetUserByName
improves clarity. Ensure thatGetUserByName
is correctly implemented and handles errors appropriately.Verification successful
Verification Successful:
GetUserByName
Implementation is CorrectThe function
GetUserByName
is correctly implemented. It uses GORM to retrieve a user by name and appropriately handles the case where the user is not found by returning a custom errorErrUserNotFound
.
- Location:
hscontrol/db/users.go
, lines 124-134Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `GetUserByName`. # Test: Search for the function definition to review its implementation. ast-grep --lang go --pattern 'func GetUserByName($_, $_) ($_, error) { $$$ }'Length of output: 617
393-393
: LGTM! VerifyUsername()
implementation.The change to use
node.User.Username()
instead ofnode.User.Name
suggests improved encapsulation. Ensure thatUsername()
is correctly implemented and encapsulates necessary logic.Also applies to: 409-409
hscontrol/policy/acls.go (3)
740-740
: Simplification approved.The removal of group name normalization simplifies the logic and reduces potential errors. Ensure this change does not affect functionality elsewhere.
929-929
: LGTM! VerifyUsername()
implementation.The change to use
node.User.Username()
standardizes user identifier access. Ensure thatUsername()
is correctly implemented.
953-953
: Standardization approved.The use of
node.User.Username()
instead ofnode.User.Name
aligns with the standardization of user identifier access and likely improves data integrity.hscontrol/app.go (3)
98-99
: Modularity improvement approved.The introduction of
authProvider
encapsulates authentication logic within a single interface, enhancing modularity and flexibility.
444-448
: Router setup transition approved.The use of
authProvider.RegisterHandler
and conditional OIDC callback setup reflects the transition to the new authentication mechanism, enhancing flexibility.
154-178
: Enhanced authentication setup approved.The initialization of
authProvider
with a fallback mechanism improves error handling and flexibility. Ensure thatAuthProvider
implementations are correctly integrated.Tools
golangci-lint
157-157: Magic number: 30, in detected
(gomnd)
hscontrol/types/config.go (4)
74-74
: Configuration simplification approved.The removal of
DNSUserNameInMagicDNS
simplifies the configuration model, reducing complexity.
315-330
: Improved error handling approved.The inclusion of fatal error checks for deprecated keys and improved error messaging enhances error handling and ensures users update their configurations.
767-772
: Configuration validation improvements approved.The removal of references to deprecated fields and improved error messaging streamline the configuration validation process and improve clarity.
Also applies to: 806-810
918-925
: Deprecation handling approved.The updates to the
deprecator
struct and its methods ensure proper handling and logging of deprecated keys, guiding users to update their configurations.Also applies to: 930-937, 945-952, 961-968, 975-980
hscontrol/policy/acls_test.go (2)
651-651
: LGTM! Updated expected output.The expected output for the "Expand emails in group" test case has been updated to reflect full email addresses. This aligns with the intended functionality.
651-651
: Verify the removal of the email domain stripping test case.The test case for stripping domains from email addresses has been removed. Ensure that this functionality is either no longer required or covered elsewhere.
Verification successful
Removal of email domain stripping test case is justified.
The functionality for stripping email domains appears to have been deprecated, as indicated by the references in
hscontrol/types/config.go
. Therefore, the removal of the related test case is consistent with the current codebase. No further action is necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the functionality for stripping email domains is covered elsewhere in the codebase. # Test: Search for any function or test related to email domain stripping. Expect: At least one occurrence. rg --type go 'stripEmailDomain' -A 5Length of output: 36
Script:
#!/bin/bash # Description: Broaden the search to find any functionality related to email processing or domain handling. # Test: Search for keywords related to email processing or domain handling. rg --type go 'email' -A 5Length of output: 4339
func NewAuthProviderWeb(serverURL string) *AuthProviderWeb { | ||
return &AuthProviderWeb{ | ||
serverURL: serverURL, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider error handling in NewAuthProviderWeb
.
The constructor NewAuthProviderWeb
does not currently handle any errors. If serverURL
requires validation, consider adding error handling.
func NewAuthProviderWeb(serverURL string) (*AuthProviderWeb, error) {
if serverURL == "" {
return nil, fmt.Errorf("serverURL cannot be empty")
}
return &AuthProviderWeb{
serverURL: serverURL,
}, nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- hscontrol/types/users.go (5 hunks)
Additional comments not posted (5)
hscontrol/types/users.go (5)
21-43
: Enhancements toUser
struct approved.The additions of
DisplayName
,ProviderIdentifier
,Provider
, andProfilePicURL
enhance user data management, particularly for OIDC authentication.
52-54
:Username
method implementation approved.The method effectively prioritises user identifiers and uses
strconv.FormatUint
for performance.
58-59
:DisplayNameOrUsername
method implementation approved.The method provides a sensible fallback mechanism for user display names, enhancing user experience.
63-64
:profilePicURL
method implementation approved.The method encapsulates access to the profile picture URL, promoting encapsulation and reusability.
110-132
:OIDCClaims
struct andFromClaim
method implementation approved.The struct and method ensure user data remains in sync with the authentication provider, enhancing functionality and flexibility.
ccbab98
to
e78aaa8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
hscontrol/db/preauth_keys.go (1)
Line range hint
25-47
: The changes look good! 👍The updates to the
CreatePreAuthKey
function improve the specificity of user identification by explicitly using the user's name. The control flow remains intact, and the changes do not introduce any issues.The comment about potentially using user ID in the future is a good suggestion. It would provide a more robust user identification mechanism.
Consider creating a new issue to track the future enhancement of using user ID instead of name. This will ensure that the suggestion is not forgotten and can be prioritised appropriately.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (26)
- flake.nix (1 hunks)
- hscontrol/app.go (5 hunks)
- hscontrol/auth.go (11 hunks)
- hscontrol/db/db.go (3 hunks)
- hscontrol/db/node.go (4 hunks)
- hscontrol/db/preauth_keys.go (3 hunks)
- hscontrol/db/routes.go (1 hunks)
- hscontrol/db/users.go (6 hunks)
- hscontrol/db/users_test.go (2 hunks)
- hscontrol/grpcv1.go (3 hunks)
- hscontrol/handlers.go (3 hunks)
- hscontrol/mapper/mapper.go (3 hunks)
- hscontrol/mapper/mapper_test.go (4 hunks)
- hscontrol/mapper/tail.go (1 hunks)
- hscontrol/oidc.go (12 hunks)
- hscontrol/policy/acls.go (3 hunks)
- hscontrol/policy/acls_test.go (3 hunks)
- hscontrol/suite_test.go (1 hunks)
- hscontrol/types/config.go (13 hunks)
- hscontrol/types/config_test.go (4 hunks)
- hscontrol/types/node.go (2 hunks)
- hscontrol/types/node_test.go (4 hunks)
- hscontrol/types/users.go (5 hunks)
- hscontrol/util/dns.go (2 hunks)
- hscontrol/util/dns_test.go (1 hunks)
- integration/auth_oidc_test.go (3 hunks)
Files skipped from review due to trivial changes (3)
- flake.nix
- hscontrol/types/config_test.go
- hscontrol/util/dns_test.go
Additional comments not posted (89)
hscontrol/suite_test.go (1)
49-49
: Verify if the change aligns with the intended behaviour and has been thoroughly tested.Please ensure that this change:
- Aligns with the intended behaviour of the
ResetDB
function and the overall test suite.- Has been thoroughly tested to ensure it doesn't introduce any regressions in the authentication flow and user management.
To verify the impact of this change, please run the following script:
Please provide the output of running this script and confirm if:
- There are no other occurrences of
StripEmaildomain
being set tofalse
.- The relevant tests exist, have been updated if necessary, and are passing.
Verification successful
Change aligns with intended behaviour but verify test coverage manually.
The change to the
OIDC
configuration inhscontrol/suite_test.go
aligns with the intended behaviour, as there are no conflicting configurations found in the codebase. Tests related to OIDC configuration exist, suggesting that the functionality is being tested. However, it is recommended to manually verify that these tests cover the specific change to ensure comprehensive test coverage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the OIDC configuration change. # Test 1: Search for usage of the `StripEmaildomain` property. # Expect: No occurrences of `StripEmaildomain` being set to `false`. rg --type go -A 5 $'StripEmaildomain: false' # Test 2: Search for tests related to the OIDC configuration. # Expect: Relevant tests exist and have been updated if necessary. rg --type go -A 5 $'OIDC.*Config'Length of output: 3740
hscontrol/db/users_test.go (2)
23-23
: LGTM!The function name change from
GetUser
toGetUserByName
improves clarity and matches the list of alterations. The test logic remains the same, ensuring that the user is deleted after callingDestroyUser
.
76-76
: LGTM!The function name change from
GetUser
toGetUserByName
improves clarity and matches the list of alterations. The test logic remains the same, ensuring that the user is renamed and checking for errors when the user is not found or when the new user name already exists.Also applies to: 79-79
hscontrol/mapper/tail.go (1)
79-81
: LGTM!The code changes are approved. The modification simplifies the
GetFQDN
method call by only passing the base domain as an argument, streamlining the function's logic. The error handling remains intact, ensuring any failure in generating the FQDN is still reported appropriately.hscontrol/types/users.go (8)
21-43
: LGTM!The addition of new fields to the
User
struct improves the representation of user data. The comments on theName
and
45-54
: LGTM!The
Username
method provides a clear and consistent way to retrieve the user's identifier based on the availability ofName
,ProviderIdentifier
, or the user's ID. This ensures a unified approach across the codebase.
56-59
: LGTM!The
DisplayNameOrUsername
method offers a handy way to retrieve the user's display name, falling back to the username if the display name is not available. This ensures a user-friendly representation of the user's identity.
64-64
: LGTM!The modification to the
profilePicURL
method ensures it returns the actualProfilePicURL
when available, aligning with the purpose of the new field.
70-71
: LGTM!The updates to the
TailscaleUser
andTailscaleLogin
methods ensure consistent user identification by utilizing the newUsername
andDisplayNameOrUsername
methods for theLoginName
andDisplayName
fields. This promotes a unified approach across different contexts.Also applies to: 84-86
96-97
: LGTM!The updates to the
TailscaleUserProfile
method ensure consistent user identification by utilizing the newUsername
andDisplayNameOrUsername
methods for theLoginName
andDisplayName
fields, promoting a unified approach.
110-121
: LGTM!The addition of the
OIDCClaims
struct enhances the integration of OIDC within the user management system by providing a structured way to handle and access various OIDC claims. This improves the overall functionality and flexibility of user data handling.
123-132
: LGTM!The
FromClaim
method provides a convenient way to update theUser
fields based on the provided OIDC claims, excluding the ID. This facilitates the synchronization of user data from OIDC claims to theUser
struct while preserving the existing user ID.hscontrol/db/users.go (8)
52-52
: LGTM!The change from
GetUser
toGetUserByName
improves clarity and is consistent with the function renaming.
93-93
: LGTM!The change from
GetUser
toGetUserByName
improves clarity and is consistent with the function renaming.
101-101
: LGTM!The change from
GetUser
toGetUserByName
improves clarity and is consistent with the function renaming.
118-122
: LGTM!The function renaming from
GetUser
toGetUserByName
improves clarity and is consistent with the list of alterations.
Line range hint
124-134
: LGTM!The function renaming from
GetUser
toGetUserByName
improves clarity and is consistent with the list of alterations.
136-140
: LGTM!The new function
GetUserByOIDCIdentifier
expands the functionality to include user identification via OIDC, as mentioned in the AI-generated summary. It follows a similar pattern to the existing user retrieval methods, ensuring consistency.
142-152
: LGTM!The new function
GetUserByOIDCIdentifier
expands the functionality to include user identification via OIDC, as mentioned in the AI-generated summary. It follows a similar pattern to the existing user retrieval methods, ensuring consistency.
176-176
: LGTM!The change from
GetUser
toGetUserByName
improves clarity and is consistent with the function renaming.hscontrol/db/preauth_keys.go (1)
Line range hint
109-119
: LGTM! 🚀The updates to the
ListPreAuthKeys
function, similar to theCreatePreAuthKey
function, improve the specificity of user identification by explicitly using the user's name. The control flow remains intact, and the changes do not introduce any issues.hscontrol/util/dns.go (3)
Line range hint
1-1
: LGTM: Removal of unused Viper import statement.The removal of the Viper import statement is approved as the associated functions
NormalizeToFQDNRulesConfigFromViper
andNormalizeToFQDNRules
have been removed from the file. This change simplifies the codebase by removing the dependency on the Viper configuration management library.
Line range hint
1-1
: Verify the impact of removing theNormalizeToFQDNRulesConfigFromViper
function.The removal of the
NormalizeToFQDNRulesConfigFromViper
function indicates a shift in how user names are processed. Please ensure that:
- There are no remaining calls to this function in the codebase, as they will result in errors.
- The removal of this function does not break any existing functionality or impact the overall user experience.
Run the following script to verify the impact of removing the function:
Line range hint
1-1
: Verify the impact of removing theNormalizeToFQDNRules
function.The removal of the
NormalizeToFQDNRules
function indicates a shift in how user names are processed. Please ensure that:
- There are no remaining calls to this function in the codebase, as they will result in errors.
- The removal of this function does not break any existing functionality or impact the overall user experience.
Run the following script to verify the impact of removing the function:
Verification successful
Verification Successful:
NormalizeToFQDNRules
Function RemovedThe
NormalizeToFQDNRules
function and all its calls have been successfully removed from the codebase. No remaining references to this function were found, indicating that its removal does not impact the current functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `NormalizeToFQDNRules` function. # Test 1: Search for any remaining calls to the removed function. Expect: No results. rg --type go $'NormalizeToFQDNRules' # Test 2: Search for the implementation of the removed function. Expect: No results. ast-grep --lang go --pattern $'func NormalizeToFQDNRules($_, $_) { $$$ }'Length of output: 112
Script:
#!/bin/bash # Re-run the verification to ensure the function and its calls are absent. # Test 1: Search for any remaining calls to the removed function. Expect: No results. rg --type go 'NormalizeToFQDNRules' # Test 2: Search for the implementation of the removed function. Expect: No results. ast-grep --lang go --pattern 'func NormalizeToFQDNRules($_, $_) { $$$ }'Length of output: 112
hscontrol/handlers.go (3)
171-173
: LGTM!The code changes are approved.
181-186
: LGTM!The code changes are approved.
Line range hint
193-257
: LGTM!The code changes are approved.
integration/auth_oidc_test.go (2)
Line range hint
42-42
: Verify the impact of removing theStripEmaildomain
field.Ensure that removing the
StripEmaildomain
field from theOIDCConf
structure does not affect the expected behaviour of the OIDC authentication tests.Run the following script to verify the impact:
Verification successful
Removal of
StripEmaildomain
Field VerifiedThe
StripEmaildomain
field is not used elsewhere in the codebase, confirming that its removal does not impact the functionality of the OIDC authentication tests or other components.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `StripEmaildomain` field. # Test: Search for usages of the `StripEmaildomain` field. Expect: No occurrences. rg --type go $'StripEmaildomain'Length of output: 31
Line range hint
51-51
: Verify the impact of removing theHEADSCALE_OIDC_STRIP_EMAIL_DOMAIN
configuration.Ensure that removing the
HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN
configuration from the test cases does not affect the expected behaviour of the OIDC authentication tests.Run the following script to verify the impact:
Also applies to: 84-84
Verification successful
Removal of
HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN
Configuration VerifiedThe removal of the
HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN
configuration does not impact the expected behaviour of the OIDC authentication tests. The tests are functioning with the current OIDC setup, which does not include this configuration.
- The test cases
TestOIDCAuthenticationPingAll
andTestOIDCExpireNodesBasedOnTokenExpiry
do not reference this configuration and are designed to work with the existing OIDC configurations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN` configuration. # Test: Search for usages of the `HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN` configuration. Expect: No occurrences. rg --type go $'HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN'Length of output: 48
Script:
#!/bin/bash # Description: Search for any related configurations or logic that might have replaced `HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN`. # Search for any configuration or logic related to OIDC email handling. rg --type go 'OIDC' -A 5 # Search for the test cases `TestOIDCAuthenticationPingAll` and `TestOIDCExpireNodesBasedOnTokenExpiry` to understand their current behaviour. rg 'func TestOIDCAuthenticationPingAll' -A 20 rg 'func TestOIDCExpireNodesBasedOnTokenExpiry' -A 20Length of output: 36161
hscontrol/types/node_test.go (4)
167-172
: LGTM!The new test case is correctly testing the behaviour of the
GetFQDN
function when the username is very long. It expects the function to return an error indicating that the hostname is too long and cannot exceed 255 ASCII characters.
189-190
: LGTM!The removal of the
cfg
parameter from theGetFQDN
function call simplifies the test structure by eliminating the configuration setup for each test case. The function now only requires thedomain
parameter.
191-191
: LGTM!The log statement is useful for debugging purposes and can help in understanding the values returned by the
GetFQDN
function.
Line range hint
193-203
:hscontrol/mapper/mapper_test.go (3)
79-79
: Simplification of DNS configuration handling.The following changes simplify the DNS configuration handling:
- The
Routes
field in theDNSConfig
struct is initialized as an empty map instead of containing predefined routes, allowing for more dynamic handling of DNS routes.- The
DNSUserNameInMagicDNS
field is removed from theConfig
struct instantiation, simplifying the configuration parameters.Also applies to: 129-129
Line range hint
141-433
:
15-15
: Significant change:User
struct now includesgorm.Model
.The
User
struct now includes aModel
field of typegorm.Model
, which is a significant change that enables ORM operations with GORM.Verify the impact of this change on the codebase by running the following script:
Also applies to: 32-34
hscontrol/types/node.go (1)
Line range hint
396-420
: Simplification of theGetFQDN
method's interfaceThe removal of the
cfg
parameter simplifies the method's interface and reduces coupling with theConfig
struct, which improves the method's modularity and testability.However, the removal of the
cfg.DNSUserNameInMagicDNS
logic changes the behavior of the method, as it no longer considers the user's name when generating the FQDN.Please verify that this change does not introduce any unintended side effects in the system. You can use the following script to search for any potential issues:
Verification successful
Verification of
GetFQDN
method changesThe removal of the
cfg.DNSUserNameInMagicDNS
logic from theGetFQDN
method does not appear to affect other parts of the codebase, as there are no remaining references to this logic. Additionally, the call toGetFQDN
inhscontrol/mapper/tail.go
aligns with the new method signature, usingcfg.BaseDomain
appropriately.Please ensure that the change in behaviour due to the removal of
cfg.DNSUserNameInMagicDNS
logic does not introduce any unintended side effects in the system.
- File to review:
hscontrol/mapper/tail.go
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for potential issues related to the removal of the `cfg.DNSUserNameInMagicDNS` logic. # Test 1: Search for any references to `cfg.DNSUserNameInMagicDNS` in the codebase. # Expect: No references found. rg --type go $'cfg\.DNSUserNameInMagicDNS' # Test 2: Search for any calls to `GetFQDN` that still pass a `cfg` parameter. # Expect: No calls found. rg --type go $'GetFQDN\(.*cfg.*\)'Length of output: 151
hscontrol/mapper/mapper.go (2)
97-100
: LGTM!The change from using user names to user IDs as keys in the
userMap
is an improvement. It enhances the uniqueness of user identification and reduces the risk of collisions that could arise from users having the same name. The update to the loop that populates theuserMap
for peers maintains consistency with the new keying strategy.
125-125
: Please verify the impact of removing the DNS configuration logic.The removal of the substantial block of code that handles the configuration of DNS settings when MagicDNS is enabled suggests a shift in how DNS configurations are managed. It potentially indicates a move towards a more simplified or different approach to DNS handling within the application.
To verify the impact of this change, please run the following script:
Please provide the results of running these verification tests so that we can assess the impact of this change on the overall DNS configuration management.
Verification successful
The removal of DNS configuration logic is covered by existing tests.
The
TestDNSConfigMapResponse
test case includes scenarios for DNS configuration whenmagicDNS
is enabled or disabled, ensuring that the impact of the removed logic is verified through these tests. No further action is required unless additional test coverage is deemed necessary.
- The
TestDNSConfigMapResponse
test case covers theProxied
field and domain settings.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the DNS configuration logic on the overall system. # Test 1: Search for occurrences of `dnsConfig.Proxied` in the codebase. # Expect: No occurrences, indicating that the removed logic is not used elsewhere. rg --type go $'dnsConfig\.Proxied' # Test 2: Search for occurrences of DNS route construction based on user names. # Expect: No occurrences, indicating that the removed logic is not used elsewhere. rg --type go $'dnsConfig\.Routes\[[^]]+\] = .*\.Name' # Test 3: Verify if there are any test cases covering the removed DNS configuration logic. # Expect: No test cases, indicating that the removed logic was not actively tested. fd _test.go --exec rg $'func Test.*DNS.*Config'Length of output: 185
Script:
#!/bin/bash # Description: Extract the details of the TestDNSConfigMapResponse test case to assess its relevance to the removed DNS configuration logic. # Locate the file containing the TestDNSConfigMapResponse test case and display its content. fd _test.go --exec rg -A 20 $'func TestDNSConfigMapResponse'Length of output: 491
hscontrol/oidc.go (15)
50-59
: LGTM!The introduction of the
AuthProviderOIDC
structure is a great way to encapsulate the OIDC-related components and improve the modularity of the code. Well done!
62-100
: LGTM!The
NewAuthProviderOIDC
constructor is a great addition that streamlines the initialization process for theAuthProviderOIDC
structure. Moving the OIDC provider and OAuth2 configuration setup into this constructor enhances the clarity and maintainability of the code. Nice work!
102-106
: LGTM!The modification of the
AuthURL
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. Good job!
109-114
: LGTM!The modification of the
determineTokenExpiration
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. Well done!
Line range hint
119-167
: LGTM!The modification of the
RegisterHandler
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. Good job!
Line range hint
190-269
: LGTM!The modification of the
OIDCCallback
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. The improved error handling, where errors are returned instead of writing responses directly, is a great change that enhances the flexibility and maintainability of the code. Well done!
285-298
: LGTM!The modification of the
getIDTokenForOIDCCallback
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. The improved error handling, where errors are returned instead of writing responses directly, is a great change that enhances the flexibility and maintainability of the code. Good job!
302-312
: LGTM!The modification of the
verifyIDTokenForOIDCCallback
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. The improved error handling, where errors are returned instead of writing responses directly, is a great change that enhances the flexibility and maintainability of the code. Well done!
319-319
: LGTM!The modification of the
validateOIDCAllowedDomains
function to accepttypes.OIDCClaims
instead ofIDTokenClaims
aligns with the overall refactoring and enhances the type safety of the code. Good job!
337-337
: LGTM!The modification of the
validateOIDCAllowedGroups
function to accepttypes.OIDCClaims
instead ofIDTokenClaims
aligns with the overall refactoring and enhances the type safety of the code. Well done!
356-356
: LGTM!The modification of the
validateOIDCAllowedUsers
function to accepttypes.OIDCClaims
instead ofIDTokenClaims
aligns with the overall refactoring and enhances the type safety of the code. Good job!
Line range hint
371-438
: LGTM!The modification of the
validateNodeForOIDCCallback
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. Well done!
444-474
: LGTM!The modification of the
createOrUpdateUserFromClaim
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. Good job!
Line range hint
477-507
: LGTM!The modification of the
registerNodeForOIDCCallback
method to use theAuthProviderOIDC
receiver aligns with the overall refactoring and enhances the modularity of the code. Well done!
511-511
: LGTM!The modification of the
renderOIDCCallbackTemplate
function to accepttypes.OIDCClaims
instead ofIDTokenClaims
aligns with the overall refactoring and enhances the type safety of the code. Good job!hscontrol/db/routes.go (1)
647-647
: LGTM!The change from comparing
approvedAlias
withnode.User.Name
tonode.User.Username()
appears to be intentional and aligns with the goal of improving user identification accuracy. The overall logic flow remains intact, but the semantic meaning of the comparison has changed, which could affect the behaviour of the route approval process. However, this change seems reasonable and well-aligned with the codebase.hscontrol/db/db.go (2)
413-424
: LGTM!The new migration entry enhances the database schema management by automatically creating or updating the
User
table structure as needed. This is a useful addition that ensures the database schema stays in sync with the application's data model.
268-268
: Verify the impact of removing the hostname normalization.The change simplifies the assignment logic by directly assigning the
Hostname
toGivenName
without any normalization. This may impact how hostnames are stored in the database, potentially leading to inconsistencies if the original hostname format is not standardized.Run the following script to verify the hostname format of existing nodes in the database:
hscontrol/grpcv1.go (3)
44-44
: LGTM!The method name change from
GetUser
toGetUserByName
improves clarity by explicitly indicating that the user is retrieved by name.
73-73
: LGTM!The method name change is consistent with the previous update and improves clarity.
777-777
: LGTM!The method name change is consistent with the previous updates and improves clarity.
hscontrol/auth.go (5)
21-24
: LGTM!The new
AuthProvider
interface provides a clean abstraction for authentication providers. TheRegisterHandler
andAuthURL
methods enable flexibility in implementing custom authentication flows.
182-182
: LGTM!The removal of the unused
machineKey
parameter simplifies thehandleNodeLogOut
function signature.
190-190
: LGTM!The removal of the unused
machineKey
parameter simplifies thehandleNodeWithValidRegistration
function signature.
476-476
: LGTM!Using
h.authProvider.AuthURL(machineKey)
to generate the authentication URL is a good improvement. It centralizes the URL generation logic within theAuthProvider
interface, promoting better encapsulation and simplifying future modifications to the authentication process.
701-701
: LGTM!Using
h.authProvider.AuthURL(machineKey)
to generate the authentication URL in thehandleNodeExpiredOrLoggedOut
function is a good improvement. It centralizes the URL generation logic within theAuthProvider
interface, promoting better encapsulation and simplifying future modifications to the authentication process.hscontrol/db/node.go (3)
340-340
: LGTM!The change from
GetUser
toGetUserByName
improves code clarity by explicitly indicating that the user is retrieved by name.
393-393
: LGTM!The change from
node.User.Name
tonode.User.Username()
improves code consistency by using theUsername()
method instead of directly accessing theName
field.Also applies to: 409-409
620-621
: LGTM!The changes to the
generateGivenName
function simplify the logic and improve error handling:
- Removing the hostname normalization step and directly checking the length of the supplied name against
util.LabelHostnameLength
simplifies the function.- Returning an error if the supplied name exceeds the length limit ensures that invalid hostnames are not generated.
- Operating directly on the
suppliedName
variable instead of the previously normalized hostname reduces complexity.Also applies to: 627-628, 636-636, 639-639
hscontrol/policy/acls.go (3)
929-929
: LGTM!The change is consistent with the modification in the
filterNodesByUser
function and suggests a shift towards a more robust or standardized method for retrieving usernames.
953-953
: LGTM!The change is consistent with the modification in the
TagsOfNode
function and suggests a shift towards a more robust or standardized method for retrieving usernames.
740-740
: Verify the impact of removing the group name normalization.The change simplifies the function by eliminating error handling related to group normalization. However, this may impact how groups are processed if they are not in the expected format.
Run the following script to verify the usage of the
expandUsersFromGroup
function:Consider adding a comment to document the expected group format to avoid potential issues.
Verification successful
Verify test coverage for group name formatting in
expandUsersFromGroup
.The removal of group name normalization could affect the function's reliability if group names are not in the expected format. However, error handling and tests suggest that this is managed. Ensure that tests cover scenarios with improperly formatted group names to maintain robustness.
- Check
hscontrol/policy/acls_test.go
for test cases involving group name formatting.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `expandUsersFromGroup` function. # Test: Search for the function usage. Expect: No issues with the group format. rg --type go -A 5 $'expandUsersFromGroup'Length of output: 1909
hscontrol/app.go (4)
98-98
: LGTM!The changes to the
Headscale
struct are approved. The removal of theoidcProvider
andoauth2Config
fields and the addition of theauthProvider
field of typeAuthProvider
centralise the authentication logic.
154-178
: LGTM!The changes to the
NewHeadscale
function are approved. The new authentication logic enhances the robustness of the authentication process by:
- Adding a timeout context for the OIDC provider setup.
- Attempting to initialize the
oidcProvider
and assigning it toauthProvider
if the OIDC issuer is specified.- Falling back to CLI-based authentication if the OIDC initialization fails, maintaining backward compatibility.
444-448
: LGTM!The changes to the
createRouter
function are approved. The routing logic has been updated to:
- Directly call
h.authProvider.RegisterHandler
for the registration endpoint.- Conditionally route the OIDC callback based on the type of
authProvider
.These changes streamline the routing process and encapsulate the authentication logic within the
AuthProvider
interface, promoting better separation of concerns.
Line range hint
1-1000
: Skipped reviewing the remaining functions.The remaining functions in the file have not been modified, so they do not require a review.
hscontrol/types/config.go (13)
74-74
: LGTM!The code change simplifies the configuration by removing the
DNSUserNameInMagicDNS
field and adding theDNSConfig
field to directly use the Tailscale DNS configuration type.
92-96
: LGTM!The code change simplifies the DNS configuration by removing the
UserNameInMagicDNS
field and adding the relevant DNS configuration fields directly to theDNSConfig
struct.
314-316
: LGTM!The code change removes the deprecated configuration keys and uses the
fatal
method to handle the removal, ensuring that any attempts to use these keys will result in a fatal error.
320-328
: LGTM!The code change adds an additional check to ensure that the removed configuration keys are not present in the configuration file. If any of the removed keys are found, a fatal error is logged, prompting the user to update their configuration file.
770-775
: LGTM!The code change improves the error handling for the
prefixes.allocation
configuration key. It checks if the configured value is a valid allocation strategy and returns a descriptive error message if an invalid value is provided, including the allowed options.
809-813
: LGTM!The code change adds a validation check to ensure that the
server_url
does not contain thebase_domain
value. This is important because if theserver_url
contains thebase_domain
, it can cause the headscale server and embedded DERP to become unreachable from the Tailscale node. The error message provides a clear explanation of the issue.
843-843
: LGTM!The code change sets the
DNSConfig
field of theConfig
struct by calling theDNSToTailcfgDNS
function with thednsConfig
variable. This ensures that the DNS configuration is properly converted to the Tailscale DNS configuration format.
855-862
: LGTM!The code change sets the OIDC configuration fields of the
OIDCConfig
struct using the values from the corresponding configuration keys. This ensures that the OIDC configuration is properly populated based on the provided configuration values.
897-901
: LGTM!The code change sets the tuning configuration fields of the
Tuning
struct using the values from the corresponding configuration keys. This ensures that the tuning parameters are properly populated based on the provided configuration values.
921-928
: LGTM!The code change adds a warning message to the
warns
set of thedeprecator
if the old configuration key is set. The warning message informs the user about the deprecated key, the new key to use instead, and that the old key will be removed in the future. This helps in communicating the deprecation of configuration keys to the user.
933-940
: LGTM!The code change adds a fatal message to the
fatals
set of thedeprecator
if the old configuration key is set. The fatal message informs the user that the old key has been removed and directs them to the changelog for more details. This helps in communicating the removal of configuration keys to the user and ensures that the application fails if a removed key is still being used.
948-955
: LGTM!The code change adds a fatal message to the
fatals
set of thedeprecator
if the old configuration key is set and the new key is not set. The fatal message informs the user about the deprecated key, the new key to use instead, and that the old key has been removed. This helps in communicating the removal of configuration keys to the user and ensures that the application fails if a removed key is still being used without the new key being set.
964-971
: **LGTM!hscontrol/policy/acls_test.go (1)
651-651
: Expected output for email expansion test corrected.The expected result for the "Expand emails in group" test case has been updated from
[]string{"joe.bar.gmail.com", "john.doe.yahoo.fr"}
to[]string{"joe.bar@gmail.com", "john.doe@yahoo.fr"}
.This correction ensures that the test accurately reflects the intended behaviour of the email expansion feature, which should return the full email addresses rather than just the usernames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- hscontrol/types/users.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- hscontrol/types/users.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments not posted (3)
CHANGELOG.md (3)
7-8
: Clarify the impact of removingdns.use_username_in_magic_dns
.The removal of the
dns.use_username_in_magic_dns
configuration option is noted as a breaking change. It would be beneficial to provide more context on how this change affects existing configurations and whether any migration steps are necessary for users relying on this feature.
10-11
: Detail the transition to using thesub
claim for user identification.The shift from using the username to the
sub
claim in the ID token for OIDC is a significant change. It's important to ensure that all documentation and user guides reflect this new method of user identification to prevent confusion and to aid in a smooth transition for users.
12-14
: Expand on the implications of new user fields.The addition of new fields such as username, display name, profile picture URL, and email to the user entity is a notable enhancement. Clarify how these fields are utilised within the system, particularly how they interact with existing data structures and whether they require any database migrations or additional configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitchellkellett ill have a look. |
7c6f4e6
to
e66d149
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
hscontrol/handlers.go (1)
175-179
: Consider validating theserverURL
parameter in the constructor.To ensure that a valid URL is always set, consider adding validation to the
NewAuthProviderWeb
constructor. For example:func NewAuthProviderWeb(serverURL string) (*AuthProviderWeb, error) { if serverURL == "" { return nil, errors.New("serverURL cannot be empty") } // Additional URL validation can be added here return &AuthProviderWeb{ serverURL: serverURL, }, nil }hscontrol/types/node.go (1)
Line range hint
396-414
: Simplified FQDN generation, but reduced flexibility.The changes to the
GetFQDN
function have simplified its implementation and improved readability by removing thecfg
parameter and the associated logic for conditionally appending the username to the hostname.However, this simplification comes at the cost of reduced flexibility. The function no longer supports scenarios where the username might be relevant for FQDN generation based on configuration settings.
Please consider the following:
- Will this change break any existing code that relies on the previous behaviour?
- Is the reduced flexibility acceptable for all use cases?
- What was the reasoning behind removing the configuration-based behaviour?
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (27)
- CHANGELOG.md (1 hunks)
- flake.nix (1 hunks)
- hscontrol/app.go (3 hunks)
- hscontrol/auth.go (5 hunks)
- hscontrol/db/db.go (2 hunks)
- hscontrol/db/node.go (4 hunks)
- hscontrol/db/preauth_keys.go (3 hunks)
- hscontrol/db/routes.go (1 hunks)
- hscontrol/db/users.go (6 hunks)
- hscontrol/db/users_test.go (2 hunks)
- hscontrol/grpcv1.go (3 hunks)
- hscontrol/handlers.go (3 hunks)
- hscontrol/mapper/mapper.go (1 hunks)
- hscontrol/mapper/mapper_test.go (4 hunks)
- hscontrol/mapper/tail.go (1 hunks)
- hscontrol/oidc.go (11 hunks)
- hscontrol/policy/acls.go (3 hunks)
- hscontrol/policy/acls_test.go (2 hunks)
- hscontrol/suite_test.go (1 hunks)
- hscontrol/types/config.go (10 hunks)
- hscontrol/types/config_test.go (4 hunks)
- hscontrol/types/node.go (1 hunks)
- hscontrol/types/node_test.go (2 hunks)
- hscontrol/types/users.go (5 hunks)
- hscontrol/util/dns.go (0 hunks)
- hscontrol/util/dns_test.go (0 hunks)
- integration/auth_oidc_test.go (0 hunks)
Files not reviewed due to no reviewable changes (3)
- hscontrol/util/dns.go
- hscontrol/util/dns_test.go
- integration/auth_oidc_test.go
Files skipped from review due to trivial changes (3)
- hscontrol/grpcv1.go
- hscontrol/mapper/tail.go
- hscontrol/types/config_test.go
Additional comments not posted (78)
hscontrol/suite_test.go (1)
49-49
: Simplification of OIDC configuration looks good, but verify the impact on email domain handling.The change simplifies the initialisation of the
OIDC
field by using an emptyOIDCConfig
struct, which improves readability. However, it's important to ensure that the removal of the explicitStripEmaildomain
property setting aligns with the expected behaviour.Run the following script to verify the impact on email domain handling:
Verification successful
Approval: OIDC configuration change aligns with broader updates to email domain handling.
The removal of the explicit
StripEmaildomain
property is part of a larger change in how email domains are processed. TheOIDCConfig
struct now includesAllowedDomains
, which appears to be the new approach for handling email domains. Recent commits also indicate ongoing updates to user-related functionality, including email normalisation.Please ensure that:
- The new
AllowedDomains
approach meets all requirements previously handled byStripEmaildomain
.- Any code relying on the old
StripEmaildomain
behaviour has been updated accordingly.- Documentation is updated to reflect these changes in email domain handling.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the OIDC configuration change on email domain handling. # Test 1: Search for the definition of the `OIDCConfig` struct. # Expect: To find the default value of the `StripEmaildomain` property. rg --type go 'type OIDCConfig struct' # Test 2: Search for usage of the `StripEmaildomain` property. # Expect: To find instances where the property is used to handle email domains. rg --type go 'StripEmaildomain'Length of output: 122
Script:
#!/bin/bash # Description: Further investigate the OIDC configuration and email handling # View the contents of the OIDCConfig struct echo "Contents of OIDCConfig struct:" sed -n '/type OIDCConfig struct/,/}/p' hscontrol/types/config.go # Search for email-related properties or methods in OIDC configuration echo -e "\nEmail-related properties or methods in OIDC configuration:" rg --type go -i 'email|domain' hscontrol/types/config.go # Look for recent changes related to OIDC configuration or email handling echo -e "\nRecent changes related to OIDC or email handling:" git log -n 5 --grep="OIDC\|email\|domain" --pretty=format:"%h - %s" -- hscontrol/Length of output: 2823
hscontrol/db/users_test.go (3)
23-23
: LGTM!The change in function name from
GetUser
toGetUserByName
improves clarity and maintainability by explicitly indicating that the user is retrieved by their name.
76-76
: Looks good!The updated function name
GetUserByName
accurately reflects the functionality and maintains the test's integrity in asserting that the user is not found under the old name after renaming.
79-79
: Looks good to me!The updated function name
GetUserByName
accurately reflects the functionality and maintains the test's integrity in asserting that the user is found under the new name after renaming.hscontrol/types/users.go (7)
51-53
: Looks good!The
Username()
method provides a consistent and efficient way to retrieve the username based on the available user data. The order of preference ensures that the most relevant identifier is used.
55-58
: Looks good!The
DisplayNameOrUsername()
method enhances the user experience by providing a user-friendly display name when available, while falling back to the username if necessary. This ensures a consistent and informative representation of the user throughout the application.
109-120
: Looks good!The introduction of the
OIDCClaims
struct is a valuable addition to the codebase, as it facilitates the integration of OpenID Connect authentication. By mapping the OIDC claims to theUser
struct, it enables seamless user management and authentication using OIDC providers.
122-131
: Looks good!The
FromClaim()
method is a crucial component in the integration of OpenID Connect authentication. By mapping the OIDC claims to the corresponding fields in theUser
struct, it ensures that the user data is accurately updated based on the information provided by the OIDC provider. The decision to preserve the existing user ID is a good approach to maintain consistency.
69-70
: Looks good!The updates to the
TailscaleUser()
method ensure that the most relevant and user-friendly values are used for theLoginName
andDisplayName
fields in thetailcfg.User
struct. By leveraging the newly introducedUsername()
andDisplayNameOrUsername()
methods, it provides a consistent and informative representation of the user in the Tailscale context.
83-85
: Looks good!The updates to the
TailscaleLogin()
method ensure that the most relevant and user-friendly values are used for theLoginName
andDisplayName
fields in thetailcfg.Login
struct. By leveraging the newly introducedUsername()
andDisplayNameOrUsername()
methods, it provides a consistent and informative representation of the user's login information in the Tailscale context.
95-96
: Looks good!The updates to the
TailscaleUserProfile()
method ensure that the most relevant and user-friendly values are used for theLoginName
andDisplayName
fields in thetailcfg.UserProfile
struct. By leveraging the newly introducedUsername()
andDisplayNameOrUsername()
methods, it provides a consistent and informative representation of the user's profile information in the Tailscale context.hscontrol/db/users.go (4)
52-52
: Brilliant! The function name change enhances code clarity.Renaming
GetUser
toGetUserByName
clearly conveys that the user is retrieved based on their name. This improves code readability and maintainability.
93-93
: Excellent! The consistent function name change enhances code clarity.Renaming
GetUser
toGetUserByName
within theRenameUser
function maintains consistency and improves code readability. This change aligns with the previous function name update.Also applies to: 101-101
118-122
: Fantastic! The consistent function name change enhances code clarity.Renaming
GetUser
toGetUserByName
in both theHSDatabase
method and the standalone function maintains consistency and improves code readability. This change aligns with the previous function name updates.Also applies to: 124-134
136-152
: Superb! The new functions extend the functionality to support OIDC.The addition of
GetUserByOIDCIdentifier
functions as both anHSDatabase
method and a standalone function enhances the capability to retrieve users based on their OIDC identifier. This extension aligns with the existingGetUserByName
functions and maintains a consistent implementation approach.hscontrol/db/preauth_keys.go (3)
Line range hint
25-40
: Acknowledge the TODO comment for future improvement.The TODO comment correctly identifies an opportunity to enhance the function by using a user ID instead of a name to uniquely identify users. This change would make the function more robust and less prone to issues arising from non-unique user names.
47-47
: The function renaming improves code clarity.Renaming
GetUser
toGetUserByName
enhances code readability by explicitly indicating that the user is being retrieved by their name. This change aligns with the TODO comment about using a user ID in the future and is a step towards more robust user identification.
109-109
: The function renaming improves code consistency and clarity.Renaming
GetUser
toGetUserByName
enhances code readability by explicitly indicating that the user is being retrieved by their name. This change is consistent with the similar change made in theCreatePreAuthKey
function and improves overall code consistency.flake.nix (1)
35-35
: Spot on! ThevendorHash
update is necessary and appreciated.Updating the
vendorHash
after modifyinggo.mod
orgo.sum
is crucial for maintaining the integrity of the build process. This change ensures that the correct dependencies are being used, preventing potential build failures or unexpected behaviour. Brilliant work following the best practice mentioned in the comment!hscontrol/handlers.go (3)
181-186
: LGTM!The
AuthURL
method correctly constructs the URL by appending the machine key to the server URL. The use ofstrings.TrimSuffix
to remove any trailing slash is a nice touch.
Line range hint
193-252
: LGTM!The
RegisterHandler
method looks good:
- It correctly extracts and validates the machine key from the request URL.
- It has appropriate error handling in place if the machine key is invalid.
- It renders the HTML template correctly with the machine key.
- It sets the response headers and status codes appropriately.
208-208
: LGTM!The change in the error message from "nodekey" to "machinekey" improves clarity and consistency in the codebase.
hscontrol/types/node_test.go (5)
Line range hint
14-90
:
TheTest_NodeCanAccess
function has not been modified in this diff. Skipping review.
167-172
: Excellent addition of a new test case!The new test case for handling the scenario where the username exceeds the maximum allowable length for a hostname is a great addition to improve the test coverage. Well done!
Line range hint
206-312
:
TheTestPeerChangeFromMapRequest
function has not been modified in this diff. Skipping review.
Line range hint
314-400
:
TheTestApplyPeerChange
function has not been modified in this diff. Skipping review.
Line range hint
92-204
: Verify the impact of the removed test cases.Several test cases related to the inclusion of the username in the FQDN generation have been removed. This suggests a change in the expected behaviour of the
GetFQDN
method.Please ensure that the removal of these test cases aligns with the intended behaviour of the
GetFQDN
method and does not introduce any regressions in the codebase.Verification successful
Removal of username-related test cases for GetFQDN is consistent with implementation
The verification process confirms that the removal of test cases related to including the username in FQDN generation aligns with the current implementation of the
GetFQDN
method. The method now only uses theGivenName
field, and this behaviour is consistently reflected across the codebase. The remaining test cases inhscontrol/types/node_test.go
provide adequate coverage for the current functionality, including successful FQDN generation and error handling scenarios.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the removed test cases on the `GetFQDN` method and the codebase. # Test: Search for usages of the `GetFQDN` method in the codebase. rg --type go -A 5 $'GetFQDN' # Test: Search for test cases that rely on the username being included in the FQDN. rg --type go -A 5 $'GetFQDN.*User'Length of output: 1584
hscontrol/mapper/mapper_test.go (3)
32-34
: LGTM!The addition of the
Model
field from GORM in theUser
struct is a positive change that enhances the representation of user profiles and potentially improves integration with database operations. The test logic remains unchanged, indicating that the core functionality of generating user profiles is unaffected by this struct change.
Line range hint
141-432
:
Line range hint
79-129
: Verify the impact of the DNS configuration changes.The test case has been updated with the following changes:
- The
Routes
field of thetailcfg.DNSConfig
struct is now an empty map.- The
DNSUserNameInMagicDNS
field has been removed from theConfig
struct instantiation.Please ensure that these changes align with the intended behaviour and do not introduce any unintended side effects in the DNS configuration process.
To verify the impact of these changes, consider running the following script:
hscontrol/mapper/mapper.go (2)
Line range hint
97-110
: Excellent change to use user IDs for constructing theuserMap
.Using user IDs instead of user names enhances the uniqueness and reliability of user identification, as IDs are less prone to duplication compared to names. The logic for populating the
userMap
remains largely the same, iterating through peers to add their corresponding users.This change should not have any negative impact on the functionality and is a positive improvement.
Line range hint
112-128
: Verify the impact of the removed MagicDNS code block.The removal of the substantial code block related to MagicDNS functionality indicates a shift in how DNS configurations are generated. It possibly simplifies the logic or alters the intended functionality regarding DNS routing for users.
While the remaining code suggests that the core functionality of DNS configuration remains intact, it's unclear from the provided context how this change impacts the overall DNS functionality.
Please ensure that this removal does not break any existing functionality and that the DNS configuration generation still aligns with the intended system behaviour. Consider running the following script to analyse the impact:
hscontrol/oidc.go (12)
50-59
: TheAuthProviderOIDC
struct is well-structured and modular.The struct encapsulates various components required for OIDC authentication, such as the server URL, OIDC configuration, database access, registration cache, notifier, IP allocator, OIDC provider, and OAuth2 configuration. Each field has a specific responsibility, making the code more maintainable and easier to understand.
62-100
: TheNewAuthProviderOIDC
function is a well-designed constructor.The function takes in all the necessary parameters to initialize the
AuthProviderOIDC
struct. It creates a new OIDC provider using the provided issuer configuration and initializes the OAuth2 configuration with the appropriate settings. The function returns a newAuthProviderOIDC
instance with all the fields properly initialized, ensuring a clean and consistent setup.
102-106
: TheAuthURL
method generates the authentication URL correctly.The method takes a machine key as input and constructs the authentication URL by appending the machine key to the server URL. It ensures that the server URL is properly formatted by trimming any trailing slash, preventing potential issues with URL construction.
109-114
: ThedetermineTokenExpiration
method correctly determines the token expiration time.The method takes the ID token expiration time as input and determines the token expiration time based on the configuration. If the
UseExpiryFromToken
flag is set, it returns the ID token expiration time. Otherwise, it calculates the expiration time by adding the configured expiry duration to the current time. This allows flexibility in determining the token expiration based on the specific requirements.
Line range hint
119-170
: TheRegisterHandler
method handles the OIDC registration process effectively.The method extracts the machine key from the request URL and validates it to ensure it's a valid key. It generates a random state string and stores the machine key in the registration cache using the state as the key. This allows the callback handler to retrieve the machine key later. The method constructs the authorization URL with the necessary parameters, including any extra parameters provided in the configuration, and redirects the user to the OIDC provider for authentication. The code is well-structured and follows the OIDC registration flow correctly.
Line range hint
190-270
: TheOIDCCallback
method handles the OIDC callback process comprehensively.The method follows the OIDC callback flow step by step. It validates the callback parameters, exchanges the authorization code for an ID token, and verifies the ID token. It extracts the claims from the ID token and validates them against the allowed domains, groups, and users specified in the configuration. This ensures that only authorized users are allowed to register nodes.
The method then validates the node associated with the state parameter and registers it if it's a new node. It creates or updates the user based on the claims and registers the node, associating it with the user.
Finally, it renders a callback template with the user information to provide feedback to the user.
The code is well-structured, handles errors appropriately, and covers all the necessary steps in the OIDC callback process.
285-299
: ThegetIDTokenForOIDCCallback
method retrieves the ID token correctly.The method takes the authorization code as input and exchanges it for an OAuth2 token using the OIDC provider. It then extracts the ID token from the OAuth2 token. If the ID token is missing, it returns an appropriate error. The code is straightforward and follows the expected flow for retrieving the ID token.
302-312
: TheverifyIDTokenForOIDCCallback
method verifies the ID token securely.The method takes the raw ID token as input and creates a verifier using the OIDC provider and the client ID from the configuration. It then verifies the ID token using the verifier. If the verification fails, it returns an appropriate error. This ensures that only valid and authentic ID tokens are accepted, enhancing the security of the authentication process.
Line range hint
371-441
: ThevalidateNodeForOIDCCallback
method validates the node effectively.The method retrieves the machine key from the registration cache using the state parameter. If the machine key is not found, it returns an appropriate error. It then retrieves the node information from the database using the machine key.
If the node already exists, it updates the node's expiry and notifies the node and its peers about the expiry update. This ensures that the node's expiry is properly refreshed and synchronized across the network.
If the node doesn't exist, it returns the machine key and a flag indicating that it's a new node, allowing the caller to proceed with the registration process.
The code handles both existing and new nodes correctly and takes care of updating the expiry and sending notifications when necessary.
444-474
: ThecreateOrUpdateUserFromClaim
method handles user creation and updates effectively.The method retrieves the user from the database using the OIDC identifier from the claims. If the user is not found, it attempts to retrieve the user by username for legacy compatibility. This ensures a smooth transition for existing users.
If the user still doesn't exist, it creates a new empty user. It then updates the user's fields based on the claims, ensuring that the user's information is up to date.
Finally, it saves the user to the database, persisting the changes.
The code handles both new and existing users correctly and takes care of updating the user's information based on the claims.
Line range hint
477-507
: TheregisterNodeForOIDCCallback
method registers a new node securely.The method allocates IPv4 and IPv6 addresses for the node using the IP allocator. It then registers the node in the database using the machine key, user information, expiry, and allocated IP addresses.
It uses a database transaction to ensure that the registration is atomic and consistent. If the registration fails for any reason, it returns an appropriate error.
The code handles the node registration process correctly and takes care of allocating IP addresses and storing the necessary information in the database.
Line range hint
510-522
: TherenderOIDCCallbackTemplate
function renders the callback template correctly.The function takes the OIDC claims as input and executes the OIDC callback template. It passes the user's email and a verb ("Authenticated") as the template data, providing the necessary information for rendering.
If the template rendering fails, it returns an appropriate error. Otherwise, it returns the rendered template content.
The code handles the template rendering process correctly and provides the required data to the template.
hscontrol/db/routes.go (1)
647-647
: Approve the change in user identifier comparison.The change from comparing
approvedAlias
withnode.User.Name
tonode.User.Username()
is an improvement. It ensures that the correct user identifier is used for determining approved aliases, potentially enhancing the accuracy of the auto-approval process for routes.hscontrol/db/db.go (2)
413-424
: LGTM! The new migration step for theUser
type is a necessary update.The addition of the migration step with ID
202407191627
is crucial for maintaining data integrity and supporting the updated user management functionality. TheAutoMigrate
function ensures that theUser
structure is properly integrated into the database schema.Please ensure thorough testing of the user management features to confirm a smooth transition and verify that the migration does not introduce any unintended side effects.
268-268
: Verify the impact of removing the hostname normalization.The removal of the normalization step when assigning the
GivenName
property ofNode
objects simplifies the code. However, please ensure that this change does not introduce inconsistencies or violate any expected hostname formats in the system.Consider running the following script to analyse the existing
GivenName
values and confirm that they align with the expected format:Replace
<path_to_db>
with the path to your SQLite database file. The script retrieves theGivenName
values, formats them as a JSON array, and pipes the output tojq
for pretty-printing. Review the output to ensure the values meet the expected hostname format.hscontrol/auth.go (7)
21-24
: LGTM!The
AuthProvider
interface provides a clean abstraction for handling authentication, allowing different authentication strategies to be implemented. The method signatures are well-defined and serve their intended purpose.
Line range hint
21-244
: Simplification ofhandleRegister
function.The removal of direct machine key handling in several function calls within
handleRegister
simplifies the function's signature and improves readability. The core logic of the function remains intact, handling various registration scenarios based on the state of the node in the database.
171-172
: SimplifiedhandleNodeLogOut
function signature.The removal of the
machineKey
parameter from thehandleNodeLogOut
function simplifies the function signature without affecting the core functionality of logging out a client and handling ephemeral nodes.
179-180
: SimplifiedhandleNodeWithValidRegistration
function signature.The removal of the
machineKey
parameter from thehandleNodeWithValidRegistration
function simplifies the function signature without affecting the core functionality of handling a client with a valid registration and redirecting them to the/map
endpoint.
Line range hint
189-191
: SimplifiedhandleNodeKeyRefresh
function signature.The removal of the
machineKey
parameter from thehandleNodeKeyRefresh
function simplifies the function signature without affecting the core functionality of updating the node's key in the database and responding to the client with the appropriate fields.
452-453
: Centralized authentication URL generation usingAuthProvider
interface.The utilization of the
AuthProvider
interface to generate the authentication URL in thehandleNewNode
function centralizes the URL generation logic, making the code cleaner and more maintainable. TheAuthURL
method of the interface is called with the machine key as a parameter, and the generated URL is set in the response'sAuthURL
field.
677-678
: Centralized authentication URL generation usingAuthProvider
interface.Similar to the changes in the
handleNewNode
function, the utilization of theAuthProvider
interface to generate the authentication URL in thehandleNodeExpiredOrLoggedOut
function centralizes the URL generation logic, making the code cleaner and more maintainable. TheAuthURL
method of the interface is called with the machine key as a parameter, and the generated URL is set in the response'sAuthURL
field.hscontrol/db/node.go (4)
335-335
: LGTM!The function update to use
GetUserByName
instead ofGetUser
is appropriate and consistent with the naming convention.
388-388
: LGTM!The update to use
node.User.Username()
instead ofnode.User.Name
in the log statement improves encapsulation and maintainability.
404-404
: LGTM!The update to use
node.User.Username()
instead ofnode.User.Name
in the log statement is consistent with the previous change and improves encapsulation.
624-625
: LGTM!The updates to the
generateGivenName
function improve the logic flow and simplify the code:
- Moving the hostname length check to the beginning of the function ensures early validation.
- Removing the hostname normalization step simplifies the logic.
- Using the supplied name directly for trimming improves clarity and reduces unnecessary operations.
The changes enhance the readability and maintainability of the function.
Also applies to: 631-632, 640-640
hscontrol/policy/acls.go (2)
929-929
: LGTM!The string equality check is the correct way to verify if the node's user has the authority to add the tag.
953-953
: LGTM!The string equality check is the correct way to filter nodes by user.
hscontrol/app.go (4)
98-98
: LGTM!The addition of the
authProvider
field enhances modularity by abstracting the authentication mechanism through theAuthProvider
interface. This allows for easier integration of different authentication strategies in the future.
154-178
: LGTM!The
AuthProvider
initialization logic in theNewHeadscale
function is well-structured and provides a fallback mechanism. It encapsulates the authentication setup based on the configuration, ensuring that the application can function with either OIDC or CLI-based authentication. The assignment of theauthProvider
field centralizes the authentication provider for the application.
444-444
: LGTM!The updated routing logic for the
/register/{mkey}
endpoint now directly calls theRegisterHandler
method of theauthProvider
. This change centralizes the registration handling logic within theAuthProvider
implementation, promoting a more modular and maintainable codebase.
446-448
: LGTM!The conditional registration of the OIDC callback endpoint based on the type of
authProvider
is a good practice. It ensures that the callback is only exposed when OIDC authentication is being used, preventing unnecessary exposure when OIDC is not configured. The direct registration of the callback to theOIDCCallback
method of theAuthProviderOIDC
instance keeps the OIDC-specific logic encapsulated within the provider, promoting a cleaner separation of concerns.CHANGELOG.md (6)
7-8
: **** The previous review comment is still valid and applicable:The removal of this configuration option is noted as a breaking change, which could impact existing deployments that rely on this feature for DNS configurations. It's crucial to ensure that this change is clearly communicated to users to prevent potential disruptions.
9-11
: **** The previous review comment is still valid and applicable:The removal of the
strip_email_domain
option and the shift to using thesub
claim for user identification are significant. This change enhances security and flexibility in user management but requires careful migration planning for existing systems to adapt without issues.
12-14
: **** The previous review comment is still valid and applicable:Adding fields such as username, display name, profile picture URL, and email enhances the user experience by providing more detailed user profiles. These fields also prepare the system for future enhancements, such as API/CLI access for non-OIDC users. It's important to ensure that these fields are handled securely, especially concerning data privacy and potential exposure through public interfaces.
3-4
: **** The addition of the "Next" section is a good practice.It provides a clear separation for upcoming changes or features, helping users and contributors understand what to expect in future releases.
5-6
: **** The addition of the "BREAKING" subsection is crucial.It highlights breaking changes in the upcoming release, helping users and contributors prepare for potential compatibility issues and plan their upgrades accordingly.
15-15
: **** The addition of the new release section for version 0.23.0 follows the existing changelog format.It provides a clear separation for the changes introduced in the specific release version.
hscontrol/types/config.go (6)
74-74
: LGTM!The removal of the unused
DNSUserNameInMagicDNS
field from theConfig
struct simplifies the DNS configuration and aligns with the deprecation of theuse_username_in_magic_dns
configuration key.
92-96
: LGTM!The removal of the unused
UserNameInMagicDNS
field from theDNSConfig
struct simplifies the DNS configuration and aligns with the deprecation of theuse_username_in_magic_dns
configuration key. The addition ofmapstructure
tags ensures proper mapping of configuration values to the struct fields.
320-323
: LGTM!Registering fatal errors for the deprecated configuration keys
oidc.strip_email_domain
,dns.use_username_in_musername_in_magic_dns
, anddns_config.use_username_in_musername_in_magic_dns
ensures that the application fails to start if these keys are still present in the configuration. This enforces the removal of the deprecated keys and encourages users to update their configuration.
326-334
: LGTM!The introduced loop checks for the presence of removed configuration keys and logs a fatal error if any of them are set in the configuration. The fatal error message provides clear instructions to the user to remove the key from the configuration file. This enforces the removal of the removed keys and encourages users to update their configuration.
790-795
: LGTM!Returning an error for an invalid
prefixes.allocation
configuration value ensures that the application fails to start with an invalid configuration. The error message provides clear information about the invalid value and the allowed options (sequential
andrandom
), helping users identify and correct the misconfiguration.
829-833
: LGTM!Returning an error when
server_url
containsbase_domain
prevents a configuration that would cause the headscale server and embedded DERP to become unreachable from the Tailscale node. The error message provides a clear explanation of the issue and its impact, helping users identify and correct the misconfiguration.hscontrol/policy/acls_test.go (2)
651-651
: Corrected the expected output for the "Expand emails in group" test case.The change fixes the expected result to contain the full email addresses "joe.bar@gmail.com" and "john.doe@yahoo.fr" instead of the incorrect values. This aligns the test with the intended behaviour of returning email addresses without any alterations.
344-344
: Added a new test case for IPv6 support.The "ipv6" test case is a valuable addition that verifies the correct handling of IPv6 addresses in the ACL policy. It ensures that the generated filter rules include both IPv4 and IPv6 addresses as expected. This improves the test coverage for IPv6 functionality.
} | ||
|
||
return normalizedHostname, nil | ||
return suppliedName, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the typo in the function name.
The function name "isUnqiueName" has a typo. It should be "isUniqueName" for clarity and readability.
Apply this diff to fix the typo:
-func isUnqiueName(tx *gorm.DB, name string) (bool, error) {
+func isUniqueName(tx *gorm.DB, name string) (bool, error) {
Committable suggestion was skipped due to low confidence.
) | ||
} | ||
users = append(users, grp) | ||
users = append(users, group) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate and normalize the group name before appending it to the users slice.
Appending a group directly to the users slice without any validation or normalization could lead to inconsistencies in the data structure and potential security issues.
Consider applying this diff to validate and normalize the group name:
- users = append(users, group)
+ normalizedGroup, err := util.NormalizeGroupName(group)
+ if err != nil {
+ return nil, fmt.Errorf("invalid group name %q: %w", group, err)
+ }
+ users = append(users, normalizedGroup)
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
hscontrol/oidc.go (1)
Line range hint
361-439
: Refactoring needed for clarity and single responsibilityThe
validateNodeForOIDCCallback
method is currently handling multiple responsibilities, including checking the registration cache, looking up the machine in the database, and reauthorizing the machine if it exists. The TODO comments correctly identify the need for refactoring to improve the method's clarity and adherence to the single responsibility principle.While the error handling and node reauthorization logic seem to be in place, the method's overall structure and purpose need improvement.
Consider prioritizing the refactoring of this method to enhance its clarity and ensure it focuses on a single responsibility. This will make the code more maintainable and easier to understand.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
Files selected for processing (12)
- flake.nix (1 hunks)
- go.mod (1 hunks)
- hscontrol/app.go (6 hunks)
- hscontrol/auth.go (5 hunks)
- hscontrol/db/db.go (6 hunks)
- hscontrol/db/db_test.go (3 hunks)
- hscontrol/db/node.go (6 hunks)
- hscontrol/db/routes_test.go (1 hunks)
- hscontrol/db/suite_test.go (1 hunks)
- hscontrol/grpcv1.go (4 hunks)
- hscontrol/oidc.go (9 hunks)
- hscontrol/types/node_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- flake.nix
- hscontrol/db/db.go
- hscontrol/grpcv1.go
Additional comments not posted (27)
hscontrol/db/suite_test.go (1)
62-62
: Clarify the purpose and impact ofemptyCache()
.The
newTestDB
function has been updated to include a new argumentemptyCache()
. However, the purpose and impact of this function are not clear from the provided context.Please provide more information about what
emptyCache()
does and how it affects the database setup process. This will help ensure that the change is properly reviewed and any potential issues are identified.hscontrol/db/db_test.go (3)
10-10
: Looks good!The import of the
time
package is appropriate as it is a standard library package in Go that provides functionality for measuring and displaying time.
17-17
: Import looks appropriate.The import of the
zgo.at/zcache/v2
package is appropriate as it likely provides caching functionality that is used in the code. The import path also suggests it is version 2 of the package.
127-127
: Appropriate usage of caching.The changes to the
NewHeadscaleDatabase
function call are appropriate:
- The empty string argument is likely a placeholder for a parameter that is not relevant in this test context.
- The
emptyCache()
function creates and returns a new instance ofzcache.Cache
with specific time durations for expiration and cleanup.- The cache is likely used by the
HSDatabase
for performance optimization.hscontrol/types/node_test.go (5)
Line range hint
17-103
: LGTM!The test cases for
Test_NodeCanAccess
are well-structured, cover a good range of scenarios, and follow best practices such as using thecmp
package for comparisons and helper functions for improved readability.
Line range hint
105-206
: Looks good!The changes to the test cases in
TestNodeFQDN
streamline the testing process and focus on important edge cases related to username length and DNS configurations. The addition of the new test case for handling long usernames is a valuable improvement.
Line range hint
208-298
:
Line range hint
300-374
:
Line range hint
376-420
:go.mod (2)
210-210
: Verify the necessity and compatibility of the added dependency.Please ensure that the added indirect dependency
zgo.at/zcache
at versionv1.2.0
is necessary for the project and compatible with the project's license and other dependencies.
211-211
: Verify the necessity and compatibility of the added dependency and manage potential version conflicts.Please ensure that the added indirect dependency
zgo.at/zcache/v2
at versionv2.1.0
is necessary for the project and compatible with the project's license and other dependencies.Also, note that the
zgo.at/zcache
dependency is added for bothv1.2.0
andv2.1.0
versions. Please ensure that this does not lead to any version conflicts in the project.hscontrol/oidc.go (6)
49-103
: Excellent refactoring to improve modularity and clarity!The introduction of the
AuthProviderOIDC
struct and the corresponding constructor functionNewAuthProviderOIDC
enhances the code structure by encapsulating related components and initializing the OIDC provider and OAuth2 configuration in a clear and concise manner. The error handling in the constructor function is also appropriate, returning an error if the OIDC provider creation fails.These changes promote better modularity, readability, and maintainability of the codebase.
Line range hint
122-173
: LGTM! The changes align with the refactoring.The updates to the
RegisterHandler
method, making it a method of theAuthProviderOIDC
struct, are in line with the overall refactoring of moving OIDC-related functionality into theAuthProviderOIDC
struct. The error handling is appropriate, returning HTTP errors for invalid machine key or internal server errors.The caching of the node key using the
registrationCache
is a good approach to retrieve it later in the callback.
Line range hint
192-269
: Great job with the modular structure and error handling!The updates to the
OIDCCallback
method, making it a method of theAuthProviderOIDC
struct, align with the overall refactoring. The error handling is thorough, returning appropriate HTTP errors for various failure scenarios.The method delegates the extraction of the ID token, validation of allowed domains/groups/users, and creation/update of the user to separate methods, promoting a modular and readable structure. The registration of the node is handled by the
registerNodeForOIDCCallback
method, which is a good separation of concerns.Overall, the changes enhance the clarity and maintainability of the code.
271-307
: Improved readability and error handling!The introduction of the
extractCodeAndStateParamFromRequest
function as a helper function encapsulates the extraction logic, improving the readability of the code.The changes to the
extractIDToken
method signature, making it a method of theAuthProviderOIDC
struct, align with the overall refactoring. The error handling in both functions is appropriate, returning errors for missing parameters or failed token exchange/verification.These changes enhance the clarity and robustness of the code.
Line range hint
309-359
: Clear structure and modularity in validation functions!The
validateOIDCAllowedDomains
,validateOIDCAllowedGroups
, andvalidateOIDCAllowedUsers
functions are well-structured and follow a clear logic for checking the allowed domains, groups, and users. The error handling is appropriate, returning specific errors for each validation failure scenario.The separation of the validation logic into distinct functions promotes code reusability and modularity, making the code easier to understand and maintain.
Great job with the validation functions!
442-498
: Improved user management and modular node registration!The
createOrUpdateUserFromClaim
method handles the logic for creating or updating a user based on the OIDC claims, enhancing the user management process. The inclusion of a check for legacy support, looking up the user by username if not found by OIDC identifier, is a good transitional approach. The error handling in the method is appropriate, propagating any errors that occur during user retrieval or saving.The
registerNodeForOIDCCallback
method encapsulates the node registration logic, promoting a modular and focused design. It handles the allocation of IPv4 and IPv6 addresses and registers the node with the appropriate details.These changes improve the overall structure and maintainability of the codebase.
hscontrol/auth.go (5)
21-24
: Excellent addition of theAuthProvider
interface!Defining the
AuthProvider
interface is a great step towards making the authentication mechanism more modular and flexible. It allows different authentication strategies to be easily plugged in and swapped out as needed. This change promotes a cleaner separation of concerns and improves the overall design of the authentication system.
170-170
: Good simplification of thehandleNodeLogOut
function signature.Removing the
machineKey
parameter from thehandleNodeLogOut
function is a positive change. It simplifies the function signature and improves readability by reducing the number of parameters. This suggests that thenode
parameter alone provides sufficient information for handling the node logout process. Well done on this simplification!
178-178
: Another good simplification of the function signature.Removing the
machineKey
parameter from thehandleNodeWithValidRegistration
function is consistent with the previous change and further improves the codebase. It makes the function signature cleaner and more focused. Thenode
parameter seems to provide all the required information for handling a node with a valid registration. This change enhances readability and maintainability. Great job on this simplification as well!
Line range hint
389-399
: Proper use of database transaction and error handling.The code segment demonstrates the correct usage of a database transaction using the
db.Write
function. Executing thedb.UsePreAuthKey
operation within a transaction ensures data integrity and consistency. If any error occurs during the transaction, it is properly handled, and an appropriate error response is returned. This helps maintain the reliability and stability of the system. Well done on implementing the transaction and error handling correctly!
450-450
: Effective use of theAuthProvider
interface.Assigning the value returned by
h.authProvider.AuthURL(machineKey)
toresp.AuthURL
is an excellent way to leverage theAuthProvider
interface. It allows the authentication URL to be generated dynamically based on the specificauthProvider
implementation and the providedmachineKey
. This change promotes modularity and flexibility in the authentication process. It enables different authentication providers to be easily integrated without modifying the core logic. Great job on utilizing theAuthProvider
interface effectively!hscontrol/db/node.go (2)
Line range hint
315-368
: LGTM!The
RegisterNodeFromAuthCallback
method looks good. It properly handles node registration from an API/CLI or auth callback, updating the node's details if found in the registration cache or returning an appropriate error if not found. The logic flow is clear and error handling is robust.
Line range hint
379-426
: LGTM!The
RegisterNode
function looks good. It properly handles registering a new node, distinguishing between nodes that already have IP addresses (just saving to update expiry and node key) and those that don't (ensuring a unique given name and setting IP addresses before saving). The logic flow is clear, error handling is robust, and logging is appropriate.hscontrol/db/routes_test.go (1)
335-335
: LGTM! The addition of theemptyCache()
parameter is a good practice.Passing
emptyCache()
toNewHeadscaleDatabase
ensures that each test runs with a clean cache state. This prevents potential side effects from previous tests and makes the tests more reliable and independent of each other.hscontrol/app.go (2)
Line range hint
123-179
: Excellent work on introducing theAuthProvider
abstraction! 🌟The changes in the
NewHeadscale
function significantly improve the flexibility and modularity of the authentication mechanism. By introducing theAuthProvider
interface, the code now supports different authentication strategies, such as the defaultAuthProviderWeb
and the optionalAuthProviderOIDC
. This abstraction allows for easier integration of new authentication methods in the future.Moreover, the OIDC provider initialisation is now handled gracefully. If the OIDC initialisation fails, the application can either return an error or fall back to the default authentication provider based on the configuration. This enhances the robustness of the authentication process.
Additionally, moving the registration cache initialisation within this function improves encapsulation and keeps the related logic together.
Overall, these changes demonstrate a well-structured and modular approach to handling authentication in the Headscale application. Great job! 👍
445-449
: Great refactoring of the registration route handling! 👏The modifications in the
createRouter
function demonstrate a cleaner and more modular approach to handling registration requests. By delegating the registration handling to theRegisterHandler
method of theauthProvider
, the code achieves better separation of concerns and promotes consistency across different authentication strategies.Furthermore, the conditional registration of the OIDC callback route based on the type of
authProvider
is a nice touch. It ensures that the callback route is only available when using the OIDC authentication provider, preventing unnecessary route registration and improving code clarity.These changes enhance the overall structure and maintainability of the authentication flow in the Headscale application. Well done! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
hscontrol/oidc.go (1)
Line range hint
192-269
: TheOIDCCallback
method is implemented comprehensively.It handles the OIDC callback process effectively by:
- Extracting and validating the necessary parameters from the request URL.
- Exchanging the code for an ID token and verifying it.
- Validating the claims against the configured allowed domains, groups, and users.
- Handling node validation and registration based on whether the node already exists or is new.
- Creating or updating the user based on the claims.
- Rendering an appropriate HTML template with the authenticated user information.
- Handling errors appropriately and returning meaningful error responses.
The method covers all the necessary steps and includes robust error handling.
To further improve the code, consider extracting the rendering of the HTML template into a separate function to keep the
OIDCCallback
method focused on the core logic and improve readability.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- hscontrol/db/node.go (4 hunks)
- hscontrol/db/users.go (6 hunks)
- hscontrol/grpcv1.go (4 hunks)
- hscontrol/oidc.go (9 hunks)
- hscontrol/types/users.go (5 hunks)
Files skipped from review as they are similar to previous changes (4)
- hscontrol/db/node.go
- hscontrol/db/users.go
- hscontrol/grpcv1.go
- hscontrol/types/users.go
Additional comments not posted (9)
hscontrol/oidc.go (9)
49-58
: Excellent work on encapsulating OIDC-related fields in a dedicated struct.This change enhances code organization and modularity by grouping related fields together. It will make the code more maintainable and easier to understand.
61-103
: Great job on implementing the constructor function forAuthProviderOIDC
.The function properly initializes all the necessary fields of the struct using the provided dependencies. It encapsulates the initialization logic in a single place, making it easier to create instances of
AuthProviderOIDC
correctly.
105-109
: TheAuthURL
method looks good.It correctly generates the authentication URL for a given machine key by appending it to the server URL with the appropriate path.
112-118
: ThedetermineTokenExpiration
method is implemented correctly.It properly determines the token expiration time based on the configuration settings, either using the expiry from the token or calculating a new expiry using the configured duration.
Line range hint
122-173
: TheRegisterHandler
method is implemented robustly.It handles the OIDC registration process effectively by:
- Extracting and validating the machine key from the request URL.
- Generating a random state and storing the machine key in the registration cache.
- Constructing the authorization URL with the necessary parameters and redirecting the user to the OIDC provider.
- Handling errors appropriately and returning meaningful error responses.
Well done!
271-282
: TheextractCodeAndStateParamFromRequest
function is implemented correctly.It properly extracts the code and state parameters from the request URL query and returns an error if either parameter is empty. The function is simple and serves its purpose well.
284-307
: TheextractIDToken
method is implemented securely.It handles the extraction and verification of the ID token from the OAuth2 token correctly by:
- Exchanging the code for an OAuth2 token using the proper configuration.
- Extracting the raw ID token from the OAuth2 token extras.
- Verifying the ID token using the OIDC provider's verifier and the client ID.
- Handling errors appropriately and returning meaningful error messages.
The method ensures the integrity and validity of the ID token before proceeding further.
442-473
: ThecreateOrUpdateUserFromClaim
method is implemented effectively.It handles the creation or update of a user based on the OIDC claims correctly by:
- Attempting to retrieve the user by the OIDC identifier first.
- Falling back to username lookup for legacy compatibility if the user is not found by the OIDC identifier.
- Creating a new empty user if the user is still not found.
- Updating the user fields based on the claims and saving the user to the database.
- Handling errors appropriately and returning meaningful error messages.
The method ensures that the user is properly created or updated based on the available information in the OIDC claims.
Line range hint
361-439
: ThevalidateNodeForOIDCCallback
method handles node validation and reauthentication, but there is room for improvement.The method is currently handling multiple responsibilities, including node validation, reauthentication, and rendering the HTML template. Consider refactoring the method to separate these concerns and improve readability and maintainability. For example:
- Extract the node lookup and expiry update logic into a separate method.
- Move the notification logic to a dedicated method.
- Extract the HTML template rendering to a separate function.
This will make the code more modular and easier to understand and maintain.
Additionally, please verify the impact of the changes made to the node validation and reauthentication process on the overall codebase. Ensure that the changes do not introduce any unintended side effects or break existing functionality.
Run the following script to check for potential issues:
Review the results and confirm that the changes are compatible with the existing codebase and do not introduce any regressions.
Verification successful
Refactoring suggestion for
validateNodeForOIDCCallback
is valid and can be extended furtherThe review comment's suggestion to refactor the
validateNodeForOIDCCallback
method is indeed valid and can be implemented without introducing unintended side effects. Based on the analysis of the codebase, we can extend the refactoring suggestion to improve code reuse and maintainability:
- Extract the node lookup and expiry update logic into a separate method, as it's used in multiple places across the codebase.
- Create a dedicated method for sending notifications, which can be reused in other parts of the codebase that perform similar operations.
- Move the HTML template rendering to a separate function, as suggested in the original review.
These changes will not only improve the readability and maintainability of the
validateNodeForOIDCCallback
method but also enhance the overall structure of the codebase by promoting code reuse.To implement these changes:
- Create utility functions for node lookup, expiry update, and notifications in a separate file (e.g.,
node_utils.go
).- Update all occurrences of these operations throughout the codebase to use the new utility functions.
- Refactor the
validateNodeForOIDCCallback
method to use these utility functions and separate the HTML rendering logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usages of the validateNodeForOIDCCallback method rg --type go -A 5 $'validateNodeForOIDCCallback' # Check for potential issues related to node expiry and notifications rg --type go -A 5 $'NodeSetExpiry|NotifyByNodeID|NotifyWithIgnore'Length of output: 7024
There could be an issue if we use the Another scenario is when I use Keycloak, but we switch the backend from OpenLDAP to AD. In this case, the The candidate claims for the username can be limited to |
From how I understand, the
Taking into consideration the migration between multiple OIDC providers feels like unnecessary work for a small number of cases. I dont think we benefit a lot from having a very flexible configuration vs having a simple, and hopefully more correct codebase. If you change your OIDC provider, a reasonable migration path is to write your own script mapping the new
More or less same argument as above for this one. It seems like an external change outside out our system. |
Tagging a couple of people that have shown interest, might be helpful reviewing this: |
@@ -16,19 +18,57 @@ import ( | |||
// that contain our machines. | |||
type User struct { | |||
gorm.Model | |||
|
|||
// Username for the user, is used if email is empty | |||
// Should not be used, please use Username(). | |||
Name string `gorm:"unique"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, Name
is the user full name (given + family)... not necesarily unique.
https://openid.net/specs/openid-connect-basic-1_0-22.html#id_res
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the username, or, in OIDC terms: preferred_username
, I suppose there is a risk that the upstream does not enforce uniqueness here, but also, we are kind of at the mercy of the non-oidc setup where we have had this unique all the time.
The Name you are referred to is going under DisplayName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was actually pointing at the wrong field, so fixed that.
15e0688
to
801d99b
Compare
expand user, add claims to user This commit expands the user table with additional fields that can be retrieved from OIDC providers (and other places) and uses this data in various tailscale response objects if it is available. This is the beginning of implementing https://docs.google.com/document/d/1X85PMxIaVWDF6T_UPji3OeeUqVBcGj_uHRM5CI-AwlY/edit trying to make OIDC more coherant and maintainable in addition to giving the user a better experience and integration with a provider. remove usernames in magic dns, normalisation of emails this commit removes the option to have usernames as part of MagicDNS domains and headscale will now align with Tailscale, where there is a root domain, and the machine name. In addition, the various normalisation functions for dns names has been made lighter not caring about username and special character that wont occur. Email are no longer normalised as part of the policy processing. untagle oidc and regcache, use typed cache This commits stops reusing the registration cache for oidc purposes and switches the cache to be types and not use any allowing the removal of a bunch of casting. try to make reauth/register branches clearer in oidc Currently there was a function that did a bunch of stuff, finding the machine key, trying to find the node, reauthing the node, returning some status, and it was called validate which was very confusing. This commit tries to split this into what to do if the node exists, if it needs to register etc. Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
801d99b
to
2055f3d
Compare
The failing test is breaking because of tailscale/tailscale@1eaad7d, I will merge this and file a separate issue as it is only HEAD that is failing. |
Yup. :) The other issue here is if you key on things like Here is an example of a similar bug in Mastodon: GHSA-vm39-j3vx-pch3 It looks like before this PR, Headscale keyed on the username alone, so has a similar vulnerability (and IMHO, there should be an advisory about it). It also looks like the vulnerability is still there, even when an account is migrated to using Lines 444 to 456 in 9515040
There should be another check here to ensure the returned Because otherwise, a new malicious user (whose PS: Yes, I'm aware I'm reporting a security bug publicly. However, the bug is already in the public domain because of this PR... and it is not actually fixed. 😉 |
* Look up users by email, not `preferred_username` (though, this field didn't exist before juanfont#2020) * Check OIDC claim for `email_verified`, and reject it if false or unset. This is can be disabled with `oidc.allow_unverified_email = true`. * Don't create a new user if one with the same `email` exists and has an OIDC identifier set.
thoughts
Copy of Changelog:
dns.use_username_in_magic_dns
configuration option #2020strip_email_domain
has been removed, domain is always part of the username for OIDC.sub
claim in the ID token instead of username, allowing the username, name and email to be updated.Related issues:
Closes #1990
Closes #1980
Closes #1981
Closes #1997
Closes #1594
Closes #938
These are closed as we will not support custom features outside of the OIDC standard.
Closes #1858
Closes #1934
Summary by CodeRabbit
Summary by CodeRabbit
New Features
strip_email_domain
and a shift to using thesub
claim for user identification.Bug Fixes
Refactor
AuthProvider
interface.Documentation
CHANGELOG.md
to reflect changes in configuration and user identification mechanisms.