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

httpcaddyfile: Implement force_automate #6712

Merged
merged 1 commit into from
Dec 24, 2024
Merged

httpcaddyfile: Implement force_automate #6712

merged 1 commit into from
Dec 24, 2024

Conversation

francislavoie
Copy link
Member

Closes #5933

I think this should work according to the JSON config output, but I didn't actually test it at runtime to verify that it yields the expected behaviour. @mholt you had a repro set up locally if you want to give it a spin while reviewing.

@francislavoie
Copy link
Member Author

francislavoie commented Nov 27, 2024

What's annoying about this is it's hard to test a real-world scenario because the TLS automation stuff has special conditions for .localhost domains (to use the internal issuer), so using fake domains may have different results than using real domains.

But I did confirm at least that using tls force_automate does correctly hoist a connection policy to the top making an automated cert take precedence over a wildcard loaded cert.

I'm not able to test with a real domain right now (time/effort) so I can't confirm with certainty that the change to automation policies has the intended effect. The behaviour I'm seeing is that all hostnames are having certs automated, even a wildcard with a loaded cert, even if auto_https ignore_loaded_certs is set. That probably shouldn't be happening, but in practice it's not a big deal cause local issuance is super fast and doesn't take up much storage etc.

Configuring like acme_ca global option or whatever to use a fake ACME issuer (e.g. another Caddy with acme_server) also might not produce a proper test result because that overrides a default, which changes how policy consolidation works. Like, any config added which doesn't just use plain defaults can make the test invalid.

@SimJoSt
Copy link

SimJoSt commented Nov 28, 2024

Tested, works.
Took me some time to solve some permission issues I caused myself after building it for linux arm64, but I made it :)

While I cannot speak to the implementation and the general approach, I successfully tested this in production 🙈 with real domains and services.

@SimJoSt
Copy link

SimJoSt commented Nov 28, 2024

In another test I ran caddy adapt, like mentioned in #5933 (comment) and ran into an issue.
Apparently, this command doesn't know about the new force_automate option for the tls directive yet, and fails.
It seems there are more places in the code where this new option needs to be defined.

@francislavoie
Copy link
Member Author

You just ran adapt with your old version of Caddy. Make sure you run adapt with the correct binary.

Thanks for testing! Appreciated.

@SimJoSt
Copy link

SimJoSt commented Nov 28, 2024

You are absolutely right :D Sorry, for the confusion. adapt works without any issue.

@mholt
Copy link
Member

mholt commented Dec 4, 2024

Thanks for working on this. To be sure I'm clear, is there anything left on this issue? #5933 (comment) seems to suggest it has some unintuitive components... and I agree, this does feel like a hack; and I'm still wondering if we should just fix the existing behavior...

@SimJoSt
Copy link

SimJoSt commented Dec 7, 2024

@mholt from testing the feature for my specific use-case, it was feature complete for me.
I cannot speak to an integration perspective or if @francislavoie has more changes planned.

From how I interpret the hierarchy in Caddys (Caddyfile) config I would expect this option to allow me to force the tls automation, if there was something else set at the global level. I am not sure, if it is possible to load certificates globally..

The current behavior of using loaded certs from a separate site with a wildcard domain in sites with no tls directive (default) or one with just an email provide for automated certificates, is still unintuitive in my opinion.
Why is a loaded cert from another scope/site used in a scope/site where it is not mentioned.
I also don't expect the root directive from the wildcard site to be used in my other sites... ;)

If this behavior will not be changed and this PR makes it to upstream, I would suggest changing it a little.
If I supply tls [email protected], I am already indicating that I want automated certs. Shouldn't that be enough to override the loaded certs?

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

I have one more thought before trying this in 2.9. What if we call it or something more specific like ignore_wildcard or automate_separately?

@francislavoie
Copy link
Member Author

I still think force_automate is most descriptive of what it's doing. It's forcing the Caddyfile adapter to produce config to automate for that name, regardless of any of the other rules in Automatic HTTPS. This feature doesn't actually directly interact with wildcard names, even if that's the main purpose of it.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Alrighty, sounds good to me. Let's try it, but let's be sure to document this as experimental, since I still have a hunch there's a cleaner way to do this. I suspect there may be edge cases we haven't tested for here as well; but I can't think of them.

Thank you for your patience with me on this one!

@mholt mholt merged commit afa778a into master Dec 24, 2024
33 checks passed
@mholt mholt deleted the force-automate branch December 24, 2024 15:58
@mholt mholt added this to the v2.9.0 milestone Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Certificate for catch-all site is used for requests to other sites
3 participants