Support requesting certificates using ACME by nothingmuch · Pull Request #1026 · payjoin/rust-payjoin

nothingmuch · 2025-08-30T00:21:46Z

The first commit adds a serve_acme method to the directory service struct using the tokio_rustls_acme crate, feature gated behind acme.

The second commit adds configuration fields (with no environment variable support, but see #1023), and uses serve_acme unconditionally if the acme feature is enabled.

TODO:

decide if let's encrypt's production or staging env is to be used by default
decide whether to support more than one domain (potentially a privacy footgun, so i am leaning towards discouraging)
decide whether acme builds should make acme listening optional

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 · 2025-08-30T00:45:00Z

Pull Request Test Coverage Report for Build 18357389084

Details

1 of 82 (1.22%) changed or added relevant lines in 3 files are covered.
4 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.5%) to 83.737%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-directory/src/main.rs	0	10	0.0%
payjoin-directory/src/lib.rs	1	35	2.86%
payjoin-directory/src/config.rs	0	37	0.0%

Files with Coverage Reduction	New Missed Lines	%
payjoin-directory/src/lib.rs	1	56.55%
payjoin-directory/src/main.rs	1	0.0%
payjoin-directory/src/config.rs	2	0.0%

Totals
Change from base Build 18288677845:	-0.5%
Covered Lines:	8954
Relevant Lines:	10693

💛 - Coveralls

nothingmuch · 2025-08-30T17:12:30Z

Thinking more about how to feature gate this more nicely, i think the appropriate thing is to make acme support opt-in through config even if the feature is enabled, that would require some refactoring, and to also make _manual_tls into a non hidden feature, with configuration options that are mutually exclusive with the acme support, in case people want to use a self signed certificate or manage their own with certbot or something.

payjoin-directory/src/cli.rs

luisschwab · 2025-09-11T22:13:17Z

cACK 22081a4

I was able to successfully get the certificate when running payjoin-directory --acme-domain payjoin.luisschwab.net --acme-contact mailto:netops@luisschwab.net --acme-cache-dir /root/acme-cache --lets-encrypt-production.

DanGould · 2025-10-06T19:34:26Z

decide if let's encrypt's production or staging env is to be used by default

How about default to Let’s Encrypt production when ACME is enabled but add a one-switch --dry-run? Isn't that what certbot does?

decide whether to support more than one domain (potentially a privacy footgun, so i am leaning towards discouraging)

Let's just not do this until a request to do so comes in. KISS.

decide whether acme builds should make acme listening optional

Let's not make it optional because it's simpler.

nothingmuch · 2025-10-06T20:39:26Z

decide if let's encrypt's production or staging env is to be used by default

How about default to Let’s Encrypt production when ACME is enabled but add a one-switch --dry-run? Isn't that what certbot does?

i just renamed it to --lets-encrypt-staging, so staging env is opt in in the CLI

Let's just not do this until a request to do so comes in. KISS.

👍

Let's not make it optional because it's simpler.

i kinda disagree, because then enabling the option breaks things if acme not in used, making it effectively mandatory, and that doesn't make much sense as a default enabled option, which it probably should be?

we already have manual TLS (just not exposed in the CLI)

nothingmuch · 2025-10-06T20:40:12Z

(if you feel strongly about acme being mandatory then my last fixup commit can be discarded instead of squashed, the code is kinda ugly for it due to clap limitations wrt Option<Bundle>)

DanGould · 2025-10-07T00:57:13Z

I don't feel strongly about acme support being mandatory. It makes more sense to be opt-in at runtime. I misunderstood on first read.

payjoin-directory/src/lib.rs

DanGould

Looks like caching must be implemented in order to accept this change

payjoin-directory/src/main.rs

payjoin-directory/src/config.rs

DanGould · 2025-10-07T01:26:13Z

payjoin-directory/src/lib.rs

+    #[cfg(feature = "acme")]
+    pub async fn serve_acme<EC, EA>(self, listener: TcpListener, acme_config: AcmeConfig<EC, EA>)
+    where
+        EC: 'static + std::fmt::Debug,
+        EA: 'static + std::fmt::Debug,
+    {
+        let tcp_incoming = TcpListenerStream::new(listener);
+
+        let tls_incoming = acme_config.incoming(tcp_incoming, Vec::new());
+
+        self.serve_connections(tls_incoming).await;
+    }


better in a feature gated file imo but that's a nit

as an extension trait? or can a second impl block be added in a separate mod?

i believe a second impl block may be added in a separate mod and then imported at the top level in the parent.

writing impl<D: crate::Db> crate::Service<D> { works from an acme.rs submodule for this. It's something that could be done to the manual_tls implementations too. @zealsham perhaps you'd like to pick this up as a follow up?

nothingmuch · 2025-10-07T16:06:46Z

Looks like caching must be implemented in order to accept this change

fwiw why i intended by possibly making it optional is to provide a default path instead of requiring an explicit path, but yes, storing the certificate state on disk is a requirement otherwise you get throttled by let's encrypt

payjoin-directory/src/config.rs

nothingmuch · 2025-10-07T16:49:05Z

See clap-rs/clap#5687 for context about Option<AcmeCli> where AcmeCli { foo: String } (theoretically nicer) vs. AcmeCli { foo: Option<String> } (what's actually implemented)

DanGould · 2025-10-07T17:27:44Z

See clap-rs/clap#5687 for context about Option<AcmeCli> where AcmeCli { foo: String } (theoretically nicer) vs. AcmeCli { foo: Option }` (what's actually implemented)

Couldn't a specific acme subommand be used with args `required to fix this grouping?

nothingmuch · 2025-10-07T18:50:51Z

Couldn't a specific acme subommand be used with args `required to fix this grouping?

i guess, but that seems messier, makes --help less useful/understandable, and arguably even more hacky than the little bit of boilerplate that we have for dealing with the values being wrapped in Option since we ignore the subcommand

edited to add screenshot of current behavior, which seems like what we want

nothingmuch · 2025-10-07T21:14:07Z

i guess --cache-dir should definitely be --acme-cache-dir, but should--domain and --contact be prefixed as well? leaning towards yes. --lets-encrypt-staging seems sufficient as is though.

DanGould · 2025-10-07T21:18:25Z

prefix sounds good

nothingmuch · 2025-10-08T19:32:41Z

Hmm, I failed to consider when CLI options are used as overrides... I'm leaning towards removing the requires clap modifiers, and instead only enforcing the values are set in the config

nothingmuch · 2025-10-08T19:38:39Z

also there's a bug in the handling of the ohttp-keys, since it has a clap default value, it will always override the config file even if not specified explicitly, i'm gonna fix it in this PR since it's basically the same change for --lets-encrypt-staging

DanGould · 2025-10-08T19:50:26Z

Have you considered an enum to prevent boolean hell?

.directory_lets_encrypt(matches!(acme_config.lets_encrypt_mode, LetsEncryptMode::Production))

nothingmuch · 2025-10-08T19:54:50Z

Have you considered an enum to prevent boolean hell?

yeah, but that seems like a lot of boilerplate, not sure it's any improvement in readability

Makes all CLI args `Option<T>`, because unless explicitly provided, args shouldn't override config values.

Currently the certificate cache directory is mandatory. In principle it doesn't have to be.

nothingmuch · 2025-10-08T20:36:27Z

rebased on top of #1157

DanGould

Booleans in f8a147d seem sane to me.

uTACK f8a147d, but I know @spacebear21 said he'd give a review as well

spacebear21

utACK f8a147d (I tested the command with various arg combinations but haven't tested with an actually valid acme configuration).

spacebear21 · 2025-10-08T20:46:58Z

payjoin-directory/src/cli.rs

        help = "A directory for writing mailbox data."
    )]
-    pub storage_dir: PathBuf,
+    pub storage_dir: Option<PathBuf>,


This one is a special case because it's actually a required field. When not provided it now produces a less legible error message:

❯ cargo run Compiling payjoin-directory v0.0.3 (/Users/spacebear/Projects/rust-payjoin/payjoin-directory) Finished `dev` profile [unoptimized + debuginfo] target(s) in 5.23s Running `/Users/spacebear/Projects/rust-payjoin/target/debug/payjoin-directory` Logging initialized Error: configuration property "storage_dir" not found

vs. before:

❯ cargo run Blocking waiting for file lock on build directory Compiling payjoin-directory v0.0.3 (/Users/spacebear/Projects/rust-payjoin/payjoin-directory) Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.03s Running `/Users/spacebear/Projects/rust-payjoin/target/debug/payjoin-directory` Logging initialized error: the following required arguments were not provided: --storage-dir <STORAGE_DIR> Usage: payjoin-directory --storage-dir <STORAGE_DIR>

Maybe the solution is to just print a custom message if a storage_dir is not found in either config the file or cli args. Weird that clap doesn't really support these seemingly obvious usecases...

yeah the error kind of sucks, but the change is intentional, with the previous behavior if it was provided in the config file then it would still error on the CLI

the CLI args are provided as overrides to the config which relies on Option

i'm not sure how to make the error more legible without adding a ton of boilerplate and changing this approach to composing cli & config

I think it's fine for now, if we get more people running directories and getting confused by this message I'd consider it a good problem to have.

spacebear21

re-ACK

nothingmuch requested a review from DanGould August 30, 2025 00:21

nothingmuch force-pushed the acme branch 2 times, most recently from 1e6ff82 to 02f682d Compare August 30, 2025 00:40

nothingmuch force-pushed the acme branch from 02f682d to 22081a4 Compare August 30, 2025 00:47

arminsabouri added this to the payjoin-directory-1.0 milestone Sep 8, 2025

arminsabouri assigned nothingmuch Sep 8, 2025

luisschwab suggested changes Sep 11, 2025

View reviewed changes

payjoin-directory/src/cli.rs Outdated Show resolved Hide resolved

zealsham mentioned this pull request Sep 17, 2025

WIP: Migrate to axum from hyper #1068

Closed

15 tasks

nothingmuch force-pushed the acme branch from 22081a4 to eb84f71 Compare October 6, 2025 20:32

DanGould reviewed Oct 7, 2025

View reviewed changes

payjoin-directory/src/lib.rs Outdated Show resolved Hide resolved

DanGould reviewed Oct 7, 2025

View reviewed changes

nothingmuch commented Oct 7, 2025

View reviewed changes

payjoin-directory/src/config.rs Outdated Show resolved Hide resolved

spacebear21 mentioned this pull request Oct 8, 2025

1.0 Integrations tracking issue #1156

Open

30 tasks

nothingmuch force-pushed the acme branch 3 times, most recently from 84abaca to 4dbacb7 Compare October 8, 2025 18:58

nothingmuch marked this pull request as ready for review October 8, 2025 18:59

nothingmuch requested a review from luisschwab October 8, 2025 19:29

nothingmuch added 3 commits October 8, 2025 22:11

use serve_metrics_tcp in main.rs

ae2be4c

make metrics serving optional and opt-in

889f553

don't override config values with cli defaults

44e74b0

Makes all CLI args `Option<T>`, because unless explicitly provided, args shouldn't override config values.

nothingmuch force-pushed the acme branch 3 times, most recently from 971b5d6 to 0c9c56f Compare October 8, 2025 20:30

nothingmuch mentioned this pull request Oct 8, 2025

Directory cli fixes #1157

Merged

2 tasks

nothingmuch added 2 commits October 8, 2025 22:35

Add feature gated serve_acme

2a69d33

Expose ACME support in payjoin directory binary

f8a147d

Currently the certificate cache directory is mandatory. In principle it doesn't have to be.

nothingmuch force-pushed the acme branch from 0c9c56f to f8a147d Compare October 8, 2025 20:35

DanGould approved these changes Oct 8, 2025

View reviewed changes

spacebear21 approved these changes Oct 8, 2025

View reviewed changes

spacebear21 merged commit 823154d into payjoin:master Oct 8, 2025
10 checks passed

zealsham mentioned this pull request Oct 8, 2025

Remove hyper and migrate to axum. #1101

Closed

2 tasks

Conversation

nothingmuch commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

coveralls commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18357389084

Details

💛 - Coveralls

Uh oh!

nothingmuch commented Aug 30, 2025

Uh oh!

Uh oh!

luisschwab commented Sep 11, 2025

Uh oh!

DanGould commented Oct 6, 2025

Uh oh!

nothingmuch commented Oct 6, 2025

Uh oh!

nothingmuch commented Oct 6, 2025

Uh oh!

DanGould commented Oct 7, 2025

Uh oh!

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanGould Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

nothingmuch Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Oct 8, 2025 • 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!

nothingmuch commented Oct 7, 2025

Uh oh!

Uh oh!

nothingmuch commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented Oct 7, 2025

Uh oh!

nothingmuch commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nothingmuch commented Oct 7, 2025

Uh oh!

DanGould commented Oct 7, 2025

Uh oh!

nothingmuch commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nothingmuch commented Oct 8, 2025

Uh oh!

DanGould commented Oct 8, 2025

Uh oh!

nothingmuch commented Oct 8, 2025

Uh oh!

nothingmuch commented Oct 8, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

nothingmuch Oct 8, 2025

nothingmuch commented Aug 30, 2025 •

edited

Loading

coveralls commented Aug 30, 2025 •

edited

Loading

DanGould Oct 8, 2025 •

edited

Loading

nothingmuch commented Oct 7, 2025 •

edited

Loading

nothingmuch commented Oct 7, 2025 •

edited

Loading

nothingmuch commented Oct 8, 2025 •

edited

Loading