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

refactor!: Move crypto operations into a trait. #127

Merged

Conversation

chrysn
Copy link
Collaborator

@chrysn chrysn commented Nov 5, 2023

This is a reroll of #103, which has become quite inconvenient to merge.

As a practical difference, the functions that used to be moved into a CryptoExt trait now just take a crypto first argument -- not much of an ergonomics setback, but easier to understand code.

@chrysn chrysn force-pushed the crypto-as-trait-2 branch 2 times, most recently from b95cea2 to 95ddb6d Compare November 5, 2023 00:47
@chrysn
Copy link
Collaborator Author

chrysn commented Nov 5, 2023

There are still quite a few instances of default_crypto() around that eventually should be relegated to tests and example only. The most prominent leftovers are in lib/src/lib.rs that is not generic yet -- but that is probably easier to do once the C API is in the freezer.

@chrysn chrysn force-pushed the crypto-as-trait-2 branch from 95ddb6d to b957e68 Compare November 5, 2023 00:50
@chrysn
Copy link
Collaborator Author

chrysn commented Nov 5, 2023

Ready from my PoV -- please review. This would then be a sensible base for the big typestating bout.

@chrysn chrysn mentioned this pull request Nov 5, 2023
Copy link
Collaborator

@geonnave geonnave left a comment

Choose a reason for hiding this comment

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

Looks good to me, just left a minor comment below.

crypto/edhoc-crypto-cryptocell310-sys/src/lib.rs Outdated Show resolved Hide resolved
@geonnave
Copy link
Collaborator

geonnave commented Nov 7, 2023

Just to note, I am having trouble with installing and running [hax] on my computer, to run against this PR. I am on it, but the build takes time (and fails after a lot o fime). Once I solve that and the fstar generation works, I would be happy to merge this PR.

@geonnave
Copy link
Collaborator

geonnave commented Nov 8, 2023

I was finally able to install and run hax in my mac -- and these changes do not affect hax into fstar.

There are still some errors but they were already present at the previous implementation, i.e., the only complaints are due to the recursive order of some elements in the consts crate. I leave the full output below for reference.

Details

cargo-hax -C -p edhoc-rs -p edhoc-consts --no-default-features --features="crypto-hacspec, ead-none" --release \; into -i '-c_wrapper::** -edhoc_rs::generate_connection_identifier_cbor -edhoc_rs::generate_connection_identifier' fstar

   Compiling edhoc-consts v0.1.0 (/Users/geovane/Developer/inria/dev/edhoc-rs-FORK/consts)
warning: unused import: `super::structs::*`
   --> consts/src/lib.rs:225:9
    |
225 |     use super::structs::*;
    |         ^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unused_imports)]` on by default

thread 'rustc' panicked at 'attempted to read from stolen value: rustc_middle::thir::Thir', cli/driver/src/exporter.rs:142:22
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: The THIR body of item DefId(0:57 ~ edhoc_consts[76f7]::structs::BytesSuites::{constant#0}) was stolen.
       This is not supposed to happen.
       This is a bug in Hax's frontend.
       This is discussed in issue https://github.com/hacspec/hacspec-v2/issues/27.
       Please comment this issue if you see this error message!
  --> consts/src/lib.rs:69:33
   |
69 |     pub type BytesSuites = [u8; SUITES_LEN];
   |                                 ^^^^^^^^^^

thread 'rustc' panicked at 'attempted to read from stolen value: rustc_middle::thir::Thir', cli/driver/src/exporter.rs:142:22
error: The THIR body of item DefId(0:59 ~ edhoc_consts[76f7]::structs::BytesSupportedSuites::{constant#0}) was stolen.
       This is not supposed to happen.
       This is a bug in Hax's frontend.
       This is discussed in issue https://github.com/hacspec/hacspec-v2/issues/27.
       Please comment this issue if you see this error message!
  --> consts/src/lib.rs:70:42
   |
70 |     pub type BytesSupportedSuites = [u8; SUPPORTED_SUITES_LEN];
   |                                          ^^^^^^^^^^^^^^^^^^^^

note: Writing file "/Users/geovane/Developer/inria/dev/edhoc-rs-FORK/consts/proofs/fstar/extraction/Edhoc_consts.fst"

note: Writing file "/Users/geovane/Developer/inria/dev/edhoc-rs-FORK/consts/proofs/fstar/extraction/Edhoc_consts.Cbor.fst"

note: Writing file "/Users/geovane/Developer/inria/dev/edhoc-rs-FORK/consts/proofs/fstar/extraction/Edhoc_consts.Common_edhoc_parsing.fst"

note: Writing file "/Users/geovane/Developer/inria/dev/edhoc-rs-FORK/consts/proofs/fstar/extraction/Edhoc_consts.Consts.fst"

note: Writing file "/Users/geovane/Developer/inria/dev/edhoc-rs-FORK/consts/proofs/fstar/extraction/Edhoc_consts.Helpers.fst"

note: Writing file "/Users/geovane/Developer/inria/dev/edhoc-rs-FORK/consts/proofs/fstar/extraction/Edhoc_consts.Structs.fst"

warning: `edhoc-consts` (lib) generated 1 warning
error: could not compile `edhoc-consts` (lib) due to 2 previous errors; 1 warning emitted

@geonnave
Copy link
Collaborator

geonnave commented Nov 8, 2023

@malishav I am ok with merging this. Do you wish to provide any input?

@geonnave geonnave merged commit 219dd0c into openwsn-berkeley:main Nov 9, 2023
24 checks passed
@chrysn chrysn deleted the crypto-as-trait-2 branch November 10, 2023 10:09
@geonnave geonnave added the enhancement New feature or request label Nov 10, 2023
chrysn added a commit to chrysn-pull-requests/edhoc-rs that referenced this pull request Nov 15, 2023
This allows pushing back the edhoc-crypto ("the default implementation
that is selected statically, making all implementations possible
dependencies") into the dev-dependencies.

Follow-up-for: openwsn-berkeley#127
chrysn added a commit to chrysn-pull-requests/edhoc-rs that referenced this pull request Nov 15, 2023
This allows pushing back the edhoc-crypto ("the default implementation
that is selected statically, making all implementations possible
dependencies") into the dev-dependencies.

The crypto-* features are removed from edhoc-rs; testing depends on
edhoc-crypto being pulled in in parallel to the test, and a feature
selected on that.

Follow-up-for: openwsn-berkeley#127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants