Replace fs-watch based TLS with new Identity service#213
Conversation
Previously, the proxy accepted an identity template and namespace and merged them during configuration. We can rely on external configuration systems to do this templating, and so we simplify the proxy by no longer taking a pod namespace environment and instead taking a `LINKERD2_PROXY_LOCAL_IDENTITY`.
Signed-off-by: Sean McArthur <[email protected]>
Dockerfile
Outdated
|
|
||
| ARG RUST_IMAGE=rust:1.32.0 | ||
| ARG RUNTIME_IMAGE=gcr.io/linkerd-io/base:2017-10-30.01 | ||
| ARG RUNTIME_IMAGE=gcr.io/linkerd-io/proxy:git-820f9cd8 |
There was a problem hiding this comment.
Note that this dockerfile now uses a proxy image so that we can just overwrite the binary. This is now necessary because we require utilities built in the other repo.
|
I should note that three tests are now disabled... two of which require a mock identity service implementation for integration tests |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
This diff is pretty hard to review because it's quite large and a lot of code was moved around. However, it seems good to me.
I commented on a number of nits that are generally not blockers (with the exception of removing the #![allow(...)]` attribute which we should definitely do prior to merging).
| // ===== impl Config ===== | ||
|
|
||
| impl Config { | ||
| impl dns::ConfigureResolver for Config { |
There was a problem hiding this comment.
I like the change to traits for configuration --- I suspect this will make writing tests for certain configurations much more pleasant.
|
|
||
| let metrics_retain_idle = parse(strings, ENV_METRICS_RETAIN_IDLE, parse_duration); | ||
|
|
||
| let dns_min_ttl = parse(strings, ENV_DNS_MIN_TTL, parse_duration); |
There was a problem hiding this comment.
This is relatively minor, but I notice that we've basically namespaced config values in the Config structs by prepending the thing we're configuring to the name; i.e. dns_{min_ttl, max_ttl, canonicalize_timeout} and so on. I wonder if it's worthwhile to change this so that Config is composed of sub-structs, like
struct Config {
dns_config: DnsConfig,
...
}
struct DnsConfig {
min_ttl: Duration,
max_ttl: Duration,
canonicalize_timeout: Duration,
...
}
// and so onThis might make the places where the config is actually used a bit simpler --- I've noticed that sometimes it's necessary to borrow the entire config struct to access one field, making the code in main a bit more convoluted than it needs to be; similarly we have several functions that take a number of related config fields as individual arguments to avoid borrowing the whole Config structure. Grouping related config fields into structs might make this code a little less painful, as we could (for example) just pass &config.dns_config into the function that constructs a DNS resolver, and so on.
This is admittedly not high priority. I'm only bringing it up now because I've put it off for a while to avoid changing config.rs, and I thought that as this branch is making a lot of major changes to the config module, we might want to consider some refactoring.
There was a problem hiding this comment.
Yeah, I think this is a good general direction -- each app module should have its own config struct, and then the main config i sresponsible for parsing/validating them.
|
|
||
| let disabled = strings | ||
| .get(ENV_IDENTITY_DISABLED)? | ||
| .map(|d| !d.is_empty()) |
There was a problem hiding this comment.
I'm not sure how I feel about the implication that LINKERD_PROXY_IDENTITY_DISABLED=false will disable identity, but this isn't user facing so 🤷♀️
| li?, | ||
| tok?, | ||
| min_refresh?, | ||
| max_refresh?, |
There was a problem hiding this comment.
Nit: It's possible that this match is the clearest way to represent this logic, but I find it a little hard to follow. Can't really come up with a better way to do it though...
| min_refresh, | ||
| max_refresh, | ||
| ) => { | ||
| let key = { |
There was a problem hiding this comment.
TIOLI: maybe the common logic for parsing the key and the CSR could be factored out into a function? Not a huge deal.
Co-Authored-By: olix0r <[email protected]>
3 participants
This change replaces the proxy's TLS configuration logic so that certificates
are dynamically provisioned by the proxy via a new Identity controller service.
When the proxy processes its configuration, it now requires that a
LINKERD2_PROXY_IDENTITY_DISABLEDenvironment is set; oterhwise,a valid identity configuration must be provided. An identity configuration
includes:
LINKERD2_PROXY_IDENTITY_TRUST_ANCHORSenvironment;LINKERD2_PROXY_IDENTITY_DIR, containing twofiles:
key.p8andcsr.der. The key includes a PKCS#8-encoded privatekey; and the CSR includes a certificate request, signed by the private key,
that can be sent to the identity service;
LINKERD2_PROXY_IDENTITY_TOKEN_FILEthat the proxy will read each time it requests a new certificate.
The
linkerd2_fs_watchcrate is no longer used and should probably be splitout into a utility (perhaps tokio-fs?).
Various other configuration options have changed for consistency:
controller clients) has been renamed appropriately.
Once the configuration is processed---if Identity is not completely
disabled---then a client is established to the identity service and its
identity is validated. As a certificate is acquired, it is validated and
published into the listeners and connect stacks (rather than into the
routing stack). This removes the need for rebinding watch layers, since these
connection layers lazily acquire the proper configuration.
When a certificate has not yet been acquired, then ALL inbound TLS
connections that should be terminated by the process fail, and all outbound
connections are server-validated but no client identity is provided. Once the
certificate is acquired, client identities are provided on subsequent
connections and the listener will properly accept connections.
Depends on linkerd/linkerd2-proxy-api#25