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

registration with invalid allowfrom fails open #43

Closed
cpu opened this issue Feb 28, 2018 · 3 comments
Closed

registration with invalid allowfrom fails open #43

cpu opened this issue Feb 28, 2018 · 3 comments

Comments

@cpu
Copy link
Contributor

cpu commented Feb 28, 2018

I didn't read the documentation very carefully and thought I could POST /registration with an allowfrom entry for a single host address instead of a CIDR range, e.g.:

{
    "allowfrom": [
        "127.0.0.1"
    ]
}

acme-dns filters this invalid allowfrom in acmedb.Register using cidrslice.ValidEntries. The registration is created successfully with no errors, but the returned allowfrom is empty and so is the field in the database for this user:

{
  "username": "<rand>",
  "password": "<rand>",
  "fulldomain": "<rand>.example.com",
  "subdomain": "<rand>",
  "allowfrom": []
}

The documentation specifically says to use a CIDR range but I think there might be a case to be made for rejecting the registration with an error when the allowfrom contains invalid entries. Since allowfrom is a security control I think acme-dns should be conservative and fail fast when it can't fulfill the request as received.

@joohoi What do you think?

@joohoi
Copy link
Owner

joohoi commented Mar 1, 2018

I think you are on the right track here, and we should fail early. But from the UX perspective, I think what would be best way to handle this, would be to automatically add /32 bitmask if none is specified but the IP address itself is valid.

@cpu
Copy link
Contributor Author

cpu commented Mar 1, 2018

from the UX perspective, I think what would be best way to handle this, would be to automatically add /32 bitmask if none is specified but the IP address itself is valid.

That sounds like a nice compromise 👍

@joohoi
Copy link
Owner

joohoi commented Feb 5, 2019

This is now finally fixed. I opted to not to introduce magic I proposed in #43 (comment) . The only magic is removing [ and ] from the IP addresses as the net.ParseCIDR cannot handle that notation of IPv6 addresses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants