Implement known_keys_file support for PubKey authentication by milyin · Pull Request #2590 · eclipse-zenoh/zenoh

milyin · 2026-05-05T12:24:57Z

Description

Improve AuthPubKey::from_config: add known_keys_file parameter support previously marked as TBD

What does this PR do?

Adds support of known_keys_file config parameter
Adds test for AuthPubKey::from_config
Allows to define one key locally and other from file. Not absolutely necessary but also no reason to forbid it
Explicitly documented config elements and behavior if both inline and file keys defined (inline key wins, this behavior existed before)

Why is this change needed?

Declared functionality was missing with TBD comment

Related Issues

Fixes #1339, based on #2211

🏷️ Label-Based Checklist

Based on the labels applied to this PR, please complete these additional requirements:

Labels: enhancement

✨ Enhancement Requirements

Since this PR enhances existing functionality:

Enhancement scope documented - Clear description of what is being improved
Minimum necessary code - Implementation is as simple as possible, doesn't overcomplicate the system
Backwards compatible - Existing code/APIs still work unchanged
No new APIs added - Only improving existing functionality
Tests updated - Existing tests pass, new test cases added if needed
Performance improvement measured - If applicable, before/after metrics provided
Documentation updated - Existing docs updated to reflect improvements
User impact documented - How users benefit from this enhancement

Remember: Enhancements should not introduce new APIs or breaking changes.

Instructions:

Check off items as you complete them (change - [ ] to - [x])
The PR checklist CI will verify these are completed

This checklist updates automatically when labels change, but preserves your checked boxes.

Copilot

Pull request overview

This PR adds support for the transport/auth/pubkey/known_keys_file configuration option in the unicast transport PubKey authenticator, and updates auth initialization to accommodate async file I/O.

Changes:

Make AuthPubKey::from_config async and load authorized public keys from known_keys_file.
Wire the new async AuthPubKey::from_config into transport auth initialization.
Minor import reordering and dependency lockfile update.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.

File	Description
`zenoh/src/api/builders/scouting.rs`	Import order tweak (no functional change).
`io/zenoh-transport/src/unicast/establishment/ext/auth/pubkey.rs`	Implement `known_keys_file` loading and refactor keypair selection logic; `from_config` becomes async.
`io/zenoh-transport/src/unicast/establishment/ext/auth/mod.rs`	Await the now-async pubkey authenticator config initialization.
`Cargo.lock`	Records updated dependency graph (adds `const_format` entry).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

oteffahi

IMHO pubkey auth extension is a naive and very verbose attempt at what mTLS already performs in a much safer and scalable way (which is also already integrated in ACL). This extension is internally used for mutlilink, which does not require the known_keys file. The best course of action to me is to remove auth/pub_key from the config file.

If we still want to go along with exposing this, I've left a few comments.

oteffahi · 2026-05-07T10:24:31Z

+            for line in content.lines() {
+                let trimmed = line.trim();
+                current_key.push_str(line);
+                current_key.push('\n');
+                if trimmed == "-----END RSA PUBLIC KEY-----" {
+                    let public_key = RsaPublicKey::from_pkcs1_pem(&current_key).map_err(|e| {
+                        zerror!("{S} Invalid RSA public key in known keys file: {}.", e)
+                    })?;
+                    known_keys.insert(public_key.into());
+                    current_key.clear();
+                }
+            }
+            if current_key.chars().any(|c| !c.is_whitespace()) {
+                bail!("{S} Truncated RSA public key in known keys file.");
            }


I don't agree with this file format where PKCS#1 PEMs are chained together. I'd rather have the base64 encoding of each public key in PKCS#1 PEM, so each line is a single encoded public key, which simplifies the parsing.
Furthermore, this logic should not be inlined here. Consider extracting this in a helper function or wrapping the public keys file logic in its own type.

oteffahi · 2026-05-07T10:30:55Z

+                    /// RSA public key in PKCS#1 PEM format (inline string).
+                    /// If both `public_key_pem` and `public_key_file` are set, `public_key_pem` takes priority.
                    public_key_pem: Option<String>,
+                    /// RSA private key in PKCS#1 PEM format (inline string).
+                    /// If both `private_key_pem` and `private_key_file` are set, `private_key_pem` takes priority.
                    private_key_pem: Option<String>,
+                    /// Path to a file containing the RSA public key in PKCS#1 PEM format.
+                    /// Ignored when `public_key_pem` is also set.
                    public_key_file: Option<String>,
+                    /// Path to a file containing the RSA private key in PKCS#1 PEM format.
+                    /// Ignored when `private_key_pem` is also set.
                    private_key_file: Option<String>,
                    key_size: Option<usize>,
+                    /// Path to a file containing one or more trusted peer RSA public keys in PKCS#1 PEM format.
+                    /// Requires private and public keys to also be set.
                    known_keys_file: Option<String>,


Please add this documentation to DEFAULT_CONFIG.json5

oteffahi · 2026-05-07T10:46:10Z

