add no-std support by japaric · Pull Request #29 · rustls/pemfile

japaric · 2023-11-03T12:41:57Z

this PR adds a new API, read_one_from_slice, which works just like read_one but operates on a slice.

this PR also adds an opt-out "std" Cargo feature. all the API that operates on IO objects is behind the "std" feature. read_one_from_slice is available in no-std mode.

this PR is best reviewed on a commit-by-commit basis

cpu

Thanks! Here's a first pass.

Two higher-level items:

It looks like this breaks cargo test --no-default-features.
Could you add CI coverage for the no-std mode?

src/pemfile.rs

cpu · 2023-11-03T13:41:10Z

src/pemfile.rs

+    Base64Decode(DecodeError),
+}
+
+impl From<DecodeError> for Error {


This leaks the base64 crate into the public API no? I think one advantage of the previous approach was that it kept base64 internal.

Indeed. we can't use the usual Box<dyn std::error::Error> replacement because this enum has to work in no-std context. the alternatives I see are

drop the tuple field, i.e. simply use Error::Base64Decode. this loses information

use Base64Decode(Box<dyn core::fmt::Debug>). that trait object is not super common but it's close to Box<dyn Error>. however, it cannot be converted into Box<dyn Error> as there's no From / Into trait that glues the two trait object types

use Base64Decode(String) where the String is the format!-ed version of Base64Decode. unlike Box<dyn Debug> this can be converted into Box<dyn Error> using the ? operator so that's a plus

basically copy base64::DecodeError into this crate and use that. that does not expose the base64 dependency but then you have to maintain a function that maps base64::DecodeError into pemfile::DecodeError and update it if base64::DecodeError (which is not marked as non_exhaustive) ever changes.

@ctz and I yesterday were chatting that we might want to yeet the use of the base64 crate at the first sign of trouble (since we generally haven't been that happy with it) -- maybe that's this?

use Base64Decode(String) where the String is the format!-ed version of Base64Decode. unlike Box this can be converted into Box using the ? operator so that's a plus

That option seems OK to me. I suspect there's not much value in downstream code being able to programmatically determine if the data was invalid for padding, length, unknown symbol etc. In most cases I expect the error is going to be displayed to a user in string form 🤔

In most cases I expect the error is going to be displayed to a user in string form

I agree. this is not the case where knowing the exact error cause is going to let the user fix the input and retry the operation. most likely, malformed pem files are going to be skipped or result in a fatal error.

src/pemfile.rs

src/tests.rs

src/pemfile.rs

japaric · 2023-11-03T14:31:13Z

@cpu thanks for the review. I have addressed your comments in new commits to ease the review process. I can squash the commits when you are all happy with the final version

japaric · 2023-11-03T14:38:00Z

I forgot to explicit answer to this:

It looks like this breaks cargo test --no-default-features.

given that the -std API is a strict subset of the +std API I don't think running cargo t --no-default-features in addition to cargo t increases coverage. given how the tests are written, I would have to cfg away the whole tests module to make cargo t --no-default-features work. or would you like to split all the existing tests in a _slice and _io version and run only the _slice ones in -std mode?

Could you add CI coverage for the no-std mode?

done

cpu · 2023-11-03T15:18:03Z

thanks for the review. I have addressed your comments in new commits to ease the review process. I can squash the commits when you are all happy with the final version

SGTM! Thank you.

given that the -std API is a strict subset of the +std API I don't think running cargo t --no-default-features in addition to cargo t increases coverage. given how the tests are written, I would have to cfg away the whole tests module to make cargo t --no-default-features work.

From my perspective I think it's less about increasing coverage and more that I found it surprising that cargo test --no-default-features doesn't build. I expect that could confuse folks into thinking it's a bug. Gating the tests module so it doesn't fail to build seems preferable to me, even if it may mask that little testing is actually being done in that configuration. WDYT?

japaric · 2023-11-06T13:36:40Z

I found it surprising that cargo test --no-default-features doesn't build.

I've pushed a commit to make cargo t --no-default-features compile.

I think the expectation from the point of view of the contributor is that cargo test is the only thing they need to run. the reality is that aside from small library crates, project usually have complex CI that runs more than just cargo test so if one wants to pass CI locally one will have to read the .yml file and copy/paste run the commands therein. it's very few projects that take the effort of making the entire CI easy to run locally using, for example, something like an xtask.

cpu

Thanks! Looks good to me once the From trait is fixed up.

src/pemfile.rs

japaric · 2023-11-08T10:43:34Z

I've squashed the commits

djc · 2023-11-09T09:32:16Z

I think this the no-std commit has too much going on right now. IMO it would be good to have separate commits that:

Split read_one_impl() out from read_one()
Introduce the new Error type
Rename the base64 module (why is this necessary?)
Introduce read_one_from_slice()

Additionally, I find the current order of items pretty confusing. Currently the order is kind of bottom-up (contra our conventions in other crates), but this PR then adds read_one_impl() at the end instead of at the top which would be more consistent.

to make all non-core imports explicit

japaric · 2023-11-09T12:46:12Z

Split the commits.

Rename the base64 module (why is this necessary?)

in a previous version it was masking the base64 dependency and making a use base64::Error import fail. does not seem necessary after removing base64::Error from the Error::Base64Decode variant

japaric · 2023-11-09T14:29:48Z

I dropped the bit that disabled base64's "std" feature while splitting the commit (and CI still passed). I have updated CI (in the latest commit) to catch usage of libstd in dependencies.

djc

Thanks for splitting, this is much better!

src/pemfile.rs

japaric · 2023-11-09T17:43:43Z

@djc applied your suggestions

cpu reviewed Nov 3, 2023

View reviewed changes

djc reviewed Nov 3, 2023

View reviewed changes

src/pemfile.rs Show resolved Hide resolved

cpu approved these changes Nov 6, 2023

View reviewed changes

src/pemfile.rs Outdated Show resolved Hide resolved

src/pemfile.rs Show resolved Hide resolved

complexspaces reviewed Nov 8, 2023

View reviewed changes

src/pemfile.rs Show resolved Hide resolved

complexspaces mentioned this pull request Nov 8, 2023

Allow additionally parsing \r newlines in PEM files #30

Merged

japaric force-pushed the no-std-support branch from 814ee06 to d0a0c67 Compare November 8, 2023 10:42

switch from std::prelude to core::prelude

5345328

to make all non-core imports explicit

japaric force-pushed the no-std-support branch from d0a0c67 to 2f674dd Compare November 9, 2023 12:44

japaric force-pushed the no-std-support branch from 2f674dd to 089237b Compare November 9, 2023 14:28

djc approved these changes Nov 9, 2023

View reviewed changes

src/pemfile.rs Outdated Show resolved Hide resolved

src/pemfile.rs Outdated Show resolved Hide resolved

japaric added 4 commits November 9, 2023 18:41

refactor read_one's loop body into a separate function

31b5048

introduce an Error enum

3e4add6

add read_one_from_slice

74190cc

add an opt-out std feature

7bc284f

japaric force-pushed the no-std-support branch from 089237b to 7bc284f Compare November 9, 2023 17:43

djc merged commit 3aa829c into rustls:main Nov 10, 2023

japaric deleted the no-std-support branch November 10, 2023 12:48

Conversation

japaric commented Nov 3, 2023

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cpu Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

japaric Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

djc Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

cpu Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

japaric Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

japaric commented Nov 3, 2023

Uh oh!

japaric commented Nov 3, 2023

Uh oh!

cpu commented Nov 3, 2023

Uh oh!

japaric commented Nov 6, 2023

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

japaric commented Nov 8, 2023

Uh oh!

djc commented Nov 9, 2023

Uh oh!

japaric commented Nov 9, 2023

Uh oh!

japaric commented Nov 9, 2023

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

japaric commented Nov 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants