chore(common)!: replace native-certs with platform-verifier#2078
Conversation
ff9b666 to
4b2028f
Compare
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: bf25811 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2078 +/- ##
==========================================
- Coverage 73.50% 73.47% -0.03%
==========================================
Files 470 470
Lines 78398 78398
==========================================
- Hits 57624 57602 -22
- Misses 20774 20796 +22
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
4b2028f to
7a32a06
Compare
VianneyRuhlmann
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3078c4dc4d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Can you share a bit more on info on why we're doing this migration and why it is safe? Just to make sure I understand what changes here, as getting this wrong means customers not getting data (e.g. because some TLS connections fail due to missing certificates). |
|
The main reason I'm trying to do it now is because it makes loading the certificate lazy on macos. This helps mitigating a bug in dd-trace-py due to the macos Security.framework api not being thread safe. |
I'm not very familiar with it either, which is why I was kinda pointing out that getting this wrong can have a very high cost (ask me how I know.... #1796 / DataDog/dd-trace-rb#5508 ) so it's worth being very careful with changes here. |
paullegranddc
left a comment
paullegranddc
left a comment
There was a problem hiding this comment.
Can you test that https still works with either fips and normal https on linux?
| let certs = load_root_certs()?; | ||
| let client_config = ClientConfig::builder() | ||
| .with_root_certificates(certs) | ||
| .with_platform_verifier()? |
There was a problem hiding this comment.
I think this will potentially change behavior for macOS and Windows users who use SSL_CERT_FILE and SSL_CERT_DIR? If a user is using a custom CA here it won't pick them up. This might be ok, since I don't think it's common for Windows to set this (worth checking with someone from .NET), and most macOS usage is for local dev, so the agent would be on localhost anyway and not using https?
At a minimum, it's worth verifying and documenting.
There was a problem hiding this comment.
I've checked with the dotnet guild and we don't support it (actually dotnet themselves refused to add support). Since profiling already switched to platform-verifier for the reqwest client I think it's fine to move on.
There was a problem hiding this comment.
I agree, this isn't sustainable. I'll create a jira ticket to look into this deeper.
@paullegranddc @VianneyRuhlmann - I think it should work ok, and also shouldn't pull in |
| roots.add(cert).ok(); | ||
| } | ||
| if roots.is_empty() { | ||
| return Err(errors::Error::NoValidCertifacteRootsFound.into()); |
There was a problem hiding this comment.
Can / should we remove this error type? It doesn't look like it's used anywhere anymore.
ekump
left a comment
ekump
left a comment
There was a problem hiding this comment.
Not an area I'm an expert in, but looks reasonable. Let's just add some tests. Otherwise LGTM.
3078c4d to
287da1c
Compare
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: waiting
and removed
mergequeue-status: waiting
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
# Release proposal for libdd-remote-config and its dependencies This PR contains version bumps based on public API changes and commits since last release. ## libdd-common **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-common-v5.0.0` ### Commits - chore(profiling): Use SECURITY_ANONYMOUS when connecting to named pipe server (#2134) - fix: Fix http PathAndQuery Uri Parsing (#2122) - chore(common)!: replace native-certs with platform-verifier (#2078) - feat(data-pipeline)!: CSS Trace Filters (#1985) - fix(libdd-common): Add fallback logic for resolving Azure Functions instance name [SVLS-8931] (#2077) - test: fix timeouts on heavily contended scenarios (#2093) ## libdd-remote-config **Next version:** `1.0.0` **Semver bump:** `major` **Tag:** `libdd-remote-config-v1.0.0` **Warning:** this is an initial release. Please verify that the version and commits included are correct. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: iunanua <[email protected]>
5 participants
What does this PR do?
Replace rustls-native-certs with rustls-platform-verifier as recommended in rustls-native-certs.
Motivation
Potential fix to a crash in dd-trace-py on macos
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.