+                    /// Path to a file containing the RSA private key in PKCS#1 PEM format.
+                    /// Ignored when `private_key_pem` is also set.
                    private_key_file: Option<String>,
                    key_size: Option<usize>,


This does not seem to be used anywhere in the code. I believe it was added in the event that we make the size of the generated RSA key for multilink configurable. IMO It does not belong here and therefore should be removed.

oteffahi · 2026-05-07T11:13:21Z

From testing it seems like the PR allows providing a pub/priv key-pair without providing a trusted_keys file. This should be considered an invalid config.

Since the extension requires mutual authentication, this config results in an instance that cannot connect or accept any connections, since it requires any peer to support auth_pubkey, and hold a trusted keypair (which is impossible since the trusted_keys file was not provided). This can be considered a breaking change, since we previously allowed this configuration (which doesn't make sense).

Additionally, this configuration is valid when used by the multilink extension which does not use a trusted_keys file (its requirement is that the same keypair is used for two links on the same transport, it doesn't require that said keypair is known ahead of time). Please take this into consideration when making this change.

Co-authored-by: Oussama Teffahi <70609372+oteffahi@users.noreply.github.com>

WaterWhisperer and others added 3 commits October 20, 2025 17:02

Implement known_keys_file support for PubKey authentication

e39ec6d

Update Cargo.lock

168918a

cargo fmt

aebb34c

milyin requested a review from Copilot May 5, 2026 14:08

Copilot started reviewing on behalf of milyin May 5, 2026 14:08 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

milyin added 4 commits May 5, 2026 16:52

test added

cf789f8

second key in scaffolding

1fa067d

from_config corrected

762bd0e

cargo fmt

7d3f32e

milyin requested a review from Copilot May 5, 2026 15:05

Copilot started reviewing on behalf of milyin May 5, 2026 15:06 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

milyin added 6 commits May 5, 2026 17:29

async removed

46a1d0e

cargo fmt

c1cca3d

incorrect file valiadation

71b783e

prefix temp filename in test

8266bb8

comment corrected

1fb3b4e

Merge branch 'main' into known-keys-file

362e2a3

milyin added the enhancement Existing things could work better label May 5, 2026

milyin requested a review from Copilot May 5, 2026 15:56

Copilot started reviewing on behalf of milyin May 5, 2026 15:57 View session

milyin marked this pull request as ready for review May 5, 2026 16:01

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread io/zenoh-transport/src/unicast/establishment/ext/auth/pubkey.rs

Comment thread io/zenoh-transport/src/unicast/establishment/ext/auth/pubkey.rs

Comment thread io/zenoh-transport/tests/unicast_authenticator_config.rs

milyin added 2 commits May 5, 2026 18:12

renamed

15c3011

cargo fmt

7fd7f4b

milyin requested a review from Copilot May 5, 2026 16:17

Copilot started reviewing on behalf of milyin May 5, 2026 16:18 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread io/zenoh-transport/tests/unicast_authenticator_config.rs

make local pem win

ce42922

inline key wins

820f73a

milyin requested a review from Copilot May 5, 2026 17:36

Copilot started reviewing on behalf of milyin May 5, 2026 17:37 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread io/zenoh-transport/src/unicast/establishment/ext/auth/pubkey.rs

give access to hashes instead of key

2a71302

milyin requested a review from Copilot May 5, 2026 19:12

Copilot started reviewing on behalf of milyin May 5, 2026 19:13 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread io/zenoh-transport/src/unicast/establishment/ext/auth/pubkey.rs Outdated

hashing replaced to compare

0684587

milyin requested a review from Copilot May 5, 2026 19:27

Copilot started reviewing on behalf of milyin May 5, 2026 19:28 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread commons/zenoh-config/src/lib.rs Outdated

Comment thread io/zenoh-transport/src/unicast/establishment/ext/auth/pubkey.rs

Potential fix for pull request finding

26100ea

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

milyin requested a review from fuzzypixelz May 5, 2026 19:42

oteffahi suggested changes May 7, 2026

View reviewed changes

oteffahi reviewed May 7, 2026

View reviewed changes

Update io/zenoh-transport/src/unicast/establishment/ext/auth/pubkey.rs

2db9b2c

Co-authored-by: Oussama Teffahi <70609372+oteffahi@users.noreply.github.com>

fuzzypixelz mentioned this pull request May 18, 2026

Implement known_keys_file support for PubKey authentication #2211

Open

6 tasks

Conversation

milyin commented May 5, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What does this PR do?

Why is this change needed?

Related Issues

🏷️ Label-Based Checklist

✨ Enhancement Requirements

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

oteffahi left a comment

Choose a reason for hiding this comment

Uh oh!

oteffahi May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oteffahi May 7, 2026

Choose a reason for hiding this comment

Uh oh!

oteffahi May 7, 2026

Choose a reason for hiding this comment

Uh oh!

oteffahi commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

milyin commented May 5, 2026 •

edited by github-actions Bot

Loading