Skip to content
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

Support multiple LDAP servers in a auth source #31649

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

abhishek818
Copy link

@abhishek818 abhishek818 commented Jul 17, 2024

support multiple LDAP servers and add dialing timeout, also rename cli flag 'host' to 'host-list'.

closes #6898
/claim #6898

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 17, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 17, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Jul 17, 2024
@abhishek818 abhishek818 force-pushed the support_multiple_ldap_servers branch from 75ee729 to 404171f Compare July 18, 2024 07:51
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 18, 2024
@abhishek818 abhishek818 marked this pull request as ready for review July 18, 2024 07:54
@abhishek818
Copy link
Author

@techknowlogick @silverwind pls review.

@abhishek818 abhishek818 force-pushed the support_multiple_ldap_servers branch from d5375e9 to 8574aa6 Compare July 18, 2024 09:18
@silverwind
Copy link
Member

What happens if host 1 is down and host 2 is up? Will it wait for the 10 seconds timeout on host 1 until it contacts host 2?

@6543 6543 changed the title Support multiple LDAP servers in a auth source (#6898) Support multiple LDAP servers in a auth source Jul 22, 2024
@abhishek818
Copy link
Author

abhishek818 commented Jul 22, 2024

What happens if host 1 is down and host 2 is up? Will it wait for the 10 seconds timeout on host 1 until it contacts host 2?

yes, unfortunately. I have searched, there seems to be no way around it.
But i guess it helps with providing ample timeout time for gracefully exit of connection. (for scenarios where say, 2 hosts are configured and both are down, The user again tries with the same 2 hosts within a small time frame. All dangling connections should have ended by then i.e. during the timeout duration)

also, 60 secs is by default, we are going with much lesser value.

@silverwind
Copy link
Member

silverwind commented Jul 23, 2024

A common way to solve it would be to "race" the TCP connections, e.g. connect both at once and take first connection that connects (e.g. completes the handshake) and drop the others.

Given that this happens on TCP level, there's virtually zero impact on the LDAP servers as the connection attempt never reaches the application layer.

Signed-off-by: abhishek818 <abhishekguptaatweb17@gmail.com>
)

Signed-off-by: abhishek818 <abhishekguptaatweb17@gmail.com>
Signed-off-by: abhishek kumar gupta <abhishekguptaatweb17@gmail.com>
Signed-off-by: abhishek818 <abhishekguptaatweb17@gmail.com>
Signed-off-by: abhishek818 <abhishekguptaatweb17@gmail.com>
@abhishek818 abhishek818 force-pushed the support_multiple_ldap_servers branch from 5bdec11 to f2c4cae Compare July 24, 2024 09:54
@abhishek818
Copy link
Author

done, up for review.

@abhishek818
Copy link
Author

@silverwind @techknowlogick bumping this for a review..

@abhishek818
Copy link
Author

@silverwind could you please review?

@lunny
Copy link
Member

lunny commented Aug 14, 2024

Looks like the tests failure is related.

rename host to hostlist in html template

Signed-off-by: abhishek818 <abhishekguptaatweb17@gmail.com>
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Aug 20, 2024
@abhishek818
Copy link
Author

Looks like the tests failure is related.

@lunny @silverwind Up for review, fixed the integration test failures, all tests are green now.

@abhishek818
Copy link
Author

@lunny hey can you pls review ?

@lunny
Copy link
Member

lunny commented Sep 2, 2024

@lunny hey can you pls review ?

Sorry for the late. I will review this one next week.

results := make(chan result, len(hostList))

for _, host := range hostList {
go func(host string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use a standalone function rather than a closure function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, refactored.

Signed-off-by: abhishek818 <abhishekguptaatweb17@gmail.com>
@abhishek818
Copy link
Author

addressed review comment, up for review.

@@ -25,7 +25,7 @@ import (
// Source Basic LDAP authentication service
type Source struct {
Name string // canonical name (ie. corporate.ad)
Host string // LDAP host
HostList string // list containing LDAP host(s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all these hosts should have the same port?

Copy link
Author

@abhishek818 abhishek818 Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny damnn, thats a big miss on my part.

Can you help with if we should keep the config for each ldap host as below?

// HostConfig represents the configuration for a single LDAP host
type HostConfig struct {
	Host              string            // Hostname or IP address
	Port              uint16            // Port number
	SecurityProtocol  SecurityProtocol  // Security protocol (LDAP, LDAPS, StartTLS)
	SkipVerify        bool              // Whether to skip TLS certificate verification
	BindDN            string            // Bind DN
	BindPassword      string            // Bind password
	UserBase          string            // User search base
	GroupBase         string            // Group search base
	SearchPageSize    uint32            // Search page size
	Filter            string            // User search filter
	AdminFilter       string            // Admin filter
	RestrictedFilter  string            // Restricted user filter
}

// Source represents the overall LDAP service configuration
type Source struct {
	Name               string        // Canonical name (e.g., corporate.ad)
	Hosts              []HostConfig  // List of host configurations    <-----------------------------
	AttributeUsername  string        // Username attribute
	AttributeName      string        // First name attribute
	AttributeSurname   string        // Surname attribute
	AttributeMail      string        // Email attribute
	AttributeSSHPublicKey string     // SSH Public Key attribute
	AttributeAvatar    string        // Avatar attribute
	AttributesInBind   bool          // Fetch attributes in bind context
	GroupMemberUID     string        // Group attribute containing array of UserUID
	GroupTeamMap       string        // Map LDAP groups to teams
	GroupTeamMapRemoval bool         // Remove user from teams not in corresponding LDAP group
	UserUID            string        // User attribute listed in group
	SkipLocalTwoFA     bool          // Skip 2FA for this source
	authSource         *auth.Source  // Reference to the authSource
}

Further should we go the same approach for each config input value as a list of string separated commas for both cli and web (same as host address currently) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 Bounty claim lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/docs modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple LDAP servers in a auth source
4 participants