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

openssl/quictls support #1605

Merged
merged 1 commit into from
Mar 16, 2024
Merged

openssl/quictls support #1605

merged 1 commit into from
Mar 16, 2024

Conversation

brbzull0
Copy link
Contributor

@brbzull0 brbzull0 commented Aug 29, 2023

Apache Traffic Server uses this library for handling the QUIC side of things of H3, as a part of this effort we plan to use quictls as the cryptography library, this is an effort(I work for Yahoo) to support that.


This PR includes all the previous work done in the openssl branch.

Design Notes

As some of the API are different between vendors, I have added two sub-modules to handle the specifics, for both, tls and the crypto module. The specifics of course are coded inside each submodule:
BoringSSL:

  • borinssl_crypto.rs
  • borinssl_tls.rs

OpenSSL/quictls:

  • openssl_quictls_crypto.rs
  • openssl_quictls_tls.rs

Each sub-module will be compiled depending on the feature you use (openssl or boringssl vendor) from the main module(tls, crypto).

Features

0-RTT Is not supported in this PR. It will be added afterwards. This is reflected in the README.

CI

  • we need to work around having openssl/quitls build and let quiche use it.
  • pkg-config seems not to be installed as build-dependency

Building notes for testing.

  • Make sure you have the openssl library in your LD_LIBRARY_PATH and the right path inside the PKG_CONFIG_PATH
  • Add openssl in the cargo --features list

I am using openssl/quictls 3 for this implementation.

@brbzull0 brbzull0 changed the title openssl/quictls support WIP openssl/quictls support WIP (continuation) Aug 29, 2023
@brbzull0 brbzull0 changed the title openssl/quictls support WIP (continuation) openssl/quictls support (continuation) Sep 4, 2023
@brbzull0 brbzull0 force-pushed the openssl-dev-wip branch 3 times, most recently from 4e32bfe to cc31d17 Compare September 12, 2023 09:10
@brbzull0 brbzull0 force-pushed the openssl-dev-wip branch 7 times, most recently from ffc4e32 to 9014cf9 Compare November 13, 2023 11:09
@brbzull0 brbzull0 changed the title openssl/quictls support (continuation) openssl/quictls support Nov 13, 2023
@brbzull0
Copy link
Contributor Author

CI need to include openssl/quictls so we know we are passing all build/tests with the each vendor.

@Eimji
Copy link

Eimji commented Jan 6, 2024

Hello,

I am working on a mobile application which allows to use encrypted DNS (Android for the moment, iOS right after).
I tested quiche with a little proto on Android, it works great (thanks!). So I wanted to integrate quiche into my application to develop DoQ and DoH3, but I got compilation errors due to conflicts with OpenSSL. Yes, I use OpenSSL 3.2 to enable DoT and DoH.

It would be great if we can have a pluggable TLS vendor.

@brbzull0 As OpenSSL 3.2 supports now QUIC (not all features), your PR will support legacy OpenSSL > 3.2 or only quictls? Thanks for your work.

@brbzull0 brbzull0 force-pushed the openssl-dev-wip branch 4 times, most recently from 760c487 to 8294a7a Compare January 11, 2024 11:21
@brbzull0
Copy link
Contributor Author

Hello,

I am working on a mobile application which allows to use encrypted DNS (Android for the moment, iOS right after). I tested quiche with a little proto on Android, it works great (thanks!). So I wanted to integrate quiche into my application to develop DoQ and DoH3, but I got compilation errors due to conflicts with OpenSSL. Yes, I use OpenSSL 3.2 to enable DoT and DoH.

It would be great if we can have a pluggable TLS vendor.

@brbzull0 As OpenSSL 3.2 supports now QUIC (not all features), your PR will support legacy OpenSSL > 3.2 or only quictls? Thanks for your work.

Hello @Eimji . The plan is only quictls for now. Thanks for having a look

@brbzull0 brbzull0 marked this pull request as ready for review January 11, 2024 13:29
@brbzull0 brbzull0 requested a review from a team as a code owner January 11, 2024 13:29
@brbzull0
Copy link
Contributor Author

brbzull0 commented Jan 11, 2024

Making this ready for review so we can gather some input. Thanks.

cc: @ghedo

@brbzull0
Copy link
Contributor Author

brbzull0 commented Mar 4, 2024

I think is worth having this link here, in case we want to discuss about it.

@ghedo ghedo force-pushed the openssl-dev-wip branch 4 times, most recently from ad391cc to 9cd57ae Compare March 12, 2024 16:28
ghedo
ghedo previously approved these changes Mar 12, 2024
Copy link
Contributor

@LPardue LPardue left a comment

Choose a reason for hiding this comment

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

A few nits but overall looks good


// NOTE: This structure is copied from <openssl/aead.h> in order to be able to
// statically allocate it. While it is not often modified upstream, it needs to
// be kept in sync.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add the commit ID in the comment to make tracking more easy?

quiche/src/crypto/mod.rs Outdated Show resolved Hide resolved
quiche/src/h3/mod.rs Outdated Show resolved Hide resolved
@@ -14311,7 +14327,9 @@ mod tests {
})
);
}

// openssl does not provide a straight interface to deal with custom
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// openssl does not provide a straight interface to deal with custom
// openssl does not provide a straightforward interface to deal with custom


impl Context {
pub fn set_early_data_enabled(&mut self, _enabled: bool) {
println!("## calling set_early_data_enabled context!!")
Copy link
Contributor

Choose a reason for hiding this comment

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

hangover that needs cleanup. Probably a noop or maybe an assert?

@brbzull0
Copy link
Contributor Author

Saw that we need to rebase, I do not know if you guys are doing anything with this branch so I do not want to force push a and break anything on your side.
Can I go ahead and rebae & force push?

Thanks.

@brbzull0 brbzull0 force-pushed the openssl-dev-wip branch 2 times, most recently from 98c5ba4 to 0ce9d56 Compare March 15, 2024 13:44
This adds support for building against the "quictls" fork of OpenSSL
instead of BoringSSL, to provide crypto and TLS support.

Because upstream OpenSSL doesn't (yet?) expose an API for implementing
the QUIC handshake, like BoringSSL does, a fork of it was created called
quictls.

This functionality can be useful for applications that already use
OpenSSL and where adding BoringSSL on top would create conflicts.
@ghedo ghedo force-pushed the openssl-dev-wip branch from 0ce9d56 to 2e076b5 Compare March 16, 2024 04:56
@ghedo ghedo merged commit 53dc0cc into cloudflare:master Mar 16, 2024
25 checks passed
@ghedo
Copy link
Member

ghedo commented Mar 16, 2024

@brbzull0 I was going to do it, just got delayed by travel. Thanks for your work, this is merged now!

@brbzull0
Copy link
Contributor Author

@brbzull0 I was going to do it, just got delayed by travel. Thanks for your work, this is merged now!

great stuff!
@ghedo @LPardue Thanks a lot gents!

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.

4 participants