configure admin endpoint as a listener/http-filter (e.g. to secure via RBAC) by cstrahan · Pull Request #11367 · envoyproxy/envoy

cstrahan · 2020-05-29T16:46:34Z

Commit Message: configure admin endpoint as a listener/http-filter (e.g. to secure via RBAC)
Additional Description: This adds the ability to configure the admin endpoint listener directly, allowing for RBAC policies to be declared (among other things).
Risk Level: Medium
Testing: in progress
Docs Changes: The admin listener may now be configured by a Listener message.
Release Notes:
Fixes #2763

This is still a work in progress, but I'd like to get feedback on the general approach, and a little assistance with some problems I've run into.

Some things I know need to change:

In AdminFilterConfig::createFilterFactoryFromProtoTyped, I shouldn't be downcasting context.admin() to AdminImpl&, but I don't yet know how I should approach this.
I have yet to clean up class AdminImpl, which currently still implements Network::FilterChainManager, Network::FilterChainFactory, etc (no longer needed, since I'm now using the server's listener_manager_).
Implementing the last point will then require that, in the case the user uses the existing settings (e.g. for the listening address), I'll need to translate those settings into a suitable Listener message (right now I just have a condition to check if a Listener has been provided, and if not, the code falls back to existing logic).
I need to ensure the admin filter is present (and last) in the http filters within the admin config.
I need to prevent the admin filter from being used outside of the admin config.
The following assertion fails, and I'm not sure how to fix:

