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

proxy: Tap server must only accept incoming connections from controller #2676

Closed
5 tasks
kleimkuhler opened this issue Apr 9, 2019 · 2 comments
Closed
5 tasks

Comments

@kleimkuhler
Copy link
Contributor

kleimkuhler commented Apr 9, 2019

The proxy's tap server must only accept TLS connections from the linkerd-controller. If the l5d-client-id is not the identity of the linkerd-controller, then the connection should be refused.

The following (subject to change) are requirements for this:

Behavior

  • The proxy config needs a new variable who's value is the expected TLS identity of the controller; this would be set when a proxy is injected like the rest of the config values.
  • fn serve_tap should be passed this expected identity so that it can assert equality for each incoming connection
  • If the expected identity does not equal the session.peer_identity(), we should not make a new service (we may wan to log here as well)

Testing

@kleimkuhler kleimkuhler self-assigned this Apr 9, 2019
@kleimkuhler kleimkuhler added this to To do in 2.5 - Tap Hardening via automation Apr 9, 2019
@kleimkuhler kleimkuhler added the priority/P1 Planned for Release label Apr 9, 2019
@kleimkuhler kleimkuhler changed the title Tap must only accept connections from linkerd-controller proxy: Tap must only accept connections from linkerd-controller Apr 9, 2019
@kleimkuhler kleimkuhler changed the title proxy: Tap must only accept connections from linkerd-controller proxy: Tap server must only accept incoming connections from controller Apr 25, 2019
hawkw pushed a commit to linkerd/linkerd2-proxy that referenced this issue May 8, 2019
#243)

This branch adds tls capability to the support cient used in tests. In addition to that it adds two tests verifying that a TLS connection is refused in case the identity is not certified yet. This attempts to fix #linkerd/linkerd2#2598 and provide facility to write tests for linkerd/linkerd2#2676. 

As these are still some of my first lines of Rust code, it is advised to approach everything with a healthy dose of doubt :)

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
hawkw added a commit that referenced this issue May 10, 2019
commit 5f89351081eff47a4ab8cd88e2e1a69a04f86541
Author: Oliver Gould <ver@buoyant.io>
Date:   Thu May 9 16:39:24 2019 -0700

    Upgrade tower dependencies (#249)

    Tower must be updated in order to pickup tower-rs/tower#281
    to address #2804.

    This adopts released crates where possible.

commit 5d5eed6f8180b8db4090d995e71fdf7b0890c647
Author: Zahari Dichev <zaharidichev@gmail.com>
Date:   Thu May 9 01:08:34 2019 +0300

    Assert that TLS connection is refused if identity is not certified yet (#243)

    This branch adds tls capability to the support cient used in tests. In addition to that it adds two tests verifying that a TLS connection is refused in case the identity is not certified yet. This attempts to fix ##2598 and provide facility to write tests for #2676.

    As these are still some of my first lines of Rust code, it is advised to approach everything with a healthy dose of doubt :)

    Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

commit 1b9bb3745e44c959d1d41d14fed2b2822c82b5ba
Author: Oliver Gould <ver@buoyant.io>
Date:   Wed May 8 14:28:37 2019 -0700

    Introduce dispatch timeouts around buffers (#246)

    The proxy has several buffers, especially where it routes requests over
    shared stacks. If any of these routes is unavailable, then a request may
    remain buffered indefinitely. Previously, before service profiles were
    introduced, there was a default _response_ timeout that would cause
    these requests to fail; but since this response timeout is now optional
    (and is only applied once the request has been routed within a proxy),
    then we need a new mechanism to prevent requests from getting "stuck".

    This change does the following:
    - all proxied requests are annotated with a dispatch deadline;
    - each time a request is bufered, a timeout is registered.
    - if the timeout fires, the response exception fails, a 503 is returned,
      and the request is dropped.
    - if the request is processed into the inner stack, the timeout is
      ignored.

    The dispatch timeout limits the _time a request is buffered in a proxy_.
    This is distinct from the response timeout, as the server's response may
    naturally be delayed for any number of (non-proxy-related) reasons.

    The `insert_target` module has been generalized to `insert` to support
    setting the DispatchDeadline extension.

    The `buffer` module has been augmented with generic deadline-extraction
    logic.

    The `svc` module now exposes its own builder type that notably adds
    a `buffer_pending` helper. It's helpful to pull a builder type into the
    proxy to assist debugging type errors when modifying stacks.

    Fixes #2779 #2795

commit caf899557c3b041190f63544da865396231b3e30
Author: Oliver Gould <ver@buoyant.io>
Date:   Fri May 3 15:55:32 2019 -0700

    router: Fail requests when the route is not ready (#241)

    In #2779, we plan to expire requests while they are
    buffered. However, the router _implicitly_ buffers requests in the
    executor when the inner service is not ready.

    This change alters the route to wrap all inner layers in a `LoadShed`
    so it can expect all services to `poll_ready()` immediately.

commit 587bad101d9e5daeacb24b6733097c350a798356
Author: Eliza Weisman <eliza@buoyant.io>
Date:   Fri May 3 14:18:08 2019 -0700

    Remove Destination service query concurrency limit (#244)

    Currently, the proxy enforces a limit on the number of concurrent
    queries (i.e., the number of gRPC streams) to the Destination service.
    This limit was added based on information about the behaviour of the
    Destination service that is now known to be incorrect.

    This branch removes the limit on concurrent queries from the proxy's
    `control::destination` module. Although it should now be possible to
    simplify this code as a result of this change, I've refrained from doing
    any major refactoring in this branch --- my intention is to do this
    after the DNS fallback behaviour has also been removed, as together with
    this change, that will result in a _significant_ simplification of the
    module. Additionally, I've removed the tests for the concurrency limit,
    as they are no longer relevant.

    The `LINKERD2_PROXY_DESTINATION_CLIENT_CONCURRENCY_LIMIT`
    environment variable was also removed; this is not a breaking change as
    neither the CLI nor the proxy injector will currently set this env var.

    Signed-off-by: Eliza Weisman <eliza@buoyant.io>

commit cbdf45b44f7e4d852dc0497716062167ab9539fb
Author: Sean McArthur <sean@buoyant.io>
Date:   Thu May 2 11:47:48 2019 -0700

    Remove h2::Error requirement from metrics

    Signed-off-by: Sean McArthur <sean@buoyant.io>

commit 3276949d4608dc4344b7bed3de2fc4b3080c2c6e
Author: Sean McArthur <sean@buoyant.io>
Date:   Thu May 2 09:44:00 2019 -0700

    delete unused proxy::http::metrics::class module

    Signed-off-by: Sean McArthur <sean@buoyant.io>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue May 10, 2019
commit 5f89351081eff47a4ab8cd88e2e1a69a04f86541
Author: Oliver Gould <ver@buoyant.io>
Date:   Thu May 9 16:39:24 2019 -0700

    Upgrade tower dependencies (#249)

    Tower must be updated in order to pickup tower-rs/tower#281
    to address #2804.

    This adopts released crates where possible.

commit 5d5eed6f8180b8db4090d995e71fdf7b0890c647
Author: Zahari Dichev <zaharidichev@gmail.com>
Date:   Thu May 9 01:08:34 2019 +0300

    Assert that TLS connection is refused if identity is not certified yet (#243)

    This branch adds tls capability to the support cient used in tests. In addition to that it adds two tests verifying that a TLS connection is refused in case the identity is not certified yet. This attempts to fix ##2598 and provide facility to write tests for #2676.

    As these are still some of my first lines of Rust code, it is advised to approach everything with a healthy dose of doubt :)

    Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

commit 1b9bb3745e44c959d1d41d14fed2b2822c82b5ba
Author: Oliver Gould <ver@buoyant.io>
Date:   Wed May 8 14:28:37 2019 -0700

    Introduce dispatch timeouts around buffers (#246)

    The proxy has several buffers, especially where it routes requests over
    shared stacks. If any of these routes is unavailable, then a request may
    remain buffered indefinitely. Previously, before service profiles were
    introduced, there was a default _response_ timeout that would cause
    these requests to fail; but since this response timeout is now optional
    (and is only applied once the request has been routed within a proxy),
    then we need a new mechanism to prevent requests from getting "stuck".

    This change does the following:
    - all proxied requests are annotated with a dispatch deadline;
    - each time a request is bufered, a timeout is registered.
    - if the timeout fires, the response exception fails, a 503 is returned,
      and the request is dropped.
    - if the request is processed into the inner stack, the timeout is
      ignored.

    The dispatch timeout limits the _time a request is buffered in a proxy_.
    This is distinct from the response timeout, as the server's response may
    naturally be delayed for any number of (non-proxy-related) reasons.

    The `insert_target` module has been generalized to `insert` to support
    setting the DispatchDeadline extension.

    The `buffer` module has been augmented with generic deadline-extraction
    logic.

    The `svc` module now exposes its own builder type that notably adds
    a `buffer_pending` helper. It's helpful to pull a builder type into the
    proxy to assist debugging type errors when modifying stacks.

    Fixes #2779 #2795

commit caf899557c3b041190f63544da865396231b3e30
Author: Oliver Gould <ver@buoyant.io>
Date:   Fri May 3 15:55:32 2019 -0700

    router: Fail requests when the route is not ready (#241)

    In #2779, we plan to expire requests while they are
    buffered. However, the router _implicitly_ buffers requests in the
    executor when the inner service is not ready.

    This change alters the route to wrap all inner layers in a `LoadShed`
    so it can expect all services to `poll_ready()` immediately.

commit 587bad101d9e5daeacb24b6733097c350a798356
Author: Eliza Weisman <eliza@buoyant.io>
Date:   Fri May 3 14:18:08 2019 -0700

    Remove Destination service query concurrency limit (#244)

    Currently, the proxy enforces a limit on the number of concurrent
    queries (i.e., the number of gRPC streams) to the Destination service.
    This limit was added based on information about the behaviour of the
    Destination service that is now known to be incorrect.

    This branch removes the limit on concurrent queries from the proxy's
    `control::destination` module. Although it should now be possible to
    simplify this code as a result of this change, I've refrained from doing
    any major refactoring in this branch --- my intention is to do this
    after the DNS fallback behaviour has also been removed, as together with
    this change, that will result in a _significant_ simplification of the
    module. Additionally, I've removed the tests for the concurrency limit,
    as they are no longer relevant.

    The `LINKERD2_PROXY_DESTINATION_CLIENT_CONCURRENCY_LIMIT`
    environment variable was also removed; this is not a breaking change as
    neither the CLI nor the proxy injector will currently set this env var.

    Signed-off-by: Eliza Weisman <eliza@buoyant.io>

commit cbdf45b44f7e4d852dc0497716062167ab9539fb
Author: Sean McArthur <sean@buoyant.io>
Date:   Thu May 2 11:47:48 2019 -0700

    Remove h2::Error requirement from metrics

    Signed-off-by: Sean McArthur <sean@buoyant.io>

commit 3276949d4608dc4344b7bed3de2fc4b3080c2c6e
Author: Sean McArthur <sean@buoyant.io>
Date:   Thu May 2 09:44:00 2019 -0700

    delete unused proxy::http::metrics::class module

    Signed-off-by: Sean McArthur <sean@buoyant.io>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@stale
Copy link

stale bot commented Jul 24, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 24, 2019
@kleimkuhler kleimkuhler moved this from To do to In Progress in 2.5 - Tap Hardening Jul 30, 2019
@kleimkuhler
Copy link
Contributor Author

Closing as the bullets in this issue have been duplicated in separate issues with associated progress updates.

2.5 - Tap Hardening automation moved this from In Progress to Done Jul 30, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

1 participant