Certificates with indentation fail to parse

[mattmess1221]

Example cert:

-----BEGIN CERTIFICATE-----
 MIIDaTCCAlGgAwIBAgIJAOq/zL+84IswMA0GCSqGSIb3DQEBCwUAMFoxCzAJBgNV
 BAYTAlVTMQswCQYDVQQIDAJOQzEMMAoGA1UEBwwDUlRQMQ8wDQYDVQQKDAZOZXRB
 cHAxDTALBgNVBAsMBEVTSVMxEDAOBgNVBAMMB1NTRk1DQ0EwHhcNMTcxMTAxMjEw
 OTQyWhcNMjcxMDMwMjEwOTQyWjBaMQswCQYDVQQGEwJVUzELMAkGA1UECAwCTkMx
 DDAKBgNVBAcMA1JUUDEPMA0GA1UECgwGTmV0QXBwMQ0wCwYDVQQLDARFU0lTMRAw
 DgYDVQQDDAdTU0ZNQ0NBMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA
 iaD9Ee0Yrdka0I+9GTJBIW/Fp5JU6kyjaxfOldW/R9lEubegXQFhDD2Xi1HZ+fTM
 f224glB9xLJXAHhipRK01C2MgC4kSH75WL1iAiYeOBloExqmK6OCX+sdyO7RXm/H
 Ra9tN2INWdvyO2pnmxsSnq56mCMsUZLtrRKp89FWgcxLg5r8QxH7xwfh5k54rxjE
 144TD9yrIiQOgRSIRHUrVJ9l/F/gnwzP8wcNABeXwN71Mzl7mliPA703kONQIAyU
 0E0tLpmy/U8dZdMmTBZGB7jI9f95Hl1RunfwhR371a6z38kgkvwrLzl4qflfsPjw
 K9n4omNk9rCH9H9tWkxxjwIDAQABozIwMDAdBgNVHQ4EFgQU/bFyCCnqdDFKlQBJ
 ExtV6wcMYkEwDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAOQMs
 Pz2iBD1+3RcSOsahB36WAwPCjgPiXXXpU+Zri11+m6I0Lq+OWtf+YgaQ8ylLmCQd
 0p1wHlYA4qo896SycrhTQfy9GlS/aQqN192k3oBGoJcMIUnGUBGuEvyZ2aDUfkzy
 JUqBe+0KaT7pkvvbRL7VUz34I7ouq9fQIRZ26vUDLTY3KM1n/DXBj3e30GHGMV3K
 NN2twuLXPNjnryfgpliHU1rwV7r1WvrCVn4StjimP2bO5HGqD/SbiYUL2M9LOuLK
 6mqY4OHumYXq3k7CHrvt0FepsN0L14LYEt1LvpPDFWP3SdN4z4KqT9AGqBaJnhhl
 Qiq8GWnAChspdBLxCg==
-----END CERTIFICATE-----

OpenSSL can load it via openssl x509 -in cert.crt -noout -text, so it is valid. Trying to parse this with rustls-pemfile (via rustls-native-certs) creates an error similar to this.

{ kind: InvalidData, error: "Could not load PEM file \"/usr/lib/ssl/certs/ca-certificates.crt\": Invalid byte 32, offset 0." }

[cpu]

Hi there, thanks for filing an issue.

OpenSSL can load it via openssl x509 -in cert.crt -noout -text, so it is valid.

I'm not sure we can draw that conclusion on the basis of one implementation.

I believe RFC 7468 is the authoritative source for the PEM encoding of a certificate and it says (emphasis mine):

Generators MUST wrap the base64-encoded lines so that each line consists of exactly 64 characters except for the final line, which will encode the remainder of the data (within the 64-character line boundary), and they MUST NOT emit extraneous whitespace. Parsers MAY handle other line sizes. These requirements are consistent with PEM [RFC1421].

There is some discussion in 7468 of "extant parsers" allowing looser behaviour, but I believe we intend for this crate to match the "strict format" ABNF. The Security Considerations section emphasizes that variations on handling whitespace and non-base64 characters creates ambiguities.

[ctz]

I believe RFC 7468 is the authoritative source for the PEM encoding of a certificate

Agree. But it also says:

Furthermore, parsers SHOULD ignore whitespace and other non-
base64 characters and MUST handle different newline conventions.

So I think it's reasonable to expect we should parse this correctly.

[cpu]

Furthermore, parsers SHOULD ignore whitespace and other non-base64 characters and MUST handle different newline conventions.

So I think it's reasonable to expect we should parse this correctly.

I saw this as well, but it's a SHOULD for parsers compared to a MUST for encoders, and given the other language about ambiguities I wasn't fully convinced this crate should change. (but I don't feel very strongly)

[zanieb]

It seems pretty reasonable to warn but strip the whitespace — hopefully that's enough to encourage a report to the encoder without blocking users.

[djc]

I saw this as well, but it's a SHOULD for parsers compared to a MUST for encoders, and given the other language about ambiguities I wasn't fully convinced this crate should change. (but I don't feel very strongly)

Given that this crate implements a parser, I think we should align with the SHOULD. I support the argument that the encoder is in the wrong here, but given the SHOULD for parsers it seems unlikely that the security considerations apply to this particular form of deviation.

Logging for "invalid" whitespace seems like an interesting idea, but given that we have no logging so far it means adding a dependency for tracing, and then we might want to make that optional -- not sure the complexity is worth it.

[cpu]

Here's one option to consider: #41 I used killjoy1221's .pem input for the integration test.

[cpu]

This is fixed as of rustls-pemfile v2.1.1

Thanks for reporting the issue.