Remove core_io dependency and add alternative interface for no_std environments #211

DrTobe · 2022-10-12T07:40:41Z

As already stated in #210, core_io "has not been updated for a long time, the latest rust version it supports is 2021-03-25." Since no_std usage of rust-mbedtls currently depends on core_io, this is a major barrier when adopting rust-mbedtls in no_std environments.

As opposed to #210, I propose to remove any std::io replacement altogether because we need a different interface (non io::Write, non io::Read) for DTLS anyways. So this interface can be used for TLS in no_std environments, too, which removes the necessity to have a std::io replacement at all. If you think the benefits are worth the effort, a std::io replacement could be added optionally with a feature. I have not had a detailed look at core2 but I found working with core_io very limiting, so maybe core2 could be a better option.

When working on these changes, I have also fixed some other minor issues with no_std usage. Specifically, this should also fix #208 (making #209 obsolete) while bringing rust-mbedtls to 32 bit targets (specifically the ARM thumb... targets commonly found on MCUs).

With the changes presented in this PR, I was finally able to successfully use rust-mbedtls on an STM32L452CCUx MCU.

…d writing without implementing std::io::{Read, Write} traits

…g IoCallback implements those traits, too

…th Write and Read trait

…ize_of to reduce architecture dependency

DrTobe · 2022-11-07T08:38:33Z

@zugzwang Would you mind having a look at this or find another maintainer who could find the time?

zugzwang · 2022-11-11T09:03:15Z

Hello @DrTobe, thanks for the PR!
We will shortly be in touch, please allow some days.

zugzwang · 2022-11-21T11:25:27Z

mbedtls/src/cipher/raw/serde.rs

@@ -341,20 +341,20 @@ unsafe impl BytesSerde for gcm_context {}

 // If the C API changes, the serde implementation needs to be reviewed for correctness.

-unsafe fn _check_cipher_context_t_size(ctx: cipher_context_t) -> [u8; 96] {
+unsafe fn _check_cipher_context_t_size(ctx: cipher_context_t) -> [u8; size_of::<cipher_context_t>()] {


I think we need to use constants here, as compilation of these functions alerts us of changes in these objects.

E.g. if for some reason cipher_context_t changes size, then this will result in a compilation failure. However using cipher_context_t against its own size will always work.

Does this make sense?

Ah yes, I see, this definitely makes sense. But the value of that constant must be platform-dependent. So, it should roughly be x + y * size_of::<usize>() I guess?

I don't think so, all these structs fields are only dependent on crypto (see e.g. https://github.com/Mbed-TLS/mbedtls/blob/development/include/mbedtls/cipher.h#L319 for cipher_context_t). E.g. any platform should serialize a cipher_context_t in exactly 96 bytes.

(Orthogonally, I do think that these constants should be declared separately e.g. const CIPHER_CONTEXT_T_SIZE: usize = 96;)

Okay, I haven't checked this specific check in detail but I have changed it because some of the checks failed when building for thumbv7em-none-eabihf so I just assumed now this would be due to different usize sizes. I will take a look at this again soon.

Oh, different usize sizes? I thought usize was always 8 bytes. If this is not the case, you are right, we should adjust by target (and perhaps add some sanity check test that checks that serialized objects are really exactly 96, 288, 128, etc bytes, if possible).

If this is not the case, we should check which of the functions compilation is failing and why exactly.

usize is the type that comes to my mind when talking about different sizes in memory because it is

The pointer-sized unsigned integer type.

The size of this primitive is how many bytes it takes to reference any location in memory. For example, on a 32 bit target, this is 4 bytes and on a 64 bit target, this is 8 bytes.

See https://doc.rust-lang.org/std/primitive.usize.html

Thanks, so let us adjust those constants according to the target.

I had a look at the underlying C structs and calculated the expected sizes. On a 64 bit machine, this works. I still have to test this when building for a 32 bit target.

With the latest commit, this also works for 32 bit targets 👍

…fields

jyao1 · 2023-01-05T02:45:29Z

Hello, this seems good solution to replace #210.
We do need make it work in no_std environment with latest rust toolchain.

@zugzwang, Any chance to merge ?

jyao1 · 2023-01-16T01:34:36Z

@MihirLuthra, any chance to merge this one?

zugzwang

Thanks for the PR. Looks good to me.

mbedtls/src/cipher/raw/serde.rs

zugzwang · 2023-01-16T10:28:11Z

@DrTobe do you think that we can add tests for these changes? (apart from the adapted the client_server test?)

jyao1 · 2023-02-13T02:24:48Z

Ping!

What is the status of this PR?

DrTobe · 2023-02-15T07:39:48Z

Ping!

What is the status of this PR?

Sorry, I have not found the time yet to look into @zugzwang's request to add tests.

DrTobe · 2023-02-17T14:50:58Z

@DrTobe do you think that we can add tests for these changes? (apart from the adapted the client_server test?)

I have had a look at this again and to me, the adaption in the client_server test looks just right because it exactly tests the changes made in this PR. What other test are you imagining?

zugzwang · 2023-02-21T17:48:29Z

@DrTobe it was more of a question in case you thought more tests could be added.
Thanks for the PR!

zugzwang · 2023-02-21T17:49:51Z

bors r+

211: Remove core_io dependency and add alternative interface for no_std environments r=zugzwang a=DrTobe As already stated in #210, `core_io` "has not been updated for a long time, the latest rust version it supports is 2021-03-25." Since `no_std` usage of `rust-mbedtls` currently depends on `core_io`, this is a major barrier when adopting `rust-mbedtls` in `no_std` environments. As opposed to #210, I propose to remove any `std::io` replacement altogether because we need a different interface (non `io::Write`, non `io::Read`) for DTLS anyways. So this interface can be used for TLS in `no_std` environments, too, which removes the necessity to have a `std::io` replacement at all. If you think the benefits are worth the effort, a `std::io` replacement could be added optionally with a feature. I have not had a detailed look at `core2` but I found working with `core_io` very limiting, so maybe `core2` could be a better option. When working on these changes, I have also fixed some other minor issues with `no_std` usage. Specifically, this should also fix #208 (making #209 obsolete) while bringing `rust-mbedtls` to 32 bit targets (specifically the ARM `thumb...` targets commonly found on MCUs). With the changes presented in this PR, I was finally able to successfully use `rust-mbedtls` on an STM32L452CCUx MCU. Co-authored-by: Tobias Naumann <[email protected]>

bors · 2023-02-21T18:14:23Z

Build failed:

continuous-integration/travis-ci/push

DrTobe · 2023-03-07T12:03:01Z

The tests failed because the verify_chain test uses certificates which expired last december. I have included the callback which removes the CERT_EXPIRED flag to every verification attempt. This seems to be the right thing to do because the test results should not depend on the current time.

Additionally, the test did the same checks twice. I have removed that duplication.

@zugzwang Assuming that the checks succeed now, can you try to merge again?

zugzwang · 2023-03-07T12:26:33Z

bors r+

bors · 2023-03-07T13:21:26Z

Build succeeded:

continuous-integration/travis-ci/push

jethrogb · 2023-03-20T20:34:06Z

We should add a safe abstraction for IoCallback

237: Add a new IO abstraction r=Taowyoo a=jethrogb Implement an IO abstraction as [previously suggested](#211 (comment)) to replace Read+Write if you don't have that available. This allows you to layer any `Io` types like you would Read+Write. cc `@DrTobe` hope you can live with the API change. Co-authored-by: Jethro Beekman <[email protected]>

DrTobe added 9 commits October 11, 2022 15:15

Remove core_io dependency and offer an alternative API for reading an…

0b7ad90

…d writing without implementing std::io::{Read, Write} traits

Only implement std::io::{Read, Write} for Context<T> if the underlyin…

b2894fc

…g IoCallback implements those traits, too

Remove core_io dependency from unittests in context.rs

170479e

Rename Context::write and read to send and recv to avoid ambiguity wi…

46d082c

…th Write and Read trait

Disable the 'std' feature of the byteorder crate

1f5f2b3

Replace hard-coded serialization sizes by const calls to core::mem::s…

5027a7c

…ize_of to reduce architecture dependency

Fix the cmake build for Cortex-M (e.g. thumbv6m-none-eabi) targets

0e23d99

Fix client_server test

5857f45

Merge branch master into remove-core-io to update pull request

abc473a

zugzwang reviewed Nov 21, 2022

View reviewed changes

DrTobe added 2 commits November 23, 2022 16:18

Calculate the expected serialized sizes from the underlying c-struct …

612e36c

…fields

Consider 8-byte-alignment of gcm_context

5c112b7

DrTobe force-pushed the remove-core-io branch from 3854aea to 5c112b7 Compare November 24, 2022 15:18

MihirLuthra mentioned this pull request Nov 24, 2022

Replace crate core_io with core2 to support more rust toolchain versions #210

Closed

zugzwang approved these changes Jan 16, 2023

View reviewed changes

mbedtls/src/cipher/raw/serde.rs Show resolved Hide resolved

DrTobe added 2 commits March 6, 2023 11:51

Merge branch 'master' into remove-core-io

5719241

Fix verify_chain test

cbb542d

bors bot merged commit 2879a2a into fortanix:master Mar 7, 2023

This was referenced Mar 10, 2023

Update expired certs for testing #222

Closed

How to link rust-mbedtls to a no_std project? #220

Closed

Update to mbedtls-sys 2.28, update test keys, and make sure async tests are run #225

Merged

This was referenced Mar 20, 2023

INTENT TO ARCHIVE jethrogb/rust-core_io#38

Open

Add a new IO abstraction #237

Merged

Remove core_io dependency and add alternative interface for no_std environments #211

Remove core_io dependency and add alternative interface for no_std environments #211

Uh oh!

Conversation

DrTobe commented Oct 12, 2022

Uh oh!

DrTobe commented Nov 7, 2022

Uh oh!

zugzwang commented Nov 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zugzwang Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyao1 commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyao1 commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zugzwang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zugzwang commented Jan 16, 2023

Uh oh!

jyao1 commented Feb 13, 2023

Uh oh!

DrTobe commented Feb 15, 2023

Uh oh!

DrTobe commented Feb 17, 2023

Uh oh!

zugzwang commented Feb 21, 2023

Uh oh!

zugzwang commented Feb 21, 2023

Uh oh!

bors bot commented Feb 21, 2023

Uh oh!

DrTobe commented Mar 7, 2023

Uh oh!

zugzwang commented Mar 7, 2023

Uh oh!

bors bot commented Mar 7, 2023

Uh oh!

jethrogb commented Mar 20, 2023

Uh oh!

Uh oh!

zugzwang Nov 22, 2022 •

edited

Loading

jyao1 commented Jan 5, 2023 •

edited

Loading

jyao1 commented Jan 16, 2023 •

edited

Loading