-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
base: main
Are you sure you want to change the base?
Support multiple LDAP servers in a auth source #31649
Conversation
75ee729
to
404171f
Compare
@techknowlogick @silverwind pls review. |
d5375e9
to
8574aa6
Compare
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. also, 60 secs is by default, we are going with much lesser value. |
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 <[email protected]>
) Signed-off-by: abhishek818 <[email protected]>
Signed-off-by: abhishek kumar gupta <[email protected]>
Signed-off-by: abhishek818 <[email protected]>
Signed-off-by: abhishek818 <[email protected]>
5bdec11
to
f2c4cae
Compare
done, up for review. |
@silverwind @techknowlogick bumping this for a review.. |
@silverwind could you please review? |
Looks like the tests failure is related. |
rename host to hostlist in html template Signed-off-by: abhishek818 <[email protected]>
@lunny @silverwind Up for review, fixed the integration test failures, all tests are green now. |
@lunny hey can you pls review ? |
Sorry for the late. I will review this one next week. |
Signed-off-by: abhishek818 <[email protected]>
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) |
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.
So all these hosts should have the same port?
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.
@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) ?
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.
Maybe right. But then the UI will become very complex.
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.
Agreed, any alternatives you have in mind?
The UI can have an additional input text box for entering no. of ldap servers first, then providing separate input boxes for each of all common params then say 'input port value for server 1' and so on..
support multiple LDAP servers and add dialing timeout, also rename cli flag 'host' to 'host-list'.
closes #6898
/claim #6898