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

Fix dns value encoding to be compliant with acme v01 spec #1295

Merged
merged 7 commits into from Jan 4, 2016

Conversation

Projects
None yet
6 participants
@r0ro
Contributor

r0ro commented Dec 18, 2015

"The record provisioned to the DNS is the base64url encoding of this digest"

Fix dns value encoding according to acme v01 spec
"The record provisioned to the DNS is the base64url encoding of this digest"

r0ro referenced this pull request Dec 18, 2015

Richard Barnes

@jsha jsha added the r=jsha label Dec 18, 2015

@jsha

This comment has been minimized.

Contributor

jsha commented Dec 18, 2015

Nice catch, thanks!

@jsha jsha removed the r=jsha label Dec 18, 2015

@jsha

This comment has been minimized.

Contributor

jsha commented Dec 18, 2015

Actually, I realized this problem should be caught by more than just the integration test. Can you check if there's an appropriate unittest for this code path? If so, please fix it to make sure it catches this bug. If not, could you write one?

Thanks,
Jacob

@r0ro

This comment has been minimized.

Contributor

r0ro commented Dec 18, 2015

I think I saw some unit test for dns01 but it was marked as 'coverage' only test since it relies on network data.
I won't be able to work on this before the end of December. When do you plan on updating the staging server ?
I think it's important to merge this before the next update.

Le 18 déc. 2015 à 19:52, Jacob Hoffman-Andrews notifications@github.com a écrit :

Actually, I realized this problem should be caught by more than just the integration test. Can you check if there's an appropriate unittest for this code path? If so, please fix it to make sure it catches this bug. If not, could you write one?

Thanks,
Jacob


Reply to this email directly or view it on GitHub.

@jmhodges

This comment has been minimized.

Contributor

jmhodges commented Dec 18, 2015

Thank for this change! Please write a unit test. It's important for the long health of this project and the DNS challenge itself. Integration tests won't cover all the possibilities now or in the future as the project grows.

On top of that, deploys to production are going to be, at most, very rare and narrow in their purview over the holidays. The DNS challenge is very unlikely to be turned in prod on anytime before January.

But even if that were not the case, it's important for us to take a deliberate, calm path of forward momentum. We're helping encrypt the entire web! A big deal!

Anyway, thanks again for handling this!

@jmhodges

This comment has been minimized.

Contributor

jmhodges commented Dec 18, 2015

Your schedule, of course, is not ours. We may beat you to having time to fix this with a test in place.

@shred shred referenced this pull request Dec 24, 2015

Closed

Test DNS challenge #2

@r0ro

This comment has been minimized.

Contributor

r0ro commented Dec 28, 2015

now with some test

You should update the _acme-challenge.good.bin.coffee TXT record to the value "LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo" to fix the TestDNSValidationLive. It's the expected value for the expectedToken (https://github.com/letsencrypt/boulder/blob/master/va/validation-authority_test.go#L70)

@jmhodges

This comment has been minimized.

Contributor

jmhodges commented Dec 29, 2015

LGTM

Thank you!

@jmhodges jmhodges added the r=jmhodges label Dec 29, 2015

@r0ro

This comment has been minimized.

Contributor

r0ro commented Dec 29, 2015

merged with master

@jmhodges jmhodges added r=jmhodges and removed r=jmhodges labels Dec 30, 2015

@janeczku janeczku referenced this pull request Jan 1, 2016

Merged

Base64 encode dns-01 record #68

