feat(relay): reloading certificate resolver #2999

Arqu · 2024-12-03T22:10:51Z

Description

This sets us up so we can have manually/externally managed certs that will reload on a daily basis. This should close #1108 and we should follow up on the ops side to utilize this where needed.

Ok, this turned out to be much grosser than expected.

I don't like the new dependencies we introduce
I don't like the loader setup
I'm unsure whether to keep the reloading resolver in the iroh repo or move it into tokio-rustls-acme (think it should move over, but for the sake of this discussion, included it here)
Should I do anything with the _handle for the reloader in regards to shutting down?

Most of this still feels necessary. I could maybe feature flag it?
I had a really hard time trying to extract the loader creation into a standalone func which also aligns all the traits and error types so the thing compiles.

Suggestions are welcome :)

Example config:

enable_relay = true
http_bind_addr = "[::]:80"
enable_stun = true
stun_bind_addr = "[::]:3478"
enable_metrics = true
metrics_addr = "127.0.0.1:9090"

[tls]
manual_cert_path="certificate.der"
manual_key_path="private_key.der"
cert_mode = "Reloading"

Breaking Changes

Notes & open questions

Change checklist

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

github-actions · 2024-12-03T22:21:56Z

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: 146804b

Arqu · 2024-12-04T10:57:57Z

From @flub in discord (moving the discussion over here to make things a bit easier)

i have some comments on the config structure, code (and bikeshedding 😉 ), but the principle of it doesn't seem that gross?

👍

IIUC this reads certs from a file at given intervals. which seems fine

Correct

i'm also not sure why you think this might belong in tokio-acme-rustls, it doesn't seem to have anything acme about it?

I guess what I wanted it to be is mostly it's own separate thing. It's both generally useful outside of iroh and also makes the relay codebase less cluttered. I also expect this to crop up with other services we're running, I'd rather not copy it over.

wrt the comments, my biggest structural one is probably why this is separate from Manual as it seems at some level this is > just manual + an interval?

I agree, the CertMode handling should be separate, but bellow that it's the same as Manual anyways.

Thanks for the input!

flub · 2024-12-04T12:25:58Z

i'm also not sure why you think this might belong in tokio-acme-rustls, it doesn't seem to have anything acme about it?

I guess what I wanted it to be is mostly it's own separate thing. It's both generally useful outside of iroh and also makes the relay codebase less cluttered. I also expect this to crop up with other services we're running, I'd rather not copy it over.

This is not that much code and I think it fits just fine inside the relay server for now. Once we have to start copy-pasting it we can re-visit that I guess.

github-actions · 2024-12-04T15:04:31Z

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

Last updated: 2024-12-12T14:49:55Z

iroh-relay/src/server/resolver.rs

dignifiedquire · 2024-12-10T13:59:28Z

iroh-relay/src/server/resolver.rs

+    /// The inner reloadable value.
+    reloadable: Arc<Reloadable<CertifiedKey, Loader>>,
+    /// The handle to the task that reloads the certificate.
+    _handle: JoinHandle<()>,


AbortingDropJoinHandle?

Arqu · 2024-12-10T14:21:23Z

Looking to get back to this tonight

Arqu · 2024-12-11T12:44:43Z

iroh-relay/src/server/resolver.rs

+    /// The handle to the task that reloads the certificate.
+    _handle: AbortOnDropHandle<()>,
+    /// Shutdown signal sender
+    _shutdown_tx: tokio::sync::oneshot::Sender<()>,


Should I bubble this up into the relay so we call it on relay.shutdown() or is the AbortOnDropHandle enough for this usecase?

Arqu · 2024-12-11T14:24:36Z

Do we have to bump the MSRV because redb got bumped to 2.3?

dignifiedquire · 2024-12-11T16:43:49Z

Do we have to bump the MSRV because redb got bumped to 2.3?

what's their new MSRV? we probably want to bump it to normalize cargo.lock to v4

Arqu · 2024-12-11T20:14:05Z

Do we have to bump the MSRV because redb got bumped to 2.3?

what's their new MSRV? we probably want to bump it to normalize cargo.lock to v4

1.81

dignifiedquire · 2024-12-11T20:48:15Z

lets bump MSRV to 1.81 then, this will make live easier for us

dignifiedquire · 2024-12-11T20:48:20Z

but in a new PR

dignifiedquire · 2024-12-11T20:49:49Z

#3031

Arqu · 2024-12-11T21:13:09Z

Rest of the PR good now?

dignifiedquire · 2024-12-11T21:47:52Z

iroh-relay/src/server/resolver.rs

