mholt / acmez

Premier ACME client library for Go
https://pkg.go.dev/github.com/mholt/acmez/v2
Apache License 2.0
281 stars 35 forks source link

Fix `email-reply-00` response generation and add basic test #25

Closed hslatman closed 6 months ago

hslatman commented 6 months ago

Some remarks:

Have successfully tested this with a customized step-ca with support for email-reply-00. The fact that I worked on both the client and server part doesn't guarantee the implementation is correct, so it would be great if @orangepizza or someone else can confirm this implementation is correct.

hslatman commented 6 months ago

I'll see if I can test this with https://acme.castle.cloud/

orangepizza commented 6 months ago

Some remarks:

* This assumes `token-part2` is not necessarily a part of the token that is (raw) base64url encoded, but it does have to use characters from the base64url alphabet. This seems to be in line with other challenges and how ACME clients use the `token` from a JSON response. So, only `token-part1` needs to be decoded, then prefixed to `token-part2` (and the encoded account public key).

* All of the base64url encoding operations are assumed to be raw base64url encodings, meaning no padding. The RFC includes examples _with_ padding characters, but in my opinion these should all be raw base64url encoding, as that's what is used in all other places where ACME uses base64url encoding. I don't think there's a need to encoded with padding inside an email, specifically.

Have successfully tested this with a customized step-ca with support for email-reply-00. The fact that I worked on both the client and server part doesn't guarantee the implementation is correct, so it would be great if @orangepizza or someone else can confirm this implementation is correct.

when I made that code I'm not sure token that concatenates is string form or binary form: if it's string you'd just add it as string: but if we are adding binary arrays than they both need to be decoded as token-part1 may not align with x6 bits so token-part2 encode as string will be misaligned

hslatman commented 6 months ago

Hey @orangepizza, you're right about requiring decoding of token-part2. After checking how https://github.com/polhenarejos/acme_email does things and adding that back in I was able to successfully obtain a certificate from the Castle staging and production servers. I was a bit surprised that it works like that, because the RFC doesn't describe that step explicitly, whereas it does mention to encode token-part1 using base64url (with both token-part1 and token-part2 already consisting of base64url alphabet characters).

Do you know more ACME servers with support for this challenge?

orangepizza commented 6 months ago

github ate the email reply so posted again:

I don't think there are enough implementations to make a consensus and asking the authors is best at this stage: but my justification was part1/2 implies there is a full token at start, and it got split than encoded to part-1 and part-2, and their length were counted in bits(at least 128bit entropy) so it does not align base64 letters. If they were named token1 and token2 I'd use string concatenate of them.

orangepizza commented 6 months ago

@mholt I think it better merged first than patch later if RFC author disagrees, as that thing is most likely implementation server it may used on

mholt commented 6 months ago

Sounds good to me. Thanks both!!