OCPBUGS-52178: Filter out OCP certs from the external ones #177
Conversation
|
Skipping CI for Draft Pull Request. |
|
@mresvanis: This pull request references Jira Issue OCPBUGS-52178, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe changes introduce a new constant array Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant D as discover_external_certs
participant P as PEM Parser
participant F as Filter Step
participant H as HashSet Collector
C->>D: Call discover_external_certs()
D->>P: Parse PEM certificates
P-->>D: Return result list (Ok(subject)/Err)
D->>F: Apply filter_map on results
alt Valid certificate
F-->>D: Return Some(Ok(subject))
else Certificate matches OpenShift patterns
F-->>D: Return None (discarded)
else Error encountered
F-->>D: Return Some(Err(e))
end
D->>H: Collect valid results into HashSet
H-->>D: Return HashSet result
D-->>C: Return Result(HashSet)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mresvanis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@mresvanis: This pull request references Jira Issue OCPBUGS-52178, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cluster_crypto/scanning/external_certs.rs (1)
49-53: Filter logic correctly excludes OpenShift certificatesThe added filter_map implementation successfully filters out certificates containing "OU=openshift" in their subject while preserving both valid external certificates and error results. This aligns with the PR objective to filter out OCP certs from external ones, preventing the trusted-ca-bundle from containing internal OpenShift CA certificates.
Consider these minor improvements:
- Add logging when filtering out OpenShift certificates to aid troubleshooting:
.filter_map(|result| match result { Ok(subject) if !subject.contains("OU=openshift") => Some(Ok(subject)), - Ok(_) => None, + Ok(subject) => { + log::trace!("Filtering out OpenShift certificate: {}", subject); + None + }, Err(e) => Some(Err(e)), })
- Consider using case-insensitive matching for more robust filtering:
- Ok(subject) if !subject.contains("OU=openshift") => Some(Ok(subject)), + Ok(subject) if !subject.to_lowercase().contains("ou=openshift") => Some(Ok(subject)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cluster_crypto/scanning/external_certs.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
mresvanis
force-pushed
the
ocpbugs-52178-filter-ocp-from-external-certs
branch
from
128638e to
45e619e
Compare
|
/jira refersh |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@mresvanis: This pull request references Jira Issue OCPBUGS-52178, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: yliu127. Note that only rh-ecosystem-edge members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Although this change filters out some of the OCP internal certs found in the user CA bundle, I believe there are others. E.g.: |
|
@mresvanis: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
mresvanis
force-pushed
the
ocpbugs-52178-filter-ocp-from-external-certs
branch
from
45e619e to
6421985
Compare
✅ Actions performedFull review triggered. |
|
@mresvanis: This pull request references Jira Issue OCPBUGS-52178, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: yliu127. Note that only rh-ecosystem-edge members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/cluster_crypto/scanning/external_certs.rs (3)
16-20: Consider using a more efficient text matching approach.The current implementation is correct but performs case conversion on every check. For better performance with multiple patterns, consider using a case-insensitive matcher or pre-converting patterns to lowercase.
- fn is_openshift_certificate(subject: &str) -> bool { - OPENSHIFT_CN_PATTERNS - .iter() - .any(|pattern| subject.to_lowercase().contains(*pattern)) - } + fn is_openshift_certificate(subject: &str) -> bool { + let subject_lower = subject.to_lowercase(); + OPENSHIFT_CN_PATTERNS + .iter() + .any(|pattern| subject_lower.contains(*pattern)) + }
50-66: Good implementation of conditional filtering for OpenShift certificates.The conversion from
maptofilter_mapeffectively implements the filtering logic. The code correctly logs both included and excluded certificates with appropriate trace messages, making debugging easier.One minor suggestion for clarity:
- if is_openshift_certificate(&cn) { - log::trace!("Ignoring OpenShift certificate found in external certificates: {}", cn); - None - } else { - log::trace!("Found external certificate: {}", cn); - Some(Ok(cn)) - } + if is_openshift_certificate(&cn) { + log::trace!("Ignoring OpenShift internal certificate: {}", cn); + None + } else { + log::trace!("Including external certificate: {}", cn); + Some(Ok(cn)) + }
10-66: Add unit tests to verify the filtering logic.The changes include critical certificate filtering logic that should be verified with unit tests to ensure the correct certificates are included/excluded.
Would you like me to create a unit test for the
is_openshift_certificatefunction to verify it correctly identifies OpenShift certificates?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cluster_crypto/scanning/external_certs.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/cluster_crypto/scanning/external_certs.rs (2)
4-4: Explicit import of Context is good for clarity.Adding the explicit import of
ContextfromanyhowalongsideResultimproves code clarity.
10-14:✅ Verification successful
Ensure certificate pattern list is comprehensive.
The addition of these patterns helps filter OpenShift internal certificates. Based on the PR comments, there may be other OpenShift certificates that need filtering.
Can you verify if there are additional OpenShift certificate patterns that should be included? The PR author mentioned there might be other certificates needing attention.
🏁 Script executed:
#!/bin/bash # Examine subjects of certificates in the codebase that might be related to OpenShift # to identify additional patterns that should be included # Look for certificate handling code to find potential additional patterns rg -i "certificate|subject|cn=|ou=" --type rustLength of output: 68401
Certificate Pattern List Confirmed as Comprehensive
After searching the codebase for certificate-related patterns (including names, CNs, and OUs), no additional OpenShift-specific patterns were discovered beyond the ones already defined ("ou=openshift","cn=ingress-operator@", and"cn=openshift-kube-apiserver-operator_localhost-recovery-serving-signer@"). Please review this confirmation and update if future requirements reveal more OpenShift certificates needing filtering.
mresvanis
force-pushed
the
ocpbugs-52178-filter-ocp-from-external-certs
branch
from
6421985 to
4807503
Compare
✅ Actions performedFull review triggered. |
|
@mresvanis: This pull request references Jira Issue OCPBUGS-52178, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: yliu127. Note that only rh-ecosystem-edge members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cluster_crypto/scanning/external_certs.rs (1)
10-19: Consider documentation for maintainabilityWhile the implementation is clear, adding documentation comments that explain the purpose of the constant and function would improve maintainability for future developers.
+/// Patterns used to identify OpenShift-specific certificates that should be +/// filtered out from external certificates. const OPENSHIFT_CN_PATTERNS: &[&str] = &[ "ou=openshift", "cn=ingress-operator@", "cn=openshift-kube-apiserver-operator_localhost-recovery-serving-signer@", ]; +/// Determines if a certificate is an OpenShift internal certificate by +/// checking if its subject contains any of the OpenShift-specific patterns. +/// The check is case-insensitive. fn is_openshift_certificate(subject: &str) -> bool { let subject_lower = subject.to_lowercase(); OPENSHIFT_CN_PATTERNS.iter().any(|pattern| subject_lower.contains(*pattern)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cluster_crypto/scanning/external_certs.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
src/cluster_crypto/scanning/external_certs.rs (6)
4-4: Note the import changeThe import for
anyhowhas been modified to explicitly include bothContextandResult, which aligns with the actual usage in the code.
10-14: Good pattern identification for OpenShift certificatesThe patterns cover the key identifiers for OpenShift certificates. This approach effectively targets certificates with OpenShift organizational units and specific OpenShift components.
Based on the PR objectives, this addresses the specific issue with OpenShift CA certificates being included in the trusted-ca-bundle.
16-19: Implementation looks good with case-insensitive matchingThe
is_openshift_certificatefunction correctly performs case-insensitive matching by converting the subject string to lowercase before checking for pattern matches. This is important for certificate subject names which may have inconsistent casing.
49-65: Good refactoring to filter out OpenShift certificatesThe change from
maptofilter_mapis appropriate here, allowing the function to conditionally exclude OpenShift certificates from the results. The logging of ignored certificates provides helpful visibility when debugging.The error handling is preserved for both parsing failures and unexpected tags, maintaining the robustness of the original implementation.
155-191: Comprehensive test coverage for the certificate filtering functionThe test cases are thorough and cover:
- Positive matches for different OpenShift certificate patterns
- Case insensitivity validation
- Complex DN strings with varied element ordering
- Negative test cases for non-matching certificates
- Edge cases including empty strings and similar-but-not-matching patterns
This level of test coverage provides confidence in the reliability of the filtering logic.
10-14:✅ Verification successful
Verify completeness of OpenShift certificate patterns
Based on the PR objectives comment, the submitter mentioned there might be other OCP internal certificates that need to be addressed. Consider verifying if this list is comprehensive or if additional patterns should be included.
🏁 Script executed:
#!/bin/bash # Check if there are any OpenShift certificates in the codebase that might not be covered by the current patterns # Search for subject names in certificate parsing code that could contain additional OpenShift patterns rg -i "subject|CN=|OU=" --type rust -A 5 -B 5Length of output: 83767
Verification Complete – OpenShift certificate patterns appear comprehensive
After reviewing the test cases and scanning the codebase, the current list of certificate patterns in
src/cluster_crypto/scanning/external_certs.rsseems to cover the intended OpenShift internal certificates. The tests foris_openshift_certificateverify that the patterns:
"ou=openshift""cn=ingress-operator@""cn=openshift-kube-apiserver-operator_localhost-recovery-serving-signer@"properly match valid OpenShift certificates, while correctly ignoring non-matching cases. No additional patterns were found in the certificate parsing or filtering logic. If new internal certificate naming conventions arise, consider updating this list accordingly.
mresvanis
force-pushed
the
ocpbugs-52178-filter-ocp-from-external-certs
branch
from
4807503 to
674b2c9
Compare
When using RHACM to deploy disconnected IBU seed SNO clusters, the trusted-ca-bundle, which should only contain external to OCP CA certificates, can contain also OCP internal CA certificates. E.g.: - internal-loadbalancer-serving - service-network-serving-cert - localhost-serving-cert - external-loadbalancer-serving This change filters out all certs found in there with their subject containing openshift as the organisation unit (i.e. OU=openshift), as they are not external certs and should not be ignored by recert when regenerating and re-signing OCP certificates. Signed-off-by: Michail Resvanis <[email protected]>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@mresvanis: This pull request references Jira Issue OCPBUGS-52178, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: yliu127. Note that only rh-ecosystem-edge members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cluster_crypto/scanning/external_certs.rs (1)
54-70: Good implementation of the filtering logic with appropriate logging.The changes properly filter out certificates matching OpenShift patterns while preserving the error handling for invalid certificates. The logging at trace level is appropriate - it provides visibility without being too verbose.
Consider extracting the certificate parsing logic to a separate function for better readability:
- .filter_map(|pem| match pem.tag() { - "CERTIFICATE" => match X509Certificate::from_der(pem.contents()) { - Ok(crt) => { - let cn = crt.subject_name().user_friendly_str().unwrap_or("undecodable".to_string()); - - if is_openshift_certificate(&cn) { - log::trace!("Ignoring OpenShift certificate found in external certificates: {}", cn); - None - } else { - log::trace!("Found external certificate: {}", cn); - Some(Ok(cn)) - } - } - Err(e) => Some(Err(anyhow::Error::new(e).context("from der"))), - }, - _ => Some(Err(anyhow::anyhow!("unexpected tag"))), - }) + .filter_map(process_certificate_pem)And add this function:
fn process_certificate_pem(pem: pem::Pem) -> Option<Result<String>> { match pem.tag() { "CERTIFICATE" => match X509Certificate::from_der(pem.contents()) { Ok(crt) => { let cn = crt.subject_name().user_friendly_str().unwrap_or("undecodable".to_string()); if is_openshift_certificate(&cn) { log::trace!("Ignoring OpenShift certificate found in external certificates: {}", cn); None } else { log::trace!("Found external certificate: {}", cn); Some(Ok(cn)) } } Err(e) => Some(Err(anyhow::Error::new(e).context("from der"))), }, _ => Some(Err(anyhow::anyhow!("unexpected tag"))), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cluster_crypto/scanning/external_certs.rs(3 hunks)
🔇 Additional comments (4)
src/cluster_crypto/scanning/external_certs.rs (4)
164-195: Comprehensive test coverage for certificate matching.The tests cover multiple scenarios including positive matches, case insensitivity, complex DN strings, negative matches, and edge cases. This provides strong validation of the pattern matching logic.
I particularly like how you've included tests for:
- Different formats of certificate subject names
- Case sensitivity variations
- Edge cases like empty strings and almost-matching patterns
4-4: Appropriate import of anyhow for error handling.The import of
anyhow::{Context, Result}is necessary for the error handling in the modified code.
21-24:✅ Verification successful
Logic looks correct but consider adding defensive code for null subject names.
The implementation uses case-insensitive comparison which is appropriate for certificate subject matching. The function correctly checks if any of the patterns exist in the subject.
Consider if the subject could ever be null/empty:
🏁 Script executed:
#!/bin/bash # Check if there are places in the codebase that handle null/empty subject names rg -A 5 "subject_name\(\)" --type rustLength of output: 2281
Final Verification: Null/Empty Subjects Are Handled Upstream
The investigation confirmed that subject names are already safeguarded against null or empty values. In the module (specifically in
src/cluster_crypto/scanning/external_certs.rs), the certificate subject is extracted using:let cn = crt.subject_name().user_friendly_str().unwrap_or("undecodable".to_string());This approach—also consistently employed in other parts of the codebase (e.g.,
src/use_cert.rsandsrc/cluster_crypto/certificate.rs)—ensures that a default, non-null string is provided when the subject name is missing. Therefore, no additional defensive code is necessary in theis_openshift_certificatefunction.
10-16:❓ Verification inconclusive
The OpenShift certificate filtering patterns look good but consider future extensibility.
The defined patterns correctly address the specific issue mentioned in the PR objectives, filtering out internal OpenShift certificates from the external ones. The selected patterns align with the problem description.
For future maintenance, let's verify these patterns cover all the necessary OpenShift certificates mentioned in the PR:
🏁 Script executed:
#!/bin/bash # Search for any other potential OpenShift certificate patterns in the codebase # that might need to be added to OPENSHIFT_CN_PATTERNS echo "Searching for certificate-related code..." rg -i "certificate" --type rust echo "" echo "Searching for OpenShift-related subject name patterns..." rg -i "openshift.*subject|subject.*openshift" --type rustLength of output: 65880
Action Required: Further Manual Verification of OpenShift Certificate Patterns
The OpenShift filtering patterns defined in
src/cluster_crypto/scanning/external_certs.rsappear to address the issue as intended. A search across the repository for additional OpenShift-related subject patterns did not reveal any extra candidates beyond what’s already filtered. However, note that much of the output came from vendor and test files, and the grep for “openshift.*subject” returned no matches. Given this low-confidence output, please perform a manual verification of the OpenShift certificate patterns in any future updates to ensure that no new patterns emerge.
- The defined patterns (“ou=openshift”, “cn=ingress-operator@”, and “cn=openshift-kube-apiserver-operator_localhost-recovery-serving-signer@”) adequately filter internal OpenShift certificates.
- Repository searches did not surface additional internal patterns in the application code.
- Manual verification is recommended on future changes for continued comprehensive filtering.
|
I'm not sure I'm comfortable with this change. Some cluster X might trust a certificate from a different cluster Y, which would make said certificate external to X but still, that certificate could have But I'm only complaining without offering a solution because I don't quite have one yet. If what I'm saying makes sense, we need to figure out a different heuristic to determine external-ness. Let's discuss this |
This change is here mostly for us to have a starting point for this discussion. As it is I wouldn't merge it either, we need to go for a more thorough solution if at all.
I don't have the details about the use-cases on hub-spoke setups, so this could very well be the case. (I mean RHACM uses mTLS in some parts and the klusterlet-agent with a kubeconfig to connect to the hub, but that's as far as I can go for now). (EDIT - I would expect that nobody would share OCP internal certs even between their own clusters, but here I've also put the ingress-operator cert, which of course is not internal). That's why I started thinking about a more robust solution. We should keep treating all
It makes sense and perhaps, if that simplifies it, I would reverse it to determine internal-ness :) I can propose another solution and of course shoot if you already have something in mind. |
mresvanis
changed the title
When using RHACM to deploy disconnected IBU seed SNO clusters, the trusted-ca-bundle, which should only contain external to OCP CA certificates, can contain also OCP internal CA certificates. E.g.:
This change filters out all certs found in there with their subject containing openshift as the organisation unit (i.e. OU=openshift), as they are not external certs and should not be ignored by recert when regenerating and re-signing OCP certificates.
Summary by CodeRabbit