feat(iroh)!: switch TLS authentication to raw public keys #2937

dignifiedquire · 2024-11-15T18:48:53Z

Description

This introduces the configuration of the TLS authentication method, allowing to enable the usage of raw public keys, which will lead to us being able to remove the hack of using self signed certificates.

Breaking Changes

By default iroh endpoints will now use the new RawPublicKey TLS configuration. This means they will not be able to talk to older iroh nodes. If needed, the old mechanism can still be used by configuring the endpoint like this

let endpoint = Endpoint::builder()
   .tls_x509() // <--- this enables the old style TLS authentication
   // ...
   .bind();

Closes #2798

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
Tests if relevant.
All breaking changes documented.

github-actions · 2024-11-15T18:52:00Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2937/docs/iroh/

Last updated: 2025-03-14T13:04:06Z

github-actions · 2024-11-15T19:04:03Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 4b65c00

matheus23 · 2024-11-18T09:06:37Z

For a hot minute I was imagining a feature on rustls to disable all non-RPK paths, imagining that we could get rid of ASN.1/DER parsing code.
Then I remembered that we still use HTTPS 🙃

matheus23 · 2024-11-18T11:28:10Z

iroh-net/src/tls/resolver.rs

+use crate::tls::Authentication;
+
+#[derive(Debug)]
+pub(crate) struct AlwaysResolvesCert {


Kind of annoying that rustls has all the implementations for ResolvesClientCert and ResolvesServerCert that we need, but they're not all exposed.

There's

AlwaysResolvesClientCert

AlwaysResolvesServerCert

AlwaysResolvesClientRawPublicKeyCert

AlwaysResolvesServerRawPublicKeyCert

I'd rather not have to implement a trait outside the crate where it's defined, but eh.
Also it's super annoying that they split it up into client/server like that.

dignifiedquire · 2024-12-04T21:52:41Z

Going to wait with pushing this forward until we know if rustls/rustls#2258 might happen

dignifiedquire · 2025-02-11T10:08:39Z

Going to wait with pushing this forward until we know if rustls/rustls#2258 might happen

looks like this is not happening, or at least not soon, so moving forward with this

iroh/src/endpoint.rs

Cargo.lock

iroh/src/endpoint.rs

matheus23 · 2025-02-13T08:14:46Z

iroh/src/magicsock.rs

-
-    #[tokio::test]
-    #[traced_test]
-    async fn test_two_devices_roundtrip_quinn_rebinding_conn() -> Result<()> {


Hm. Are we sure we want to delete these tests?
I see it's weird to see a test that tests "quinn", but I think what this is actually doing is testing UdpConn (without depending on iroh being correct).
Instead of deleting these, perhaps they should be renamed & moved to udp_conn.rs?

matheus23 · 2025-02-13T08:27:53Z

iroh/src/tls.rs

-/// Error for generating iroh p2p TLS configs.
+/// TLS Authentication mechanism
+#[derive(Default, Debug, Copy, Clone, PartialEq, Eq)]
+pub enum Authentication {


I personally find it a little confusing if we have mod tls; in lib.rs, but then use pub enum etc. here.
Can we make it pub(crate) enum (and also the same with pub mod certificate etc.)?

cleaned this up

This enum also should be pub(crate) should it not?

iroh/src/tls/verifier.rs

matheus23

I'm happy with the changes, and I'd be down for this to get merged.

But I do wonder why you've removed 4 tests? Do you think they're not useful anymore? The tests are:

magicsock::test_two_devices_roundtrip_quinn_raw
magicsock::test_two_devices_roundtrip_quinn_rebinding_conn
magicsock::udp_conn::test_rebinding_conn_send_recv_ipv4
magicsock::udp_conn::test_rebinding_conn_send_recv_ipv6

I guess they're related to this PR because they use tls::make_client_config and tls::make_server_config. Was there some problem in getting them to work?

iroh/src/tls/verifier.rs

flub

Can an external observer looking at intercepted datagrams distinguish which mechanism is used?

flub · 2025-03-11T10:37:03Z

iroh/src/endpoint.rs

+    ///
+    /// For details see the libp2p spec at <https://github.com/libp2p/specs/blob/master/tls/tls.md>
+    ///
+    /// This is the only mechanism available in `iroh@0.33.0` and earlier.


We should also add that this method will be removed at some time in the future? Like to make it really clear that you should only use this to transition, but don't just use it and forget about it.

flub · 2025-03-11T10:51:48Z

iroh/src/tls.rs

-/// Error for generating iroh p2p TLS configs.
+/// TLS Authentication mechanism
+#[derive(Default, Debug, Copy, Clone, PartialEq, Eq)]
+pub enum Authentication {


This enum also should be pub(crate) should it not?

dignifiedquire · 2025-03-14T08:28:06Z

But I do wonder why you've removed 4 tests? Do you think they're not useful anymore? The tests are:

They were pretty painful to update, and I am not convinced of their usefulness tbh, as they don't really test much of our code anymore. But happy to hear other arguments

matheus23 · 2025-03-14T08:29:27Z

Can an external observer looking at intercepted datagrams distinguish which mechanism is used?

Yes

dignifiedquire · 2025-03-14T08:30:11Z

Can an external observer looking at intercepted datagrams distinguish which mechanism is used?

My understanding is that as long as we don't have encrypted client hello, this is observable during the handshake

matheus23 · 2025-03-14T08:36:49Z

Can an external observer looking at intercepted datagrams distinguish which mechanism is used?

My understanding is that as long as we don't have encrypted client hello, this is observable during the handshake

Yeah I was about to write that.
Even without RPK, iroh traffic is somewhat detectable via the ALPN extension, via the server-name-indication (it's .iroh.invalid).
I think with encrypted client hello we'd fix that.
But for that we need:

to switch away from ring, because it doesn't implement some crypto we'd need for ECH, and probably won't in the future
ECH to be implemented in rustls and quinn (? I think).

dignifiedquire · 2025-03-14T12:55:01Z

I have moved the test discussion into an issue, to not block merging this #3229

This is step 1 to land #2798

this was only used in tests, and those only tested quinn isolation things, which have been removed

Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>

## Description This introduces the configuration of the TLS authentication method, allowing to enable the usage of raw public keys, which will lead to us being able to remove the hack of using self signed certificates. ## Breaking Changes By default iroh endpoints will now use the new `RawPublicKey` TLS configuration. This means they will not be able to talk to older iroh nodes. If needed, the old mechanism can still be used by configuring the endpoint like this ```rust let endpoint = Endpoint::builder() .tls_x509() // <--- this enables the old style TLS authentication // ... .bind(); ``` Closes #2798 ## Change checklist - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [x] Tests if relevant. - [x] All breaking changes documented. --------- Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>

dignifiedquire added this to the v0.30.0 milestone Nov 15, 2024

matheus23 reviewed Nov 18, 2024

View reviewed changes

dignifiedquire force-pushed the feat-raw-public-key branch from ed3d590 to e44d964 Compare December 6, 2024 15:02

dignifiedquire removed this from the v0.30.0 milestone Dec 10, 2024

flub mentioned this pull request Jan 13, 2025

ALPNs and NodeIDs should be infallible #3123

Open

dignifiedquire force-pushed the feat-raw-public-key branch from 9b7bb71 to dfe0885 Compare February 3, 2025 17:12

dignifiedquire self-assigned this Feb 10, 2025

dignifiedquire added this to the v0.33.0 milestone Feb 10, 2025

dignifiedquire force-pushed the feat-raw-public-key branch from d21dffb to 5d89fe3 Compare February 11, 2025 09:21

dignifiedquire marked this pull request as ready for review February 11, 2025 10:08

dignifiedquire changed the title ~~[WIP] feat: implement support for raw public keys in TLS~~ feat(iroh)!: implement support for raw public keys in TLS Feb 11, 2025

matheus23 reviewed Feb 13, 2025

View reviewed changes

dignifiedquire force-pushed the feat-raw-public-key branch from 98ad8d5 to bb49ae7 Compare February 14, 2025 11:40

ramfox modified the milestones: v0.33.0, v0.34.0 Feb 25, 2025

dignifiedquire force-pushed the feat-raw-public-key branch from 40423a3 to 4f5e787 Compare March 10, 2025 18:13

matheus23 approved these changes Mar 11, 2025

View reviewed changes

iroh/src/tls/verifier.rs Outdated Show resolved Hide resolved

flub approved these changes Mar 11, 2025

View reviewed changes

dignifiedquire force-pushed the feat-raw-public-key branch from 03fac9c to ae45558 Compare March 14, 2025 12:52

dignifiedquire mentioned this pull request Mar 14, 2025

test(iroh): reevaluate removed tests #3229

Open

dignifiedquire and others added 16 commits March 14, 2025 13:55

chore(deps): upgrade rcgen to 0.13

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

fac4526

feat(iroh-net): implement support for raw public keys

06ea58d

This is step 1 to land #2798

fixup

27a8d89

keep tls private

c9a5ab6

switch default to raw public keys

e0bf114

refactor: remove code paths that could skip client auth

866e669

this was only used in tests, and those only tested quinn isolation things, which have been removed

update deps

ab8b3a0

apply first round of CR

a3cc76f

fixups

d9528e3

happy deny

d6514d5

apply cR

48fbaf6

improve docs

71ea3a5

typo

33c231c

Update iroh/src/tls/verifier.rs

ea110f8

Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>

fixup

3c01293

apply CR

Loading
Loading status checks…

6bdbbf0

dignifiedquire force-pushed the feat-raw-public-key branch from a5e653a to 6bdbbf0 Compare March 14, 2025 12:55

dignifiedquire enabled auto-merge March 14, 2025 12:55

dignifiedquire changed the title ~~feat(iroh)!: implement support for raw public keys in TLS~~ feat(iroh)!: switch TLS authentication to raw public keys Mar 14, 2025

cleanup key conversion

Loading
Loading status checks…

f1f1fda

dignifiedquire added this pull request to the merge queue Mar 14, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 14, 2025

dignifiedquire added this pull request to the merge queue Mar 14, 2025

Merged via the queue into main with commit d8c8c8e Mar 14, 2025
25 of 26 checks passed

dignifiedquire deleted the feat-raw-public-key branch March 14, 2025 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh)!: switch TLS authentication to raw public keys #2937

feat(iroh)!: switch TLS authentication to raw public keys #2937

dignifiedquire commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

matheus23 commented Nov 18, 2024

matheus23 Nov 18, 2024

dignifiedquire commented Dec 4, 2024

dignifiedquire commented Feb 11, 2025

matheus23 Feb 13, 2025

matheus23 Feb 13, 2025

dignifiedquire Feb 14, 2025

flub Mar 11, 2025

matheus23 left a comment

flub left a comment

flub Mar 11, 2025

flub Mar 11, 2025

dignifiedquire commented Mar 14, 2025

matheus23 commented Mar 14, 2025

dignifiedquire commented Mar 14, 2025

matheus23 commented Mar 14, 2025

dignifiedquire commented Mar 14, 2025

feat(iroh)!: switch TLS authentication to raw public keys #2937

feat(iroh)!: switch TLS authentication to raw public keys #2937

Conversation

dignifiedquire commented Nov 15, 2024 • edited Loading

Description

Breaking Changes

Change checklist

github-actions bot commented Nov 15, 2024 • edited Loading

github-actions bot commented Nov 15, 2024 • edited Loading

matheus23 commented Nov 18, 2024

matheus23 Nov 18, 2024

Choose a reason for hiding this comment

dignifiedquire commented Dec 4, 2024

dignifiedquire commented Feb 11, 2025

matheus23 Feb 13, 2025

Choose a reason for hiding this comment

matheus23 Feb 13, 2025

Choose a reason for hiding this comment

dignifiedquire Feb 14, 2025

Choose a reason for hiding this comment

flub Mar 11, 2025

Choose a reason for hiding this comment

matheus23 left a comment

Choose a reason for hiding this comment

flub left a comment

Choose a reason for hiding this comment

flub Mar 11, 2025

Choose a reason for hiding this comment

flub Mar 11, 2025

Choose a reason for hiding this comment

dignifiedquire commented Mar 14, 2025

matheus23 commented Mar 14, 2025

dignifiedquire commented Mar 14, 2025

matheus23 commented Mar 14, 2025

dignifiedquire commented Mar 14, 2025

dignifiedquire commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading