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

Add SSL validation and connection test to LDAP configuration methods #746

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

Conversation

adambarreiro
Copy link
Collaborator

@adambarreiro adambarreiro commented Jan 31, 2025

While PR #732 introduced basic support for LDAP in VMware Cloud Foundation Automation, the implemented methods had a few issues:

  • If the LDAP used SSL, the certificate was not trusted, hence the LDAP was configured but useless as it would always fail.
  • If LDAP host or port is wrong, the LDAP would be configured but useless as it would always fail.

This PR enhances all implemented methods:

  • They check connection against provided hostname and port when requesting setting LDAP. When the LDAP settings are being deleted, there's no check, obviously.
  • When SSL is requested in the LDAP settings input parameter, and if trustCertificate flag is true, the methods now automatically trust the certificate if needed. Otherwise (trustCertificate=false), the methods will fail.
  • If the SSL certificate is trusted, but configuring the LDAP fails, the certificate is automatically deleted.

Additionally, instead of using dummy vCenter as LDAP server, the testing configuration allows to provide a customised LDAP server:

tm:
  ...
  ldap:
    host: my.ldap.server.net
    port: 636
    isSsl: true
    username: admin
    password: StrongPassword
    baseDistinguishedName: OU=demo,DC=foo,DC=bar,DC=test,DC=net
    type: ACTIVE_DIRECTORY

Last, it also adds support of Trusted Certificates for Tenants (TmOrg), which is required for configuring LDAP with SSL on tenants.

abarreiro added 2 commits January 31, 2025 15:56
#
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro self-assigned this Jan 31, 2025
abarreiro added 6 commits January 31, 2025 16:51
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
Fix
Signed-off-by: abarreiro <[email protected]>
Fix
Signed-off-by: abarreiro <[email protected]>
}

// If SSL is configured and retrieved settings are empty, or the hostname changed, we need to trust the certificate
needsCheckCert := settings.IsSsl && (existing.HostName == "" || existing.HostName != settings.HostName)
Copy link
Collaborator Author

@adambarreiro adambarreiro Jan 31, 2025

Choose a reason for hiding this comment

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

As far as I saw, if you change the host in UI, it doesn't re-trigger the certificate trusting popup. I think it should? So not sure if this should be so "smart" or simplify, but not re-trusting cert for changed host would make LDAP useless again.

abarreiro added 6 commits January 31, 2025 17:19
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro marked this pull request as ready for review February 18, 2025 11:03
Signed-off-by: abarreiro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant