feat: make loading the WebPKI root trust store optional. - #2766
Open
jpace121 wants to merge 1 commit into
Open
Conversation
Closes eclipse-zenoh#2764 Signed-off-by: James Pace <jpace121@gmail.com> Assisted-by: Antigravity (Gemini 3.8 Flash)
Author
|
Looking at the CI:
|
Author
|
I updated the checklist. The only thing I am not sure about checking is "comprehensive testing". I didn't add an unit tests, though there aren't any that I can find for the code around what I added either? |
oteffahi
suggested changes
Sep 9, 2026
Contributor
There was a problem hiding this comment.
Changes look good.
Please add some tests that validate the new config field is taken into consideration. For example:
- A test that expects failure when starting a transport with TLS listener that has no root cert and does not use WebPKI trust store.
- Another test that succeeds in starting a transport with TLS listener with the default config and/or explicit
true.
These tests should be added under the zenoh-transport crate.
Additionally, documentation needs to be updated on zenoh.io. Repo for documentation: https://github.com/atolab/zenoh-web
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
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.
Description
What does this PR do?
Adds a new configuration parameter "use_public_pki" to control whether the root trust store for TLS and QUIC connections will load the default WebPKI root certs or not.
The value defaults to true, maintaining the behavior in main today.
Full disclosure: Gemini 3.8 Flash was used to provide an initial review of this PR.
To verify the configuration is being loaded correctly, I set up a peer config with a TLS endpoint as the only valid endpoint, and the currently valid TLS parameters set up such that TLS works. I then launched a simple publisher process with logging set to trace and looked for the messages that are logged when the webpki root store is loaded and when the user cert is added. I verified the right logs showed up at the right time with the new parameter missing, the new parameter set to true, and the new parameter set to false.
I then redid the test for QUIC.
Why is this change needed?
For my project, I would like to use certificates to identify different participants in the network.
Because I control all of the nodes, I don't need or want a public certificate authority to be used. Digicert's opinion on if something is or is not at some some IP is irrelevant. As set up in main, if I turn on TLS, I wind up having to trust the public PKI system, which is just unnecessary. By setting this new parameter to false, my loaded root cert is the only one that is used, which is what I want.
Not loading the public PKI roots also gives me a sort of lazy method of access control. No one without a cert I sign can get access to my system. As set up in main, I would have to also use the other access control methods like passwords on top of just using certs (not that that is a bad idea.)
Related Issues
Fixes #2764
🏷️ Label-Based Checklist
Based on the labels applied to this PR, please complete these additional requirements:
Labels:
new feature🆕 New Feature Requirements
Since this PR adds a new feature:
Consider: Can this feature be split into smaller, incremental PRs?
Instructions:
- [ ]to- [x])This checklist updates automatically when labels change, but preserves your checked boxes.