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.

Sign up for GitHub

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

Jump to bottom

Add support for OpenSSL #1436

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for OpenSSL #1436

wants to merge 1 commit into from

Conversation

tedjpoole
Copy link

Add support for using OpenSSL as an alternative crypto provider, implemented using the rustls-openssl and openssl crates, and guarded by a new feature called tls-openssl.

fixes #149

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 22, 2025
@istio-policy-bot
Copy link

😊 Welcome @tedjpoole! This is either your first contribution to the Istio ztunnel repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Jan 22, 2025
@istio-testing
Copy link
Contributor

Hi @tedjpoole. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

howardjohn
howardjohn reviewed Jan 22, 2025
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

FYI if you haven't seen this: #1323 (comment). Looks like you followed all the suggestions there already though!

src/tls/lib.rs Outdated
@@ -66,6 +66,19 @@ pub(super) fn provider() -> Arc<CryptoProvider> {
})
}

#[cfg(feature = "tls-openssl")]
pub(super) fn provider() -> Arc<CryptoProvider> {
// Arc::new(rustls_openssl::default_provider())
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove

Copy link
Author

Choose a reason for hiding this comment

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

Done

@tedjpoole
Add support for OpenSSL
252f024 
Signed-off-by: Ted Poole <[email protected]>
@tedjpoole tedjpoole force-pushed the add-tls-openssl-feature branch from 775baab to 252f024 Compare January 22, 2025 15:15
@tedjpoole tedjpoole marked this pull request as ready for review January 23, 2025 09:28
@tedjpoole tedjpoole requested review from a team as code owners January 23, 2025 09:28
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 23, 2025
@howardjohn
Copy link
Member

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Jan 23, 2025
@istio-testing
Copy link
Contributor

@tedjpoole: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
test_ztunnel 252f024 link true /test test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@tedjpoole
Copy link
Author

It looks like the build failed because OpenSSL header package isn't installed in the build environment. Can someone point me to where I should add it please?

   Compiling openssl-sys v0.9.104
warning: [email protected]: build/expando.c:1:10: fatal error: openssl/opensslv.h: No such file or directory
warning: [email protected]:     1 | #include <openssl/opensslv.h>
warning: [email protected]:       |          ^~~~~~~~~~~~~~~~~~~~
warning: [email protected]: compilation terminated.
error: failed to run custom build command for `openssl-sys v0.9.104`

@jwendell
Copy link
Member

jwendell commented Jan 27, 2025 •
edited
Loading

@tedjpoole https://github.com/istio/tools/blob/master/docker/build-tools/Dockerfile

This is the current image, in case you want to play with it locally: gcr.io/istio-testing/build-tools:master-ba6e7d3a0ff9666b1cc2a650507b0d99acfefff4

When changing the Dockerfile, you can give it a try locally to make sure your changes work fine, by running DRY_RUN=1 ./tools/docker/build-tools/build-and-push.sh

Feel free to ping me on Slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn approved these changes

Assignees
No one assigned
Labels
ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for OpenSSL
5 participants
@tedjpoole @istio-policy-bot @istio-testing @howardjohn @jwendell