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

Update rustls to 0.20.1 #704

Merged
merged 6 commits into from Nov 23, 2021
Merged

Update rustls to 0.20.1 #704

merged 6 commits into from Nov 23, 2021

Conversation

kazk
Copy link
Member

@kazk kazk commented Nov 11, 2021

Continuing from #692 for #644 after rebasing. webpki is no longer necessary, so this is the last one.

TODO


Closes #644

Comment on lines 112 to 171
for item in rustls_pemfile::read_all(&mut reader)
.map_err(|e| Error::SslError(format!("failed to read identity PEM: {}", e)))?
{
match item {
Item::X509Certificate(cert) => cert_chain.push(Certificate(cert)),
Item::PKCS8Key(key) => pkcs8_key = Some(PrivateKey(key)),
Item::RSAKey(key) => rsa_key = Some(PrivateKey(key)),
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to get all sections at once, but it should behave the same. Use the last found private key, and PKCS8 key is preferred.

@kazk kazk added the blocked awaiting upstream work label Nov 11, 2021
@kazk kazk force-pushed the chore/rustls020 branch 2 times, most recently from 474a52d to eb32afa Compare November 12, 2021 00:06
examples/pod_watcher.rs Outdated Show resolved Hide resolved
@kazk kazk changed the title Update rustls to 0.20.0 Update rustls to 0.20.1 Nov 15, 2021
@clux
Copy link
Member

clux commented Nov 15, 2021

This seems to work as well as before. Thought I had a regression on rustls vs rancher, but turns out rancher just never worked with local kubeconfig on rustls.

rancher gives this type of setup:

clusters:
- cluster:
    server: https://rancher.xxx.io/k8s/clusters/clusterid
  name: mycluster
users:
- name: myuser
  user:
    token: kubeconfig-u-somegenuser.c-id:somelongtoken
contexts:
- context:
    cluster: mycluster
    user: myuser
  name: mycontext
error

[2021-11-15T22:06:29Z DEBUG kube_client::config] failed to load client identity from kubeconfig: failed to load client certificate 
[2021-11-15T22:06:29Z DEBUG kube_client::client] HTTP; http.method=GET http.url=https://rancher.xxx.io/k8s/clusters/clusterid/api/v1/namespaces/default/pods? otel.name="list" otel.kind="client"
[2021-11-15T22:06:29Z DEBUG kube_client::client] requesting 
[2021-11-15T22:06:30Z WARN  rustls::conn] Sending fatal alert BadCertificate
[2021-11-15T22:06:30Z DEBUG kube_client::client] HTTP; otel.status_code="ERROR"
[2021-11-15T22:06:30Z ERROR kube_client::client] failed with error error trying to connect: invalid peer certificate contents: invalid peer certificate: UnknownIssuer 
Error: failed to perform initial object list: HyperError: error trying to connect: invalid peer certificate contents: invalid peer certificate: UnknownIssuer

Caused by:
   0: HyperError: error trying to connect: invalid peer certificate contents: invalid peer certificate: UnknownIssuer
   1: error trying to connect: invalid peer certificate contents: invalid peer certificate: UnknownIssuer
   2: invalid peer certificate contents: invalid peer certificate: UnknownIssuer

Location:
    /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/result.rs:1915:27

interestingly this works with openssl, but with rustls i have to add insecure-skip-tls-verify: true to the cluster entry. not sure if you have an idea of this. can open a separate bug if not.

@kazk
Copy link
Member Author

kazk commented Nov 15, 2021

interestingly this works with openssl, but with rustls i have to add insecure-skip-tls-verify: true to the cluster entry. not sure if you have an idea of this. can open a separate bug if not.

I think native-tls includes the system cert store by default, while rustls doesn't. We can load with rustls-native-certs which is already a dependency of hyper-rustls.

@kazk
Copy link
Member Author

kazk commented Nov 15, 2021

#711 is working :)

error[B004]: found 2 duplicate entries for crate 'rustls'
    ┌─ /github/workspace/Cargo.lock:151:1
    │  
151 │ ╭ rustls 0.19.1 registry+rust-lang/crates.io-index
152 │ │ rustls 0.20.1 registry+rust-lang/crates.io-index
    │ ╰───────────────────────────────────────────────────────────────────^ lock entries
    │  
    = rustls v0.19.1
      └── tokio-rustls v0.22.0
          └── warp v0.3.2
              └── (dev) examples v0.1.0
    = rustls v0.20.1
      ├── hyper-rustls v0.23.0
      │   └── kube-client v0.63.2

@kazk kazk force-pushed the chore/rustls020 branch 4 times, most recently from 371c0db to ee85982 Compare November 21, 2021 11:47
@kazk kazk removed the blocked awaiting upstream work label Nov 21, 2021
@kazk kazk marked this pull request as ready for review November 21, 2021 11:50
@kazk kazk added this to the 0.65.0 milestone Nov 22, 2021
kube-client/src/error.rs Outdated Show resolved Hide resolved
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Great work 👍

clux and others added 2 commits November 22, 2021 15:51
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: kazk <kazk.dev@gmail.com>
Signed-off-by: kazk <kazk.dev@gmail.com>
Remove `Error::SslError`

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

kazk commented Nov 22, 2021

I'll merge this after cleaning up the commits:

  • Move all edits to Cargo.toml to the first commit
  • Merge commit removing Error::SslError with refining error one
  • Merge commit fixing doc cfg with refining error one

I thought of squash merging, but I think it's better to keep the upgrade and refactoring separate after merging.

@kazk kazk merged commit 6b23251 into kube-rs:master Nov 23, 2021
@kazk kazk deleted the chore/rustls020 branch November 23, 2021 00:10
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.

upgrade rustls, pem, tame-oauth, and webpki
2 participants