+                        tracing::info!("Reloaded the certificate");
+                    }
+                } => {},
+                _ = shutdown_rx => {


we usually use cancellation token for this

dignifiedquire · 2024-12-12T12:28:58Z

MSRV fix is in now

dignifiedquire · 2024-12-12T14:31:37Z

iroh-relay/Cargo.toml

 reqwest = { version = "0.12", default-features = false, features = [
    "rustls-tls",
 ] }
 ring = "0.17"
 rustls = { version = "0.23", default-features = false, features = ["ring"] }
+rustls-cert-reloadable-resolver = "0.7.1"
+rustls-cert-file-reader = "0.4.1"


I think all the new dependencies should be optional and only for the server feature

flub · 2024-12-12T14:54:39Z

Cargo.toml

Can you revert this noisy change?

flub · 2024-12-12T14:55:28Z

iroh-relay/Cargo.toml

@@ -135,4 +141,4 @@ required-features = ["server"]

 [package.metadata.docs.rs]
 all-features = true
-rustdoc-args = ["--cfg", "iroh_docsrs"]
+rustdoc-args = ["--cfg", "iroh_docsrs"]


this is also a needless change. It is better to leave newlines at the end of files.

flub · 2024-12-12T15:07:11Z

iroh-relay/src/server/resolver.rs

+use tokio_util::{sync::CancellationToken, task::AbortOnDropHandle};
+
+/// The default certificate reload interval.
+pub const DEFAULT_CERT_RELOAD_INTERVAL: u64 = 60 * 60 * 24;


No units! Maybe make it a Duration? Or is that still not const? Should definitely document the units though.

flub · 2024-12-12T15:12:22Z

lol, i miss the review window again 😄

Arqu · 2024-12-12T15:12:48Z

I'll do a follow up, just for you :D

Arqu added the c-iroh-relay label Dec 3, 2024

Arqu requested review from flub, dignifiedquire and matheus23 December 3, 2024 22:10

Arqu self-assigned this Dec 3, 2024

Arqu force-pushed the arqu/reloading_certs branch from 19d291b to d232285 Compare December 3, 2024 22:14

dignifiedquire added this to the v0.30.0 milestone Dec 9, 2024

dignifiedquire reviewed Dec 10, 2024

View reviewed changes

iroh-relay/src/server/resolver.rs Show resolved Hide resolved

dignifiedquire reviewed Dec 10, 2024

View reviewed changes

Arqu force-pushed the arqu/reloading_certs branch from 675778a to caf438d Compare December 11, 2024 12:43

Arqu commented Dec 11, 2024

View reviewed changes

Arqu changed the title ~~[WIP] feat(relay): reloading certificate resolver~~ feat(relay): reloading certificate resolver Dec 11, 2024

Arqu marked this pull request as ready for review December 11, 2024 14:44

dignifiedquire reviewed Dec 11, 2024

View reviewed changes

Arqu added 3 commits December 12, 2024 14:58

feat(relay): reloading certificate resolver

7555966

some cleanup

16ece24

fix

e422440

Arqu added 9 commits December 12, 2024 14:58

CRs

3c9c7f6

cleanup

69e7352

more cleanup

4d3dc69

adjust deps

2ce2469

whoops, wrong branch

8402238

wrong toml file

540a6b6

this is anoying

c433e8f

pin redb to 2.2

e292c9c

cleanup deps

183bc97

Arqu force-pushed the arqu/reloading_certs branch from cdb0bc1 to 183bc97 Compare December 12, 2024 14:00

swap channel for token

6180b4f

dignifiedquire reviewed Dec 12, 2024

View reviewed changes

move everything behind the server feature

86ae60f

dignifiedquire approved these changes Dec 12, 2024

View reviewed changes

Arqu added this pull request to the merge queue Dec 12, 2024

flub reviewed Dec 12, 2024

View reviewed changes

Merged via the queue into main with commit c37895b Dec 12, 2024
25 of 26 checks passed

Arqu deleted the arqu/reloading_certs branch December 12, 2024 15:12

feat(relay): reloading certificate resolver #2999

feat(relay): reloading certificate resolver #2999

Uh oh!

Conversation

Arqu commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Breaking Changes

Notes & open questions

Change checklist

Uh oh!

github-actions bot commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Arqu commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flub commented Dec 4, 2024

Uh oh!

github-actions bot commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dignifiedquire Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

Arqu commented Dec 10, 2024

Uh oh!

Arqu Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

Arqu commented Dec 11, 2024

Uh oh!

dignifiedquire commented Dec 11, 2024

Uh oh!

Arqu commented Dec 11, 2024

Uh oh!

dignifiedquire commented Dec 11, 2024

Uh oh!

dignifiedquire commented Dec 11, 2024

Uh oh!

dignifiedquire commented Dec 11, 2024

Uh oh!

Arqu commented Dec 11, 2024

Uh oh!

dignifiedquire Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

dignifiedquire commented Dec 12, 2024

Uh oh!

dignifiedquire Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

flub Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

flub Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

flub Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flub commented Dec 12, 2024

Uh oh!

Arqu commented Dec 12, 2024

Uh oh!

Uh oh!

Arqu commented Dec 3, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

Arqu commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading