Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tidy examples, add tls-alpn-01 responder example #90

Closed
wants to merge 6 commits into from

Conversation

cpu
Copy link
Collaborator

@cpu cpu commented Mar 7, 2025

Opening as a draft for now while there's some questions on my mind:

  1. Is this something that makes a worthwhile example to have in-repo?
  2. Would it be better to fold into the existing provision.rs example?
  3. Should we postpone this until we land some more utility helpers in the main library to reduce the duplication between provision.rs, pebble.rs and this new example?

I mainly implemented this to have a convenient way to test the ARI code with Let's Encrypt staging/production for #85 (though I've backed out the ARI bits to simplify the example). For that testing I wanted a fully self-contained example that worked with just a DNS name + open port 443. Unlike provision.rs it handles the challenge response internally without needing a human operator to do any external setup.

WDYT?

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

I think it would be good to have something like this. On the other hand, I feel like maybe we should actually build a full-blown server that would be easy to plug, say, Axum things into, which builds in hyper-rustls and integrated ACME and all the niceties instead. I've actually started on a hyper-rustls server but it might end up being a little too opinionated for that project.

It also feels like we should probably do something about all the duplication before we merge this.

(Happy to take all/most of the cleanup before the last commit!)

@@ -18,7 +18,6 @@ async fn main() -> anyhow::Result<()> {
// Create a new account. This will generate a fresh ECDSA key for you.
// Alternatively, restore an account from serialized credentials by
// using `Account::from_credentials()`.

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh, I kinda like the whitespace for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NP. I'll revert this.

@@ -114,5 +118,5 @@ async fn main() -> anyhow::Result<()> {
#[derive(Parser)]
struct Options {
#[clap(long)]
name: String,
name: Vec<String>,
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: let's rename this to names?

Comment on lines +39 to +40
use rustls_pki_types::pem::PemObject;
use rustls_pki_types::{CertificateDer, PrivatePkcs8KeyDer};
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: import pki_types via rustls?


/// Simple [`ResolvesServerCert`] implementation that always uses the wrapped CertifiedKey
///
/// We can't use `ServerConfig::with_single_cert()` directly because it parses the certificate
Copy link
Owner

Choose a reason for hiding this comment

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

Can't we use SingleCertAndKey::from(certified_key)?

/// extension matching a challenge identifier, the server will respond with a certificate that
/// contains the SNI as a SAN and the TLS-ALPN-01 challenge response key auth digest extension.
///
/// Other types of TLS connection are ignored. In a more complete implementation, the server would
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: "connection" -> "connections".

let opts = Options::parse();

// Build a Rustls config that can validate the ACME server's certificate.
let rustls_config = if let Some(roots_pem) = &opts.server_root_cert_path {
Copy link
Owner

Choose a reason for hiding this comment

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

Why have the server_root_cert_path option, is that for local testing? Seems overkill for the example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why have the server_root_cert_path option, is that for local testing?

Yeah exactly, it was handy to do the initial development/testing locally against Pebble. It might also be handy to allow running the example in CI (but didn't implement that in this branch).

I don't feel strongly if you prefer it left out.


// Spawn a challenge response server that listens for incoming TLS connections to solve
// TLS-ALPN-01 challenges.
let (shutdown_tx, shutdown_rx) = mpsc::channel(1);
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe use CancellationToken for this stuff?


impl ChallengeResponseServer {
async fn run(mut self, port: u16) {
info!("starting challenge response server on port: {port}");
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: structured logging? (Also, elsewhere.)

}

async fn handle_connection(&self, stream: TcpStream, addr: SocketAddr) {
info!("handling conn from {}", addr);
Copy link
Owner

Choose a reason for hiding this comment

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

Inline format arguments?

@cpu
Copy link
Collaborator Author

cpu commented Mar 7, 2025

On the other hand, I feel like maybe we should actually build a full-blown server that would be easy to plug, say, Axum things into, which builds in hyper-rustls and integrated ACME and all the niceties instead. I've actually started on a hyper-rustls server but it might end up being a little too opinionated for that project.

Agreed that it would be better to find a way to plug into an actual server framework. I think the main disadvantage of this example is that it doesn't show how you would do this and have a real application on :443. Getting that working in an Axum application with a dummy health-check endpoint or something would be a more compelling/useful example.

It also feels like we should probably do something about all the duplication before we merge this.

Sounds good. It feels like what's missing is a way to take an Order and a ChallengeType and have everything except the challenge response provisioning handled for you. Maybe the AuthorizationMethod trait from pebble.rs could do the job if we removed the PATH parts that are specific to the challenge test server?

(Happy to take all/most of the cleanup before the last commit!)

👍 I'll close this for now then and break out just the cleanup commits.

@cpu cpu closed this Mar 7, 2025
@cpu cpu mentioned this pull request Mar 7, 2025
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.

2 participants