search

certbot / certbot

Certbot is EFF's tool to obtain certs from Let's Encrypt and (optionally) auto-enable HTTPS on your server. It can also act as a client for any other CA that uses the ACME protocol.
Other
31.4k stars 3.39k forks source link

HTTP header Accept/ContentType #6961

Closed vproot closed 5 years ago

vproot commented 5 years ago

Hi, recently finished development of an Java based ACME server and found one obvious bug (even in the latest version) and one controversial “feature”. The bug is located in certbot/acme/acme/client.py at line 497: content_type = DER_CONTENT_TYPE # TODO: make it a param The problem is that ContentType is bound to request and should be ‘application/jose+json’ and not ‘application/pkix-cert’ as defined at line 45 in the same file. I understand that boulder may ignore ContentType, but correct server implementation should obey the value in this attribute. Accept header tells what client accepts as a response and server can ignore it (by ACME spec) Controversial feature is decision to drop CN in CSR subject. Even though (in theory) everything but signature is optional in CSR and that Chrome and other browser ignores CN, use of the CN is security feature in enterprise environments. Since having CN in CSR Subject attribute doesn’t harm but widen possible acceptance of the ACME in enterprises, I really don’t see the problem having it back, something like: csr.get_subject().CN = domains[0] (after the line 179 in certbot/acme/acme/crypto_util.py)

Just my five cents

bmw commented 5 years ago

The CN issue has been addressed multiple times before (see https://github.com/certbot/certbot/issues/6463 for one example), but thanks for pointing out the problem with the content type.

We'd accept a PR to fix this.

bmw commented 5 years ago

On closer inspection, this issue only affects our client for "ACMEv1" which only supports an unstandardized version of the protocol.

Now that there is a final, standardized version of ACME (see https://tools.ietf.org/html/rfc8555), we're planning on completely removing support for ACMEv1 in the near future (see https://github.com/certbot/certbot/issues/6844) so I'm closing this issue as wontfix.

I'd strongly encourage you to update your ACME server to be in line with the ACME spec.