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 openssl in favor of rustls-tls #401

Merged
merged 1 commit into from Feb 7, 2023

Conversation

jpmcb
Copy link
Contributor

@jpmcb jpmcb commented Jan 27, 2023

Issue number:

N/a

Description of changes:

This PR removes our dependency on OpenSSL in favor of using rustls-tls

Testing done:

Built and pushed a custom image with these changes. Modified our bottlerocket-update-operator.yaml to use that image.
Integration tests pass!

Putting this patch up for now to get some early feedback and iteration, but could use additional validation for:

  • IPv6 cluster upgrades work
  • IPv4 cluster upgrades work

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@jpmcb
Copy link
Contributor Author

jpmcb commented Jan 27, 2023

I know what you're thinking.

"But John, I thought rustls-tls in kube didn't work with IPv6 clusters since webpki can't resolve hostnames via IP addresses?"

Thankfully, in d2b5d78 we switched to using the suggested work-around of reaching the kubernetes API server in cluster through kubernetes DNS. We now build our k8s clients by defining that they should use the incluster_dns in their config:

    let incluster_config =
        kube::Config::incluster_dns().context(controller_error::ConfigCreateSnafu)?;

    let k8s_client = kube::client::Client::try_from(incluster_config)
        .context(controller_error::ClientCreateSnafu)?;

So nothing right now is necessarily preventing us from using the rustls-tls feature in kube.

And it also looks like the next version of rustls-tls will have a fix for this upstream (which was fixed in their fork of webpki). This should flow downstream to kube and we can get the benefit of this if we need to create outside cluster clients in the future. Exciting stuff!

@jpmcb
Copy link
Contributor Author

jpmcb commented Jan 27, 2023

Testing in both IPv4 and IPv6 clusters looks good! Marking ready for review

@jpmcb jpmcb marked this pull request as ready for review January 27, 2023 23:36
agent/Cargo.toml Show resolved Hide resolved
apiserver/src/api/mod.rs Outdated Show resolved Hide resolved
apiserver/src/api/mod.rs Outdated Show resolved Hide resolved
@jpmcb
Copy link
Contributor Author

jpmcb commented Jan 30, 2023

Force pushed to address comments above and remove unused reqwest dependency in agent. Running through another quick integration test validation ✅

Further validation:

❯ rg -i openssl
CHANGELOG.md
20:* Upgraded Bottlerocket SDK to consume fix for OpenSSl CVE-2022-3602 and CVE-2022-3786 ([#331])

deny.toml
20:    "OpenSSL",
58:expression = "MIT AND ISC AND OpenSSL"

Cargo.lock
2187:name = "openssl-probe"
2633: "openssl-probe",

Looks like openssl-probe is a dependency of rustls-native-certs in the rustls dependency chain 👍🏼 no other cases of openssl in the cargo toml

- Removes unused `reqwest` dependency in `agent`
- For `kube`, `reqwest` dependency, sets `default-features = false` to ensure
  default TLS does not pull in any openssl dependencies

Signed-off-by: John McBride <jpmmcb@amazon.com>
@jpmcb
Copy link
Contributor Author

jpmcb commented Jan 30, 2023

Force pushed to rebase on develop which had removed webpki-roots in deny.toml. Added that exception back since the rustls dependency chain uses it.


Edit: integration test passed with the latest force push. All nodes upgrade from 1.9.0 to the latest bottlerocket

❯ k get brs -A
NAMESPACE                 NAME                                                STATE   VERSION   TARGET STATE   TARGET VERSION   CRASH COUNT
brupop-bottlerocket-aws   brs-ip-192-168-130-3.us-west-2.compute.internal     Idle    1.12.0    Idle                            0
brupop-bottlerocket-aws   brs-ip-192-168-134-58.us-west-2.compute.internal    Idle    1.12.0    Idle                            0
brupop-bottlerocket-aws   brs-ip-192-168-154-241.us-west-2.compute.internal   Idle    1.12.0    Idle                            0

Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Looks good to me and all tests are passing.

deny.toml Show resolved Hide resolved
Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

🚀

@jpmcb jpmcb merged commit 0899038 into bottlerocket-os:develop Feb 7, 2023
@jpmcb jpmcb deleted the rustls-tls branch February 7, 2023 18:54
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.

None yet

4 participants