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

Use application/pem-certificate-chain for PEMs #13927

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

cipherboy
Copy link
Contributor

As mentioned in #10948, it appears we're incorrectly using the
application/pkix-cert media type for PEM blobs, when
application/x-pem-file is more appropriate. Per RFC 5280 Section
4.2.1.13, application/pkix-crl is only appropriate when the CRL is in
DER form. Likewise, Section 4.2.2.1 states that application/pkix-cert
is only applicable when a single DER certificate is used.

Per recommendation in RFC 8555 ("ACME"), Section 7.4.2 and 9.1, we use
the newer application/pem-certificate-chain media type for
certificates. However, this is not applicable for CRLs, so we use fall
back to application/x-pem-file for these. Notably, no official IETF
source is present for the latter. On the OpenSSL PKI tutorial
(https://pki-tutorial.readthedocs.io/en/latest/mime.html), this type is
cited as coming from S/MIME's predecessor, PEM, but neither of the main
PEM RFCs (RFC 934, 1421, 1422, 1423, or 1424) mention this type.

Signed-off-by: Alexander Scheel <[email protected]>


While ietf-wg-acme/acme#120 (comment) seems to suggest application/x-pem-file is appropriate for PEM blobs in general (regardless of type), they ended up removing this in the final draft and introducing a new type.

As mentioned in hashicorp#10948, it appears we're incorrectly using the
`application/pkix-cert` media type for PEM blobs, when
`application/x-pem-file` is more appropriate. Per RFC 5280 Section
4.2.1.13, `application/pkix-crl` is only appropriate when the CRL is in
DER form. Likewise, Section 4.2.2.1 states that `application/pkix-cert`
is only applicable when a single DER certificate is used.

Per recommendation in RFC 8555 ("ACME"), Section 7.4.2 and 9.1, we use
the newer `application/pem-certificate-chain` media type for
certificates. However, this is not applicable for CRLs, so we use fall
back to `application/x-pem-file` for these. Notably, no official IETF
source is present for the latter. On the OpenSSL PKI tutorial
(https://pki-tutorial.readthedocs.io/en/latest/mime.html), this type is
cited as coming from S/MIME's predecessor, PEM, but neither of the main
PEM RFCs (RFC 934, 1421, 1422, 1423, or 1424) mention this type.

Signed-off-by: Alexander Scheel <[email protected]>
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 7, 2022 15:54 Inactive
Signed-off-by: Alexander Scheel <[email protected]>
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 7, 2022 15:57 Inactive
Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

Looks good to me, but another pair of eyes would be a good thing.

@cipherboy
Copy link
Contributor Author

@abriening what's your thoughts on using application/pem-certificate-chain instead of application/x-pem-file?

For what its worth, Dogtag produces application/x-pem-file: https://github.com/dogtagpki/pki/blob/master/base/common/src/main/java/com/netscape/certsrv/authority/AuthorityResource.java#L42-L45 but respects the ACME standard application/pem-certificate-chain: https://github.com/dogtagpki/pki/blob/master/base/acme/src/main/java/org/dogtagpki/acme/server/ACMECertificateService.java#L35-L39

Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

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

This makes sense to me :)

(I'm following the slack chat, but I think it's very unlikely that someone is relying on an incorrect contentType)

@abriening
Copy link
Contributor

I've used the application/pkix-* content-types in PKI testing. I only added the raw/pem endpoint to have parity with the existing raw(pem) endpoint.

Looks like ACME draft was application/x-pem-file then created application/pem-certificate-chain specifically:
ietf-wg-acme/acme@709a654

Seems like application/pem-certificate-chain is appropriate, just hope no one sends Accept: application/pem-certificate-chain expecting the full chain.

@cipherboy
Copy link
Contributor Author

@abriening Thanks! For what it is worth, it appears that Vault doesn't respect (or check) the Accept header either way, so in that regard we'll at least be consistent with other APIs.

That said, we have approvals and consensus internally that we're fine with this change (but not backporting it), so I think it is OK to merge.

Thanks all for the reviews!

@cipherboy cipherboy merged commit f267c3a into hashicorp:main Feb 8, 2022
fairclothjm pushed a commit that referenced this pull request Feb 12, 2022
* Use application/pem-certificate-chain for PEMs

As mentioned in #10948, it appears we're incorrectly using the
`application/pkix-cert` media type for PEM blobs, when
`application/x-pem-file` is more appropriate. Per RFC 5280 Section
4.2.1.13, `application/pkix-crl` is only appropriate when the CRL is in
DER form. Likewise, Section 4.2.2.1 states that `application/pkix-cert`
is only applicable when a single DER certificate is used.

Per recommendation in RFC 8555 ("ACME"), Section 7.4.2 and 9.1, we use
the newer `application/pem-certificate-chain` media type for
certificates. However, this is not applicable for CRLs, so we use fall
back to `application/x-pem-file` for these. Notably, no official IETF
source is present for the latter. On the OpenSSL PKI tutorial
(https://pki-tutorial.readthedocs.io/en/latest/mime.html), this type is
cited as coming from S/MIME's predecessor, PEM, but neither of the main
PEM RFCs (RFC 934, 1421, 1422, 1423, or 1424) mention this type.

Signed-off-by: Alexander Scheel <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy added this to the 1.10 milestone Feb 28, 2022
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