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

attempt upgrading rustls to 0.20.0 - for #644 #692

Closed
wants to merge 4 commits into from
Closed

Conversation

clux
Copy link
Member

@clux clux commented Nov 4, 2021

a bit of major change based on the massive release history in https://github.com/rustls/rustls#release-history

it has some helpful changes:

  • allowing an empty root cert store struct
  • not having to implement the no client verification strategy ourselves seems to not work
  • and we can do this without enabling dangerous configuration features no, still need it for token auth

but it also:

not sure i got it all correct, or if i have changed behaviour subly, but it compiles now and can run on some clusters.

edited out confusion about rustls

however, have currently not gotten the last From impl to work... It currently fails with:

error[E0277]: the trait bound `Arc<rustls::client::ClientConfig>: From<Arc<ClientConfig>>` is not satisfied
   --> kube-client/src/client/config_ext.rs:155:12
    |
155 |         Ok(hyper_rustls::HttpsConnector::from((http, rustls_config)))
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<Arc<ClientConfig>>` is not implemented for `Arc<rustls::client::ClientConfig>`
    |
    = help: the following implementations were found:
              <Arc<B> as From<Cow<'a, B>>>
              <Arc<CStr> as From<&CStr>>
              <Arc<CStr> as From<CString>>
              <Arc<OsStr> as From<&OsStr>>
            and 9 others
    = note: required because of the requirements on the impl of `Into<Arc<rustls::client::ClientConfig>>` for `Arc<ClientConfig>`
    = note: required because of the requirements on the impl of `From<(HttpConnector, Arc<ClientConfig>)>` for `HttpsConnector<HttpConnector>`

relies on the hyper-rustls rustls 0.20 branch

@clux

This comment has been minimized.

@clux clux changed the title attempt upgrading rustls - for #644 attempt upgrading rustls to 0.20.0 - for #644 Nov 4, 2021
@kazk
Copy link
Member

kazk commented Nov 4, 2021

I guess hyper-rustls is not ready yet? rustls/hyper-rustls#156

@clux clux added the blocked awaiting upstream work label Nov 5, 2021
@clux
Copy link
Member Author

clux commented Nov 6, 2021

ok, thanks to the linked branch i have at least gotten it to work in the accept_invalid case (which i've enabled whenever there are no root certs because my token auth test cluster needs it to work - and don't want to edit kubeconfig because in kubectl get already works).

still fails on k3d like before thanks to the unrecognised cert.

EDIT: also looks like hyper_rustls::HttpsConnector::with_native_roots() is gone.

don't have any other clusters to test against, so any input beyond this is appreciated.

a bit of major change based on the massive release history in https://github.com/rustls/rustls#release-history
it has some helpful changes, but it introduces a massive builder on the
ClientConfig which needs to be specified in a particular order.

not sure i got it all correct, or if i have changed behaviour here.

and there is one outstanding issue with converting to a HttpsConnector

Signed-off-by: clux <sszynrae@gmail.com>
+fixups on the builder

Signed-off-by: clux <sszynrae@gmail.com>

make it work in the token auth case

k3d still broken

Signed-off-by: clux <sszynrae@gmail.com>
even though i don't have a way to test it atm

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@kazk
Copy link
Member

kazk commented Nov 11, 2021

still fails on k3d like before thanks to the unrecognised cert.

You can work around that with the steps in #542.

don't have any other clusters to test against, so any input beyond this is appreciated.

Do you mind if I take it from here? I started playing with it, and got:

let config_builder = ClientConfig::builder()
    .with_safe_defaults()
    .with_root_certificates(root_store(root_certs)?);

let mut client_config = if let Some(auth) = identity_pem.map(client_auth).transpose()? {
    config_builder
        .with_single_cert(auth.cert_chain, auth.private_key)
        .map_err(|e| Error::SslError(format!("{}", e)))?
} else {
    config_builder.with_no_client_auth()
};

if accept_invalid {
    client_config
        .dangerous()
        .set_certificate_verifier(Arc::new(NoCertificateVerification {}));
}
Ok(client_config)

@clux
Copy link
Member Author

clux commented Nov 11, 2021

Yeah, absolutely feel free to continue!

@kazk kazk mentioned this pull request Nov 11, 2021
3 tasks
@kazk
Copy link
Member

kazk commented Nov 12, 2021

Superseded by #704. I think we're just waiting for hyper-rustls now.

@kazk kazk closed this Nov 12, 2021
@clux clux deleted the rustls020 branch December 6, 2021 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked awaiting upstream work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants