Skip to content

Conversation

@dwhjames
Copy link
Contributor

PEM from RFC 1421 defines header fields
https://datatracker.ietf.org/doc/html/rfc1421.html#section-9
this change detects header fields as an initial line that contains the
colon char.

PEM from RFC 7468 disallows
https://datatracker.ietf.org/doc/html/rfc7468#section-2

Unlike legacy PEM encoding [RFC1421], OpenPGP ASCII armor, and the
OpenSSH key file format, textual encoding does not define or permit
headers to be encoded alongside the data.

originally from RustCrypto/utils#630

PEM from RFC 1421 defines header fields
https://datatracker.ietf.org/doc/html/rfc1421.html#section-9
this change detects header fields as an initial line that contains the
colon char.

PEM from RFC 7468 disallows
https://datatracker.ietf.org/doc/html/rfc7468#section-2

> Unlike legacy PEM encoding [RFC1421], OpenPGP ASCII armor, and the
> OpenSSH key file format, textual encoding does *not* define or permit
> headers to be encoded alongside the data.

originally from RustCrypto/utils#630
@dwhjames
Copy link
Contributor Author

@tarcieri
Copy link
Member

Looks good now, thanks!

@tarcieri tarcieri merged commit 75ef3b5 into RustCrypto:master Sep 14, 2021
@dwhjames
Copy link
Contributor Author

@tarcieri,
There was a follow on change that I was thinking about, but needed some Rust language and library design feedback…

It would be great to be able to return the label from the encapsulation boundary as context in the error cases where the label has been found, but something is wrong in the content.

  1. are you open to that?
  2. if so, what is the convention around ownership in Error types? (&str vs. String in this case) and presumably that has some crossover with the alloc crate feature?

@dwhjames dwhjames deleted the detect-pem-header-fields branch September 14, 2021 16:21
@tarcieri
Copy link
Member

tarcieri commented Sep 14, 2021

Personally I would prefer not to reveal any part of what is potentially a private key.

You can imagine a case where someone accidentally typos a : into an otherwise valid key, and then the error message logs the entire private key (e.g. Ed25519 private keys fit on a single line)

@dwhjames
Copy link
Contributor Author

@tarcieri I mean a case such as

-----BEGIN RSA PRIVATE KEY-----
not base64!
-----END RSA PRIVATE KEY-----

returning Error::Base64("RSA PRIVATE KEY") as opposed to Error::Base64

The alternative is to re-parse for encapsulation boundaries only.

@tarcieri
Copy link
Member

tarcieri commented Sep 14, 2021

While that would probably be OK security-wise, it's also pretty tricky.

As you noted, that would probably require using an owned String type as Rust error types are generally assumed to be 'static, and this crate is designed to work in the absence of an allocator. There's not really a good way to do that.

There are a few hacks you could do (e.g. when alloc isn't available, defining a DummyString ZST that drops the additional context).

dwhjames added a commit to dwhjames/RustCrypto-formats that referenced this pull request Sep 14, 2021
Missed in PR RustCrypto#13

- add test coverage for new cases
- refactor out decoding of encapsulated text
- compromise here on estimating buffer size
  - slightly more overestimation, due to counting whitespace
  - faster due to no two-pass
  - required to ensure that we attempt a base64 decode of the first line
    before chopping the second line
dwhjames added a commit to dwhjames/RustCrypto-formats that referenced this pull request Sep 14, 2021
Missed in PR RustCrypto#13

- add test coverage for new cases
- refactor out decoding of encapsulated text
- compromise here on estimating buffer size
  - slightly more overestimation, due to counting whitespace
  - faster due to no two-pass
  - required to ensure that we attempt a base64 decode of the first line
    before chopping the second line
tarcieri pushed a commit that referenced this pull request Sep 14, 2021
Missed in PR #13

- add test coverage for new cases
- refactor out decoding of encapsulated text
- compromise here on estimating buffer size
  - slightly more overestimation, due to counting whitespace
  - faster due to no two-pass
  - required to ensure that we attempt a base64 decode of the first line
    before chopping the second line
This was referenced Sep 14, 2021
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.

2 participants