Skip to content

der: implement a buffered pem reader - #839

Merged
tarcieri merged 5 commits into
RustCrypto:masterfrom
baloo:baloo/buffered-pem-reader
Jan 6, 2023
Merged

der: implement a buffered pem reader#839
tarcieri merged 5 commits into
RustCrypto:masterfrom
baloo:baloo/buffered-pem-reader

Conversation

@baloo

@baloobaloo commented Jan 5, 2023

Copy link
Copy Markdown
Member

This fixes the parsing of x509 certificates from Pem format and brings a test for it.

baloo
baloo commented Jan 5, 2023
View reviewed changes
Comment threadder/src/reader/pem.rs
/// Inner PEM decoder.
decoder: Decoder<'i>,
/// Inner PEM decoder wrapped in a BufReader.
reader: RefCell<utils::BufReader<'i>>,

@baloobalooJan 5, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don't see how there could ever be two instances of borrow for this refcell and I think this is safe

baloo
baloo commented Jan 5, 2023
View reviewed changes
Comment threadder/src/reader/pem.rs
baloo
baloo commented Jan 5, 2023
View reviewed changes
Comment threadder/src/reader/pem.rs
@baloo
baloo marked this pull request as draft January 5, 2023 00:07
@baloo

baloo commented Jan 5, 2023

Copy link
Copy Markdown
MemberAuthor

oooh, fun! I triggered the fuzzer!

@tarcieri

tarcieri commented Jan 5, 2023

Copy link
Copy Markdown
Member

@baloo looking at this so far it feels like a lot of indirection. You have PemReader wrapping BufReader wrapping pem_rfc7468::Decoder (itself another wrapper around a buffered reader). And I'm not sure why you need RefCell at all?

It feels to me like PemReader should own an e.g. Buffer type, and ideally never use it unless a peek operation has been previously performed, which is the only thing which fills the buffer.

@baloo

baloo commented Jan 5, 2023

Copy link
Copy Markdown
MemberAuthor

Yeah, it could very well be,
The refcell is because of the Decode::peek_header where I don't have mutability and because of its nature, I can't just assume I have stored enough in the buffer already.

tarcieri
tarcieri reviewed Jan 5, 2023
View reviewed changes
Comment threadder/src/reader/pem.rs Outdated
@baloo

baloo commented Jan 5, 2023

Copy link
Copy Markdown
MemberAuthor

I'd be easier if I was to just base64 decode the whole payload and have that in a buffer. Any opinion?

baloo
baloo commented Jan 5, 2023
View reviewed changes
Comment threadder/src/asn1/integer.rs Outdated
@baloo

baloo commented Jan 5, 2023

Copy link
Copy Markdown
MemberAuthor

I'd be easier if I was to just base64 decode the whole payload and have that in a buffer. Any opinion?

pem feature pulls alloc. I think that flies. I'm not aware of any pem with unlimited size.

@tarcieri

tarcieri commented Jan 5, 2023

Copy link
Copy Markdown
Member

I'd be easier if I was to just base64 decode the whole payload and have that in a buffer. Any opinion?

That's how the existing Document types work for decoding PEM, so you'd just be reinventing the wheel.

pem feature pulls alloc. I think that flies. I'm not aware of any pem with unlimited size.

They do that because they use the Document type.

If this feature is implemented correctly, it can eliminate the alloc dependency.

The ssh-key supports heapless PEM decoding and encoding this way.

@baloo
balooforce-pushed the baloo/buffered-pem-reader branch from dc87e09 to 2fc939bCompareJanuary 5, 2023 04:24
@baloo
balooforce-pushed the baloo/buffered-pem-reader branch from 2fc939b to c73c449CompareJanuary 5, 2023 04:39
@baloo
baloo marked this pull request as ready for review January 5, 2023 04:39
tarcieri
tarcieri approved these changes Jan 6, 2023
View reviewed changes

@tarcieritarcieri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great now

@tarcieri
tarcieri merged commit 4f027c7 into RustCrypto:masterJan 6, 2023
@baloo
baloo deleted the baloo/buffered-pem-reader branch January 6, 2023 18:50
@tarcieritarcieri mentioned this pull request Feb 27, 2023
Merged
tarcieri added a commit that referenced this pull request Aug 17, 2024
@tarcieri
der: PemReader simplifications
a4ce0fd
Removes the internal buffer added in #839 in favor of just cloning the
`PemReader`'s state.
This is a partial step towards #1475 and would greatly simplify the
implementation of #1474.
@tarcieritarcieri mentioned this pull request Aug 17, 2024
Merged
tarcieri added a commit that referenced this pull request Aug 17, 2024
@tarcieri
der: PemReader simplifications
953a879
Removes the internal buffer added in #839 in favor of just cloning the
`PemReader`'s state.
This is a partial step towards #1475 and would greatly simplify the
implementation of #1474.
tarcieri added a commit that referenced this pull request Aug 17, 2024
@tarcieri
der:PemReadersimplifications (#1476)
8279b9d
Removes the internal buffer added in #839 in favor of just cloning the
`PemReader`'s state.
This is a partial step towards #1475 and would greatly simplify the
implementation of #1474.
Also reverts: "der: have `Reader::peek*` methods take `&mut self` (#1418)"
This reverts commit 42ec503.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tarcieritarcieritarcieri approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@baloo@tarcieri