Merge pull request #23832 from vespa-engine/vekterli/capability-filtering-of-content-status-pages

Add capability filtering for content layer status pages and metrics [run-systemtest]

9 files changed, 78 insertions, 34 deletions

diff --git a/fnet/src/vespa/fnet/frt/require_capabilities.cpp b/fnet/src/vespa/fnet/frt/require_capabilities.cpp
index 5f87f98436e..6996557c91e 100644
--- a/fnet/src/vespa/fnet/frt/require_capabilities.cpp
+++ b/fnet/src/vespa/fnet/frt/require_capabilities.cpp
@@ -29,7 +29,7 @@ FRT_RequireCapabilities::allow(FRT_RPCRequest& req) const noexcept
                                   "Peer at %s with %s. Call requires %s, but peer has %s",
               ((mode == CapabilityEnforcementMode::LogOnly) ? "(Dry-run only, not enforced): " : ""),
               method_name.c_str(), peer_spec.c_str(),
-              to_string(auth_ctx.peer_credentials()).c_str(),
+              auth_ctx.peer_credentials().to_string().c_str(),
               _required_capabilities.to_string().c_str(),
               auth_ctx.capabilities().to_string().c_str());
         return (mode != CapabilityEnforcementMode::Enforce);
diff --git a/storage/src/vespa/storage/common/statusmetricconsumer.h b/storage/src/vespa/storage/common/statusmetricconsumer.h
index 3da6bd3151a..337c3ea7ff0 100644
--- a/storage/src/vespa/storage/common/statusmetricconsumer.h
+++ b/storage/src/vespa/storage/common/statusmetricconsumer.h
@@ -34,6 +34,10 @@ public:
             const std::string& name = "status");
     ~StatusMetricConsumer() override;
 
+    // Metric reporting requires the "vespa.content.metrics_api" capability
+    CapabilitySet required_capabilities() const noexcept override {
+        return CapabilitySet::of({ Capability::content_metrics_api() });
+    }
     vespalib::string getReportContentType(const framework::HttpUrlPath&) const override;
     bool reportStatus(std::ostream& out, const framework::HttpUrlPath&) const override;
     void updateMetrics(const MetricLockGuard & guard) override;
diff --git a/storage/src/vespa/storage/frameworkimpl/status/statuswebserver.cpp b/storage/src/vespa/storage/frameworkimpl/status/statuswebserver.cpp
index a9b7a727a5b..7139ab0eb41 100644
--- a/storage/src/vespa/storage/frameworkimpl/status/statuswebserver.cpp
+++ b/storage/src/vespa/storage/frameworkimpl/status/statuswebserver.cpp
@@ -6,6 +6,7 @@
 #include <vespa/vespalib/util/exceptions.h>
 #include <vespa/vespalib/util/size_literals.h>
 #include <vespa/vespalib/component/vtag.h>
+#include <vespa/vespalib/net/connection_auth_context.h>
 #include <vespa/vespalib/net/crypto_engine.h>
 #include <vespa/config/subscription/configuri.h>
 #include <vespa/config/helper/configfetcher.hpp>
