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

remove ring #318

Merged
merged 12 commits into from
Aug 29, 2024
Merged

remove ring #318

merged 12 commits into from
Aug 29, 2024

Conversation

chenzhenjia
Copy link

No description provided.

@Keats
Copy link
Owner

Keats commented Jun 19, 2023

Can you add a CI setup for it?

@myFavShrimp
Copy link

@chenzhenjia Are you still working on that? If not I would like to take over

@chenzhenjia
Copy link
Author

@chenzhenjia你还在做这方面的工作吗?如果没有我想接手

I don't have time right now, if you have time you can take over

@ash-burnt
Copy link

I also need this. @Keats do you have an example of a CI setup? I can take a stab if I have something to work off of

@Keats
Copy link
Owner

Keats commented Sep 29, 2023

I don't have an example, but it should not be too hard to find one looking at Rust projects with wasm support

@Keats
Copy link
Owner

Keats commented Nov 7, 2023

I added one in #345.

I'm now wondering if we should just ditch ring vs using the pure rust sha/rsa etc crates entirely?

@Keats
Copy link
Owner

Keats commented Nov 9, 2023

If someone can try a PR that removes ring, that'd be great. I'll do it otherwise but no idea on when

@chenzhenjia chenzhenjia changed the title support wasm remove ring Mar 15, 2024
# Conflicts:
#	Cargo.toml
#	examples/custom_header.rs
#	examples/validation.rs
#	src/crypto/ecdsa.rs
#	src/crypto/rsa.rs
#	src/jwk.rs
@chenzhenjia
Copy link
Author

@Keats The ring has been completely removed

@chenzhenjia
Copy link
Author

@Keats All steps of ci have been successful

@tokarevart
Copy link

tokarevart commented Apr 4, 2024

Running cargo test --examples fails with the following error message (kept only last most useful lines)

...
     Running unittests examples/ed25519.rs (target/debug/examples/ed25519-dcfabe976d67ec15)

running 1 test
test tests::test ... FAILED

failures:

---- tests::test stdout ----
thread 'tests::test' panicked at examples/ed25519.rs:65:18:
called `Result::unwrap()` on an `Err` value: Error(InvalidEddsaKey)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `--example ed25519`

It is a result of this example expecting method DecodingKey::from_ed_der to expect actually DER encoded public key, while this method currently expects raw 32 bytes public key (see issue #362). It means that at least this PR keeps the old behavior in this situation (if supplying raw correct public key still works as it did, with no new bugs, because I didn't test this part).

@chenzhenjia
Copy link
Author

Running cargo test --examples fails with the following error message (kept only last most useful lines)

...
     Running unittests examples/ed25519.rs (target/debug/examples/ed25519-dcfabe976d67ec15)

running 1 test
test tests::test ... FAILED

failures:

---- tests::test stdout ----
thread 'tests::test' panicked at examples/ed25519.rs:65:18:
called `Result::unwrap()` on an `Err` value: Error(InvalidEddsaKey)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `--example ed25519`

It is a result of this example expecting method DecodingKey::from_ed_der to expect actually DER encoded public key, while this method currently expects raw 32 bytes public key (see issue #362). It means that at least this PR keeps the old behavior in this situation (if supplying raw correct public key still works as it did, with no new bugs, because I didn't test this part).

fixed

@chenzhenjia
Copy link
Author

@Keats hi.Any questions about the pull request?

@Keats
Copy link
Owner

Keats commented May 24, 2024

So there is another PR replacing ring: #377

I'm not sure what to do tbh, I don't want multiple backends and I don't think the rust crypto crates are FIPS certified right?

@chenzhenjia
Copy link
Author

So there is another PR replacing ring: #377

I'm not sure what to do tbh, I don't want multiple backends and I don't think the rust crypto crates are FIPS certified right?

  1. Both password and hash functionalities are developed entirely in Rust, without needing C compilation, thus speeding up the compilation process.
  2. The compiled output will be smaller.
  3. More CPU architectures will be supported (ring does not support HarmonyOS, and aws-lc-rs support is uncertain).

Although these libraries are not FIPS certified, their security should not be an issue.

@Keats Keats mentioned this pull request Jun 5, 2024
@Keats
Copy link
Owner

Keats commented Jun 5, 2024

No I completely understand the benefits. It's just that not being FIPS certified means some people can't use the crate at all.
On my end I do prefer this PR, maybe I should ask on /r/rust what people think.

@Keats Keats mentioned this pull request Jul 19, 2024
@birkmose
Copy link

I'm not sure what to do tbh, I don't want multiple backends and I don't think the rust crypto crates are FIPS certified right?

What is the rationale for not supporting multiple backends? Other crates, such as rustls, accommodate multiple cryptography providers via features like aws-lc-rs (with optional FIPS support via the fips feature) and ring. I do understand maintaining support for multiple backends does add to the maintenance burden of this crate, however the flexibility it offers can be beneficial for consuming crates. For instance, if an application is entirely ring based, switching this crate to only support aws-lc-rs could force consumers to ship their binaries with two cryptographic implementations, increasing image size, etc.

@Keats
Copy link
Owner

Keats commented Jul 25, 2024

See #399

If someone wants to do a PR having both, sure

@birkmose
Copy link

I understand. If aws-lc-rs is the only supported crypto provider, could an option to opt out of FIPS support perhaps be provided if it is not needed by the consumer? This would help avoid additional build dependencies like Perl and Go.

@Keats Keats changed the base branch from master to new-backends August 29, 2024 11:09
@Keats Keats merged commit a41c817 into Keats:new-backends Aug 29, 2024
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.

6 participants