Update CA cert discovery#1694
Conversation
|
👋 Hi anjaliratnam-msft! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
👀 Investigating |
|
🤖 CI Triage Agent — I now have a complete and confident diagnosis. Here is the full report: Summary: Clang Format Check failed on PR #1694 ( Root cause: Commit - "/etc/ssl/certs/ca-certificates.crt", // Debian/Ubuntu/Gentoo
- "/etc/pki/tls/certs/ca-bundle.crt", // Fedora/RHEL 6
+ "/etc/ssl/certs/ca-certificates.crt", // Debian/Ubuntu/Gentoo
+ "/etc/pki/tls/certs/ca-bundle.crt", // Fedora/RHEL 6
"/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", // CentOS/RHEL 7
- "/etc/ssl/cert.pem" // Alpine Linux
+ "/etc/ssl/cert.pem" // Alpine LinuxThe three lines with shorter string literals have extra trailing spaces before their Implicated commit: File: Suggested fix: Remove the manual whitespace padding from the three inline comments so they each have exactly one space before for (const auto *path : {
"/etc/ssl/certs/ca-certificates.crt", // Debian/Ubuntu/Gentoo
"/etc/pki/tls/certs/ca-bundle.crt", // Fedora/RHEL 6
"/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", // CentOS/RHEL 7
"/etc/ssl/cert.pem" // Alpine Linux
}) {Alternatively, run Related: PR #1694 — Update CA cert discovery
|
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds include, reads AZURE_STORAGE_CONNECTION_STRING via nixl::config::getValueDefaulted, implements AZURE_CA_BUNDLE with fallback probe of /etc/ssl/certs/ca-certificates.crt, and declares CurlTransportOptions while applying CAInfo only when a bundle is found. ChangesCA Bundle Discovery and CURL Transport Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/azure_blob/azure_blob_client.cpp`:
- Around line 100-110: The CA probe block is failing clang-format and the
pointer style drifts from repo convention; refactor the initializer list and
pointer declarations (e.g., replace the "const auto *path" range-for with an
explicit const char* path and ensure pointer symbols are right-adjacent to the
type like const char*), avoid manually aligning the comment columns, and then
run clang-format on the modified block in
src/plugins/azure_blob/azure_blob_client.cpp so the initializer list and
surrounding code conform to the repo style (keep references to ca_info,
caBundle, fopen, and the path list intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac9ef77e-1322-41a4-8e18-c4dc64a34d8a
📒 Files selected for processing (1)
src/plugins/azure_blob/azure_blob_client.cpp
Signed-off-by: Anjali Ratnam <anjaliratnam@microsoft.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/azure_blob/azure_blob_client.cpp (1)
105-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCA bundle probe only checks a Debian-family path; extend default CA discovery for other distros
File:
src/plugins/azure_blob/azure_blob_client.cpp(around lines 105-113)for (const auto *path : { "/etc/ssl/certs/ca-certificates.crt", // Debian/Ubuntu/Gentoo }) { if (FILE *f = fopen(path, "r")) { fclose(f); ca_info = path; break; } }The fallback loop only probes
/etc/ssl/certs/ca-certificates.crt. On Fedora/RHEL/CentOS/Alpine this will leaveca_info == nullptr, socurlOptions.CAInfowon’t be set and curl falls back to its built-in CA bundle. Since the PR’s intent is cross-distro CA discovery, add the common distro paths to the initializer list (keeping exactly one space before each//so clang-format stays green).🐛 Proposed fix to restore cross-distro probing
for (const auto *path : { "/etc/ssl/certs/ca-certificates.crt", // Debian/Ubuntu/Gentoo + "/etc/pki/tls/certs/ca-bundle.crt", // Fedora/RHEL 6 + "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", // CentOS/RHEL 7 + "/etc/ssl/cert.pem", // Alpine Linux }) {If the single-path behavior is intentional, update the PR description/commit message to explicitly document that non-Debian systems require
ca_bundle/AZURE_CA_BUNDLEfor correct CA handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/azure_blob/azure_blob_client.cpp` around lines 105 - 113, The CA bundle probe only checks Debian's "/etc/ssl/certs/ca-certificates.crt" and leaves ca_info null on other distros; update the fallback loop in src/plugins/azure_blob/azure_blob_client.cpp (the block that sets ca_info and later influences curlOptions.CAInfo) to include additional common CA paths used by Fedora/RHEL/CentOS/Alpine such as "/etc/pki/tls/certs/ca-bundle.crt", "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", "/etc/ssl/cert.pem", and "/etc/ssl/ca-bundle.pem" (keeping exactly one space before each // comment to preserve clang-format), so the loop can find a system CA bundle across distros and set ca_info for curlOptions.CAInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/plugins/azure_blob/azure_blob_client.cpp`:
- Around line 105-113: The CA bundle probe only checks Debian's
"/etc/ssl/certs/ca-certificates.crt" and leaves ca_info null on other distros;
update the fallback loop in src/plugins/azure_blob/azure_blob_client.cpp (the
block that sets ca_info and later influences curlOptions.CAInfo) to include
additional common CA paths used by Fedora/RHEL/CentOS/Alpine such as
"/etc/pki/tls/certs/ca-bundle.crt",
"/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", "/etc/ssl/cert.pem", and
"/etc/ssl/ca-bundle.pem" (keeping exactly one space before each // comment to
preserve clang-format), so the loop can find a system CA bundle across distros
and set ca_info for curlOptions.CAInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06930aa7-fe41-4a51-b774-dd0f3e3e4239
📒 Files selected for processing (1)
src/plugins/azure_blob/azure_blob_client.cpp
kyleknap
left a comment
kyleknap
left a comment
There was a problem hiding this comment.
Looks good. I just had some smaller suggestions.
| for (const auto *path : { | ||
| "/etc/ssl/certs/ca-certificates.crt", // Debian/Ubuntu/Gentoo | ||
| }) { |
There was a problem hiding this comment.
For this, I'm thinking:
- Let's avoid doing a for loop and just look for this one specific location
- Add a comment saying why we have it there and hint that we could potentially remove it. I think something along the lines of the following would work
Libcurl currently looks for CA bundles based on the platform that is built (currently CentOS) and not the
platform that it is running on. This means it will not look for certs in the correct location on Ubuntu. This
is a workaround to make sure we check for Ubuntu certs before falling back to libcurl's default location.
In the future, we can remove this check if we find a way to build libcurl to search for certs in a more
cross-distro compatible way.
| for (const auto *path : { | ||
| "/etc/ssl/certs/ca-certificates.crt", // Debian/Ubuntu/Gentoo | ||
| }) { | ||
| if (FILE *f = fopen(path, "r")) { |
There was a problem hiding this comment.
Instead of trying to read the file, I'm thinking we just check whether the file exists (e.g., std::filesystem::exists). Mainly it will make it a bit simpler and we don't have to worry about managing file handles as in most cases if the file is there it is most likely going to be readable as it is the distros managing the certs at that location.
| @@ -93,9 +96,27 @@ azureBlobClient::azureBlobClient(nixl_b_params_t *custom_params, | |||
| options.Telemetry.ApplicationId = "azpartner-nixl/0.1.0"; | |||
|
|
|||
| std::string caBundle = ::getCaBundle(custom_params); | |||
There was a problem hiding this comment.
Instead of updating the if statement could we add this additional fallback logic to the getCaBundle to help consolidate all CA bundle related resolution logic to one place?
| const char *env_conn = std::getenv("AZURE_STORAGE_CONNECTION_STRING"); | ||
| if (env_conn && env_conn[0] != '\0') return std::string(env_conn); | ||
| if (env_conn && env_conn[0] != '\0') { | ||
| return std::string(env_conn); |
There was a problem hiding this comment.
Was there a particular reason we needed to update this line? If we do need to update it, it may make sense to update it to the pattern of the rest of these that use a nixl::config helper as I noticed this was the only parameter did not get updated when this file was updated and don't think it was intentional.
| }) { | ||
| if (FILE *f = fopen(path, "r")) { | ||
| fclose(f); | ||
| ca_info = path; |
There was a problem hiding this comment.
Also if we do detect the file's existence and decide to use it, we should add a logging statement using the NIXL_DEBUG macro so it is easier to tell if we resolved to this CA bundle based on this hardcoded path or that is what libcurl is defaulting to.
kyleknap
left a comment
kyleknap
left a comment
There was a problem hiding this comment.
It's looking good. Just a couple more suggestions.
|
|
||
| { | ||
| Azure::Core::Http::CurlTransportOptions curlOptions; | ||
| curlOptions.CAInfo = caBundle; | ||
| const char *ca_info = caBundle.empty() ? nullptr : caBundle.c_str(); | ||
|
|
||
| if (ca_info) { | ||
| curlOptions.CAInfo = ca_info; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is it possible to not update these lines at all? Mainly if we don't make changes here, we can keep it localized to getCaBundle().
| if (std::filesystem::exists("/etc/ssl/certs/ca-certificates.crt")) { | ||
| NIXL_DEBUG << "Using CA bundle: /etc/ssl/certs/ca-certificates.crt"; | ||
| return "/etc/ssl/certs/ca-certificates.crt"; | ||
| } |
There was a problem hiding this comment.
Can we set the location value once and reuse it and be a little more descriptive in the debug message to better indicate why we are using it? e.g.:
const std::string ubuntu_ca_bundle = "/etc/ssl/certs/ca-certificates.crt";
if (std::filesystem::exists(ubuntu_ca_bundle)) {
NIXL_DEBUG << "Using detected CA bundle at: " << ubuntu_ca_bundle;
return ubuntu_ca_bundle;
}
| options.Telemetry.ApplicationId = "azpartner-nixl/0.1.0"; | ||
|
|
||
| std::string caBundle = ::getCaBundle(custom_params); | ||
| Azure::Core::Http::CurlTransportOptions curlOptions; |
There was a problem hiding this comment.
Let's remove this line as well. It does not appear to be needed.
| // we find a way to build libcurl to search for certs in a more cross-distro compatible way. | ||
| const std::string ubuntu_ca_bundle = "/etc/ssl/certs/ca-certificates.crt"; | ||
| if (std::filesystem::exists(ubuntu_ca_bundle)) { | ||
| NIXL_DEBUG << "Using detected CA bundle at: " << ubuntu_ca_bundle; |
There was a problem hiding this comment.
Looking more into this, it might make sense to use NIXL_INFO macro instead? Mainly it seems to better fit the contributing guidelines as "configuration loaded" and when building from the container, which installs the wheels, I'm only seeing this log message if NIXL_INFO is used. @aranadive Thoughts?
3 participants
What?
This PR is addressing a concern with finding the CA certs by automatically checking a list of known CA bundle paths across common Linux distributions. Users are still able to override this by setting the
ca_bundlebackend parameter or setting theAZURE_CA_BUNDLEenvironment variable.Why?
This PR addresses a comment made in #1329
Summary by CodeRabbit