@@ -45,15 +46,15 @@ StatusWebServer::~StatusWebServer()
 void StatusWebServer::configure(std::unique_ptr<vespa::config::content::core::StorStatusConfig> config)
 {
     int newPort = config->httpport;
-        // If server is already running, ignore config updates that doesn't
-        // alter port, or suggests random port.
+    // If server is already running, ignore config updates that doesn't
+    // alter port, or suggests random port.
     if (_httpServer) {
         if (newPort == 0 || newPort == _port) return;
     }
-        // Try to create new server before destroying old.
+    // Try to create new server before destroying old.
     LOG(info, "Starting status web server on port %u.", newPort);
     std::unique_ptr<WebServer> server;
-        // Negative port number means don't run the web server
+    // Negative port number means don't run the web server
     if (newPort >= 0) {
         try {
             server = std::make_unique<WebServer>(*this, newPort);
@@ -146,6 +147,25 @@ StatusWebServer::getListenPort() const
 }
 
 void
+StatusWebServer::invoke_reporter(const framework::StatusReporter& reporter,
+                                 const framework::HttpUrlPath& url_path,
+                                 vespalib::Portal::GetRequest& request)
+{
+    try {
+        std::ostringstream content;
+        auto content_type = reporter.getReportContentType(url_path);
+        if (reporter.reportStatus(content, url_path)) {
+            request.respond_with_content(content_type, content.str());
+        } else {
+            request.respond_with_error(404, "Not Found");
+        }
+    } catch (std::exception &e) {
+        LOG(warning, "Internal Server Error: %s", e.what());
+        request.respond_with_error(500, "Internal Server Error");
+    }
+}
+
+void
 StatusWebServer::handlePage(const framework::HttpUrlPath& urlpath, vespalib::Portal::GetRequest request)
 {
     vespalib::string link(urlpath.getPath());
@@ -157,22 +177,25 @@ StatusWebServer::handlePage(const framework::HttpUrlPath& urlpath, vespalib::Por
     if ( ! link.empty()) {
         const framework::StatusReporter *reporter = _reporterMap.getStatusReporter(link);
         if (reporter != nullptr) {
-            try {
-                std::ostringstream content;
-                auto content_type = reporter->getReportContentType(urlpath);
-                if (reporter->reportStatus(content, urlpath)) {
-                    request.respond_with_content(content_type, content.str());
-                } else {
-                    request.respond_with_error(404, "Not Found");
-                }
-            } catch (std::exception &e) {
-                LOG(warning, "Internal Server Error: %s", e.what());
-                request.respond_with_error(500, "Internal Server Error");
+            const auto& auth_ctx = request.auth_context();
+            if (auth_ctx.capabilities().contains_all(reporter->required_capabilities())) {
+                invoke_reporter(*reporter, urlpath, request);
+            } else {
+                // TODO should print peer address as well; not currently exposed
+                LOG(warning, "Peer with %s denied status page access to '%s' due to insufficient "
+                             "credentials (had %s, needed %s)",
+                    auth_ctx.peer_credentials().to_string().c_str(),
+                    link.c_str(), auth_ctx.capabilities().to_string().c_str(),
+                    reporter->required_capabilities().to_string().c_str());
+                request.respond_with_error(403, "Forbidden");
             }
         } else {
             request.respond_with_error(404, "Not Found");
         }
     } else {
+        // TODO should the index page be capability-restricted? Would be a bit strange if the root
+        //  index '/' page requires status capabilities but '/metrics' does not.
+        //  The index page only leaks the Vespa version and node type (inferred by reporter set).
         IndexPageReporter indexRep;
         indexRep << "<p><b>Binary version of Vespa:</b> "
                  << vespalib::Vtag::currentVersion.toString()
diff --git a/storage/src/vespa/storage/frameworkimpl/status/statuswebserver.h b/storage/src/vespa/storage/frameworkimpl/status/statuswebserver.h
index 429f5249441..26db7ff5069 100644
--- a/storage/src/vespa/storage/frameworkimpl/status/statuswebserver.h
+++ b/storage/src/vespa/storage/frameworkimpl/status/statuswebserver.h
@@ -24,6 +24,7 @@ namespace config {
 namespace storage {
 
 namespace framework {
+    struct StatusReporter;
     struct StatusReporterMap;
     struct ThreadHandle;
     struct ComponentRegister;
@@ -35,10 +36,10 @@ namespace framework {
 class StatusWebServer : private config::IFetcherCallback<vespa::config::content::core::StorStatusConfig>
 {
     class WebServer : public vespalib::Portal::GetHandler {
-        StatusWebServer& _status;
-        vespalib::Portal::SP _server;
+        StatusWebServer&              _status;
+        vespalib::Portal::SP          _server;
         vespalib::ThreadStackExecutor _executor;
-        vespalib::Portal::Token::UP _root;
+        vespalib::Portal::Token::UP   _root;
 
     public:
         WebServer(StatusWebServer&, uint16_t port);
@@ -81,6 +82,9 @@ public:
     int getListenPort() const;
     void handlePage(const framework::HttpUrlPath&, vespalib::Portal::GetRequest request);
 private:
+    void invoke_reporter(const framework::StatusReporter&,
+                         const framework::HttpUrlPath&,
+                         vespalib::Portal::GetRequest&);
     void configure(std::unique_ptr<vespa::config::content::core::StorStatusConfig> config) override;
 };
 
diff --git a/storage/src/vespa/storageframework/generic/status/statusreporter.h b/storage/src/vespa/storageframework/generic/status/statusreporter.h
index cc48bb841fd..3f84d5e8ae4 100644
--- a/storage/src/vespa/storageframework/generic/status/statusreporter.h
+++ b/storage/src/vespa/storageframework/generic/status/statusreporter.h
@@ -16,12 +16,15 @@
 
 #include <ostream>
 #include <vespa/storageframework/generic/status/httpurlpath.h>
+#include <vespa/vespalib/net/tls/capability_set.h>
 #include <vespa/vespalib/stllike/string.h>
 
 namespace storage::framework {
 
-struct StatusReporter
-{
+struct StatusReporter {
+    using Capability    = vespalib::net::tls::Capability;
+    using CapabilitySet = vespalib::net::tls::CapabilitySet;
+
     StatusReporter(vespalib::stringref id, vespalib::stringref name);
     virtual ~StatusReporter();
 
@@ -40,6 +43,16 @@ struct StatusReporter
     virtual bool isValidStatusRequest() const { return true; }
 
     /**
+     * By default, a status reporter requires the "vespa.content.status_pages" client capability.
+     * This can be overridden by subclasses to require reporter-specific capabilities
+     * (or none at all). If the client does not satisfy the required capabilities, a
+     * "403 Forbidden" error response will be returned to the client.
+     */
+    virtual CapabilitySet required_capabilities() const noexcept {
+        return CapabilitySet::of({ Capability::content_status_pages() });
+    }
+
+    /**
      * Called to get content type.
      * An empty string indicates page not found.
      */
diff --git a/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp b/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
index 3d19c335c19..0178443643e 100644
--- a/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
+++ b/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
@@ -622,8 +622,8 @@ TEST_F("Peer credentials are propagated to CryptoCodec", CertFixture) {
     auto& client_creds = f.server->peer_credentials();
     auto& server_creds = f.client->peer_credentials();
 
-    fprintf(stderr, "Client credentials (observed by server): %s\n", to_string(client_creds).c_str());
-    fprintf(stderr, "Server credentials (observed by client): %s\n", to_string(server_creds).c_str());
+    fprintf(stderr, "Client credentials (observed by server): %s\n", client_creds.to_string().c_str());
+    fprintf(stderr, "Server credentials (observed by client): %s\n", server_creds.to_string().c_str());
 
     EXPECT_EQUAL("rockets.wile.example.com", client_creds.common_name);
     ASSERT_EQUAL(2u, client_creds.dns_sans.size());
diff --git a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.cpp b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.cpp
index d7977f6cd2a..e088eeb4906 100644
--- a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.cpp
@@ -482,7 +482,7 @@ bool OpenSslTlsContextImpl::verify_trusted_certificate(::X509_STORE_CTX* store_c
             // Buffer warnings on peer IP address to avoid log flooding.
             LOGBT(warning, codec_impl.peer_address().ip_address(),
                   "Certificate verification of peer '%s' failed with %s",
-                  codec_impl.peer_address().spec().c_str(), to_string(creds).c_str());
+                  codec_impl.peer_address().spec().c_str(), creds.to_string().c_str());
             return (authz_mode != AuthorizationMode::Enforce);
         }
         // Store away credentials and role set for later use by requests that arrive over this connection.
diff --git a/vespalib/src/vespa/vespalib/net/tls/peer_credentials.cpp b/vespalib/src/vespa/vespalib/net/tls/peer_credentials.cpp
index 9a001e24fea..92854bdd7d5 100644
--- a/vespalib/src/vespa/vespalib/net/tls/peer_credentials.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/peer_credentials.cpp
@@ -14,7 +14,7 @@ PeerCredentials& PeerCredentials::operator=(PeerCredentials&&) noexcept = defaul
 PeerCredentials::~PeerCredentials() = default;
 
 std::ostream& operator<<(std::ostream& os, const PeerCredentials& creds) {
-    os << to_string(creds);
+    os << creds.to_string();
     return os;
 }
 
@@ -36,20 +36,20 @@ void emit_comma_separated_string_list(asciistream& os, stringref title,
 }
 }
 
-vespalib::string to_string(const PeerCredentials& creds) {
+vespalib::string PeerCredentials::to_string() const {
     asciistream os;
     os << "PeerCredentials(";
     bool emit_comma = false;
-    if (!creds.common_name.empty()) {
-        os << "CN '" << creds.common_name << "'";
+    if (!common_name.empty()) {
+        os << "CN '" << common_name << "'";
         emit_comma = true;
     }
-    if (!creds.dns_sans.empty()) {
-        emit_comma_separated_string_list(os, "DNS SANs", creds.dns_sans, emit_comma);
+    if (!dns_sans.empty()) {
+        emit_comma_separated_string_list(os, "DNS SANs", dns_sans, emit_comma);
         emit_comma = true;
     }
-    if (!creds.uri_sans.empty()) {
-        emit_comma_separated_string_list(os, "URI SANs", creds.uri_sans, emit_comma);
+    if (!uri_sans.empty()) {
+        emit_comma_separated_string_list(os, "URI SANs", uri_sans, emit_comma);
     }
     os << ')';
     return os.str();
diff --git a/vespalib/src/vespa/vespalib/net/tls/peer_credentials.h b/vespalib/src/vespa/vespalib/net/tls/peer_credentials.h
index b81772d2bce..22c98c023b5 100644
--- a/vespalib/src/vespa/vespalib/net/tls/peer_credentials.h
+++ b/vespalib/src/vespa/vespalib/net/tls/peer_credentials.h
@@ -23,10 +23,10 @@ struct PeerCredentials {
     PeerCredentials(PeerCredentials&&) noexcept;
     PeerCredentials& operator=(PeerCredentials&&) noexcept;
     ~PeerCredentials();
+
+    vespalib::string to_string() const;
 };
 
 std::ostream& operator<<(std::ostream&, const PeerCredentials&);
 
-vespalib::string to_string(const PeerCredentials&);
-
 }