[2020-05-29 11:35:44.282][1004][critical][assert] [bazel-out/k8-fastbuild/bin/source/common/singleton/_virtual_includes/threadsafe_singleton/common/singleton/threadsafe_singleton.h:56] assert failure: loader_ != nullptr. Details: InjectableSingleton used prior to initialization
[2020-05-29 11:35:44.282][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:104] Caught Aborted, suspect faulting address 0x3e8000003ec
[2020-05-29 11:35:44.282][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2020-05-29 11:35:44.282][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:92] Envoy version: 9acb66888db430d03e8ebc77d3044efb0aa99ade/1.15.0-dev/Modified/DEBUG/BoringSSL
[2020-05-29 11:35:44.317][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #0: Envoy::SignalAction::sigHandler() [0x55c929dbf40c]
[2020-05-29 11:35:44.317][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #1: __restore_rt [0x7f213b706540]
[2020-05-29 11:35:44.349][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #2: Envoy::Server::InstanceImpl::runtime() [0x55c9291b7331]
[2020-05-29 11:35:44.382][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #3: Envoy::Server::ListenerFactoryContextBaseImpl::runtime() [0x55c9291eb0fd]
[2020-05-29 11:35:44.415][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #4: Envoy::Server::PerFilterChainFactoryContextImpl::runtime() [0x55c92923bbcb]
[2020-05-29 11:35:44.448][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #5: Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerConfig::HttpConnectionManagerConfig() [0x55c9292a6386]
[2020-05-29 11:35:44.481][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #6: __gnu_cxx::new_allocator<>::construct<>() [0x55c9292beb76]
[2020-05-29 11:35:44.513][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #7: std::allocator_traits<>::construct<>() [0x55c9292be8c7]
[2020-05-29 11:35:44.545][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #8: std::_Sp_counted_ptr_inplace<>::_Sp_counted_ptr_inplace<>() [0x55c9292be5b1]
[2020-05-29 11:35:44.578][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #9: std::__shared_count<>::__shared_count<>() [0x55c9292be300]
[2020-05-29 11:35:44.610][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #10: std::__shared_ptr<>::__shared_ptr<>() [0x55c9292be192]
[2020-05-29 11:35:44.642][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #11: std::shared_ptr<>::shared_ptr<>() [0x55c9292be0b7]
[2020-05-29 11:35:44.675][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #12: std::allocate_shared<>() [0x55c9292bdf69]
[2020-05-29 11:35:44.707][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #13: std::make_shared<>() [0x55c9292acd3d]
[2020-05-29 11:35:44.740][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #14: Envoy::Extensions::NetworkFilters::HttpConnectionManager::Utility::createConfig() [0x55c9292a59dc]
[2020-05-29 11:35:44.772][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #15: Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped() [0x55c9292a5ab3]
[2020-05-29 11:35:44.805][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #16: Envoy::Extensions::NetworkFilters::Common::FactoryBase<>::createFilterFactoryFromProto() [0x55c9292b0d3c]
[2020-05-29 11:35:44.838][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #17: Envoy::Server::ProdListenerComponentFactory::createNetworkFilterFactoryList_() [0x55c929203cf9]
[2020-05-29 11:35:44.871][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #18: Envoy::Server::ProdListenerComponentFactory::createNetworkFilterFactoryList() [0x55c929219768]
[2020-05-29 11:35:44.903][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #19: Envoy::Server::ListenerFilterChainFactoryBuilder::buildFilterChainInternal() [0x55c92920e56d]
[2020-05-29 11:35:44.936][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #20: Envoy::Server::ListenerFilterChainFactoryBuilder::buildFilterChain() [0x55c92920e1c3]
[2020-05-29 11:35:44.969][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #21: Envoy::Server::FilterChainManagerImpl::addFilterChain() [0x55c92923cc67]
[2020-05-29 11:35:45.001][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #22: Envoy::Server::ListenerImpl::buildFilterChains() [0x55c9291ed85f]
[2020-05-29 11:35:45.034][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #23: Envoy::Server::ListenerImpl::ListenerImpl() [0x55c9291ec067]
[2020-05-29 11:35:45.067][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #24: std::make_unique<>() [0x55c929217c15]
[2020-05-29 11:35:45.100][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #25: Envoy::Server::ListenerManagerImpl::addOrUpdateListenerInternal() [0x55c929208a9d]
[2020-05-29 11:35:45.133][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #26: Envoy::Server::ListenerManagerImpl::addOrUpdateListener() [0x55c929207e70]
[2020-05-29 11:35:45.166][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #27: Envoy::Server::InstanceImpl::initialize() [0x55c9291b1def]
[2020-05-29 11:35:45.198][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #28: Envoy::Server::InstanceImpl::InstanceImpl() [0x55c9291ae861]
[2020-05-29 11:35:45.230][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #29: std::make_unique<>() [0x55c9279292a9]
[2020-05-29 11:35:45.262][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #30: Envoy::MainCommonBase::MainCommonBase() [0x55c927926bfe]
[2020-05-29 11:35:45.295][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #31: Envoy::MainCommon::MainCommon() [0x55c92792783d]
[2020-05-29 11:35:45.328][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #32: std::make_unique<>() [0x55c9278d29d2]
[2020-05-29 11:35:45.360][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #33: main [0x55c9278d1f5f]
[2020-05-29 11:35:45.360][1004][critical][backtrace] [bazel-out/k8-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #34: __libc_start_main [0x7f213b5271e3]

That's with this config:

admin:
  access_log_path: /tmp/admin_access.log
  listener:
    address:
      socket_address:
        protocol: TCP
        address: 127.0.0.1
        port_value: 9901
    filter_chains:
      filters:
      - name: envoy.http_connection_manager
        typed_config:
          "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
          codec_type: auto
          stat_prefix: admin
          route_config:
            virtual_hosts:
          http_filters:
            - name: envoy.filters.http.admin
static_resources:
  listeners:
  - name: listener_0
    address:
      socket_address:
        protocol: TCP
        address: 0.0.0.0
        port_value: 10000
    filter_chains:
    - filters:
      - name: envoy.filters.network.http_connection_manager
        typed_config:
          "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
          stat_prefix: ingress_http
          route_config:
            name: local_route
            virtual_hosts:
            - name: local_service
              domains: ["*"]
              routes:
              - match:
                  prefix: "/"
                route:
                  host_rewrite: www.google.com
                  cluster: service_google
          http_filters:
          - name: envoy.filters.http.router
  clusters:
  - name: service_google
    connect_timeout: 0.25s
    type: LOGICAL_DNS
    dns_lookup_family: V4_ONLY
    lb_policy: ROUND_ROBIN
    load_assignment:
      cluster_name: service_google
      endpoints:
      - lb_endpoints:
        - endpoint:
            address:
              socket_address:
                protocol: TCP
                address: www.google.com
                port_value: 443
    transport_socket:
      name: envoy.transport_sockets.tls
      typed_config:
        "@type": type.googleapis.com/envoy.api.v2.auth.UpstreamTlsContext
        sni: www.google.com

repokitteh-read-only · 2020-05-29T16:46:40Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11367 was opened by cstrahan.

see: more, trace.

There are various singletons that must be initialized before the admin listener can be added.

htuch · 2020-06-01T11:24:55Z

api/envoy/config/bootstrap/v3/bootstrap.proto

  repeated core.v3.SocketOption socket_options = 4;
+
+  // Static :ref:`Listener <envoy_api_msg_config.listener.v3.Listener>`.
+  listener.v3.Listener listener = 5;


One question I have on this API is whether we should have a Listener embedded in admin or somehow provide an admin filter that can be hooked into an arbitrary listener? I suspect what you have is the right thing given how admin works today, since it probably wants to be running on the main thread for thread safety with some of the data structures it serves. CC @envoyproxy/api-shepherds

However, by placing a listener here, it's providing a bigger "hole" than is needed. Listeners can be configured to do lots of things, e.g. act as a Thrift proxy, and only a narrow slice of this configuration will apply. Can you document how Listener is intended to be used when configured inside the Admin message?

My gut reaction is this config is right, but we should document the restrictions of the listener config which can be checked in code. I had discussed this offline with @cstrahan. So yeah +1 for documentation on the restrictions of the listener object and all of the check we should do in code on the config.

I think I'd rather see it as a normal http filter attached to a normal listener. For things that must happen on the main thread, have the filter send a message back and forth for that work.

I'm worried that running arbitrary code on the main thread will expose issues because nobody intended all the non-admin filters or listener code to run on the main thread.

For things that must happen on the main thread, have the filter send a message back and forth for that work.

I think this will likely end up being a really huge effort, though it's technically possible for sure.

Another way of accomplishing the same thing is to post the entire request from the worker to the main thread, and post the response back to the worker. So leave everything done by the current admin endpoint on the main thread, and leave all the normal listener and filter-chain stuff on the worker thread.

stale · 2020-06-09T19:12:21Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-06-17T08:11:01Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

cstrahan · 2020-06-25T20:22:33Z

Can we re-open this? Also, in which direction should this move: a regular http filter on a regular listener (as per @ggreenway's suggestion), or remain a special listener configured within the existing Admin config (as discussed with @mattklein123 earlier, and as the PR currently stands)?

mattklein123 · 2020-06-25T22:28:28Z

Also, in which direction should this move: a regular http filter on a regular listener (as per @ggreenway's suggestion), or remain a special listener configured within the existing Admin config (as discussed with @mattklein123 earlier, and as the PR currently stands)?

My feeling is that it would be simpler if this were a special listener. I think dong this is a normal listener with all of the posting back and forth is likely to be more complicated than we would like, though it's certainly possible. @ggreenway?

ggreenway · 2020-06-26T15:43:10Z

Also, in which direction should this move: a regular http filter on a regular listener (as per @ggreenway's suggestion), or remain a special listener configured within the existing Admin config (as discussed with @mattklein123 earlier, and as the PR currently stands)?

My feeling is that it would be simpler if this were a special listener. I think dong this is a normal listener with all of the posting back and forth is likely to be more complicated than we would like, though it's certainly possible. @ggreenway?

My feeling is the opposite: we'll run into more corner cases by running all the regular listener/filter code on the main thread. I think it can be limited to a single post in each direction, roughly what is done currently for AdminFilter::onComplete() in calling admin_server_callback_func_().

Also, after thinking about this more, I think it would make the most sense for this to be a Route FilterAction so that existing routing could be applied, if needed. However, FilterAction isn't implemented yet, so that may be out of scope for this.

I don't feel super strongly about this; Matt, if you do feel strongly, let's do it on the main thread.

mattklein123 · 2020-06-26T15:51:29Z

I don't feel super strongly about this; Matt, if you do feel strongly, let's do it on the main thread.

I don't feel that strongly either. I do have some concerns about whether a listener gets delivered over LDS and gets removed over post, multiple listeners that have the admin filter, etc., but maybe it doesn't matter if we are posting everything and are careful about using weak pointers and locking, etc. @cstrahan do you want to give it a shot and see how it goes?

justincely · 2020-07-02T15:35:53Z

@cstrahan bump

ggreenway · 2020-07-06T20:10:25Z

/wait

cstrahan · 2020-07-07T16:00:51Z

My feeling is that it would be simpler if this were a special listener. I think dong this is a normal listener with all of the posting back and forth is likely to be more complicated than we would like, though it's certainly possible. @ggreenway?

My feeling is the opposite: we'll run into more corner cases by running all the regular listener/filter code on the main thread. I think it can be limited to a single post in each direction, roughly what is done currently for AdminFilter::onComplete() in calling admin_server_callback_func_().

@cstrahan do you want to give it a shot and see how it goes?

Yes, I'll give that a go and report back -- thanks for the feedback, @ggreenway & @mattklein123.

stale · 2020-07-18T18:56:00Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

justincely · 2020-07-23T16:06:57Z

@cstrahan status please?

cstrahan · 2020-07-31T14:33:16Z

Just a heads up that I'm working through the suggested changes. I'll report back soon with any new developments, or new open questions.

stale · 2020-08-08T09:29:41Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-08-23T05:26:16Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

chadr123 · 2020-09-01T05:54:15Z

Is it still in progress?

justincely · 2020-09-01T13:29:40Z

@cstrahan is still working on it, he can report on any progress

cstrahan · 2020-09-04T16:07:54Z

This is slow going, but this is still in the works.

maxres-ch · 2023-03-14T20:28:04Z

@cstrahan do you recall what happened to this?

secure admin

2aa5df9

repokitteh-read-only bot added the api label May 29, 2020

fix admin listener initialization

4eb88e1

There are various singletons that must be initialized before the admin listener can be added.

mattklein123 assigned ggreenway May 30, 2020

htuch reviewed Jun 1, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 9, 2020

stale bot closed this Jun 17, 2020

mattklein123 reopened this Jun 25, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 25, 2020

repokitteh-read-only bot added the waiting label Jul 6, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 18, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 31, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 8, 2020

stale bot closed this Aug 23, 2020

ggreenway mentioned this pull request Apr 22, 2022

admin: Richer HTML home page with forms for params #19546

Merged

ggreenway mentioned this pull request Mar 14, 2024

admin: add streaming variant of the admin API #32346

Merged

jmarantz mentioned this pull request Mar 18, 2024

Admin endpoint security #2763

Open

Conversation

cstrahan commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 29, 2020

Uh oh!

htuch Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

ggreenway Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

ggreenway Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jun 9, 2020

Uh oh!

stale bot commented Jun 17, 2020

Uh oh!

cstrahan commented Jun 25, 2020

Uh oh!

mattklein123 commented Jun 25, 2020

Uh oh!

ggreenway commented Jun 26, 2020

Uh oh!

mattklein123 commented Jun 26, 2020

Uh oh!

justincely commented Jul 2, 2020

Uh oh!

ggreenway commented Jul 6, 2020

Uh oh!

cstrahan commented Jul 7, 2020

Uh oh!

stale bot commented Jul 18, 2020

Uh oh!

justincely commented Jul 23, 2020

Uh oh!

cstrahan commented Jul 31, 2020

Uh oh!

stale bot commented Aug 8, 2020

Uh oh!

stale bot commented Aug 23, 2020

Uh oh!

chadr123 commented Sep 1, 2020

Uh oh!

justincely commented Sep 1, 2020

Uh oh!

cstrahan commented Sep 4, 2020

Uh oh!

maxres-ch commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cstrahan commented May 29, 2020 •

edited

Loading