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

Split cryptographic dependencies into a dedicated crate #1307

Merged
merged 34 commits into from
Oct 18, 2021

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Oct 14, 2021

The ring/rustls/webpki crates provide the cryptographic primitives
that we use for the proxy's mTLS functionality. But there's a desire to
support other cryptographic implementations (i.e. openssl/boringssl),
especially for FIPS 140-2.

This change introduces a new crate, linkerd-tls-rustls, into which all
types that depend on ring/rustls/webpki are moved. Specifically,
Key, Crt, and CrtKey are moved from linkerd-identity into
rustls. The linkerd-tls crate becomes generic over its TLS
implementation by using a NewService to build client connectors and a
Service to terminate server-side TLS connections.

The proxy-identity crate currently depends on the linkerd-tls-rustls
crate, as do the various app crates. In followup changes, these crates
will be further decoupled from the rustls types.


:; cargo tree -i ring -e normal
ring v0.16.20
├── linkerd-tls-rustls v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/tls/rustls)
│   ├── linkerd-app-core v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/core)
│   │   ├── linkerd-app v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app)
│   │   │   ├── linkerd-app-integration v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/integration)
│   │   │   └── linkerd2-proxy v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd2-proxy)
│   │   ├── linkerd-app-admin v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/admin)
│   │   │   └── linkerd-app v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app) (*)
│   │   ├── linkerd-app-gateway v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/gateway)
│   │   │   └── linkerd-app v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app) (*)
│   │   ├── linkerd-app-inbound v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/inbound)
│   │   │   ├── linkerd-app v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app) (*)
│   │   │   ├── linkerd-app-admin v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/admin) (*)
│   │   │   └── linkerd-app-gateway v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/gateway) (*)
│   │   ├── linkerd-app-integration v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/integration)
│   │   ├── linkerd-app-outbound v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/outbound)
│   │   │   ├── linkerd-app v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app) (*)
│   │   │   └── linkerd-app-gateway v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/gateway) (*)
│   │   └── linkerd-app-test v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/test)
│   │       └── linkerd-app-integration v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/integration)
│   ├── linkerd-proxy-identity v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/proxy/identity)
│   │   └── linkerd-app-core v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/core) (*)
│   └── linkerd-proxy-tap v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/proxy/tap)
│       └── linkerd-app-core v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/core) (*)
├── rustls v0.19.1
│   └── tokio-rustls v0.22.0
│       ├── linkerd-app-integration v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/integration)
│       └── linkerd-tls-rustls v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/tls/rustls) (*)
├── sct v0.6.1
│   └── rustls v0.19.1 (*)
└── webpki v0.21.4 (https://github.com/linkerd/webpki?branch=cert-dns-names-0.21#a4acca51)
    ├── linkerd-app-integration v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/app/integration)
    ├── linkerd-tls-rustls v0.1.0 (/home/ver/b/linkerd2-proxy/linkerd/tls/rustls) (*)
    ├── rustls v0.19.1 (*)
    └── tokio-rustls v0.22.0 (*)

Copy webpki's DNS name types into our dns-name crate so that webpki and
ring aren't needed for basic name types.
The `ring`/`rustls`/`webpki` crates provide the cryptographic primitives
that we use for the proxy's mTLS functionality. But there's a desire to
support other cryptographic implementations (i.e. openssl/boringssl),
especially for FIPS 140-2.

This change introduces a new crate, `linkerd-tls-rustls`, into which all
types that depend on `ring`/`rustls`/`webpki` are moved. Specifically,
`Key`, `Crt`, and `CrtKey` are moved from `linkerd-identity` into
`rustls`. The `linkerd-tls` crate becomes generic over its TLS
implementation by using a `NewService` to build client connectors and a
`Service` to terminate server-side TLS connections.
@olix0r olix0r changed the title Ver/tls rustls split Split cryptographic dependencies into a dedicated crate Oct 17, 2021
@olix0r olix0r marked this pull request as ready for review October 17, 2021 20:44
@olix0r olix0r requested a review from a team October 17, 2021 20:44
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this looks good to me!