@@ -425,7 +426,7 @@ func (va *ValidationAuthorityImpl) validateDNS01(identifier core.AcmeIdentifier,
// Compute the digest of the key authorization file
h := sha256.New()
h.Write([]byte(challenge.KeyAuthorization.String()))
authorizedKeysDigest := hex.EncodeToString(h.Sum(nil))
authorizedKeysDigest := base64.RawURLEncoding.EncodeToString(h.Sum(nil))

This comment has been minimized.

@janeczku

janeczku Jan 1, 2016

base64.RawURLEncoding omits the pad character(s). This should use base64.URLEncoding to be really compliant with the acme spec.

@janeczku

This comment has been minimized.

janeczku commented Jan 1, 2016

According to acme v01 spec the "(t)he record provisioned to the DNS is the base64url encoding of [the SHA-256 digest]."

"base64url" encoding is specified in RFC 4648 section 5 and is subject to the section 3.2 provisions concerning padding of encoded data:

"Implementations MUST include appropriate pad characters at the end of encoded data unless the specification referring to this document explicitly states otherwise."

The acme 01 specification does not state to omit pad characters in the dns value, accordingly a compliant implementation of the dns-01 challenge has to use padded base64url encoded data when provisioning/validating the TXT record.

Since boulder does a byte-to-byte comparison of the encoded data this is critical!

/cc @r0ro @jmhodges

@r0ro

This comment has been minimized.

Contributor

r0ro commented Jan 1, 2016

The spec says:

"For Base64 encoding, we use the variant defined in [RFC7515]. The important features of this encoding are (1) that it uses the URL-safe character set, and (2) that "=" padding characters are stripped."

I think it also applies for dns value

But perhaps the best solution would be to accept both variants ?

Le 1 janv. 2016 à 16:00, Jan B notifications@github.com a écrit :

According to acme v01 spec the "(t)he record provisioned to the DNS is the base64url encoding of [the SHA-256 digest]."

"base64url" encoding is defined in RFC 4648 section 5 and is subject to the section 3.2 provisions concerning padding of encoded data:

"Implementations MUST include appropriate pad characters at the end of encoded data unless the specification referring to this document explicitly states otherwise."

Since the acme 01 specification does not state to omit pad characters in the dns value, a compliant implementation of the dns-01 challenge has to use padded base64url encoded SHA256 digest when provisioning/validating the TXT record.

/cc @r0ro @jmhodges


Reply to this email directly or view it on GitHub.

@jhass

This comment has been minimized.

jhass commented Jan 1, 2016

+1 for being tolerant to both.

@janeczku

This comment has been minimized.

janeczku commented Jan 1, 2016

@r0ro I don't see how that provision applies to encoding of key authorization resources. When read in context it specifies the encoding of JSON message data:

"Since JSON is a text-based format, binary fields are Base64-encoded. For Base64 encoding, we use the variant defined in {{RFC7515}}. The important features of this encoding are (1) that it uses the URL-safe character set, and (2) that "=" padding characters are stripped."

But i see how the spec leaves room for interpretation in that regard. And i agree that the safest solution is to accept both forms.

@jmhodges

This comment has been minimized.

Contributor

jmhodges commented Jan 2, 2016

No, we will only allow the one that lines up with all of the other challenges as implemented.

The spec is in draft and can be clarified and Postel's Law is a quick way to build insecure software.

@r0ro

This comment has been minimized.

Contributor

r0ro commented Jan 4, 2016

so the current pull request is ok ?

@@ -37,6 +37,12 @@ func (mock *DNSResolver) LookupTXT(hostname string) ([]string, error) {
if hostname == "_acme-challenge.servfail.com" {
return nil, fmt.Errorf("SERVFAIL")
}
if hostname == "_acme-challenge.good-dns01.com" {
// base64(sha254("LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0"

This comment has been minimized.

@jsha

jsha Jan 4, 2016

Contributor

*sha256

@jsha

This comment has been minimized.

Contributor

jsha commented Jan 4, 2016

Basically LGTM. There's a small comment typo, and this needs update to latest master anyhow.

@jsha jsha added r=jsha and removed r=jmhodges labels Jan 4, 2016

@jmhodges jmhodges added the r=jmhodges label Jan 4, 2016

jmhodges added a commit that referenced this pull request Jan 4, 2016

Merge pull request #1295 from r0ro/fix-dns-value-enc
Fix dns value encoding to be compliant with acme v01 spec

@jmhodges jmhodges merged commit b4ebd95 into letsencrypt:master Jan 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@r0ro

This comment has been minimized.

Contributor

r0ro commented Jan 4, 2016

Don't forget to update the TXT entry for _acme-challenge.good.bin.coffee to LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo

@r0ro

This comment has been minimized.

Contributor

r0ro commented Jan 21, 2016

_acme-challenge.good.bin.coffee TXT record is still outdated, I don't know who is able to update its value to match the expected value from validation-authority_test.go

@jcjones

This comment has been minimized.

Contributor

jcjones commented Jan 21, 2016

Update is rolling out; sorry I missed this.

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