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

Change the default TLS to OpenSSL #863

Merged
merged 1 commit into from Mar 31, 2022
Merged

Conversation

kazk
Copy link
Member

@kazk kazk commented Mar 31, 2022

Motivation

native-tls feature exists because kube used to depend on reqwest.

The feature doesn't make sense for us because all targets still depend on openssl anyway. This is because native-tls requires PKCS#12 to create Identity for authentication with client certificates, and openssl is the only trusted option to generate them.
A feature to create Identity from PKCS#8 was added a few days ago, but openssl is still necessary because we need to support more private key formats.

native-tls feature is currently broken on macOS because Security Framework cannot import PKCS#12 generated by OpenSSL v3 (#691). I'm sure more users will run into this, especially the new ones, so it's not a good default.

Solution

Make OpenSSL the default because we can't make Rustls due to #153. I don't think there are any downsides?

We can also remove native-tls feature. openssl will continue to be required because I don't think the different key formats we need will be supported anytime soon.

@kazk kazk added the changelog-change changelog change category for prs label Mar 31, 2022
@codecov-commenter

This comment was marked as spam.

@@ -27,6 +27,7 @@ jobs:
- name: install openssl
if: matrix.os == 'windows-latest'
run: |
$ErrorActionPreference = "Stop"
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/kube-rs/kube-rs/runs/5766982332?check_suite_focus=true failed to install OpenSSL, but this step didn't fail. Setting this should stop it.
It's usually prepended automatically, but we have fail-fast: false.
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference

@clux
Copy link
Member

clux commented Mar 31, 2022

Yeah, I agree with this as well. I was slightly worried it was going to impact the musl builds somehow (as it bundles its own openssl, possibly avoiding the native-tls magic), but controller-rs is happy with it either way from this branch.

@clux clux added this to the 0.71.0 milestone Mar 31, 2022
@kazk
Copy link
Member Author

kazk commented Mar 31, 2022

Cool, I'll rebase and merge this.

What do you think of removing native-tls feature?

`native-tls` feature exists because `kube` used to depend on `reqwest`.

The feature doesn't make sense for us because all targets still
depend on `openssl` anyway. This is because `native-tls` requires PKCS#12
to create `Identity` for authentication with client certificates, and
`openssl` is the only trusted option to generate them.
A feature to create `Identity` from PKCS#8 was added a few days ago, but
`openssl` is still necessary because we need to support more private key
formats.

`native-tls` feature is currently broken on macOS because Security
Framework cannot import PKCS#12 generated by OpenSSL v3 (kube-rs#691).

https://openradar.appspot.com/FB8988319

Signed-off-by: kazk <kazk.dev@gmail.com>
@kazk
Copy link
Member Author

kazk commented Mar 31, 2022

I just noticed all the tests requiring a cluster are skipped except for e2e. We should be able to run them in e2e job. I'll see if we can make them run on macos and windows as well. k3d has binary for them, and kubectl is installed already according to https://github.com/actions/virtual-environments.

@clux
Copy link
Member

clux commented Mar 31, 2022

What do you think of removing native-tls feature?

I think that also makes sense. However, for sanity, maybe best to give it one version between changing default and removing.

@clux
Copy link
Member

clux commented Mar 31, 2022

I just noticed all the tests requiring a cluster are skipped except for e2e.

coverage job also runs integration tests

@kazk
Copy link
Member Author

kazk commented Mar 31, 2022

However, for sanity, maybe best to give it one version between changing default and removing.

Good idea.

coverage job also runs integration tests

👍


I'll see if we can make them run on macos and windows as well.

macOS doesn't have Docker installed because of some licensing issue.

On Windows, GitHub Action to set up k3d doesn't even run because of some kind of path issue.

D:\a\_temp\63d579d2-03e5-4286-ab77-6b945904e356.sh: line 1: D:a_actionsnolarsetup-k3d-k3sv1/action.sh: No such file or directory

@kazk kazk merged commit 88616b4 into kube-rs:master Mar 31, 2022
@kazk kazk deleted the default-to-openssl-tls branch March 31, 2022 10:33
@clux
Copy link
Member

clux commented Apr 1, 2022

I think this might need a follow-up: the default from the facade-crate is still native-tls: https://github.com/kube-rs/kube-rs/blob/master/kube/Cargo.toml#L18-L19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants