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

Relax subdomain validation from UUID to actual subdomain #243

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

jvanasco
Copy link
Contributor

@jvanasco jvanasco commented Sep 8, 2020

I hope you will consider this PR as it will make implementing and maintaining a private acme-dns server easier for certain users. I fully understand the concerns against merging this, and won't be too disappointed if rejected... but I do hope you consider this...

The current check for valid subdomains (

acme-dns/validation.go

Lines 27 to 33 in 75d4a30

func validSubdomain(s string) bool {
_, err := uuid.Parse(s)
if err == nil {
return true
}
return false
}
) parses the subdomain candidate to ensure it is a UUID. This works perfectly, because acme-dns generates UUIDs for subdomains. UUID created == UUID validated. Simple enough!

This PR replaces the UUID parsing with a regular expression to check if the payload is actually a valid subdomain component (initial and trailing characters are alpha-numeric, middle characters are alphanumeric + dash).

The reason for this PR - this change will allow an acme-dns administrator to manually update the database after a call to /register, and replace the generated uuid subdomain with a custom domain. This ability allows for an ACME user to delegate authorization to predictable subdomains on the acme-dns server - which can then streamline enrollment and make reinstalls or server changes painless. For example, the change to DNS when validating foo.example.com might be be:

- CNAME	_acme-challenge	uuid-uuid-uuid-uuid.auth.example.com
+ CNAME	_acme-challenge	com-example-foo.auth.example.com

This PR does not support this feature itself, nor am I proposing that acme-dns support this feature; this PR simply enables a server administrator to have a bit more control if they want to. The acme-dns server administrator would still need to manually edit the database and update the records and txt tables with a custom domain. This is all advanced stuff that violates the Public API, but small changes like this PR open up a lot of possibilities.

Getting custom subdomains to work with acme-dns is tricky. Because the subdomain is validated when /update is invoked, an acme-dns client must use the original subdomain (i.e. the records table must remain intact). I've been using a hack to update the subdomains in the txt table after invoking an /update, but that takes increasingly more work to keep running and won't scale well.

I would love a public api tool/script that admins could use to handle some of this stuff too, like #161 and even something to update the subdomain on an account... but I think this is a good first step for a certain type of users.

99 out of 100 acme-dns users will not need this feature -- but those working with automated systems and/or whitelabel platforms do. As we often do not control the DNS ourselves we need the abilities to (1) pre-determine target subdomains, because we are telling out clients where/how to delegate; (2) ensure/force a set of credentials into the database, because our customers must have a seamless experience if we encounter a data loss (accidents, server migrations, etc).

Thank you for your consideration.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 90.546% when pulling d099acd on jvanasco:feature-relaxed_validation into 19069f5 on joohoi:master.

@joohoi
Copy link
Owner

joohoi commented Jan 11, 2021

Thanks for the PR. Sorry it took a while to get around to review it though. I do see the use case here, and I think it's a reasonable one.

The implementation looks good, merging this in!

@joohoi joohoi merged commit 9c6ca25 into joohoi:master Jan 11, 2021
@jvanasco
Copy link
Contributor Author

wow! thanks!

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.

3 participants