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

acmeserver: add policy field to define allow/deny rules #5796

Merged
merged 15 commits into from
Feb 23, 2024
Merged

Conversation

mohammed90
Copy link
Member

@mohammed90 mohammed90 commented Sep 3, 2023

TODO:

  • Tests
  • Docs
  • Caddyfile support

To be merged after #5794

@mohammed90 mohammed90 added feature ⚙️ New feature or request needs docs ✍️ Requires documentation changes needs tests 💯 Requires automated tests labels Sep 3, 2023
@mohammed90 mohammed90 added this to the 2.9.0 milestone Sep 3, 2023
@mholt
Copy link
Member

mholt commented Sep 7, 2023

Cool, this looks great so far. Again, no real strong feedback yet -- it's similar to how I'd probably do it if I had need of this.

@hslatman
Copy link
Contributor

@mohammed90 you could drop the URI domains from this. They're only used for when there are URIs in a certificate (request), and those are not a valid ACME identifier type.

Let me know if you hit issues with the policy configuration / evaluation. We kept it fairly simple intentionally, and sometimes it can be too limiting (e.g. strict equality check of the common name), so happy to hear feedback about it.

@mohammed90
Copy link
Member Author

@mohammed90 you could drop the URI domains from this. They're only used for when there are URIs in a certificate (request), and those are not a valid ACME identifier type.

Thanks for the tip! I'll make the change when I circle back to this PR. Currently the progress on this PR is blocked #5794 since both touch the same file. I'm stuck on testing the ACME server due to my knowledge gap of how to use github.com/mholt/acmez 😶

@mohammed90 mohammed90 marked this pull request as ready for review February 23, 2024 23:03
@mohammed90 mohammed90 requested a review from mholt February 23, 2024 23:03
@mohammed90 mohammed90 removed needs docs ✍️ Requires documentation changes needs tests 💯 Requires automated tests labels Feb 23, 2024
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.

Awesome, looking good and clean and tested. Just one random thought but nothing of importance. Thank you Mohammed, this is great!

modules/caddypki/acmeserver/policy.go Show resolved Hide resolved
@mohammed90 mohammed90 merged commit 931656b into master Feb 23, 2024
25 checks passed
@mohammed90 mohammed90 deleted the acme-policy branch February 23, 2024 23:26
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.

3 participants