Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughRefactors LDAP client to add DNS SRV discovery, authenticated bind support, and configurable CA certificate loading/enforcement (TLS ≥1.2). Removes TLS skip-verify, changes group base DN derivation and group deduplication, and updates runtime init and Kubernetes manifests to supply bind and CA configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Backend App
participant DNS as DNS Resolver
participant LDAP as LDAP Server
App->>App: NewClient(url, srvDomain, baseDN, groupBaseDN, bindDN, bindPassword, caCertPath)
alt srvDomain provided
App->>DNS: SRV lookup "_ldap._tcp.<srvDomain>"
DNS-->>App: list of SRV targets
loop try each SRV target
App->>LDAP: Dial ldaps://target:636 (TLS using caCertPath, TLS1.2+)
alt dial success
LDAP-->>App: TCP+TLS established
alt bindDN provided
App->>LDAP: Bind(bindDN, bindPassword)
LDAP-->>App: Bind success / error
end
App-->>App: return connected client or bind error
else dial failed
LDAP--xApp: dial error
end
end
else no SRV or SRV failed
App->>LDAP: Dial configured URL (ldaps://...)
LDAP-->>App: TCP+TLS established or error
alt bindDN provided
App->>LDAP: Bind(bindDN, bindPassword)
LDAP-->>App: Bind result
end
App-->>App: return client or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
Contributor
Author
|
@ktdreyer @Gkrumbach07 When this gets deployed for the first time, someone needs to manually upload the cert and LDAP creds into the relevant ConfigMap/Secret. Does that get done manually, or do we have any deployment scripts/playbooks I need to modify somewhere? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/ldap/client.go`:
- Around line 86-97: The code reads a CA file into caCertPath and calls
rootCAs.AppendCertsFromPEM(...) but doesn't check the boolean result; if the PEM
contains no valid certs the TLS trust root will be incomplete. Change the block
that sets tlsConfig.RootCAs to validate AppendCertsFromPEM returned true and
return an error if it returned false (include caCertPath in the error), ensuring
this check occurs after os.ReadFile and before assigning tlsConfig.RootCAs;
reference the caCertPath, rootCAs, AppendCertsFromPEM, and tlsConfig.RootCAs
symbols when making the change.
In `@components/backend/main.go`:
- Around line 138-153: The LDAP setup currently fatals on any NewClient error
and logs "initialized" before a real bind; change ldap.NewClient error handling
to treat failures as non-fatal (set handlers.LDAPClient = nil and log a warning)
unless an explicit required flag is set, and after creating ldapClient call its
real connectivity/auth method (e.g., ldapClient.Bind or ldapClient.Ping —
whichever exists) and only assign handlers.LDAPClient and log "LDAP client
initialized" after that succeeds; if the bind/check fails, disable the LDAP
client, log the detailed error, and continue startup instead of calling
log.Fatalf. Ensure you reference ldap.NewClient, handlers.LDAPClient and the
client Bind/Ping method in the fix.
In `@components/manifests/base/core/backend-deployment.yaml`:
- Around line 297-300: The ldap-ca-cert ConfigMap volume in the Deployment's Pod
spec is not marked optional, which will cause pods to fail when LDAP isn't
configured; update the volume definition for the volume named "ldap-ca-cert"
(the entry under volumes where configMap: name: ldap-ca-cert) to include
optional: true so the mount is skipped when the ConfigMap is absent.
In `@components/manifests/overlays/production/ldap-credentials.yaml`:
- Around line 1-9: The production overlay currently commits a tracked Secret
named "ldap-credentials" containing a placeholder LDAP_BIND_PASSWORD; remove the
hardcoded stringData entry and stop versioning the secret. Instead, remove
LDAP_BIND_PASSWORD from this manifest (or delete the Secret resource entirely)
and wire the deployment to obtain the real password from your secret manager/CI
secret-injection mechanism (e.g., ExternalSecrets/SecretProviderClass/CI vault
injection) so the actual value is never stored in git; update any references to
the Secret name "ldap-credentials" in manifests to match the injected/managed
secret name or document the injection step in your deployment pipeline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9318714-a999-4331-91a6-7ea2f97082be
📒 Files selected for processing (12)
components/backend/ldap/client.gocomponents/backend/ldap/client_test.gocomponents/backend/main.gocomponents/manifests/base/core/backend-deployment.yamlcomponents/manifests/overlays/kind/kustomization.yamlcomponents/manifests/overlays/kind/ldap-config.yamlcomponents/manifests/overlays/kind/ldap-credentials.yamlcomponents/manifests/overlays/local-dev/kustomization.yamlcomponents/manifests/overlays/local-dev/ldap-config.yamlcomponents/manifests/overlays/local-dev/ldap-credentials.yamlcomponents/manifests/overlays/production/ldap-config.yamlcomponents/manifests/overlays/production/ldap-credentials.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/manifests/overlays/production/ldap-credentials.yaml (1)
1-9:⚠️ Potential issue | 🟠 MajorDon't ship a production bind-password placeholder.
Applying this overlay still creates
ldap-credentialswith a bogus value, so production LDAP is either broken until the cluster is patched manually or the real password gets committed here. Provision this Secret via your secret manager / deployment automation instead of a versioned manifest.As per coding guidelines, focus on major issues impacting performance, readability, maintainability and security.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/manifests/overlays/production/ldap-credentials.yaml` around lines 1 - 9, The manifest currently commits a production Secret named "ldap-credentials" with a real-looking LDAP_BIND_PASSWORD value; remove the hardcoded LDAP_BIND_PASSWORD entry from the overlay and stop checking this Secret into version control — instead wire the deployment to fetch the secret at deploy-time via your secret manager or tooling (e.g., ExternalSecret/SealedSecret, k8s secretGenerator excluded from git, or your CI/CD vault integration) and update any references in deployment manifests to use that external secret name; ensure no plaintext password remains in the "ldap-credentials" resource or repo and add documentation/comments in the overlay explaining that the Secret is provided by the secret manager at deploy time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/manifests/overlays/kind/ldap-config.yaml`:
- Around line 8-12: The LDAP_CA_CERT_PATH variable in the overlay should not
point at a CA file that this overlay doesn't provision; either remove/unset
LDAP_CA_CERT_PATH from the manifest (so startup won't attempt to load a
non-existent file) or add provisioning for the CA material (create a
Secret/ConfigMap containing the CA and update the overlay to mount it at
/etc/pki/custom-ca/rh-it-root-ca.pem and reference that mount); update the
manifest entry LDAP_CA_CERT_PATH accordingly and ensure the
deployment/volume/mount that supplies the file is present so LDAP client
initialization can reliably find the file.
In `@components/manifests/overlays/local-dev/ldap-config.yaml`:
- Around line 8-13: The LDAP_CA_CERT_PATH environment variable in the LDAP
config overlay (LDAP_CA_CERT_PATH in
manifests/overlays/local-dev/ldap-config.yaml) points to a CA file that this
overlay does not provision; remove or leave LDAP_CA_CERT_PATH unset in this
overlay (or change to an explicitly optional/empty value) so startup does not
try to load a non-existent CA file, or alternatively add provisioning of the CA
artifact to the same overlay/deployment so the referenced path is guaranteed to
exist before the app reads it.
---
Duplicate comments:
In `@components/manifests/overlays/production/ldap-credentials.yaml`:
- Around line 1-9: The manifest currently commits a production Secret named
"ldap-credentials" with a real-looking LDAP_BIND_PASSWORD value; remove the
hardcoded LDAP_BIND_PASSWORD entry from the overlay and stop checking this
Secret into version control — instead wire the deployment to fetch the secret at
deploy-time via your secret manager or tooling (e.g.,
ExternalSecret/SealedSecret, k8s secretGenerator excluded from git, or your
CI/CD vault integration) and update any references in deployment manifests to
use that external secret name; ensure no plaintext password remains in the
"ldap-credentials" resource or repo and add documentation/comments in the
overlay explaining that the Secret is provided by the secret manager at deploy
time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcbd04e0-082b-47ea-87e8-1eca374fa85a
📒 Files selected for processing (12)
components/backend/ldap/client.gocomponents/backend/ldap/client_test.gocomponents/backend/main.gocomponents/manifests/base/core/backend-deployment.yamlcomponents/manifests/overlays/kind/kustomization.yamlcomponents/manifests/overlays/kind/ldap-config.yamlcomponents/manifests/overlays/kind/ldap-credentials.yamlcomponents/manifests/overlays/local-dev/kustomization.yamlcomponents/manifests/overlays/local-dev/ldap-config.yamlcomponents/manifests/overlays/local-dev/ldap-credentials.yamlcomponents/manifests/overlays/production/ldap-config.yamlcomponents/manifests/overlays/production/ldap-credentials.yaml
Contributor
Manually today until we have vault (https://redhat.atlassian.net/browse/RHOAIENG-47031) In the meantime you can store the secrets in the team Bitwarden collection |
Migrate from ldap.corp.redhat.com (anonymous binds, deprecated) to ipa.corp.redhat.com (authenticated binds via service account). Go client: - Replace skipTLSVerify with bindDN, bindPassword, caCertPath params - connect() loads custom CA cert (appended to system pool) and performs authenticated Bind() - Default groupBaseDN derivation updated from ou=managedGroups to cn=groups (IPA pattern) - Deduplicate group memberships Manifests: - Update ldap-config.yaml (all overlays) with IPA URL and base DNs, add LDAP_BIND_DN, remove LDAP_SKIP_TLS_VERIFY - Add ldap-credentials.yaml Secret (all overlays) for LDAP_BIND_PASSWORD - Add LDAP_CA_CERT_PATH env var and ldap-ca-cert volume mount to backend deployment (optional, for TLS verification) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
Contributor
Review Queue Status
Action needed: Address CodeRabbit Major issues on CA cert path and credentials management
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrate from ldap.corp.redhat.com (anonymous binds, deprecated) to ipa.corp.redhat.com (authenticated binds via service account).
Go client:
Manifests: