OHTTP keys should be rotated by zealsham · Pull Request #1449 · payjoin/rust-payjoin

zealsham · 2026-03-29T13:20:15Z

This pr addresses #445. It implements OHTTP-key rotation to payjoin-mailroom Mailroom operators can now decide the time interval for keys to be rotated. Also if a key has expired, a 422 error is returned to clients. Clients can handle they key-rotation via the cach-control header returned by the directory.

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2026-03-29T13:25:48Z

Coverage Report for CI Build 24348647476

Coverage increased (+0.02%) to 84.357%

Details

Coverage increased (+0.02%) from the base build.
Patch coverage: 43 uncovered changes across 3 files (281 of 324 lines covered, 86.73%).
1 coverage regression across 1 file.

Uncovered Changes

File	Changed	Covered	%
payjoin-mailroom/src/directory.rs	107	77	71.96%
payjoin-mailroom/src/config.rs	14	2	14.29%
payjoin-mailroom/src/key_config.rs	62	61	98.39%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
payjoin-mailroom/src/key_config.rs	1	97.7%

Coverage Stats


Relevant Lines:	13092
Covered Lines:	11044
Line Coverage:	84.36%
Coverage Strength:	403.75 hits per line

💛 - Coveralls

nothingmuch

max-age handling as implemented will not be correct without an Age (or possibly Date) header. i also have a bunch of suggestions to simplify, some more to do with the code itself others with the approach

nothingmuch · 2026-03-29T13:59:20Z

payjoin-mailroom/src/config.rs

            listener: "[::]:8080".parse().expect("valid default listener address"),
            storage_dir: PathBuf::from("./data"),
            timeout: Duration::from_secs(30),
+            ohttp_keys_max_age: Some(Duration::from_secs(604800)),


Suggested change

ohttp_keys_max_age: Some(Duration::from_secs(604800)),

ohttp_keys_max_age: Some(Duration::from_secs(7 * 24 * 60 * 60)),

easier to read IMO. Duration::from_weeks also exists but only in nightly

nothingmuch · 2026-03-29T14:01:22Z

payjoin-mailroom/src/config.rs

+{
+    // duration value of 0 isn't accepted
+    let secs: Option<u64> = Option::deserialize(deserializer)?;
+    Ok(secs.filter(|&s| s > 0).map(Duration::from_secs))


if an invalid duration is given this will silently deserialize as Ok(None) instead of giving an error

nothingmuch · 2026-03-29T14:08:44Z

payjoin-mailroom/src/directory.rs

+#[derive(Debug)]
+pub struct OhttpKeySet {
+    pub current_key_id: u8,
+    pub servers: BTreeMap<u8, ohttp::Server>,


why is this pub and named servers which seems like a reflection of an implementation detail?

also BTreeMap seems overkill if this is constrained to have only IDs 1 and 2. an [Option<ohttp::Server>; 2] could be used and key ID used to derive an index by subtracting 1, seems simpler?

True!, my initial implementation wasn't deleting old keys automatically and would load them back in memory on restart but only accept the one with largest id number. i'll change this

nothingmuch · 2026-03-29T14:10:05Z

payjoin-mailroom/src/directory.rs

+/// All keys in the `servers` map are accepted for decapsulation, allowing clients
+/// with cached older keys to continue working during the overlap window.
+#[derive(Debug)]
+pub struct OhttpKeySet {


the name of this struct seems a bit at odds with its functionality, which handles OHTTP decapsulation as well, maybe KeyRotatingServer is more appropriate?

nothingmuch · 2026-03-29T14:16:32Z

payjoin-mailroom/src/directory.rs

+        if let Some(max_age) = self.ohttp_keys_max_age {
+            res.headers_mut().insert(
+                CACHE_CONTROL,
+                HeaderValue::from_str(&format!("public, max-age={}", max_age.as_secs()))


this will specify the max age of the HTTP resource relative to the time at which the response was generated, but the semantics for the key expiration and self.ohttp_keys_max_age is relative to when the key was initialized

the Age header can be used to specify the age of the response, which is to be subtracted from max_age or this could be done server side

s-maxage is arguably the more appropriate cache control directive to use since this can be shared (c.f. key consistency expired draft), or the equivalent public, max-age=... can be used. immutable can also be set.

This was my first time seeing the "immutable" cache header

implementing this .

nothingmuch · 2026-03-29T14:19:31Z

payjoin-mailroom/src/directory.rs

+///  A new key with the next key_id is generated and persisted to disk.
+///  The new key becomes the current key served to clients.
+///  After `max_age` elapses the old key is dropped from memory and its  .ikm` file is deleted from disk.
+pub fn spawn_key_rotation(


perhaps it'd be simpler to do the keygen logic in the request handling path instead of as a background task?

another way to potentially simplify this is to use a KDF to generate keys as a function of time, i.e. divide the current time by the max key age, use that as a nonce/counter in a KDF based on a single persisted master key, and just generate the per-epoch key deterministically, that would remove the need for persisting the key material on disk (and so avoid any consistency issues and the need for a sync() call while the rwlock is held to ensure that key material is in durable storage)

another way to potentially simplify this is to use a KDF to generate keys as a function of time, i.e. divide the current time by the max key age, use that as a nonce/counter in a KDF based on a single persisted master key, and just generate the per-epoch key deterministically, that would remove the need for persisting the key material on disk (and so avoid any consistency issues and the need for a sync() call while the rwlock is held to ensure that key material is in durable storage)

not sure i completely understand what you mean here. could this be elaborated or discussed in the upcoming mob programming session if you got the time ?

i think my suggestion was a mistake, see below for reasoning. at any rate to answer the question:

simplest version:

generate a root secret, save it on disk

current epoch number = current unix time / epoch length

epoch key = KDF(root secret, current epoch number)

so at every point in time there's a deterministically derived epoch key

ratcheting version:

generate initial epoch key, save it on disk

epoch key = H(previous epoch key), overwrite previous saved epoch key

but thinking about this more, assuming a temporary compromise (same as in forward secrecy) where a key from an earlier epoch was leaked, and after that the adversary loses access and then a new randomly generated key is introduced, then secrecy will be recovered. this is no longer the case with a deterministic approach.

it could be argued that this model is too optimistic for real world settings. once key material is exflitrated, supposing the compromise is detected and a new host is provisioned... what is gained by copying the key key to the new instance? i think it's better to revoke it entirely (i.e. respond with 422 and force re-configuration).

on the other hand, if compromise isn't detected so no new host is provisioned, why would a real world access lose the ability to compromise the host again if they have enough access to exfiltrate key material at an earlier time? that seems like a pretty contrived scenario, it could be realized using scheduled reboots and ephemeral root filesystem combined to ensure that persistent access can't be set up, and with regular updates that might patch the vulnerability recovering from the compromise... anyway the point is actually realizing this threat model takes significant effort.

so, if this reason is not strong enough to want to do this, what other reasons are there to rotate keys? i can't think of one... and if it is strong enough, we definitely need the stateful version you've implemented, not a deterministic approach since that seems to invalidate the only justification

nothingmuch · 2026-04-03T00:07:57Z

payjoin-mailroom/src/directory.rs

+            .and_then(|i| self.keys[i as usize].as_ref());
+        match server {
+            Some(s) => s.decapsulate(ohttp_body),
+            None => Err(ohttp::Error::Truncated),


Suggested change

None => Err(ohttp::Error::Truncated),

None => Err(ohttp::Error::KeyId),

why truncated?

nothingmuch · 2026-04-03T00:13:41Z

payjoin-mailroom/src/directory.rs

+        if self.ohttp.read().await.current_key_created_at().elapsed() < max_age {
+            return Ok(());
+        }
+        let mut keyset = self.ohttp.write().await;


both keyset and self.ohttp are hard to read... i would consider renaming, maybe self.servers_by_keyid?

nothingmuch · 2026-04-03T00:15:10Z

payjoin-mailroom/src/directory.rs

+        let new_key_id = keyset.next_key_id();
+        if let Some(dir) = &self.ohttp_keys_dir {
+            let _ = tokio::fs::remove_file(dir.join(format!("{new_key_id}.ikm"))).await;
+        }
+        let config = crate::key_config::gen_ohttp_server_config_with_id(new_key_id)
+            .map_err(HandlerError::InternalServerError)?;
+        if let Some(dir) = &self.ohttp_keys_dir {
+            crate::key_config::persist_key_config(&config, dir)
+                .map_err(HandlerError::InternalServerError)?;
+        }


seems cleaner to try generating before zapping the old key

Suggested change

let new_key_id = keyset.next_key_id();

if let Some(dir) = &self.ohttp_keys_dir {

let _ = tokio::fs::remove_file(dir.join(format!("{new_key_id}.ikm"))).await;

}

let config = crate::key_config::gen_ohttp_server_config_with_id(new_key_id)

.map_err(HandlerError::InternalServerError)?;

if let Some(dir) = &self.ohttp_keys_dir {

crate::key_config::persist_key_config(&config, dir)

.map_err(HandlerError::InternalServerError)?;

}

let new_key_id = keyset.next_key_id();

let config = crate::key_config::gen_ohttp_server_config_with_id(new_key_id)

.map_err(HandlerError::InternalServerError)?;

if let Some(dir) = &self.ohttp_keys_dir {

let _ = tokio::fs::remove_file(dir.join(format!("{new_key_id}.ikm"))).await;

crate::key_config::persist_key_config(&config, dir)

.map_err(HandlerError::InternalServerError)?;

}

but actually what i would prefer even more is if the old key was cleared sooner, so that the array contains an Option and a None most of the time, and the two keyids are only valid for a grace period accounting for reasonable clock skew and latency (so on the order of a few seconds)

if two keyids are always available for use, then clients using the old key leak information about their local clock, which is a strong fingerprint

that said we definitely should support two keyids during a short time window, because if only one key is ever available then clients who are slightly ahead will expire their key but then fetch the same key and effectively need to poll the ohttp configuration until it changes, which is wasteful, or clients who are behind will make requests with a stale key, get an error, need to do key configuration, and then submit the request under the new key. since this is wasteful and degrades ux, some allowance is necessary.

nothingmuch · 2026-04-03T00:32:50Z

payjoin-mailroom/src/directory.rs

        }
    }

+    async fn maybe_rotate_keys(&self) -> Result<(), HandlerError> {


since i retracted my suggestion to do deterministic keys, i think it also no longer makes sense to do this on the request path, and it should go back to being done in a background loop

the deterministic approach would not have required an RwLock or any IO... the combination of both of these things including IO that happens while holding the RwLock will now potentially block long running requests, and long polling requests which will hold a read() lock for their entire duration.

rwlock does not define reader/writer priority, so depending on the OS either many long polls will block key rotation until all long poll requests expire (though this can be fixed, see other comment), and any new requests will likewise block while attempting to obtain an rwlock because the current key expired

Dropping the read guard just after decapsulation fixes this in a way. Although i'm dropping the function entirely in favour of the background task one

nothingmuch · 2026-04-03T00:33:37Z

payjoin-mailroom/src/directory.rs

-        let (bhttp_req, res_ctx) = self
-            .ohttp
+        // Decapsulate OHTTP request using the key matching the message's key_id
+        let keyset = self.ohttp.read().await;


this holds the rwlock with read for entirety of the request handling, but it could be dropped right after decapsulation since res_ctx contains all the information required to respond

nothingmuch · 2026-04-03T01:01:22Z

a simpler way to support two keyids only during transition window is to always have two of them, no Option, instead keep a valid_from time for each separately.

after valid_from + ttl + grace period, a background loop can overwrite the inactive key with a new one.

when decapsulating, if the key is stale but hasn't been overwritten yet, it's still in the grace period so the request should be accepted. if the key is stale then the keyid is now representing the key of the next epoch, but that hasn't been served via config yet so it's impossible for any client to use it.

then the config can start serving the next key ~1/2 of the grace period before the current key expires (valid_from + ttl), it should already usable because it was overwritten shortly after the end of the last epoch, and serving it slightly before expiry means that clients whose clock is slightly ahead and therefore have expired their local copy won't refetch the same exact key with a really short remaining time to live

finally, if the RwLock inside of the array, then the two locks should generally not be in contention either (assuming they aren't on the same cache line, which may need some alignment pragma), since the rwlock on the stale key should only be taken after requests involving that keyid kinda die out. this means the loop can freely hold the rwlock while it does IO without introducing any jitter to requests that wouldn't have generated an error anyway

nothingmuch · 2026-04-03T01:07:49Z

payjoin-mailroom/src/lib.rs

+        let mut iter = configs.into_iter();
+        let (current_cfg, current_mtime) = iter.next().expect("non-empty");
+        let current_age =
+            std::time::SystemTime::now().duration_since(current_mtime).unwrap_or_default();


should expect() not default, a negative duration indicates the system clock was reset

nothingmuch · 2026-04-03T19:33:45Z

payjoin-mailroom/src/directory.rs

+
+impl KeyRotatingServer {
+    pub fn from_single(server: ohttp::Server, key_id: u8) -> Self {
+        assert!(key_id == 1 || key_id == 2, "key_id must be 1 or 2");


hmm, key identifiers are an arbitrary u8: https://www.ietf.org/rfc/rfc9458.html#section-3.1

so probably easier to use key ids 0 and 1 instead of 1 and 2? i think i said 1 and 2 somewhere so sorry if i made it seem like that was an important detail

This needs to be addressed. Ohttp targets can set whatever key id they want

but the mailroom only deals with mailroom ohttp targets and the only reason to support more than 1 key is to do graceful rotation

this may change if say a PQ KEM is added to the OHTTP layer or something like that, but right now supporting more than two distinct keyids serves no purpose that i can see and neither rfc 9458 nor our gateway opt-in extension allows for arbitrary targets, only the mailbox service

nothingmuch · 2026-04-03T19:36:28Z

payjoin-mailroom/src/directory.rs

+    pub fn from_single(server: ohttp::Server, key_id: u8) -> Self {
+        assert!(key_id == 1 || key_id == 2, "key_id must be 1 or 2");
+        let mut keys = [None, None];
+        keys[(key_id - 1) as usize] = Some(server);


it'd be simpler to just initialize both and avoid the Option IMO, no need to pattern match or expect everywhere

btw that's also why i suggested changing "created_at" to "valid_from", since both are created at the same time but only one is valid from Instant::now()

nothingmuch · 2026-04-03T19:37:35Z

payjoin-mailroom/src/directory.rs

+            assert!(id == 1 || id == 2, "key_id must be 1 or 2");
+            keys[(id - 1) as usize] = Some(server);
+        }
+        let created_at = Instant::now().checked_sub(current_key_age).unwrap_or_else(Instant::now);


this should .expect(), not `.unwrap_or_else. if there's a key claimed to be from the future it's best to crash the server because the host machine is clearly misconfigured

nothingmuch · 2026-04-03T19:41:58Z

payjoin-mailroom/src/directory.rs

+        self.current_key_id = new_key_id;
+        self.current_key_created_at = Instant::now();


Suggested change

self.current_key_id = new_key_id;

self.current_key_created_at = Instant::now();

best if this rotates the next key but doesn't yet activate it, see my previous comments discussing why we want two keys and why we want a grace period, if clients whose clock is running a bit fast expire their local key they will request the same key repeatedly, so it's better to already offer the next key (and decapsulate from it) a few seconds before the current key expires, and to still accept an expired key a few seconds after expiry, but then after this grace period already generate the new key to invalidate the old one, removing the need for retire as well

nothingmuch · 2026-04-03T19:45:46Z

payjoin-mailroom/src/directory.rs

+                }
+            };
+            let _ = tokio::fs::remove_file(keys_dir.join(format!("{new_key_id}.ikm"))).await;
+            if let Err(e) = crate::key_config::persist_key_config(&config, &keys_dir) {


IO, especially blocking should not be done while holding a write lock on the whole keyset, since that prevents requests from being handled

see also my suggestion to use a Arc<[Box<RwLock<Server>; 2]> inside of keyset as that eliminates this concern, i think it's perfectly fine to write the key while holding a lock there, but persist_key_config should be async

i opted to make the keyid an atomic u8 as plain u8 isn't safe for concurrent reads and writes ,if switch() writes to it while decapsulate() reads it, that might lead to data-races

arminsabouri

There are couple unaddressed comments from the previous review. In general the hardcoded key ids and array indexing seem like a footgun. See my comment about creating two fields in the key config struct.

Edit: A fixed sized array with indexing is fine. However, i would still like to understand why the fields are optional. The primary key should never be None FWIU, and the secondary key should always be present as well -- only used during the grace period. @zealsham

payjoin-mailroom/src/directory.rs

arminsabouri · 2026-04-03T21:20:35Z

payjoin-mailroom/src/directory.rs

+
+impl KeyRotatingServer {
+    pub fn from_single(server: ohttp::Server, key_id: u8) -> Self {
+        assert!(key_id == 1 || key_id == 2, "key_id must be 1 or 2");


This needs to be addressed. Ohttp targets can set whatever key id they want

arminsabouri · 2026-04-03T21:31:53Z

payjoin-mailroom/src/directory.rs

+// with a cached previous key still work during the overlap window.
+#[derive(Debug)]
+pub struct KeyRotatingServer {
+    keys: [Option<ohttp::Server>; 2],


Would we ever have more than 2 active keys? If not, I would suggest having two fields in this struct instead of indexing

{ current_key: ohttp::Server, previous_key: Option<ohttp::Server>, current_key_created_at: Instant, }

This at least ensures that the primary key is always Some. ~~And may obviate the need for key ids to be in {1,2}~~

Yes , we would have two key for a short while , the grace period of the previous key before it is retired

I understand. However, it seems odd to me that they are optional. Shouldn't the current key always be Some?

both keys should always be Some #1449 (comment)

arminsabouri · 2026-04-03T21:34:39Z

payjoin-mailroom/src/lib.rs

        None
    };
-    Ok(crate::directory::Service::new(db, ohttp_config.into(), sentinel_tag, v1))
+    let svc =


Nit: unnecessary abbreviation

Suggested change

let svc =

let service =

nothingmuch

mtime of the key file corresponds to when the key was generated, as does some of the usage of the valid_from field, it's still semantically a created_at field

i think the math would be a bit simpler if it was valid_until instead of valid_from, see suggestions but the main thing is that when rotating the key the mtime has to be bumped or the key rotation will switch between one long lived key and another key which nominally lasts a few microseconds longer

nothingmuch · 2026-04-07T11:05:40Z

payjoin-mailroom/src/directory.rs

+
+#[derive(Debug)]
+pub struct KeyRotatingServer {
+    keys: [RwLock<KeySlot>; 2],


i believe this needs to be a Box or to have some alignment declaration, or these are effectively one RwLock since locking works on a cache line granularity

nothingmuch · 2026-04-07T18:40:40Z

payjoin-mailroom/src/directory.rs

+#[derive(Debug)]
+pub(crate) struct KeySlot {
+    pub(crate) server: ohttp::Server,
+    pub(crate) valid_from: Instant,


since the relationship between these is always a difference of epoch length, it's enough to still keep that as a top level field and bump it along with the current keyid as it was before...

also seems like making it valid_until would simplify some of the code to deal with this?

nothingmuch · 2026-04-07T18:49:21Z

payjoin-mailroom/src/directory.rs

+            let config = match crate::key_config::gen_ohttp_server_config_with_id(old_key_id) {
+                Ok(c) => c,
+                Err(e) => {
+                    tracing::error!("Failed to generate OHTTP key: {e}");


this should panic, not just log, if there's no ability to generate key material something has gone terribly wrong

nothingmuch · 2026-04-07T18:49:46Z

payjoin-mailroom/src/directory.rs

+            };
+            let _ = tokio::fs::remove_file(keys_dir.join(format!("{old_key_id}.ikm"))).await;
+            if let Err(e) = crate::key_config::persist_key_config(&config, &keys_dir).await {
+                tracing::error!("Failed to persist OHTTP key: {e}");


this should also panic, if it's impossible to write to disk then again something has gone terribly wrong

nothingmuch · 2026-04-07T18:52:32Z

payjoin-mailroom/src/directory.rs

+    // Replace a slot with fresh key material.
+    pub async fn overwrite(&self, key_id: u8, server: ohttp::Server) {
+        assert!(key_id <= 1, "key_id must be 0 or 1");
+        *self.keys[key_id as usize].write().await = KeySlot { server, valid_from: Instant::now() };


it's not valid from now, it's valid from the end of the previous epoch

the point of renaming it is to indicate that this represents when the key starts being active not when it is generated

nothingmuch · 2026-04-07T18:58:02Z

payjoin-mailroom/src/directory.rs

        let mut res = Response::new(full(ohttp_keys));
        res.headers_mut().insert(CONTENT_TYPE, HeaderValue::from_static("application/ohttp-keys"));
+        if let Some(max_age) = self.ohttp_keys_max_age {
+            let remaining = max_age.saturating_sub(self.ohttp.current_valid_from().await.elapsed());


in line with the other simplification of the delays, adding something like ROTATION_GRACE / 3 to this duration should be done here

nothingmuch · 2026-04-07T19:01:42Z

payjoin-mailroom/src/directory.rs

+            let switch_delay = {
+                let valid_from = keyset.current_valid_from().await;
+                let switch_at = valid_from + interval - ROTATION_GRACE / 2;
+                switch_at.saturating_duration_since(Instant::now())
+            };
+            tokio::time::sleep(switch_delay).await;


Suggested change

let switch_delay = {

let valid_from = keyset.current_valid_from().await;

let switch_at = valid_from + interval - ROTATION_GRACE / 2;

switch_at.saturating_duration_since(Instant::now())

};

tokio::time::sleep(switch_delay).await;

let switch_delay = {

let switch_at = keyset.current_valid_until().await;

switch_at.saturating_duration_since(Instant::now())

};

tokio::time::sleep(switch_delay).await;

i think all the math would be easier to follow if the field was defined like this

tokio::time::sleep_until exists, no need to calculate the difference from now

nothingmuch · 2026-04-07T19:02:04Z

payjoin-mailroom/src/directory.rs

+            let overwrite_delay = {
+                let valid_from = keyset.valid_from(old_key_id).await;
+                let overwrite_at = valid_from + interval + ROTATION_GRACE;
+                overwrite_at.saturating_duration_since(Instant::now())
+            };
+            tokio::time::sleep(overwrite_delay).await;


Suggested change

let overwrite_delay = {

let valid_from = keyset.valid_from(old_key_id).await;

let overwrite_at = valid_from + interval + ROTATION_GRACE;

overwrite_at.saturating_duration_since(Instant::now())

};

tokio::time::sleep(overwrite_delay).await;

tokio::time::sleep(ROTATION_GRACE).await;

nothingmuch · 2026-04-07T19:08:15Z

payjoin-mailroom/src/lib.rs

+        let valid_from = now.checked_sub(age).unwrap_or(now);
+        let key_id = cfg.key_id();
+        slots[key_id as usize] =
+            Some(crate::directory::KeySlot { server: cfg.into_server(), valid_from });


valid_from only makes sense as mtime for the initial key

secondly, if one or both of these keys are expired, then the key generation logic should trigger here during initialization

nothingmuch · 2026-04-07T19:09:40Z

payjoin-mailroom/src/directory.rs

+            };
+            tokio::time::sleep(switch_delay).await;
+
+            let old_key_id = keyset.current_key_id();


the mtime of the key we're about to switch to should be updated at this moment

nothingmuch · 2026-04-07T23:04:22Z

payjoin-mailroom/src/directory.rs


+// Grace period after a switch during which the old key is still
+// accepted for decapsulation.
+const ROTATION_GRACE: Duration = Duration::from_secs(30);


Suggested change

const ROTATION_GRACE: Duration = Duration::from_secs(30);

const ROTATION_GRACE: Duration = Duration::from_secs(15);

this delay should be reduced somewhat, it should only really account for end to end latency, and even then the expected delays not worst case so thinking about it more 30 seconds seems a bit excessive

nothingmuch · 2026-04-07T23:10:35Z

payjoin-mailroom/src/lib.rs

        }
    }
+
+    let configs = crate::key_config::read_server_configs_by_mtime(ohttp_keys_dir)?;


this sorts by mtime, but then they are installed by keyid... isn't it simpler to just return them in keyid order. the older mtime would correspond to the valid_from of the earlier epoch, allowing the later epoch's valid_from to be calculated from that even if it hadn't yet started (server was shutdown before a key it was rotated into use but after it was created)

nothingmuch · 2026-04-08T17:00:56Z

payjoin-mailroom/src/directory.rs

+        current.0 = 1 - current.0;
+        current.1 = Instant::now() + interval;


in my suggestion i believe i wrote RwLock<(u8, Instant)> but i didn't mean that literally, just the tuple a struct would be equivalent to, IMO this should have named fields

nothingmuch · 2026-04-08T17:03:44Z

payjoin-mailroom/src/directory.rs

+                .await
+                .saturating_duration_since(Instant::now())
+                .min(max_age)
+                .saturating_sub(ROTATION_GRACE / 3);


this should add a few seconds, not subtract, the purpose is to avoid clients expiring this before the key is actually rotated (a client whose clock is running fast) only to fetch the same key and have it expire basically immediately

nothingmuch · 2026-04-08T17:04:58Z

payjoin-mailroom/src/directory.rs

+        loop {
+            // Sleep until just before the current key expires.
+            let valid_until = keyset.valid_until().await;
+            let switch_at = valid_until.checked_sub(ROTATION_GRACE / 2).unwrap_or(valid_until);


since the only meaning of the valid_until field is to time this, why not just set it to the time things should be rotated (no grace period related adjustment)

i made this adjustments

nothingmuch · 2026-04-08T17:05:48Z

payjoin-mailroom/src/directory.rs

+            let valid_until = keyset.valid_until().await;
+            let overwrite_at =
+                valid_until.checked_sub(interval).unwrap_or(valid_until) + ROTATION_GRACE;
+            tokio::time::sleep_until(overwrite_at.into()).await;


... then this can just be tokio::time::sleep(ROTATION_GRACE)

nothingmuch · 2026-04-08T17:08:54Z

payjoin-mailroom/src/lib.rs

+    let slot1 = slots[1].take().expect("slot 1 missing after init");
+    let valid_until = match interval {
+        Some(ivl) => now + ivl.saturating_sub(current_age),
+        None => now + std::time::Duration::from_secs(365 * 24 * 3600),


doesn't this force rotation on a yearly schedule even if not configured?

Yes it does, but this part of the code is never reached as it's dependent on max-age being "some". i set it to 10 years now

nothingmuch · 2026-04-08T17:11:20Z

payjoin-mailroom/src/directory.rs

+            // Capture old key id before switching, then switch.
+            // `switch` stamps valid_until = Instant::now() + interval, anchored
+            // to the actual moment the new key goes live.
+            let old_key_id = keyset.current_key_id().await;


the new key file's mtime needs to be updated here before switching, otherwise it may be served in response a key config GET request before the server shuts down, and on the next startup it will be invalidated, causing the client to get a 422 response for their cached key

nothingmuch · 2026-04-08T17:15:12Z

please consider all scenarios and make sure they are handled correctly:

server initializes with two expired keys
server initializes with one valid key and one expired key
server initializes with two valid keys

as far as i can tell only the last one is, and even that is buggy due to not updating mtime

there should be tests covering this

This pr addresses payjoin#445. It implements OHTTP-key rotation to payjoin-mailroom Mailroom operators can now decide the time interval for keys to be rotated. Also if a key has expired, a 422 error is returned to clients. Clients can handle they key-rotation via the cach-control header returned by the directory. fix spawn rotation

zealsham · 2026-04-14T10:04:19Z

please consider all scenarios and make sure they are handled correctly:

server initializes with two expired keys

server initializes with one valid key and one expired key

server initializes with two valid keys

as far as i can tell only the last one is, and even that is buggy due to not updating mtime

there should be tests covering this

implemented this
when the server init with two expired keys , new keys are generated and an active key assigned to the key with id=0
when the server init with one valid key, the remaining inteval is calculated from the mtime of the key , the key is then set as the active key an valid_untill set to the remaining time
if the server initizializes with two valid keys, the key with the older mtime is set to the active key and the remaining time calculated for rotation

nothingmuch reviewed Mar 29, 2026

View reviewed changes

zealsham force-pushed the key-rotation branch 2 times, most recently from 71abcf7 to aa33a74 Compare April 2, 2026 16:59

nothingmuch suggested changes Apr 3, 2026

View reviewed changes

nothingmuch reviewed Apr 3, 2026

View reviewed changes

zealsham force-pushed the key-rotation branch 3 times, most recently from add6ced to 93eeb8b Compare April 3, 2026 19:16

nothingmuch suggested changes Apr 3, 2026

View reviewed changes

arminsabouri requested changes Apr 3, 2026

View reviewed changes

zealsham force-pushed the key-rotation branch from 93eeb8b to d0400d1 Compare April 7, 2026 10:00

nothingmuch suggested changes Apr 7, 2026

View reviewed changes

zealsham force-pushed the key-rotation branch from d0400d1 to 7b2eaeb Compare April 8, 2026 01:04

nothingmuch reviewed Apr 8, 2026

View reviewed changes

zealsham force-pushed the key-rotation branch from 7b2eaeb to 0767504 Compare April 13, 2026 12:47

zealsham force-pushed the key-rotation branch from 0767504 to aad828b Compare April 13, 2026 14:23

	ohttp_keys_max_age: Some(Duration::from_secs(604800)),
	ohttp_keys_max_age: Some(Duration::from_secs(7 * 24 * 60 * 60)),

	None => Err(ohttp::Error::Truncated),
	None => Err(ohttp::Error::KeyId),

		self.current_key_id = new_key_id;
		self.current_key_created_at = Instant::now();

	const ROTATION_GRACE: Duration = Duration::from_secs(30);
	const ROTATION_GRACE: Duration = Duration::from_secs(15);

		current.0 = 1 - current.0;
		current.1 = Instant::now() + interval;

Conversation

zealsham commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 24348647476

Coverage increased (+0.02%) to 84.357%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch commented Apr 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zealsham commented Mar 29, 2026 •

edited

Loading

coveralls commented Mar 29, 2026 •

edited

Loading

nothingmuch Apr 3, 2026 •

edited

Loading

nothingmuch Apr 3, 2026 •

edited

Loading

arminsabouri left a comment •

edited

Loading

arminsabouri Apr 3, 2026 •

edited

Loading