Skip to content

Commit

Permalink
httpcaddyfile: Set challenge ports when http_port or https_port are used
Browse files Browse the repository at this point in the history
  • Loading branch information
@mholt
mholt committed May 12, 2024
1 parent 4356635 commit 583c585
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
20 changes: 19 additions & 1 deletion caddyconfig/httpcaddyfile/tlsapp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ func fillInGlobalACMEDefaults(issuer certmagic.Issuer, options map[string]any) e
globalACMEEAB := options["acme_eab"]
globalPreferredChains := options["preferred_chains"]
globalCertLifetime := options["cert_lifetime"]
globalHTTPPort, globalHTTPSPort := options["http_port"], options["https_port"]

if globalEmail != nil && acmeIssuer.Email == "" {
acmeIssuer.Email = globalEmail.(string)
Expand All @@ -480,7 +481,24 @@ func fillInGlobalACMEDefaults(issuer certmagic.Issuer, options map[string]any) e
if globalPreferredChains != nil && acmeIssuer.PreferredChains == nil {
acmeIssuer.PreferredChains = globalPreferredChains.(*caddytls.ChainPreference)
}

if globalHTTPPort != nil && (acmeIssuer.Challenges == nil || acmeIssuer.Challenges.HTTP == nil || acmeIssuer.Challenges.HTTP.AlternatePort == 0) {
if acmeIssuer.Challenges == nil {
acmeIssuer.Challenges = new(caddytls.ChallengesConfig)
}
if acmeIssuer.Challenges.HTTP == nil {
acmeIssuer.Challenges.HTTP = new(caddytls.HTTPChallengeConfig)
}
acmeIssuer.Challenges.HTTP.AlternatePort = globalHTTPPort.(int)
}
if globalHTTPSPort != nil && (acmeIssuer.Challenges == nil || acmeIssuer.Challenges.TLSALPN == nil || acmeIssuer.Challenges.TLSALPN.AlternatePort == 0) {
if acmeIssuer.Challenges == nil {
acmeIssuer.Challenges = new(caddytls.ChallengesConfig)
}
if acmeIssuer.Challenges.TLSALPN == nil {
acmeIssuer.Challenges.TLSALPN = new(caddytls.TLSALPNChallengeConfig)
}
acmeIssuer.Challenges.TLSALPN.AlternatePort = globalHTTPSPort.(int)
}
if globalCertLifetime != nil && acmeIssuer.CertificateLifetime == 0 {
acmeIssuer.CertificateLifetime = globalCertLifetime.(caddy.Duration)
}
Expand Down
8 changes: 8 additions & 0 deletions caddytest/integration/caddyfile_adapt/global_options_acme.caddyfiletest
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@
"issuers": [
{
"ca": "https://example.com",
"challenges": {
"http": {
"alternate_port": 8080
},
"tls-alpn": {
"alternate_port": 8443
}
},
"email": "test@example.com",
"external_account": {
"key_id": "4K2scIVbBpNd-78scadB2g",
Expand Down

4 comments on commit 583c585

@francislavoie
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Are you sure about this? What does this end up doing? Many users use http_port to say "this is the port running in Docker but publicly I'm using 80", will that still work?

@mholt
Copy link
Member Author

@mholt mholt commented on 583c585 May 12, 2024

Choose a reason for hiding this comment

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

Yeah. It tells the ACME challenge solvers to bind to the ports designated for HTTP and HTTPS by the config. Those are only for internal interpretation anyway -- you can't change what port the world uses as HTTP and HTTPS. So this implies a port forwarding scenario.

Without this change, setting http_port or https_port while using a global option like acme_ca will cause the HTTP/TLS challenges to still happen on 80/443, but because http_port and https_port are set, we are to understand that those ports are being forwarded and we should serve the challenges on the altered ports. I saw this happening when I was testing that Cloudflare thing on my home machine, and got permission errors when it tried binding to port 80+443, even though I set ports > 1024.

The bug only happens, AFAICT, when a global option sets an ACME issuer. Without another relevant global option such as acme_ca, you'd see that the challenges happen on the adjusted ports as expected.

@francislavoie
Copy link
Member

Choose a reason for hiding this comment

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

Why would it behave differently with global option acme_ca vs tls directive? 🤔 I feel like we would've noticed this if it was a problem with tls as well.

@mholt
Copy link
Member Author

@mholt mholt commented on 583c585 May 12, 2024

Choose a reason for hiding this comment

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

Because with another ACME global option set like acme_ca, it creates an explicit automation policy & issuer, so it needs to have the ports specified. Before this change the logic only covers implicit automation policies.

Please sign in to comment.