From 88d3e83faaba18b24dba233ba2370171d22e1f1a Mon Sep 17 00:00:00 2001 From: Mahati Chamarthy Date: Mon, 8 Aug 2022 15:21:22 +0100 Subject: [PATCH] Populate all error details on authentication failure (#4093) When an application is configured with multiple auth options, respond with auth specific error details on failure. Co-authored-by: Mahati Chamarthy --- cmake/common.cmake | 1 - .../authentication/authentication_types.h | 8 +++++++- .../ccf/endpoints/authentication/empty_auth.h | 5 +++++ .../ccf/endpoints/authentication/jwt_auth.h | 5 +++++ .../ccf/endpoints/authentication/sig_auth.h | 10 ++++++++++ include/ccf/odata_error.h | 16 +++++++++++++++- include/ccf/rpc_context.h | 19 ++++++++++++++++++- samples/apps/logging/logging.cpp | 5 +++++ .../authentication/authentication_types.cpp | 18 ------------------ src/node/rpc/frontend.h | 18 +++++++++++++++++- tests/governance.py | 4 ++-- 11 files changed, 84 insertions(+), 25 deletions(-) delete mode 100644 src/endpoints/authentication/authentication_types.cpp diff --git a/cmake/common.cmake b/cmake/common.cmake index 458978986..d4d56c908 100644 --- a/cmake/common.cmake +++ b/cmake/common.cmake @@ -169,7 +169,6 @@ set(CCF_ENDPOINTS_SOURCES ${CCF_DIR}/src/endpoints/base_endpoint_registry.cpp ${CCF_DIR}/src/endpoints/common_endpoint_registry.cpp ${CCF_DIR}/src/endpoints/json_handler.cpp - ${CCF_DIR}/src/endpoints/authentication/authentication_types.cpp ${CCF_DIR}/src/endpoints/authentication/cert_auth.cpp ${CCF_DIR}/src/endpoints/authentication/empty_auth.cpp ${CCF_DIR}/src/endpoints/authentication/jwt_auth.cpp diff --git a/include/ccf/endpoints/authentication/authentication_types.h b/include/ccf/endpoints/authentication/authentication_types.h index 1ca824bc6..a31b58994 100644 --- a/include/ccf/endpoints/authentication/authentication_types.h +++ b/include/ccf/endpoints/authentication/authentication_types.h @@ -35,10 +35,16 @@ namespace ccf std::string& error_reason) = 0; virtual void set_unauthenticated_error( - std::shared_ptr ctx, std::string&& error_reason); + std::shared_ptr ctx, std::string&& error_reason) + {} virtual std::optional get_openapi_security_schema() const = 0; + + virtual std::string get_security_scheme_name() + { + return "BaseAuthPolicy"; + } }; using AuthnPolicies = std::vector>; diff --git a/include/ccf/endpoints/authentication/empty_auth.h b/include/ccf/endpoints/authentication/empty_auth.h index 00ba38659..11d1e44d8 100644 --- a/include/ccf/endpoints/authentication/empty_auth.h +++ b/include/ccf/endpoints/authentication/empty_auth.h @@ -29,5 +29,10 @@ namespace ccf { return unauthenticated_schema; } + + std::string get_security_scheme_name() override + { + return SECURITY_SCHEME_NAME; + } }; } diff --git a/include/ccf/endpoints/authentication/jwt_auth.h b/include/ccf/endpoints/authentication/jwt_auth.h index b13b05ede..d1ff334a8 100644 --- a/include/ccf/endpoints/authentication/jwt_auth.h +++ b/include/ccf/endpoints/authentication/jwt_auth.h @@ -39,5 +39,10 @@ namespace ccf { return security_schema; } + + std::string get_security_scheme_name() override + { + return SECURITY_SCHEME_NAME; + } }; } diff --git a/include/ccf/endpoints/authentication/sig_auth.h b/include/ccf/endpoints/authentication/sig_auth.h index b45921ddf..2923f052b 100644 --- a/include/ccf/endpoints/authentication/sig_auth.h +++ b/include/ccf/endpoints/authentication/sig_auth.h @@ -47,6 +47,11 @@ namespace ccf { return security_schema; } + + std::string get_security_scheme_name() override + { + return SECURITY_SCHEME_NAME; + } }; struct MemberSignatureAuthnIdentity : public AuthnIdentity @@ -90,5 +95,10 @@ namespace ccf { return security_schema; } + + std::string get_security_scheme_name() override + { + return SECURITY_SCHEME_NAME; + } }; } diff --git a/include/ccf/odata_error.h b/include/ccf/odata_error.h index cf2b1531b..15bca9584 100644 --- a/include/ccf/odata_error.h +++ b/include/ccf/odata_error.h @@ -7,14 +7,28 @@ namespace ccf { + struct ODataErrorDetails + { + std::string auth_policy; + std::string code; + std::string message; + + bool operator==(const ODataErrorDetails&) const = default; + }; + + DECLARE_JSON_TYPE(ODataErrorDetails); + DECLARE_JSON_REQUIRED_FIELDS(ODataErrorDetails, auth_policy, code, message); + struct ODataError { std::string code; std::string message; + std::vector details = {}; }; - DECLARE_JSON_TYPE(ODataError); + DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(ODataError); DECLARE_JSON_REQUIRED_FIELDS(ODataError, code, message); + DECLARE_JSON_OPTIONAL_FIELDS(ODataError, details); struct ODataErrorResponse { diff --git a/include/ccf/rpc_context.h b/include/ccf/rpc_context.h index d85fc2022..e1665900f 100644 --- a/include/ccf/rpc_context.h +++ b/include/ccf/rpc_context.h @@ -112,6 +112,18 @@ namespace ccf set_response_header(name, std::to_string(n)); } + /// Construct OData-formatted response to capture multiple error details + virtual void set_error( + http_status status, + const std::string& code, + std::string&& msg, + std::vector& details) + { + nlohmann::json body = + ccf::ODataErrorResponse{ccf::ODataError{code, std::move(msg), details}}; + set_response(body, status); + } + /// Construct OData-formatted error response. virtual void set_error( http_status status, const std::string& code, std::string&& msg) @@ -124,11 +136,16 @@ namespace ccf { nlohmann::json body = ccf::ODataErrorResponse{ ccf::ODataError{std::move(error.code), std::move(error.msg)}}; + set_response(body, error.status); + } + + virtual void set_response(nlohmann::json& body, http_status status) + { // Set error_handler to replace, to avoid throwing if the error message // contains non-UTF8 characters. Other args are default values const auto s = body.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace); - set_response_status(error.status); + set_response_status(status); set_response_body(std::vector(s.begin(), s.end())); set_response_header( http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON); diff --git a/samples/apps/logging/logging.cpp b/samples/apps/logging/logging.cpp index 92810b5fc..7426b4c9a 100644 --- a/samples/apps/logging/logging.cpp +++ b/samples/apps/logging/logging.cpp @@ -126,6 +126,11 @@ namespace loggingapp // return nullopt return std::nullopt; } + + std::string get_security_scheme_name() override + { + return "CustomAuthPolicy"; + } }; // SNIPPET_END: custom_auth_policy diff --git a/src/endpoints/authentication/authentication_types.cpp b/src/endpoints/authentication/authentication_types.cpp deleted file mode 100644 index 16ec28f32..000000000 --- a/src/endpoints/authentication/authentication_types.cpp +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the Apache 2.0 License. - -#include "ccf/endpoints/authentication/authentication_types.h" - -#include "ccf/rpc_context.h" - -namespace ccf -{ - void AuthnPolicy::set_unauthenticated_error( - std::shared_ptr ctx, std::string&& error_reason) - { - ctx->set_error( - HTTP_STATUS_UNAUTHORIZED, - ccf::errors::InvalidAuthenticationInfo, - std::move(error_reason)); - } -} \ No newline at end of file diff --git a/src/node/rpc/frontend.h b/src/node/rpc/frontend.h index 46a0e1e5d..99db4cb6f 100644 --- a/src/node/rpc/frontend.h +++ b/src/node/rpc/frontend.h @@ -258,6 +258,7 @@ namespace ccf if (!endpoint->authn_policies.empty()) { std::string auth_error_reason; + std::vector error_details; for (const auto& policy : endpoint->authn_policies) { identity = policy->authenticate(tx, ctx, auth_error_reason); @@ -265,13 +266,28 @@ namespace ccf { break; } + else + { + // Collate error details + error_details.push_back( + {policy->get_security_scheme_name(), + ccf::errors::InvalidAuthenticationInfo, + auth_error_reason}); + } } if (identity == nullptr) { - // If none were accepted, let the last set an error + // If none were accepted, let the last set the response header endpoint->authn_policies.back()->set_unauthenticated_error( ctx, std::move(auth_error_reason)); + // Return collated error details for the auth policies declared + // in the request + ctx->set_error( + HTTP_STATUS_UNAUTHORIZED, + ccf::errors::InvalidAuthenticationInfo, + "Invalid info", + error_details); update_metrics(ctx, endpoint); return ctx->serialise_response(); } diff --git a/tests/governance.py b/tests/governance.py index a3a2414a3..347b92334 100644 --- a/tests/governance.py +++ b/tests/governance.py @@ -359,8 +359,8 @@ def test_invalid_client_signature(network, args): ).json() assert r["error"]["code"] == "InvalidAuthenticationInfo" assert ( - expected_error_msg in r["error"]["message"] - ), f"Expected error message '{expected_error_msg}' not in '{r['error']['message']}'" + expected_error_msg in r["error"]["details"][0]["message"] + ), f"Expected error message '{expected_error_msg}' not in '{r['error']['details'][0]['message']}'" # Verify that _some_ HTTP signature parsing errors are communicated back to the client post_proposal_request_raw(