Using MS Edge and probably other clients with the Conduit proxy when
TLS is enabled fails because Rustls doesn't take into consideration
that Conduit only supports one signature scheme (ECDSA P-256 SHA-256).
This bug was fixed in Rustls when ECDSA support was added, after the
latest release. With this change MS Edge can talk to Conduit.
Signed-off-by: Brian Smith <brian@briansmith.org>
Previously, the proxy would not attempt to load its TLS certificates until a fs
watch detected that one of them had changed. This means that if the proxy was
started with valid files already at the configured paths, it would not load
them until one of the files changed.
This branch fixes that issue by starting the stream of changes with one event
_followed_ by any additional changes detected by watching the filesystem.
I've manually tested that this fixes the issue, both on Linux and on macOS, and
can confirm that this fixes the issue. In addition, when I start writing
integration tests for certificate reloading, I'll make sure to include a test
to detect any regressions.
Closes#1133.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Refactor the way the TLS trust anchors are configured in preparation
for the client and server authenticating each others' certificates.
Make the use of client certificates optional pending the implementation
of authorization policy.
Signed-off-by: Brian Smith <brian@briansmith.org>
When a TLS handshake error occurs, the proxy just stops accepting
requests. It seems my expectations of how `Stream` handles errors
were wrong.
The test for this will be added in a separate PR after the
infrastructure needed for TLS testing is added. (This is a chicken
and egg problem.)
Signed-off-by: Brian Smith <brian@briansmith.org>
This PR changes the proxy's Inotify watch code to avoid always falling back to
polling the filesystem when the watched files don't exist yet. It also contains
some additional cleanup and refactoring of the inotify code, including moving
the non-TLS-specific filesystem watching code out of the `tls::config` module
and into a new `fs_watch` module.
In addition, it adds tests for both the polling-based and inotify-based watch
implementations, and changes the polling-based watches to hash the files rather
than using timestamps from the file's metadata to detect changes. These changes
are originally from #1094 and #1091, respectively, but they're included here
because @briansmith asked that all the changes be made in one PR.
Closes#1094. Closes#1091. Fixes#1090. Fixes#1097. Fixes#1061.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
prost-0.4.0 has been released, which removes unnecessary dependencies.
tower-grpc is being updated simultaneously, as this is the proxy's
primary use of prost.
See: https://github.com/danburkert/prost/releases/tag/v0.4.0
* proxy: Update `rand` to 0.5.1
The proxy depends on rand-0.4, which is superceded by newer APIs in
rand-0.5. Since we're already using rand-0.5 via the tower-balance
crate, it seems appropriate to upgrade the proxy.
* Expand lock files in reviews
In e2093e3, we created a `convert` crate when refactoring the proxy's
gRPC bindings into a dedicated crate.
It's not really necessary to handle `convert` as a crate, given that it
holds a single 39-line file that's mostly comments. It's possible to
"vendor" this file in the proxy, and controller-grpc crate doesn't
even need this trait (in fact, the proxy probably doesn't either).
`tower-balance` has been updated with a Peak-EWMA load balancer; and a
new crate, `tower-h2-balance` has been introduced to make the load
balancer aware of some H2 stream events.
The Peak-EWMA balancer is designed to reduce tail latency by maintaining
an Exponentially Weighted Moving Average of latencies to each endpoint
which decay over a 10s window.
This commit adds the initial wiring to forward TLS config changes to the
watches used by TLS clients as well as TLS servers. As the TLS clients
are not yet implemented, the config type is currently `()`, but once
the client config is implemented, we should be able to drop it in
seamlessly.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: Brian Smith <brian@briansmith.org>
Depends on #1032.
This branch makes some additional changes to the proxy's DNS code. In
particular, since we no longer need to clone the resolver on every lookup,
it removes some `clone()` calls in `DestinationSet::reset_dns_query`.
I've also changed the DNS futures to use the new contextual logging code
on master.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
* Fix non-Linux builds.
The change to signal.rs is needed for Windows.
The change to config.rs is needed for Windows and maybe other platforms.
Signed-off-by: Brian Smith <brian@briansmith.org>
* Proxy: Better encapsulate the details of TLS config watching.
Encapsulate more of the TLS configuration logic in the TLS submodule. This allows
for easier refactoring. In particular, this will make adding the client TLS configuration
easier.
Signed-off-by: Brian Smith <brian@briansmith.org>
This branch adds an inotify-based implementation of filesystem watches
for the TLS config files. On Linux, where inotify is available, this is
used instead of the polling-based code I added in #1056 and #1076.
In order to avoid the issues detecting changes to files in Kubernetes
ConfigMaps described in #1061, we watch the directory _containing_ the
files we care about rather than the files themselves. I've tested this
manually in Docker for Mac Kubernetes and can confirm that ConfigMap
changes are detected successfully.
Closes#1061. Closes#369.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
* Proxy: Map Kubernetes Pod Namespace/Name to TLS identity.
Map the Kubernetes identity into a DNS name that can be used to
validate the peer's certificate. The final mapping is TBD; the
important thing for now is that the mapped name doesn't collide
with any real DNS name.
Encapsulate the mapping logic within the TLS submodule.
Minimize `Arc`ing and `Clone`ing of TLS identities.
This has no effect in default configurations since the settings that
enable the functionality are not set by default.
Signed-off-by: Brian Smith <brian@briansmith.org>
This branch changes the polling-based implementation of TLS config file watches
to fully canonicalize the path to each config file prior to polling for its
metadata. Doing so fixes the issues detecting changes when the watched path is
a symbolic link to another symbolic link (see #1061), which is how Kubernetes
implements ConfigMaps mounted as volumes.
I've manually tested this with Conduit running in Docker for Mac Kubernetes,
by volume-mounting a ConfigMap containing the TLS config files, and
regenerating, deleting, and adding the certificates. Watching the Conduit logs
confirms that the changes are now successfully detected.
Note that we have to re-canonicalize the path every time we poll the filesystem
for metadata. Otherwise, if the file is a symlink and the link target changes,
we will continue polling the _old_ link target's path, and fail to detect any
changes to the _new_ link target.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
webpki's DNSName type does full validation and normalization (lowercasing) of
DNS names, which is exactly what `dns::Name` does. webpki's DNSName type
considers a DNS name to be valid according to the rules for TLS certificates,
which is slightly stricter than what a DNS library might otherwise allow. In
anticipation of possible compatibility issues, introduce separate tls::DnsName
and dns::Name names for this type. In the future, if we find that tls::DnsName
is too strict for non-TLS cases, we can have these types diverge without
affecting TLS validation.
Signed-off-by: Brian Smith <brian@briansmith.org>
Encapsulate HTTP request ID generation logic.
Request IDs need to be globally unique, so there can only be one request ID
sequence per process. Simplify the request ID generation with that in mind,
and make it more efficient.
Signed-off-by: Brian Smith <brian@briansmith.org>
During code review of another change I noticed that a lot of types seem
to derive `Hash` (and `Eq`, `PartialEq`) even though the types should
never (for performance reasons) be used as keys of a hash table, and
where it is kind of questionable what equality should mean for those
types. Then I noticed that similarly many types implement `Clone` even
though I expect we should never be cloning them, again because of our
performance goals.
Because these types derive these traits, then whenever we add a field
to them, that field also has to implement these traits. That means we
then have to expand the problem, deriving implementations of these
traits for types that don't otherwise want/need to implement these
traits. This makes review complicated, because, for example, we have
to decide whether something should be compared case-insensitively or
case-sensitively when really we don't want to compare those things at
all.
To prove that we can get by by doing less, to speed up code review
(particularly related to some stuff related to TLS), stop deriving
`Clone`, `Eq`, `PartialEq`, and `Hash` for these types.
I believe that, in particular, the change to key the Tap hash table
based on request ID, instead of the whole request, should speed up
the tap feature since we don't hash and/or compare every field,
recursively, of requests.
Later more such cleanup of this sort should be done.
Signed-off-by: Brian Smith <brian@briansmith.org>
Depends on #1006. Depends on #1041.
This PR adds a `tls_identity` field to the endpoint `Metadata` struct, which
contains the `TlsIdentity` metadata sent by the control plane's Destination
service.
I changed the `ctx::transport::Client` context struct to hold a `Metadata`,
rather than just the labels, so the TLS support determination is always
available. In addition, I've added it as an additional parameter to
`transport::Connect::new`, so that when we create a new connection, the TLS
code will be able to determine whether or not TLS is supported and, if it is,
how to verify the endpoint's identity.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
The proxy can't actually support 10K clients currently (for one, we can't open
10K resolution streams to the destination service). 100 is a more-realistic
but sufficiently-high default.
This PR modifies the proxy's TLS code so that the TLS config files are reloaded
when any of them has changed (including if they did not previously exist).
If reloading the configs returns an error, we log an error and continue using
the old config.
Currently, this is implemented by polling the file system for the time they
were last modified at a fixed interval. However, I've implemented this so
that the changes are passed around as a `Stream`, and that reloading and
updating the config is in a separate function the one that detects changes.
Therefore, it should be fairly easy to plug in support for `inotify` (and
other FS watch APIs) later, as long as we can use them to generate a
`Stream` of changes.
Closes#369
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Depends on #974. Closes#859.
This PR updates the proxy's `dns` module to use the new `AsyncResolver` API I
added to `trust-dns-resolver` in bluejekyll/trust-dns#487. This allows us to
spawn one `Future` that will drive DNS resolution in the background, rather
than having to repeatedly clone a heavyweight `ResolverFuture` for every
lookup. It also means that we no longer have to clone the name to resolve in
quite as many places.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Closes#711. Depends on #967.
This PR changes the proxy's `destination` module to honor the TTLs associated
with DNS lookups, now that bluejekyll/trust-dns#444 has been merged and we can
access this information from the Trust-DNS Resolver API.
The `destination::background::DestinationSet` type has been modified so that,
when a successful result is received for a DNS query, the DNS server will be
polled again after the deadline associated with that query, rather than after
a fixed deadline. The fixed deadline is still used to determine when to poll
again for negative DNS responses or for errors.
Furthermore, Conduit now accepts an optional CONDUIT_PROXY_DNS_MIN_TTL
environment variable that will configure a minimum TTL for DNS results. If the
deadline of a DNS response gives it a TTL shorter than the configured minimum,
Conduit will not poll DNS again until after that minimum TTL is elapsed. By
default, there is no minimum value set, as this feature is intended primarily
for when Conduit is run locally for development purposes.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
* Add initial infrastructure for optinally accepting TLS connections.
If the environment gives us the paths to the certificate chain and private key
then use TLS for all accepted TCP connections. Otherwise, continue on using
plaintext for all accepted TCP connections. The default behavior--no TLS--isn't
changed.
Later we'll make this smarter by adding protocol detection so that when the TLS
configuration is available, we'll accept both TLS and non-TLS connections.
Signed-off-by: Brian Smith <brian@briansmith.org>
Required for #1008.
This PR adds the `TlsIdentity` message to the Destination service proto,
to describe what strategy the proxy should use for verifying an endpoint's TLS
certificates. It also adds a `TlsIdentity` field to the `WeightedAddr` message.
Currently, there is one possible variant for `TlsIdentity`, `KubernetesPodName`,
which consists of the Kubernetes pod name of the endpoint, the namespace of
the endpoint, and the namespace of that pod's Conduit control plane. The proxy
should attempt to connect over TLS if the control plane namespace matches its
own control plane namespace. The pod name and namespace are used to verify
the endpoint's TLS certificate.
See https://github.com/runconduit/conduit/issues/386#issuecomment-392948046.
This change was initially part of #1008, but I factored it out to make the diff
smaller.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
While debugging proxy issues, I found it necessary to change how logging contexts are
instrumented, especially for clients.
This change moves away from using `Debug` types to in favor of `Display` types.
Furthermore, the `logging` module now provides a uniform set of logging contexts to be
used throughout the application. All clients, servers, and background tasks should now be
instrumented so that their log messages contain predictable metadata.
Some small improvements have been made to ensure that logging contexts are correct
when a `Future` is dropped (which is important for some H2 uses, especially).
b3170af changed the DstLabels api, but the bench test was not updated
accordingly.
Furthermore, since bench tests require a nightly rust version, we've
avoided running them in CI. This makes it easy for these tests to break, however.
This updates the benches/record.rs. Additionally, in CI, we pin the rust nightly'
version to a known-good version so that we can reliably run these bench test
without the fear of external changes breaking our build.
The proxy receives a hash map of endpoint labels from the destination
service. As this map is serialized into a string, its keys and values
do not have a stable ordering.
To fix this, we sort the keys for all labels before constructing an
instance of `DstLabels`.
This change was much more difficult to test than it was to fix, so tests
this change was tested manually.
Fixes#1015
Depends on tower-rs/tower#75. Required for #386
In order for the proxy to use the TLS support metadata from the Destination
service correctly, we determined that the code for dynamically changing the
labels on an already-bound service should be removed, and any change in
metadata should cause an endpoint to be rebound.
I've modified the proxy so that we no longer update the labels using
`futures-watch` (as a sidenote, we no longer depend on that crate). Metadata
update events now cause the `tower-discover::Discover` implementation for
`DestinationSet` to re-insert the changed endpoint into the load balancer.
Upstream PR tower-rs/tower#75 in tower-balance changes the load balancer
to honor duplicate insertions by replacing the old endpoint rather than
ignoring them; that change is necessary for the tests to pass on this branch.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Commit b861a6df317c937123825098a7ef0b50cf52e281 moved the code the
comment was describing, but didn't move the comment.
Signed-off-by: Brian Smith <brian@briansmith.org>
* Rename so_original_dst.rs to addr_info.rs.
Prepare for expanding the functionality of this module by renaming it.
Signed-off-by: Brian Smith <brian@briansmith.org>
* Abstract I/O interface into a trait.
Instead of pattern matching over an `Io` variant, use a `Box<Io>` to
abstract the I/O interface. This will make it easier to add a TLS
transport.
Signed-off-by: Brian Smith <brian@briansmith.org>
Changes to `BoundPort::listen_and_fold` inadvertently broke the
`::logging::context_future`s on the `serve` futures for the Inbound and
outbound proxies, leading to log messages that didn't have the appropriate
context. This fixes that.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Refactor `listen_and_fold()` to make it possible to insert more futures
into the chain before the folding.
Signed-off-by: Brian Smith <brian@briansmith.org>
It appears that hyper does not necessarily poll bodies to completion,
and instead simply drops a body as soon as `content-length` is reached
(hyperium/hyper#1521).
This change implements Drop for MeasuredBody such that the stream-end
event is triggered if it had not been triggered previously. This ensures
that response latencies and counts are recorded for HTTP/1 streams.
Fixes#994
In the h1-h2 glue code, we incorrectly called `is_empty()` to determine
if an H1 stream had ended. `is_empty` only returns true if there was no
body at all (rather than if the body has been fully consumed).
By changing this to call `hyper::body::Payload::is_end_stream`, h1
bodies now behave the same as h2 bodies.
Relates to #994
Currently, the proxy records a request's latency as the time between
when a request is opened and when its response stream completes. This is
not what we intend to record, especially when a response is long-lived.
In order to more accurate record latency, we want to track the time at
which the first response body frame is received (which is a close
approximation of time-to-first-byte).
Telemetry aggregation has been changed to use the first-frame time to
compute latencies; tests have been updated to exercise this behavior; and
the metrics documentation has been updated to reflect this change.
Addresses #818
Relates to #980
Proxy tasks emit events to the telemetry system. These events are used
aggregate counts and latencies, as well as to inform Tap requests.
Initially, these events included durations, describing the relevant time
that elapsed between this event and another.
This approach is somewhat inflexible -- it unnecessarily constrains the
set of measurements that can computed in the telemetry system.
To remedy this, the `Event` types can be changed to report discrete
`Instant`s (rather than `Duration`s). Then, when latencies are computed
in the telemetry system, these discrete instants can be compared to
produce durations.
There are no functional changes in this PR.
A common pattern when using the old Tokio API was separating the configuration
of a task from binding it to an executor to run on. This was often necessary
when we wanted to construct a type corresponding to some task before the
reactor on which it would execute was initialized. Typically, this was
accomplished with two separate types, one of which represented the
configuration and exposed only a method to take a reactor `Handle` and
transform it to the other type, representing the actual task.
After we migrate to the new Tokio API in #944, executors no longer need to be
passed explictly, as we can use `DefaultExecutor::current` or
`current_thread::TaskExecutor::current` to spawn a task on the current
executor. Therefore, a lot of this complexity can be refactored away.
This PR refactors the `Config` and `Process` structs in
i`control::destination::background` into a single `Background` struct, and
removes the `dns::Config` and `telemetry::MakeControl` structs (`dns::Resolver`
and `telemetry::Control` are now constructed directly). It should not cause
any functional changes.
Closes#966
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Now that `impl Trait` is stable, we don't need to box as many futures. We still
need to box before spawning them on an executor, but the component futures no
longer require their own boxes.
Signed-off-by: Eliza Weisman <eliza@buoyant.io
Closes#888. Closes#867.
This branch upgrades Conduit to use the new Tokio API. It was also necessary to
upgrade some other dependencies (including `hyper`, and `trust-dns`) alongside
this upgrade.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Instead of having connect errors destroy all buffered requests,
this changes Bind to return a service that can rebind itself when
there is a connect error.
It won't try to establish the new connection itself, but waits for
the buffer to poll again. Combing this with changes in tower-buffer
to remove canceled requests from the buffer should mean that we
won't loop on connect errors for forever.
Signed-off-by: Sean McArthur <sean@seanmonstar.com>
A proxy dispatches requests over a constrained number of routes. When
the router's upper bound is reached---and potentially in other future
scenarios---router capacity is created by removing unused routes, their
load balancers, and all related endpoint stacks.
However, in the current regime, the controller subsystem will continue
to monitor discovery observations. As the number of active observations
expands over time, the controller task ends up with more and more work
to do.
This change introduces a shared atomic boolean between the resolution
returned to the load balancer and the state maintained when
communicating with the service. Before the controller polls its active
resolutions, it first ensures that all unused resolutions are dropped.
A recent upstream change in `tower-h2` (tower-rs/tower-h2@d9b3140) caused some
HTTP/2 streams that were previously terminated by TRAILERS frames to be
terminated by empty DATA frames with the end of stream bit set, instead.
This broke some tests in my dev branch for #944, as our test server also uses
`tower-h2`, and some of the metrics tests were no longer seeing the expected
`StreamResponseEnd` events due to this change. This issue may also occur in
other cases, resulting in incorrect metrics.
This PR changes `MeasuredBody::poll_data` to trigger the Stream End event if
it sees a DATA frame that ends the stream.
Fixes#954
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Oliver Gould
Brian Smith
Eliza Weisman
Sean McArthur
Carl Lerche