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

Use ring v0.17 #391

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Use ring v0.17 #391

merged 5 commits into from
Dec 12, 2023

Conversation

mhchia
Copy link
Collaborator

@mhchia mhchia commented Dec 7, 2023

What's wrong?

ring 0.17 seems to support WASM now. To make tlsn-prover and tlsn-core support WASM, simply upgrade ring to 0.17. I'm sorry I must have done something wrong so I thought it didn't. Thanks @heeckhau for the efforts on this! 🙏

What has been done?

  • Upgrade ring to 0.17 for tlsn-prover and tlsn-core
  • webpki-roots is updated from 0.24 to 0.26 to make it depend on ring 0.17
  • The codebase is slightly changed due to the API changes since ring 0.17

Note

I'm unsure if I modified our code correctly to work with the new API. Please let me know if there is anything wrong 🙏

components/tls/tls-client/src/sign.rs Show resolved Hide resolved
components/tls/tls-client/src/sign.rs Show resolved Hide resolved
"key agreement failed".to_string(),
)),
},
Err(_) => Err(Error::PeerMisbehavedError(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we have a different error here from the one above?

Copy link
Member

@sinui0 sinui0 Dec 7, 2023

Choose a reason for hiding this comment

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

We should be able to just map_err as it was before.

See how rustls handled this API change:

https://github.com/rustls/rustls/blob/3a35efe7dd30850e7a8d103ca3b458d1a4fad0ae/rustls/src/crypto/ring/mod.rs#L177-L203

Copy link
Collaborator Author

@mhchia mhchia Dec 8, 2023

Choose a reason for hiding this comment

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

Thanks for the review! The original

        ring::agreement::agree_ephemeral(self.privkey, &peer_key, f)
            .map_err(|()| Error::PeerMisbehavedError("key agreement failed".to_string()))

fails because f returns Result<T, ()> and thus ring::agreement::agree_ephemeral would return Result<Result<T, ()>, Unspecified>. To make it work, it seems we would need an extra unwrap to make the return value become the type Result<T, Error>? Like the following code snippet

        ring::agreement::agree_ephemeral(self.privkey, &peer_key, f)
            .unwrap().map_err(|_| Error::PeerMisbehavedError("key agreement failed".to_string()))

or should we change the type of f from impl FnOnce(&[u8]) -> Result<T, ()> to impl FnOnce(&[u8]) -> T? or am I understanding it wrong?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is the last thing you mentioned:

    pub(crate) fn complete<T>(self, peer: &[u8], f: impl FnOnce(&[u8]) -> T) -> Result<T, Error> {
        let peer_key = ring::agreement::UnparsedPublicKey::new(self.skxg.agreement_algorithm, peer);
        ring::agreement::agree_ephemeral(self.privkey, &peer_key, f)
            .map_err(|_| Error::PeerMisbehavedError("key agreement failed".to_string()))
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the direction! Changed it in 6c597d3

Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

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

Thanks, I think we just need to fix the error handling and get CI passing.

Copy link
Member

@heeckhau heeckhau left a comment

Choose a reason for hiding this comment

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

Can you check components/tls/tls-client/src/lib.rs
On my system the build complained about outdated example code:

 //! let mut root_store = rustls::RootCertStore::empty();
 //! root_store.add_server_trust_anchors(
 //!     webpki_roots::TLS_SERVER_ROOTS
-//!         .0
 //!         .iter()
 //!         .map(|ta| {
 //!             rustls::OwnedTrustAnchor::from_subject_spki_name_constraints(
-//!                 ta.subject,
-//!                 ta.spki,
-//!                 ta.name_constraints,
+//!                 ta.subject.as_ref().to_owned(),
+//!                ta.subject_public_key_info.as_ref().to_owned(),
+//!                ta.name_constraints
+//!                    .as_ref()
+//!                    .map(|nc| nc.as_ref().to_owned()),
 //!             )

Comment on lines 31 to 33
ta.subject.to_vec(),
ta.subject_public_key_info.to_vec(),
ta.name_constraints.as_ref().map(|nc| nc.to_vec()),
Copy link
Member

Choose a reason for hiding this comment

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

In https://github.com/rustls/pki-types/blob/main/src/lib.rs they use .as_ref().to_owned() instead of to_vec(). Not sure if this is important here:

ta.subject.as_ref().to_owned(),
ta.subject_public_key_info.as_ref().to_owned(),
ta.name_constraints.as_ref().map(|nc| nc.as_ref().to_owned()),

Copy link
Collaborator Author

@mhchia mhchia Dec 12, 2023

Choose a reason for hiding this comment

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

Thanks for the review and the suggestions 🙏

  • Outdated example code is addressed in 041176a
  • I've changed to_vec to as_ref in 94e41fb. It seems we can save a copy. Let me know if I misunderstood anything or did it wrong

Copy link
Member

@heeckhau heeckhau left a comment

Choose a reason for hiding this comment

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

Thanks 🙇

@mhchia
Copy link
Collaborator Author

mhchia commented Dec 12, 2023

@sinui0 I've made some changes to the PR. Could you take a look and let me know if there are any additional modifications needed? Thanks!

Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

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

🙏

@mhchia
Copy link
Collaborator Author

mhchia commented Dec 12, 2023

@heeckhau @sinui0 since I don't seem to have the permission, could you help me merge the PR when you think it's ready? Thanks a lot 🙏

@sinui0 sinui0 merged commit 5fcae87 into tlsnotary:dev Dec 12, 2023
12 checks passed
@mhchia mhchia deleted the feat/use-ring-v0.17 branch December 14, 2023 05:55
heeckhau added a commit that referenced this pull request Dec 15, 2023
heeckhau added a commit that referenced this pull request Dec 15, 2023
heeckhau added a commit that referenced this pull request Dec 18, 2023
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.

3 participants