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

Expand draft to use dns-scoping & create DNS-01 #34

Merged
merged 45 commits into from
Feb 10, 2024
Merged

Conversation

aaomidi
Copy link
Owner

@aaomidi aaomidi commented Feb 7, 2024

No description provided.

Copy link

github-actions bot commented Feb 7, 2024

@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 01:29 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 01:32 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 01:36 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 01:37 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 01:50 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 01:55 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 18:46 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 18:56 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 7, 2024 18:58 Inactive
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

I'm really excited for this next version of the document. A few minor suggestions and line-edits:

draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request February 8, 2024 23:13 Inactive
@aaomidi aaomidi changed the title Expanded DNS scoping Expand draft to use dns-scoping & create DNS-01 Feb 8, 2024
@github-actions github-actions bot temporarily deployed to pull request February 8, 2024 23:15 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 8, 2024 23:15 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 8, 2024 23:22 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 8, 2024 23:23 Inactive
Copy link
Collaborator

@jdkasten jdkasten left a comment

Choose a reason for hiding this comment

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

Thanks Amir for all of your work getting this incorporated! It is looking great!

I will make another pass on formatting and final flow once the few comments are addressed. Feel free to push back on anything.

draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
draft-ietf-acme-dns-account-01.mkd Outdated Show resolved Hide resolved
_NODE NAME: _acme-challenge_*
Reference: This document
~~~
The first 10 bytes were picked as a tradeoff: the value needs to be short enough to not significantly impact DNS record and response size, long enough to provide sufficient probability of collision avoidance across ACME accounts, and just the right size to have Base32 require no padding. As the algorithm is used for a uniform distribution of inputs, and not for integrity, we do not consider the trimming a security issue.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The main length concern was to make sure we didn't place additional pressure on the max length of DNS names (253) and make the challenge unusable for abnormally large names. (Statistically there are very very few when I ran this against Censys many years ago.)

I don't think we were too concerned about record size or response size, but it is fine to keep that in.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think by record size we meant the label size. The thing is, 253 is the soft limit I believe right? Else it just gets fragmented and forced into TCP?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've clarified this a bit now

Copy link
Collaborator

Choose a reason for hiding this comment

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

The max valid length of a DNS name is 253 characters. The max label length is 63. So the full sha256 digest could fit into a base32 encoded label, but it would make more DNS names theoretically not be able to use this challenge.
https://devblogs.microsoft.com/oldnewthing/20120412-00/?p=7873

Certainly smaller responses in general are always preferred to avoid issues with fragmentation attacks.
https://www.ietf.org/archive/id/draft-ietf-dnsop-avoid-fragmentation-16.html

@github-actions github-actions bot temporarily deployed to pull request February 9, 2024 20:02 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 9, 2024 20:03 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 9, 2024 20:05 Inactive
@aaomidi aaomidi requested a review from jdkasten February 9, 2024 20:09
@github-actions github-actions bot temporarily deployed to pull request February 9, 2024 20:12 Inactive
Copy link
Collaborator

@jdkasten jdkasten left a comment

Choose a reason for hiding this comment

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

I think this is a great step forward. Happy to land this and do more iterations after as necessary.

draft-ietf-acme-dns-account-01.mkd Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request February 10, 2024 01:56 Inactive
@aaomidi aaomidi merged commit c48a4b2 into main Feb 10, 2024
3 checks passed
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.

4 participants