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

fix(ldap): Support client certificate authentication without fallback to anonymous bind #4026

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crafa2
Copy link

@crafa2 crafa2 commented Mar 5, 2025

Overview

This PR fixes an issue where Dex incorrectly drops client certificates before performing an LDAP bind, leading to unintended anonymous authentication instead of certificate-based authentication.

What this PR does / why we need it

  • Ensures that if a client certificate is provided, Dex does not perform an anonymous bind that could override the certificate-based authentication.
  • Simplifies the binding logic to make it more readable and maintainable.
  • Addresses cases where bindDN and bindPW are empty while a certificate is provided.

Closes #<4025>.

Special notes for your reviewer

  • This change assumes that if a client certificate is provided, it should take precedence over an anonymous or credential-based bind.
  • The updated logic ensures that Dex behaves consistently when handling different authentication methods.
  • Let us know if additional tests are required to validate different LDAP configurations.

@crafa2 crafa2 force-pushed the fix-ldap-client-cert-auth branch from 74b32fd to 91502ea Compare March 5, 2025 10:03
@nabokihms
Copy link
Member

Is this a breaking change? We probably need a new test to cover this case.

@crafa2
Copy link
Author

crafa2 commented Mar 6, 2025

This change should not be a breaking change for existing configurations because it only modifies behavior when a client certificate is explicitly provided and no bindDN/bindPW is set.

  • If no certificate is provided, Dex continues to behave as before:

    • If bindDN and bindPW are set → Dex performs a credential-based bind.
    • If bindDN and bindPW are empty → Dex falls back to an anonymous bind.
  • If a certificate is provided:

    • If bindDN and bindPW are also set, Dex still performs the credential-based bind, keeping the previous behavior.
    • If bindDN and bindPW are empty, Dex now uses the certificate authentication instead of falling back to an anonymous bind (which was unintended behavior before).

So effectively, this fix just ensures that when a client certificate is used, it actually gets used for authentication instead of being dropped.

@crafa2 crafa2 force-pushed the fix-ldap-client-cert-auth branch from 91502ea to 47fb772 Compare March 6, 2025 10:48
… to anonymous bind

- If a **client certificate is provided**, Dex now **skips the anonymous bind** to ensure that authentication proceeds with the client certificate.
- If no credentials (bindDN/bindPW or client cert) are provided, Dex falls back to the expected anonymous bind.

- Ensures proper support for client certificate authentication in LDAP.
- Prevents unnecessary fallback to anonymous bind when a certificate is available.

Signed-off-by: crafa2 <[email protected]>
@crafa2 crafa2 force-pushed the fix-ldap-client-cert-auth branch from 47fb772 to 09dbf65 Compare March 6, 2025 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants