Skip to content

Add ACME certificate management to payjoin-service#1315

Merged
spacebear21 merged 2 commits intopayjoin:masterfrom
spacebear21:payjoin-service-acme
Feb 5, 2026
Merged

Add ACME certificate management to payjoin-service#1315
spacebear21 merged 2 commits intopayjoin:masterfrom
spacebear21:payjoin-service-acme

Conversation

@spacebear21
Copy link
Copy Markdown
Collaborator

Add an acme feature flag and config section modeled after the payjoin-directory feature.

This implementation uses tokio-rustls-acme's AxumAcceptor following the example in
https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_axum.rs As a result the AcmeConfig differs slightly from the one in payjoin-directory to more closely reflect rustls-acme's AcmeConfig struct.

Pull Request Checklist

Please confirm the following before requesting review:

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Feb 4, 2026

Pull Request Test Coverage Report for Build 21719481218

Details

  • 31 of 88 (35.23%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 83.276%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin-service/src/main.rs 0 5 0.0%
payjoin-service/src/config.rs 15 37 40.54%
payjoin-service/src/lib.rs 6 36 16.67%
Totals Coverage Status
Change from base Build 21718187822: -0.4%
Covered Lines: 10238
Relevant Lines: 12294

💛 - Coveralls

@spacebear21 spacebear21 force-pushed the payjoin-service-acme branch 4 times, most recently from f63d789 to 56fe208 Compare February 4, 2026 18:18
@spacebear21 spacebear21 requested a review from benalleng February 4, 2026 18:30
@spacebear21 spacebear21 mentioned this pull request Feb 4, 2026
29 tasks
Copy link
Copy Markdown
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CACK 56fe208 though we should probably validate that this runs in the container we created first

@DanGould
Copy link
Copy Markdown
Contributor

DanGould commented Feb 5, 2026

I realize payjoin-service still only listens on one port, when in reality it should probably listen on :443 with acme and :80 without it so that environments without TLS, e.g. Bitcoin Core, can still reach the directory while relying on bootstrap mechanisms other than TLS.

Copy link
Copy Markdown
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK on prod server but with config.toml not envvars.

My only concern is the default timeouts changing (why do this?) see inline comments below.

Environment variables like for the collection failed with getting payjoin-directory-1 | Error: invalid type: map, expected a sequence for key acme.domainswhen I did PJ_ACME_DOMAINS_0: "lets.payjo.in"

services:
  payjoin-service:
    image: payjoin-service:acme
    environment:
      # ...
      PJ_ACME_DOMAINS_0: "lets.payjo.in"
      PJ_ACME_CONTACT_0: "mailto:admin@example.com"

and now payjoin-directory-1 | Error: invalid type: string "[\"lets.payjo.in\"]", expected a sequence for key acme.domains`` when I used PJ_ACME_DOMAINS_0: '["lets.payjo.in"]'

and even double postfix __0 instead of _0 so I'm not sure if that parser is totally right. Anyway, ended up using config.toml and it was fine.

Am I supposed to use these envvars without an index?

Comment thread payjoin-service/src/lib.rs Outdated
listener: "[::]:0".parse().expect("valid listener address"),
storage_dir: tempdir.path().to_path_buf(),
timeout: Duration::from_secs(2),
..Default::default()
Copy link
Copy Markdown
Contributor

@DanGould DanGould Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This not a behavior change from 2 seconds for testing to 30 seconds default? Isn't this gonna make our tests lag in a big way?

Comment thread payjoin-test-utils/src/lib.rs Outdated
listener: "[::]:0".parse().expect("valid listener address"), // let OS assign a free port
storage_dir: tempdir.path().to_path_buf(),
timeout: Duration::from_secs(2),
..Default::default()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same behavior change here?

Copy link
Copy Markdown
Contributor

@DanGould DanGould Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After playing on this worktree with Codex I am wondering if this was an LLM edit that got overlooked that we want to revert.

I suppose the magic number isn't explanatory, so we might be better off having a static timeout: BRIEF_TEST_TIMEOUT = Duration::from_secs(2); so that the LLM (and future human readers) don't change this. Perhaps there are more places in the repo we face this idk.

Copy link
Copy Markdown
Collaborator Author

@spacebear21 spacebear21 Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a manual change actually because the LLM introduced ..Default::default(), but this caused linting issues when the acme feature is disabled (due to no other fields to default). Deleting the custom timeout shouldn't impact tests time unless something breaks so I thought it should be fine to remove. I suppose we could just manually specify acme: None behind the feature gate instead of using Default everywhere. edit: this turned out to be a nightmare with different feature combinations across crates, keeping default() seems much cleaner for now. I think the idiomatic solution would be to make a ConfigBuilder that can be initialized with all default values first and individual values modified as needed.

@DanGould DanGould mentioned this pull request Feb 5, 2026
2 tasks
@spacebear21
Copy link
Copy Markdown
Collaborator Author

The env var parsing was broken, I fixed it in the latest push. No need for indices, you can use comma-separated lists e.g. PJ_ACME__DOMAINS=lets.payjo.in PJ_ACME__CONTACTS=d@ngould.dev,hi@spacebear.dev etc. Note the nesting separator is now a double underscore to prevent conflicts with fields that contain an actual underscore (directory_url).

This satisfies trait bounds for AxumAcceptor in the next commit.
Add an `acme` feature flag and config section modeled after the
payjoin-directory feature.

This implementation uses `tokio-rustls-acme`'s AxumAcceptor following
the example in
https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_axum.rs

As a result the AcmeConfig differs slightly from the one in
payjoin-directory to more closely reflect rustls-acme's AcmeConfig
struct.
Copy link
Copy Markdown
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2f93ec8 waiting for CI

@spacebear21 spacebear21 merged commit a541e41 into payjoin:master Feb 5, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants