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

[3/3] no-std support phase I #1502

Merged
merged 27 commits into from
Feb 27, 2024
Merged

[3/3] no-std support phase I #1502

merged 27 commits into from
Feb 27, 2024

Conversation

japaric
Copy link
Contributor

@japaric japaric commented Sep 29, 2023

this PR implements phase I of RFC #1399 . NOTE that this PR is built on top of PR #1583

originally phase I aimed to only allowing client functionality in no-std context but turns out that the changes were sufficient to also enable server functionality as evidenced in #1534

breaking changes: implementing the RFC requires no breaking changes

blockers:

this PR is best reviewed on a commit by commit basis (do skip PR#1583 commits)

@japaric japaric mentioned this pull request Sep 29, 2023
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: Patch coverage is 85.90164% with 129 lines in your changes are missing coverage. Please review.

Project coverage is 95.96%. Comparing base (a31c5da) to head (2444558).

Files Patch % Lines
rustls/src/quic.rs 61.32% 70 Missing ⚠️
rustls/src/conn.rs 85.27% 19 Missing ⚠️
rustls/src/server/server_conn.rs 88.99% 12 Missing ⚠️
rustls/src/client/handy.rs 94.56% 5 Missing ⚠️
rustls/src/server/handy.rs 95.65% 5 Missing ⚠️
rustls/src/webpki/mod.rs 44.44% 5 Missing ⚠️
rustls/src/error.rs 87.87% 4 Missing ⚠️
rustls/src/msgs/deframer.rs 90.24% 4 Missing ⚠️
rustls/src/client/tls12.rs 71.42% 2 Missing ⚠️
rustls/src/common_state.rs 93.93% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1502      +/-   ##
==========================================
- Coverage   95.97%   95.96%   -0.02%     
==========================================
  Files          85       86       +1     
  Lines       18727    18818      +91     
==========================================
+ Hits        17974    18059      +85     
- Misses        753      759       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ctz
Copy link
Member

ctz commented Sep 29, 2023

  • pki_types::UnixTime will need to gain a constructor so that no_std users can implement the GetCurrentTime trait

I think this exists and is UnixTime::since_unix_epoch(core::time::Duration)

@djc
Copy link
Member

djc commented Oct 2, 2023

I think this would be much easier to digest if it was split into smaller commits, for example:

  • One commit for importing Vec explicitly
  • One commit for TimeProvider
  • One commit for dealing with the cache
  • One commit for compiling out all the server stuff

@japaric
Copy link
Contributor Author

japaric commented Oct 5, 2023

the PR has been split in commits to make it easier to review. all the "rm (..)" commits can be squashed at a later time. also, there are several warning that I haven't fixed and they involve cfg-ing away more code. for now, I have only removed what's need to make rustls compile with #![no_std]

@djc
Copy link
Member

djc commented Oct 6, 2023

I think the rm * commits would actually be nice to keep, assuming each can pass cargo check on its own? Would make it a lot easier to review!

@japaric
Copy link
Contributor Author

japaric commented Oct 6, 2023

I think the rm * commits would actually be nice to keep, assuming each can pass cargo check on its own?

I reordered the commits a bit and now only these contiguous commits:

  • add std feature
  • no-std: remove field from *Error::Other variants
  • no-std: add TimeProvider to ClientConfig
  • no-std: rm TicketSwitcher
  • no-std: add TimeProvider to ServerConfig

make cargo b --no-default-features fail. cargo b works on all the commits of this PR.


The first commit of this PR basically adds this to src/lib.rs

#![no_std]
extern crate std;

this switches the prelude from std::prelude to core::prelude and forces one to explicitly import anything that's not in core, that is mainly alloc API.

I think it would be worthwhile to land that change in a separate PR ahead of this one: it'll prevent relying on implicit alloc imports so all future PRs will have to include explicit alloc imports to avoid breaking the build. That would not only prevent this PR from bitrotting but I think it would help maintain no-std support long term. I'll submit that commit as a separate PR with a fleshed out rationale.

@japaric
Copy link
Contributor Author

japaric commented Oct 6, 2023

I think it would be worthwhile to land that change in a separate PR ahead of this one

extracted that into PR #1524

@japaric
Copy link
Contributor Author

japaric commented Oct 12, 2023

quick update: I have rebased this PR now that #1524 has been merged. I have also pushed a commit that makes cargo b --no-default-features --features ring work on Linux. however, I tried to build rustls with that set of features (-std +ring) for the x86_64-unknown-none target and it failed to compile.

turns out that ring has this sealed SecureRandom trait (note that there are two of them in the ring codebase; one is public and the other is not) that's implemented for SystemRandom but only for a known list of OSes. as "OS agnostic" targets like x86_64-unknown-none have os = "none" in their target specification, SystemRandom lacks the SecureRandom trait and that causes problems in the rustls codebase: RsaSigner, KxGroup, etc.

for more context: I'm building a small rustls no-std demo that only depends on libc bindings and should work on any OS that has POSIX layer as way to exercise this PR. I'm running the demo on Linux so I do have a "secure random" getrandom implementation but I can't signal that to ring. I'll explore using an alternative crypto provider as showcased in #1405.

@djc
Copy link
Member

djc commented Oct 13, 2023

@japaric probably also good to open a ring issue to signal this there and see if there is a better solution there. (Please link the issue here if you do so.)

@japaric
Copy link
Contributor Author

japaric commented Oct 13, 2023

@djc done. submitted briansmith/ring#1734

@japaric
Copy link
Contributor Author

japaric commented Nov 14, 2023

rebased this on top of #1583 and marked it as ready for review

@japaric japaric changed the title [draft implementation] add no-std support no-std support phase I Nov 14, 2023
japaric and others added 24 commits February 27, 2024 13:44
will be back in phase II
no-std users must use the new ServerConfig::builder_with_details(),
which requires a `TimeProvider` to be provided up-front.

For std builds, the `ServerConfig` uses the `DefaultTimeProvider`
for existing, other `ServerConfig::builder*` functions.
default Resumption to disabled
will be back in phase II
When built w/ the `std` feature, the `DEFAULT_CRYPTO_PROVIDER` and
assoc. fns use a standard `once_cell::sync::OnceCell`. When built w/o
the `std` feature we use `once_cell::race::OnceBox`, requiring some
minor adjustments to account for the expectation of storing
a `Box<Arc<CryptoProvider>>` in place of a `Arc<CryptoProvider>`.
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks for landing those last tweaks @ctz

@cpu cpu added this pull request to the merge queue Feb 27, 2024
Merged via the queue into rustls:main with commit 7770f2b Feb 27, 2024
23 of 24 checks passed
@tshepang tshepang deleted the no-std-support branch February 27, 2024 16:57
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.

None yet

7 participants