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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

httpcaddyfile: Add preferred_chains global option and issuer subdirective #4192

Merged

Conversation

Klooven
Copy link
Contributor

@Klooven Klooven commented Jun 5, 2021

This PR adds support for the preferred_chains option in the Caddyfile; both as an option for the ACME issuer and as a global option. If merged, this PR would resolve #4185.


The structure is the following:

preferred_chains [smallest] {
  root_common_name <common names...>
  any_common_name  <common names...>
}

The Caddyfile adapt tests added in this branch show how the options can be used both "locally" and globally. More details can be found in the issue linked above.

And, I'm of course happy to answer any questions that might arise 馃檪

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2021

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie added this to the v2.5.0 milestone Jun 5, 2021
@francislavoie francislavoie added the under review 馃 Review is pending before merging label Jun 5, 2021
@francislavoie francislavoie changed the title Caddyfile support for preferred_chains httpcaddyfile: Caddyfile support for preferred_chains Jun 6, 2021
@francislavoie francislavoie changed the title httpcaddyfile: Caddyfile support for preferred_chains httpcaddyfile: Add preferred_chains global option and tls subdirective Jun 6, 2021
@francislavoie francislavoie changed the title httpcaddyfile: Add preferred_chains global option and tls subdirective httpcaddyfile: Add preferred_chains global option and issuer subdirective Jun 6, 2021
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.

This looks pretty good. The only nit I have is that the parsing code is basically identical between that of the issuer config and the global option.

Can you consolidate that?

Since package httpcaddyfile imports package caddytls, maybe the parsing code for this could be in caddytls as its own function.

@Klooven
Copy link
Contributor Author

Klooven commented Jun 7, 2021

This looks pretty good. The only nit I have is that the parsing code is basically identical between that of the issuer config and the global option.

Can you consolidate that?

You're right. I'll have a look at it and send in an update to the PR when I've changed that.

@Klooven Klooven force-pushed the preferred-chains-caddyfile-support branch from 04102d1 to 7f22cd3 Compare June 8, 2021 18:34
@Klooven
Copy link
Contributor Author

Klooven commented Jun 8, 2021

Ok, I managed to consolidate most of the stuff into a ParseCaddyfilePreferredChainsOptions function I created. Don't know if it's in the best place (I can move it if you feel like it should be in another place).

I encountered some issues on the way, so I don't know if the code is as elegant as it could be (comments welcome on this too). Did some testing and everything seemed to still work like before the "consolidation".

@Klooven Klooven force-pushed the preferred-chains-caddyfile-support branch from 7f22cd3 to 9319c19 Compare June 8, 2021 20:02
@mholt mholt removed the under review 馃 Review is pending before merging label Jun 8, 2021
@mholt
Copy link
Member

mholt commented Jun 8, 2021

Great, looks good now! Thank you very much! Hope you'll contribute again.

@mholt mholt merged commit 1e92258 into caddyserver:master Jun 8, 2021
@Klooven
Copy link
Contributor Author

Klooven commented Jun 8, 2021

Thanks! This has been a great way to learn more about how Caddy works and to code with Go (which has been on my to-do list for too long 馃槃).

@Klooven Klooven deleted the preferred-chains-caddyfile-support branch June 8, 2021 20:14
@francislavoie francislavoie modified the milestones: v2.5.0, v2.4.2 Sep 1, 2021
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.

Support for preferred_chains in Caddyfile
4 participants