type Connection<T, I> = ((tls::ConditionalServerTls, T), tls::server::Io<I>);
type Connection<T, I> = (
(tls::ConditionalServerTls, T),
io::EitherIo<linkerd_tls_rustls::ServerIo<tls::server::DetectIo<I>>, tls::server::DetectIo<I>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

oof...

pub use linkerd_service_profiles as profiles;
pub use linkerd_stack_metrics as stack_metrics;
pub use linkerd_stack_tracing as stack_tracing;
pub use linkerd_tls as tls;
pub use linkerd_tls_rustls as rustls;
Copy link
Contributor

Choose a reason for hiding this comment

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

take it or leave it, but i'm kind of on the fence about reexporting this as rustls? it seems like it's kind of unclear in downstream code whether imports from this module are library APIs or our code, which could be confusing to future readers?

not a b locker either way though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I expect this to change in a followup

Comment on lines +132 to +138
let permit =
allow.check_authorized(client.client_addr, &tls)?;
Ok(svc::Either::A(Local {
addr: Remote(ServerAddr(addr)),
permit,
client_id: client.client_id,
}))
Copy link
Contributor

Choose a reason for hiding this comment

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

it took me a min to realize that this (and some of the other code in this file) was not actually changed, and rustfmt just changed its mind about which line to wrap at or something. :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why but rustfmt was just ignoring this whole stack before and something changed and now it wants to format it all

Copy link
Contributor

Choose a reason for hiding this comment

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

wacky!

Comment on lines +42 to +44
let server_id = webpki::DNSNameRef::try_from_ascii(client_tls.server_id.as_bytes())
.expect("identity must be a valid DNS name")
.to_owned();
Copy link
Contributor

Choose a reason for hiding this comment

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

kind of a bummer we have to do the conversion by-ref and then convert it into a new owned DNSName, rather than just moving the String from the dns::Name if it's valid. but, idk if this is worth trying to change it upstream...

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah. we're on a fork of webpki so we could perhaps patch our fork to avoid it... but this will be a small allocation once per connection, so I'm not particularly worried

}

#[test]
#[ignore] // XXX this doesn't fail because we don't actually check the key against the cert...
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be an issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is just moved from the TLS crate, so it's been a long standing issue. practically, I don't think it's an issue we're likely to hit; and I don't know that we have a path forward to fixing it given the state of things.

@olix0r olix0r merged commit 384aa21 into main Oct 18, 2021
@olix0r olix0r deleted the ver/tls-rustls-split branch October 18, 2021 22:22
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Oct 19, 2021
This release fixes a bug where the outbound proxy could loop infinitely
while handling errors on meshed HTTP/1 connections. This would typically
cause proxies to be fail health checks and be restarted.

Furthermore, the proxy now requires identity. Proxies will log an error
and fail to start if identity is disabled.

---

* dns: Avoid allocating in `Name::is_localhost` (linkerd/linkerd2-proxy#1303)
* metrics: Implement FmtMetrics for Option (linkerd/linkerd2-proxy#1302)
* tracing: simplify subscriber construction with `Box`ed layers (linkerd/linkerd2-proxy#1304)
* Require identity configuration (linkerd/linkerd2-proxy#1305)
* build(deps): bump thiserror from 1.0.29 to 1.0.30 (linkerd/linkerd2-proxy#1306)
* build(deps): bump tower from 0.4.8 to 0.4.9 (linkerd/linkerd2-proxy#1308)
* build(deps): bump trust-dns-resolver (linkerd/linkerd2-proxy#1311)
* build(deps): bump actions/checkout from 2.3.4 to 2.3.5 (linkerd/linkerd2-proxy#1313)
* dns-name: Remove `webpki` dependency (linkerd/linkerd2-proxy#1316)
* build(deps): bump libc from 0.2.103 to 0.2.104 (linkerd/linkerd2-proxy#1315)
* inbound: Add a box layer to reduce compile times (linkerd/linkerd2-proxy#1317)
* Split cryptographic dependencies into a dedicated crate (linkerd/linkerd2-proxy#1307)
* Fix an infinite loop when downgrading HTTP/2 errors (linkerd/linkerd2-proxy#1318)
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Oct 20, 2021
This release fixes a bug where the outbound proxy could loop infinitely
while handling errors on meshed HTTP/1 connections. This would typically
cause proxies to be fail health checks and be restarted.

Furthermore, the proxy now requires identity. Proxies will log an error
and fail to start if identity is disabled.

---

* dns: Avoid allocating in `Name::is_localhost` (linkerd/linkerd2-proxy#1303)
* metrics: Implement FmtMetrics for Option (linkerd/linkerd2-proxy#1302)
* tracing: simplify subscriber construction with `Box`ed layers (linkerd/linkerd2-proxy#1304)
* Require identity configuration (linkerd/linkerd2-proxy#1305)
* build(deps): bump thiserror from 1.0.29 to 1.0.30 (linkerd/linkerd2-proxy#1306)
* build(deps): bump tower from 0.4.8 to 0.4.9 (linkerd/linkerd2-proxy#1308)
* build(deps): bump trust-dns-resolver (linkerd/linkerd2-proxy#1311)
* build(deps): bump actions/checkout from 2.3.4 to 2.3.5 (linkerd/linkerd2-proxy#1313)
* dns-name: Remove `webpki` dependency (linkerd/linkerd2-proxy#1316)
* build(deps): bump libc from 0.2.103 to 0.2.104 (linkerd/linkerd2-proxy#1315)
* inbound: Add a box layer to reduce compile times (linkerd/linkerd2-proxy#1317)
* Split cryptographic dependencies into a dedicated crate (linkerd/linkerd2-proxy#1307)
* Fix an infinite loop when downgrading HTTP/2 errors (linkerd/linkerd2-proxy#1318)
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.

2 participants