From b3625cb90045ea0d28b789d858752da2f08be76e Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Wed, 18 Nov 2020 16:21:28 +0000 Subject: [PATCH 01/18] auto-refresh jwt keys (wip) --- python/ccf/proposal_generator.py | 2 + src/enclave/interface.h | 5 +- src/host/main.cpp | 10 ++ src/node/jwt.h | 6 +- src/node/node_state.h | 174 +++++++++++++++++++++++++++++++ src/node/rpc/member_frontend.h | 50 +++++++++ 6 files changed, 244 insertions(+), 3 deletions(-) diff --git a/python/ccf/proposal_generator.py b/python/ccf/proposal_generator.py index 7f4af6d7f48f..a5cd920e8455 100644 --- a/python/ccf/proposal_generator.py +++ b/python/ccf/proposal_generator.py @@ -458,6 +458,8 @@ def set_jwt_issuer(json_path: str, **kwargs): "issuer": obj["issuer"], "key_filter": obj.get("key_filter", "all"), "key_policy": obj.get("key_policy"), + "ca_cert_name": obj.get("ca_cert_name"), + "auto_refresh": obj.get("auto_refresh", False), "jwks": obj.get("jwks"), } return build_proposal("set_jwt_issuer", args, **kwargs) diff --git a/src/enclave/interface.h b/src/enclave/interface.h index 76a5d6c27d1c..a2cdf0dadf51 100644 --- a/src/enclave/interface.h +++ b/src/enclave/interface.h @@ -80,6 +80,8 @@ struct CCFConfig std::string subject_name; std::vector subject_alternative_names; + size_t jwt_key_refresh_interval; + MSGPACK_DEFINE( consensus_config, node_info_network, @@ -90,7 +92,8 @@ struct CCFConfig genesis, joining, subject_name, - subject_alternative_names); + subject_alternative_names, + jwt_key_refresh_interval); }; /// General administrative messages diff --git a/src/host/main.cpp b/src/host/main.cpp index c273a3789c09..0d3780259f3d 100644 --- a/src/host/main.cpp +++ b/src/host/main.cpp @@ -318,6 +318,14 @@ int main(int argc, char** argv) "Subject Alternative Name in node certificate. Can be either " "iPAddress:xxx.xxx.xxx.xxx, or dNSName:sub.domain.tld"); + size_t jwt_key_refresh_interval = 1800; + app + .add_option( + "--jwt-key-refresh-interval-s", + jwt_key_refresh_interval, + "Interval in seconds for JWT public signing key refresh.") + ->capture_default_str(); + size_t memory_reserve_startup = 0; app .add_option( @@ -653,6 +661,8 @@ int main(int argc, char** argv) ccf_config.subject_name = subject_name; ccf_config.subject_alternative_names = subject_alternative_names; + ccf_config.jwt_key_refresh_interval = jwt_key_refresh_interval; + if (*start) { start_type = StartType::New; diff --git a/src/node/jwt.h b/src/node/jwt.h index c595b99e9e60..f90c180e6aaf 100644 --- a/src/node/jwt.h +++ b/src/node/jwt.h @@ -41,13 +41,15 @@ namespace ccf { JwtIssuerKeyFilter key_filter; std::optional key_policy; + std::optional ca_cert_name; + bool auto_refresh = false; - MSGPACK_DEFINE(key_filter, key_policy); + MSGPACK_DEFINE(key_filter, key_policy, ca_cert_name, auto_refresh); }; DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(JwtIssuerMetadata); DECLARE_JSON_REQUIRED_FIELDS(JwtIssuerMetadata, key_filter); - DECLARE_JSON_OPTIONAL_FIELDS(JwtIssuerMetadata, key_policy); + DECLARE_JSON_OPTIONAL_FIELDS(JwtIssuerMetadata, key_policy, ca_cert_name, auto_refresh); using JwtIssuer = std::string; using JwtKeyId = std::string; diff --git a/src/node/node_state.h b/src/node/node_state.h index 84d923a00f8c..bd2c0012f5f4 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -596,6 +596,180 @@ namespace ccf std::chrono::milliseconds(config.joining.join_timer)); } + void refresh_jwt_keys() + { + auto tx = network.tables->create_read_only_tx(); + auto jwt_issuers_view = tx.get_read_only_view(network.jwt_issuers); + auto ca_certs_view = tx.get_read_only_view(network.ca_certs); + jwt_issuers_view->foreach([this,&ca_certs_view]( + const JwtIssuer& issuer, const JwtIssuerMetadata& metadata) { + if (!metadata.auto_refresh) + { + LOG_DEBUG_FMT( + "Skipping JWT issuer '{}' during key auto-refresh", + issuer); + return true; + } + auto& ca_cert_name = metadata.ca_cert_name.value(); + auto ca_cert_der = ca_certs_view->get(ca_cert_name); + if (!ca_cert_der.has_value()) + { + LOG_FATAL_FMT( + "CA cert with name '{}' not found while refreshing JWT keys for issuer {}", + ca_cert_name, + issuer); + return true; + } + auto ca = std::make_shared(ca_cert_der.value()); + auto ca_cert = std::make_shared(ca); + + auto metadata_url = issuer + "/.well-known/openid-configuration"; + + auto url_protocol_length = 8; // https:// + auto metadata_url_path_start = issuer.find('/', url_protocol_length); + auto metadata_url_host = issuer.substr(url_protocol_length, metadata_url_path_start - url_protocol_length); + auto metadata_url_path = issuer.substr(metadata_url_path_start); + + auto http_client = rpcsessions->create_client(ca_cert); + http_client->connect( + metadata_url_host, + "443", + [this,&issuer,&ca_cert,&url_protocol_length]( + http_status status, http::HeaderMap&&, std::vector&& data) { + std::lock_guard guard(lock); + + if (status != HTTP_STATUS_OK) + { + LOG_FAIL_FMT( + "An error occurred while requesting the OpenID metadata document: {} {}{}", + status, + http_status_str(status), + data.empty() ? + "" : + fmt::format(" '{}'", std::string(data.begin(), data.end()))); + return false; + } + + auto metadata = nlohmann::json::parse(data); + auto jwks_url = metadata.at("jwks_uri").get(); + + auto jwks_url_path_start = jwks_url.find('/', url_protocol_length); + auto jwks_url_host = jwks_url.substr(url_protocol_length, jwks_url_path_start - url_protocol_length); + auto jwks_url_path = jwks_url.substr(jwks_url_path_start); + + auto http_client = rpcsessions->create_client(ca_cert); + http_client->connect( + jwks_url_host, + "443", + [this,&issuer]( + http_status status, http::HeaderMap&&, std::vector&& data) { + std::lock_guard guard(lock); + + if (status != HTTP_STATUS_OK) + { + LOG_FAIL_FMT( + "An error occurred while requesting the JWKS document: {} {}{}", + status, + http_status_str(status), + data.empty() ? + "" : + fmt::format(" '{}'", std::string(data.begin(), data.end()))); + return false; + } + + // call internal endpoint to update keys + auto jwks = nlohmann::json::parse(data).get(); + auto msg = SetJwtPublicSigningKeys{ issuer, jwks }; + auto body = serdes::pack(msg, serdes::Pack::Text); + + http::Request request(fmt::format( + "/{}/{}", ccf::get_actor_prefix(ccf::ActorsType::members), "internal/set_jwt_public_signing_keys")); + request.set_header( + http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON); + request.set_body(&body); + // TODO cannot sign as frontend would deny access + // -> add special skip_verification flag (like is_create_request)? + //http::sign_request(request, node_sign_kp); + auto packed = request.build_request(); + + auto node_session = std::make_shared( + enclave::InvalidSessionId, node_cert.raw()); + auto ctx = enclave::make_rpc_context(node_session, packed); + + const auto actor_opt = http::extract_actor(*ctx); + if (!actor_opt.has_value()) + { + throw std::logic_error("Unable to get actor"); + } + + const auto actor = rpc_map->resolve(actor_opt.value()); + auto frontend_opt = this->rpc_map->find(actor); + if (!frontend_opt.has_value()) + { + throw std::logic_error( + "RpcMap::find returned invalid (empty) frontend"); + } + auto frontend = frontend_opt.value(); + frontend->process(ctx); + return true; + }); + + LOG_DEBUG_FMT( + "Requesting JWKS document at {}", + jwks_url); + + http::Request r(jwks_url_path, HTTP_GET); + r.set_header(http::headers::HOST, jwks_url_host); + http_client->send_request(r.build_request()); + + return true; + }); + + LOG_DEBUG_FMT( + "Requesting OpenID metadata document at {}", + metadata_url); + + http::Request r(metadata_url_path, HTTP_GET); + r.set_header(http::headers::HOST, metadata_url_host); + http_client->send_request(r.build_request()); + return true; + }); + } + + void auto_refresh_jwt_keys(CCFConfig& config) + { + std::lock_guard guard(lock); + + struct RefreshTimeMsg + { + RefreshTimeMsg(NodeState& self_, CCFConfig& config_) : + self(self_), + config(config_) + {} + + NodeState& self; + CCFConfig& config; + }; + + auto refresh_msg = std::make_unique>( + [](std::unique_ptr> msg) { + auto delay = + std::chrono::seconds(msg->data.config.jwt_key_refresh_interval); + threading::ThreadMessaging::thread_messaging.add_task_after( + std::move(msg), delay); + if (msg->data.self.consensus->is_primary()) + { + msg->data.self.refresh_jwt_keys(); + } + }, + *this, + config); + + threading::ThreadMessaging::thread_messaging.add_task_after( + std::move(refresh_msg), + std::chrono::seconds(config.jwt_key_refresh_interval)); + } + // // funcs in state "readingPublicLedger" // diff --git a/src/node/rpc/member_frontend.h b/src/node/rpc/member_frontend.h index 9e367c994302..9861d72cbd5d 100644 --- a/src/node/rpc/member_frontend.h +++ b/src/node/rpc/member_frontend.h @@ -732,6 +732,24 @@ namespace ccf const auto parsed = args.get(); auto issuers = tx.get_view(this->network.jwt_issuers); + if (parsed.auto_refresh) + { + if (!parsed.ca_cert_name.has_value()) + { + LOG_FAIL_FMT( + "Proposal {}: ca_cert_name is missing but required if auto_refresh is true", + proposal_id); + return false; + } + if (!nonstd::starts_with(parsed.issuer, "https://")) + { + LOG_FAIL_FMT( + "Proposal {}: issuer must be a valid URL starting with https:// if auto_refresh is true", + proposal_id); + return false; + } + } + bool success = true; if (parsed.jwks.has_value()) { @@ -1800,6 +1818,38 @@ namespace ccf make_endpoint("create", HTTP_POST, json_adapter(create)) .set_require_client_identity(false) .install(); + + auto internal_set_jwt_public_signing_keys = + [this](kv::Tx& tx, CallerId, nlohmann::json&& args) { + // TODO check request is coming from own node + + const auto parsed = args.get(); + + auto issuers = tx.get_view(this->network.jwt_issuers); + auto issuer_metadata_ = issuers->get(parsed.issuer); + if (!issuer_metadata_.has_value()) + { + return make_error( + HTTP_STATUS_FORBIDDEN, + fmt::format("{} is not a valid issuer", parsed.issuer)); + } + auto& issuer_metadata = issuer_metadata_.value(); + + ObjectId dummy = 0; + if (!set_jwt_public_signing_keys( + tx, dummy, parsed.issuer, issuer_metadata, parsed.jwks)) + { + return make_error( + HTTP_STATUS_FORBIDDEN, + fmt::format("error while storing signing keys for issuer {}", parsed.issuer)); + } + + return make_success(true); + }; + make_endpoint( + "internal/set_jwt_public_signing_keys", HTTP_POST, json_adapter(internal_set_jwt_public_signing_keys)) + .set_require_client_identity(false) + .install(); } }; From a20494e47de3f3190519df0752be199426218ffb Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Wed, 18 Nov 2020 17:56:51 +0000 Subject: [PATCH 02/18] add test --- tests/infra/e2e_args.py | 5 ++ tests/infra/network.py | 1 + tests/infra/remote.py | 4 ++ tests/jwt_test.py | 107 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+) diff --git a/tests/infra/e2e_args.py b/tests/infra/e2e_args.py index 3cc7661dabe7..f5f9d38c2748 100644 --- a/tests/infra/e2e_args.py +++ b/tests/infra/e2e_args.py @@ -249,6 +249,11 @@ def cli_args(add=lambda x: None, parser=None, accept_unknown=False): help="Number of transactions between two snapshots", default=None, ) + parser.add_argument( + "--jwt-key-refresh-interval-s", + help="JWT key refresh interval in seconds", + default=None, + ) add(parser) diff --git a/tests/infra/network.py b/tests/infra/network.py index ab89712ff5e6..c7ba4cfa549a 100644 --- a/tests/infra/network.py +++ b/tests/infra/network.py @@ -77,6 +77,7 @@ class Network: "ledger_chunk_bytes", "domain", "snapshot_tx_interval", + "jwt_key_refresh_interval_s" ] # Maximum delay (seconds) for updates to propagate from the primary to backups diff --git a/tests/infra/remote.py b/tests/infra/remote.py index f842caada389..bd6699d29852 100644 --- a/tests/infra/remote.py +++ b/tests/infra/remote.py @@ -590,6 +590,7 @@ def __init__( ledger_chunk_bytes=(5 * 1000 * 1000), domain=None, snapshot_tx_interval=None, + jwt_key_refresh_interval_s=None, ): """ Run a ccf binary on a remote host. @@ -670,6 +671,9 @@ def __init__( if snapshot_tx_interval: cmd += [f"--snapshot-tx-interval={snapshot_tx_interval}"] + if jwt_key_refresh_interval_s: + cmd += [f"--jwt-key-refresh-interval-s={jwt_key_refresh_interval_s}"] + for read_only_ledger_dir in self.read_only_ledger_dirs: cmd += [f"--read-only-ledger-dir={os.path.basename(read_only_ledger_dir)}"] data_files += [os.path.join(self.common_dir, read_only_ledger_dir)] diff --git a/tests/jwt_test.py b/tests/jwt_test.py index ec0d14941dfc..fc48635ac1ca 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -3,13 +3,21 @@ import os import tempfile import json +import time import base64 +from http.server import HTTPServer, BaseHTTPRequestHandler +from http import HTTPStatus +import ssl +import threading +from contextlib import AbstractContextManager import infra.network import infra.path import infra.proc import infra.net import infra.e2e_args import suite.test_requirements as reqs +from infra.proposal import ProposalState +import ccf.proposal_generator from loguru import logger as LOG @@ -252,6 +260,103 @@ def test_jwt_with_sgx_key_filter(network, args): return network +class OpenIDProviderServer(AbstractContextManager): + def __init__(self, port: int, tls_key_pem: str, tls_cert_pem: str, jwks: dict): + host = "localhost" + metadata = {"jwks_uri": f"https://{host}:{port}/keys"} + routes = {"/.well-known/openid-configuration": metadata, "/keys": jwks} + + class MyHTTPRequestHandler(BaseHTTPRequestHandler): + def do_GET(self): + body = routes.get(self.path) + if body is None: + self.send_error(HTTPStatus.NOT_FOUND) + return + self.send_response(HTTPStatus.OK) + self.end_headers() + self.wfile.write(json.dumps(body).encode()) + + with tempfile.NamedTemporaryFile( + prefix="ccf", mode="w+" + ) as keyfile_fp, tempfile.NamedTemporaryFile( + prefix="ccf", mode="w+" + ) as certfile_fp: + keyfile_fp.write(tls_key_pem) + keyfile_fp.flush() + certfile_fp.write(tls_cert_pem) + certfile_fp.flush() + + self.httpd = HTTPServer((host, port), MyHTTPRequestHandler) + self.httpd.socket = ssl.wrap_socket( + self.httpd.socket, + keyfile=keyfile_fp.name, + certfile=certfile_fp.name, + server_side=True, + ) + self.thread = threading.Thread(None, self.httpd.serve_forever) + self.thread.start() + + def __exit__(self): + self.httpd.shutdown() + self.httpd.server_close() + self.thread.join() + + +@reqs.description("JWT with auto_refresh enabled") +def test_jwt_key_auto_refresh(network, args): + primary, _ = network.find_nodes() + + key_priv_pem, key_pub_pem = infra.crypto.generate_rsa_keypair(2048) + cert_pem = infra.crypto.generate_cert(key_priv_pem) + kid = "my_kid" + issuer_host = "localhost" + issuer_port = 8888 + issuer = f"https://{issuer_host}:{issuer_port}" + + LOG.info("Add CA cert for JWT issuer") + with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as ca_cert_fp: + ca_cert_fp.write(key_pub_pem) + ca_cert_fp.flush() + proposal_body, _ = ccf.proposal_generator.update_ca_cert("jwt", ca_cert_fp.name) + proposal = network.consortium.get_any_active_member().propose( + primary, proposal_body + ) + assert proposal.state == ProposalState.Accepted + + LOG.info("Add JWT issuer with auto-refresh") + with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: + json.dump( + {"issuer": issuer, "auto_refresh": True, "ca_cert_name": "jwt"}, metadata_fp + ) + metadata_fp.flush() + network.consortium.set_jwt_issuer(primary, metadata_fp.name) + + LOG.info("Start local OpenID endpoint server") + jwks = create_jwks(kid, cert_pem) + with OpenIDProviderServer(issuer_port, key_pub_pem, cert_pem, jwks): + LOG.info("Wait for key refresh to happen") + # Note: refresh interval is set to 1s, see network args below. + time.sleep(2) + + LOG.info("Check that keys got refreshed") + with primary.client( + f"member{network.consortium.get_any_active_member().member_id}" + ) as c: + r = c.post( + "/gov/read", + {"table": "public:ccf.gov.jwt_public_signing_keys", "key": kid}, + ) + assert r.status_code == 200, r.status_code + # Note that /gov/read returns all data as JSON. + # Here, the stored data is a uint8 array, therefore it + # is returned as an array of integers. + cert_kv_der = bytes(r.body.json()) + cert_kv_pem = infra.crypto.cert_der_to_pem(cert_kv_der) + assert infra.crypto.are_certs_equal( + cert_pem, cert_kv_pem + ), "stored cert not equal to input cert" + + def run(args): with infra.network.network( args.nodes, args.binary_dir, args.debug_nodes, args.perf_nodes, pdb=args.pdb @@ -260,6 +365,7 @@ def run(args): network = test_jwt_without_key_policy(network, args) network = test_jwt_with_sgx_key_policy(network, args) network = test_jwt_with_sgx_key_filter(network, args) + network = test_jwt_key_auto_refresh(network, args) if __name__ == "__main__": @@ -267,4 +373,5 @@ def run(args): args = infra.e2e_args.cli_args() args.package = "liblogging" args.nodes = infra.e2e_args.max_nodes(args, f=0) + args.jwt_key_refresh_interval_s = 1 run(args) From af9c95b054b7b12354cff3cbf2c92dceffaed24a Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Thu, 19 Nov 2020 11:13:19 +0000 Subject: [PATCH 03/18] use existing url parser --- src/http/http_parser.h | 34 +++++++++++++++++++++++++ src/node/node_state.h | 46 +++++++++++++++++----------------- src/node/rpc/member_frontend.h | 31 +++++++++++++++++++++-- 3 files changed, 86 insertions(+), 25 deletions(-) diff --git a/src/http/http_parser.h b/src/http/http_parser.h index 46064543b8a3..9f575f0a5c47 100644 --- a/src/http/http_parser.h +++ b/src/http/http_parser.h @@ -164,6 +164,40 @@ namespace http extract_url_field(parser_url, UF_QUERY, url)); } + struct URL + { + std::string_view schema; + std::string_view host; + std::string_view port; + std::string_view path; + std::string_view query; + std::string_view fragment; + }; + + inline URL parse_url_full(const std::string& url) + { + LOG_TRACE_FMT("Received url to parse: {}", url); + + http_parser_url parser_url; + http_parser_url_init(&parser_url); + + const auto err = + http_parser_parse_url(url.data(), url.size(), 0, &parser_url); + if (err != 0) + { + throw std::runtime_error(fmt::format("Error parsing url: {}", err)); + } + + return { + extract_url_field(parser_url, UF_SCHEMA, url), + extract_url_field(parser_url, UF_HOST, url), + extract_url_field(parser_url, UF_PORT, url), + extract_url_field(parser_url, UF_PATH, url), + extract_url_field(parser_url, UF_QUERY, url), + extract_url_field(parser_url, UF_FRAGMENT, url) + }; + } + class Parser { protected: diff --git a/src/node/node_state.h b/src/node/node_state.h index bd2c0012f5f4..7d784ea32f4c 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -623,18 +623,15 @@ namespace ccf auto ca = std::make_shared(ca_cert_der.value()); auto ca_cert = std::make_shared(ca); - auto metadata_url = issuer + "/.well-known/openid-configuration"; - - auto url_protocol_length = 8; // https:// - auto metadata_url_path_start = issuer.find('/', url_protocol_length); - auto metadata_url_host = issuer.substr(url_protocol_length, metadata_url_path_start - url_protocol_length); - auto metadata_url_path = issuer.substr(metadata_url_path_start); + auto metadata_url_str = issuer + "/.well-known/openid-configuration"; + auto metadata_url = http::parse_url_full(metadata_url_str); + auto metadata_url_port = !metadata_url.port.empty() ? metadata_url.port : "443"; auto http_client = rpcsessions->create_client(ca_cert); http_client->connect( - metadata_url_host, - "443", - [this,&issuer,&ca_cert,&url_protocol_length]( + std::string(metadata_url.host), + std::string(metadata_url_port), + [this,&issuer,&ca_cert]( http_status status, http::HeaderMap&&, std::vector&& data) { std::lock_guard guard(lock); @@ -651,16 +648,15 @@ namespace ccf } auto metadata = nlohmann::json::parse(data); - auto jwks_url = metadata.at("jwks_uri").get(); + auto jwks_url_str = metadata.at("jwks_uri").get(); - auto jwks_url_path_start = jwks_url.find('/', url_protocol_length); - auto jwks_url_host = jwks_url.substr(url_protocol_length, jwks_url_path_start - url_protocol_length); - auto jwks_url_path = jwks_url.substr(jwks_url_path_start); + auto jwks_url = http::parse_url_full(jwks_url_str); + auto jwks_url_port = !jwks_url.port.empty() ? jwks_url.port : "443"; auto http_client = rpcsessions->create_client(ca_cert); http_client->connect( - jwks_url_host, - "443", + std::string(jwks_url.host), + std::string(jwks_url_port), [this,&issuer]( http_status status, http::HeaderMap&&, std::vector&& data) { std::lock_guard guard(lock); @@ -715,22 +711,26 @@ namespace ccf }); LOG_DEBUG_FMT( - "Requesting JWKS document at {}", - jwks_url); + "Requesting JWKS document at https://{}:{}{}", + jwks_url.host, + jwks_url_port, + jwks_url.path); - http::Request r(jwks_url_path, HTTP_GET); - r.set_header(http::headers::HOST, jwks_url_host); + http::Request r(jwks_url.path, HTTP_GET); + r.set_header(http::headers::HOST, std::string(jwks_url.host)); http_client->send_request(r.build_request()); return true; }); LOG_DEBUG_FMT( - "Requesting OpenID metadata document at {}", - metadata_url); + "Requesting OpenID metadata document at https://{}:{}{}", + metadata_url.host, + metadata_url_port, + metadata_url.path); - http::Request r(metadata_url_path, HTTP_GET); - r.set_header(http::headers::HOST, metadata_url_host); + http::Request r(metadata_url.path, HTTP_GET); + r.set_header(http::headers::HOST, std::string(metadata_url.host)); http_client->send_request(r.build_request()); return true; }); diff --git a/src/node/rpc/member_frontend.h b/src/node/rpc/member_frontend.h index 9861d72cbd5d..6dd6cf68918d 100644 --- a/src/node/rpc/member_frontend.h +++ b/src/node/rpc/member_frontend.h @@ -731,6 +731,7 @@ namespace ccf [this](ObjectId proposal_id, kv::Tx& tx, const nlohmann::json& args) { const auto parsed = args.get(); auto issuers = tx.get_view(this->network.jwt_issuers); + auto ca_certs = tx.get_read_only_view(this->network.ca_certs); if (parsed.auto_refresh) { @@ -741,10 +742,36 @@ namespace ccf proposal_id); return false; } - if (!nonstd::starts_with(parsed.issuer, "https://")) + if (!ca_certs->has(parsed.ca_cert_name.value())) { LOG_FAIL_FMT( - "Proposal {}: issuer must be a valid URL starting with https:// if auto_refresh is true", + "Proposal {}: No CA cert found with name '{}'", + proposal_id, + parsed.ca_cert_name.value()); + return false; + } + http::URL issuer_url; + try { + issuer_url = http::parse_url_full(parsed.issuer); + } + catch (const std::runtime_error&) + { + LOG_FAIL_FMT( + "Proposal {}: issuer must be a URL if auto_refresh is true", + proposal_id); + return false; + } + if (issuer_url.schema != "https") + { + LOG_FAIL_FMT( + "Proposal {}: issuer must be a URL starting with https:// if auto_refresh is true", + proposal_id); + return false; + } + if (!issuer_url.query.empty() || !issuer_url.fragment.empty()) + { + LOG_FAIL_FMT( + "Proposal {}: issuer must be a URL without query/fragment if auto_refresh is true", proposal_id); return false; } From ed7aa302b7b0946ee661c04ab1ff7a1c5e5d69f5 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Thu, 19 Nov 2020 16:34:35 +0000 Subject: [PATCH 04/18] more fixes, still wip --- python/ccf/proposal_generator.py | 12 +------ src/node/node_state.h | 56 ++++++++++++++++++++++++-------- src/runtime_config/gov.lua | 7 ++++ tests/infra/consortium.py | 5 +++ tests/jwt_test.py | 26 +++++++-------- 5 files changed, 68 insertions(+), 38 deletions(-) diff --git a/python/ccf/proposal_generator.py b/python/ccf/proposal_generator.py index a5cd920e8455..9bff97275c0b 100644 --- a/python/ccf/proposal_generator.py +++ b/python/ccf/proposal_generator.py @@ -18,8 +18,6 @@ from loguru import logger as LOG # type: ignore -CERT_OID_SGX_QUOTE = "1.2.840.113556.10.1.1" - def dump_to_file(output_path: str, obj: dict, dump_args: dict): with open(output_path, "w") as f: @@ -432,20 +430,12 @@ def update_ca_cert(cert_name, cert_path, skip_checks=False, **kwargs): if not skip_checks: try: - cert = x509.load_pem_x509_certificate( + x509.load_pem_x509_certificate( cert_pem.encode(), crypto_backends.default_backend() ) except Exception as exc: raise ValueError("Cannot parse PEM certificate") from exc - try: - oid = x509.ObjectIdentifier(CERT_OID_SGX_QUOTE) - _ = cert.extensions.get_extension_for_oid(oid) - except x509.ExtensionNotFound as exc: - raise ValueError( - "X.509 extension with SGX quote not found in certificate" - ) from exc - args = {"name": cert_name, "cert": cert_pem} return build_proposal("update_ca_cert", args, **kwargs) diff --git a/src/node/node_state.h b/src/node/node_state.h index 7d784ea32f4c..4e061bc21358 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -289,6 +289,7 @@ namespace ccf } accept_network_tls_connections(args.config); + auto_refresh_jwt_keys(args.config); reset_data(quote); sm.advance(State::partOfNetwork); @@ -300,6 +301,7 @@ namespace ccf // TLS connections are not endorsed by the network until the node // has joined accept_node_tls_connections(); + auto_refresh_jwt_keys(args.config); sm.advance(State::pending); @@ -355,6 +357,7 @@ namespace ccf } accept_network_tls_connections(args.config); + auto_refresh_jwt_keys(args.config); sm.advance(State::readingPublicLedger); @@ -598,6 +601,7 @@ namespace ccf void refresh_jwt_keys() { + LOG_DEBUG_FMT("Starting JWT key auto-refresh"); auto tx = network.tables->create_read_only_tx(); auto jwt_issuers_view = tx.get_read_only_view(network.jwt_issuers); auto ca_certs_view = tx.get_read_only_view(network.ca_certs); @@ -610,6 +614,7 @@ namespace ccf issuer); return true; } + LOG_DEBUG_FMT("Refreshing JWT keys for issuer '{}'", issuer); auto& ca_cert_name = metadata.ca_cert_name.value(); auto ca_cert_der = ca_certs_view->get(ca_cert_name); if (!ca_cert_der.has_value()) @@ -647,9 +652,17 @@ namespace ccf return false; } + LOG_DEBUG_FMT( + "Received OpenID metadata document for JWT issuer '{}'", + issuer); + auto metadata = nlohmann::json::parse(data); auto jwks_url_str = metadata.at("jwks_uri").get(); + LOG_DEBUG_FMT( + "Extracted 'jwks_uri' from OpenID metadata document for JWT issuer '{}': {}", + issuer, jwks_url_str); + auto jwks_url = http::parse_url_full(jwks_url_str); auto jwks_url_port = !jwks_url.port.empty() ? jwks_url.port : "443"; @@ -736,38 +749,55 @@ namespace ccf }); } - void auto_refresh_jwt_keys(CCFConfig& config) + void auto_refresh_jwt_keys(const CCFConfig& config) { - std::lock_guard guard(lock); - + auto jwt_key_refresh_interval = config.jwt_key_refresh_interval; + struct RefreshTimeMsg { - RefreshTimeMsg(NodeState& self_, CCFConfig& config_) : + RefreshTimeMsg(NodeState& self_, size_t jwt_key_refresh_interval) : self(self_), - config(config_) + jwt_key_refresh_interval(jwt_key_refresh_interval) {} NodeState& self; - CCFConfig& config; + size_t jwt_key_refresh_interval; }; auto refresh_msg = std::make_unique>( [](std::unique_ptr> msg) { + if (!msg->data.self.consensus->is_primary()) + { + LOG_DEBUG_FMT( + "Node is not primary, skipping JWT key auto-refresh"); + } + else + { + try + { + msg->data.self.refresh_jwt_keys(); + } + catch (std::exception& e) + { + LOG_FATAL_FMT( + "Unexpected error happened during JWT key auto-refresh: {}", e.what()); + } + } + LOG_DEBUG_FMT( + "Scheduling JWT key auto-refresh in {}s", msg->data.jwt_key_refresh_interval); auto delay = - std::chrono::seconds(msg->data.config.jwt_key_refresh_interval); + std::chrono::seconds(msg->data.jwt_key_refresh_interval); threading::ThreadMessaging::thread_messaging.add_task_after( std::move(msg), delay); - if (msg->data.self.consensus->is_primary()) - { - msg->data.self.refresh_jwt_keys(); - } }, *this, - config); + jwt_key_refresh_interval); + LOG_DEBUG_FMT( + "Scheduling JWT key auto-refresh in {}s", jwt_key_refresh_interval); threading::ThreadMessaging::thread_messaging.add_task_after( std::move(refresh_msg), - std::chrono::seconds(config.jwt_key_refresh_interval)); + std::chrono::seconds(jwt_key_refresh_interval)); } // diff --git a/src/runtime_config/gov.lua b/src/runtime_config/gov.lua index 08416030efb2..2bb6ffed93bb 100644 --- a/src/runtime_config/gov.lua +++ b/src/runtime_config/gov.lua @@ -98,4 +98,11 @@ return { end return true]], + update_ca_cert = [[ + tables, args = ... + t = tables["public:ccf.gov.ca_cert_ders"] + cert_der = pem_to_der(args.cert) + t:put(args.name, cert_der) + return true + ]], } diff --git a/tests/infra/consortium.py b/tests/infra/consortium.py index b9a255a32b4f..53e4050f7382 100644 --- a/tests/infra/consortium.py +++ b/tests/infra/consortium.py @@ -389,6 +389,11 @@ def set_jwt_public_signing_keys(self, remote_node, issuer, jwks_path): proposal = self.get_any_active_member().propose(remote_node, proposal_body) return self.vote_using_majority(remote_node, proposal, careful_vote) + def update_ca_cert(self, remote_node, cert_name, cert_pem_path): + proposal_body, careful_vote = self.make_proposal("update_ca_cert", cert_name, cert_pem_path) + proposal = self.get_any_active_member().propose(remote_node, proposal_body) + return self.vote_using_majority(remote_node, proposal, careful_vote) + def accept_recovery(self, remote_node): proposal_body, careful_vote = self.make_proposal("accept_recovery") proposal = self.get_any_active_member().propose(remote_node, proposal_body) diff --git a/tests/jwt_test.py b/tests/jwt_test.py index fc48635ac1ca..0c689e79c479 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -272,6 +272,7 @@ def do_GET(self): if body is None: self.send_error(HTTPStatus.NOT_FOUND) return + LOG.debug("OpenID provider server: {}", self.path) self.send_response(HTTPStatus.OK) self.end_headers() self.wfile.write(json.dumps(body).encode()) @@ -296,7 +297,7 @@ def do_GET(self): self.thread = threading.Thread(None, self.httpd.serve_forever) self.thread.start() - def __exit__(self): + def __exit__(self, exc_type, exc_value, traceback): self.httpd.shutdown() self.httpd.server_close() self.thread.join() @@ -308,35 +309,32 @@ def test_jwt_key_auto_refresh(network, args): key_priv_pem, key_pub_pem = infra.crypto.generate_rsa_keypair(2048) cert_pem = infra.crypto.generate_cert(key_priv_pem) + ca_cert_name = "jwt" kid = "my_kid" issuer_host = "localhost" - issuer_port = 8888 + issuer_port = 12345 issuer = f"https://{issuer_host}:{issuer_port}" LOG.info("Add CA cert for JWT issuer") with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as ca_cert_fp: - ca_cert_fp.write(key_pub_pem) + ca_cert_fp.write(cert_pem) ca_cert_fp.flush() - proposal_body, _ = ccf.proposal_generator.update_ca_cert("jwt", ca_cert_fp.name) - proposal = network.consortium.get_any_active_member().propose( - primary, proposal_body - ) - assert proposal.state == ProposalState.Accepted + network.consortium.update_ca_cert(primary, ca_cert_name, ca_cert_fp.name) LOG.info("Add JWT issuer with auto-refresh") with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: json.dump( - {"issuer": issuer, "auto_refresh": True, "ca_cert_name": "jwt"}, metadata_fp + {"issuer": issuer, "auto_refresh": True, "ca_cert_name": ca_cert_name}, metadata_fp ) metadata_fp.flush() network.consortium.set_jwt_issuer(primary, metadata_fp.name) LOG.info("Start local OpenID endpoint server") jwks = create_jwks(kid, cert_pem) - with OpenIDProviderServer(issuer_port, key_pub_pem, cert_pem, jwks): + with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks): LOG.info("Wait for key refresh to happen") # Note: refresh interval is set to 1s, see network args below. - time.sleep(2) + time.sleep(5) LOG.info("Check that keys got refreshed") with primary.client( @@ -362,9 +360,9 @@ def run(args): args.nodes, args.binary_dir, args.debug_nodes, args.perf_nodes, pdb=args.pdb ) as network: network.start_and_join(args) - network = test_jwt_without_key_policy(network, args) - network = test_jwt_with_sgx_key_policy(network, args) - network = test_jwt_with_sgx_key_filter(network, args) + #network = test_jwt_without_key_policy(network, args) + #network = test_jwt_with_sgx_key_policy(network, args) + #network = test_jwt_with_sgx_key_filter(network, args) network = test_jwt_key_auto_refresh(network, args) From d8cc6438fda8353d3b5ee9e979137f979274e04f Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Thu, 19 Nov 2020 17:46:17 +0000 Subject: [PATCH 05/18] minor fix --- src/node/node_state.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/node/node_state.h b/src/node/node_state.h index 4e061bc21358..283c6a4d26fb 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -636,7 +636,7 @@ namespace ccf http_client->connect( std::string(metadata_url.host), std::string(metadata_url_port), - [this,&issuer,&ca_cert]( + [this,issuer,ca_cert]( http_status status, http::HeaderMap&&, std::vector&& data) { std::lock_guard guard(lock); @@ -670,7 +670,7 @@ namespace ccf http_client->connect( std::string(jwks_url.host), std::string(jwks_url_port), - [this,&issuer]( + [this,issuer]( http_status status, http::HeaderMap&&, std::vector&& data) { std::lock_guard guard(lock); @@ -686,6 +686,10 @@ namespace ccf return false; } + LOG_DEBUG_FMT( + "Received JWKS document for JWT issuer '{}'", + issuer); + // call internal endpoint to update keys auto jwks = nlohmann::json::parse(data).get(); auto msg = SetJwtPublicSigningKeys{ issuer, jwks }; From f64e60af46048a922f1518a58b8be815aae512d1 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Thu, 19 Nov 2020 18:03:02 +0000 Subject: [PATCH 06/18] fix cn --- tests/infra/crypto.py | 4 ++-- tests/jwt_test.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/infra/crypto.py b/tests/infra/crypto.py index d178f9aba97e..7ff8dc1241df 100644 --- a/tests/infra/crypto.py +++ b/tests/infra/crypto.py @@ -147,12 +147,12 @@ def generate_rsa_keypair(key_size: int) -> Tuple[str, str]: return priv_pem, pub_pem -def generate_cert(priv_key_pem: str) -> str: +def generate_cert(priv_key_pem: str, cn="dummy") -> str: priv = load_pem_private_key(priv_key_pem.encode("ascii"), None, default_backend()) pub = priv.public_key() subject = issuer = x509.Name( [ - x509.NameAttribute(NameOID.COMMON_NAME, "dummy"), + x509.NameAttribute(NameOID.COMMON_NAME, cn), ] ) cert = ( diff --git a/tests/jwt_test.py b/tests/jwt_test.py index 0c689e79c479..16164d589f7c 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -307,14 +307,15 @@ def __exit__(self, exc_type, exc_value, traceback): def test_jwt_key_auto_refresh(network, args): primary, _ = network.find_nodes() - key_priv_pem, key_pub_pem = infra.crypto.generate_rsa_keypair(2048) - cert_pem = infra.crypto.generate_cert(key_priv_pem) ca_cert_name = "jwt" kid = "my_kid" issuer_host = "localhost" issuer_port = 12345 issuer = f"https://{issuer_host}:{issuer_port}" + key_priv_pem, _ = infra.crypto.generate_rsa_keypair(2048) + cert_pem = infra.crypto.generate_cert(key_priv_pem, cn=issuer_host) + LOG.info("Add CA cert for JWT issuer") with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as ca_cert_fp: ca_cert_fp.write(cert_pem) From 2ae6e05d878db59622ab854b9900411edbb0b0e7 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Thu, 19 Nov 2020 18:42:10 +0000 Subject: [PATCH 07/18] fix test --- tests/jwt_test.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/jwt_test.py b/tests/jwt_test.py index 16164d589f7c..86c3bd119c79 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -272,10 +272,14 @@ def do_GET(self): if body is None: self.send_error(HTTPStatus.NOT_FOUND) return - LOG.debug("OpenID provider server: {}", self.path) + body = json.dumps(body).encode() self.send_response(HTTPStatus.OK) + self.send_header('Content-Length', str(len(body))) self.end_headers() - self.wfile.write(json.dumps(body).encode()) + self.wfile.write(body) + + def log_message(self, fmt: str, *args): + LOG.debug(f"OpenIDProviderServer: {fmt % args}") with tempfile.NamedTemporaryFile( prefix="ccf", mode="w+" @@ -335,7 +339,7 @@ def test_jwt_key_auto_refresh(network, args): with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks): LOG.info("Wait for key refresh to happen") # Note: refresh interval is set to 1s, see network args below. - time.sleep(5) + time.sleep(2) LOG.info("Check that keys got refreshed") with primary.client( From 69543273eb081a26737f9bc3d08a1499bdce5681 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Fri, 20 Nov 2020 09:01:12 +0000 Subject: [PATCH 08/18] extend test --- tests/jwt_test.py | 74 +++++++++++++++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 25 deletions(-) diff --git a/tests/jwt_test.py b/tests/jwt_test.py index 86c3bd119c79..224820d2fc94 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -326,22 +326,7 @@ def test_jwt_key_auto_refresh(network, args): ca_cert_fp.flush() network.consortium.update_ca_cert(primary, ca_cert_name, ca_cert_fp.name) - LOG.info("Add JWT issuer with auto-refresh") - with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: - json.dump( - {"issuer": issuer, "auto_refresh": True, "ca_cert_name": ca_cert_name}, metadata_fp - ) - metadata_fp.flush() - network.consortium.set_jwt_issuer(primary, metadata_fp.name) - - LOG.info("Start local OpenID endpoint server") - jwks = create_jwks(kid, cert_pem) - with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks): - LOG.info("Wait for key refresh to happen") - # Note: refresh interval is set to 1s, see network args below. - time.sleep(2) - - LOG.info("Check that keys got refreshed") + def check_kv_jwt_key_matches(kid, cert_pem): with primary.client( f"member{network.consortium.get_any_active_member().member_id}" ) as c: @@ -349,15 +334,54 @@ def test_jwt_key_auto_refresh(network, args): "/gov/read", {"table": "public:ccf.gov.jwt_public_signing_keys", "key": kid}, ) - assert r.status_code == 200, r.status_code - # Note that /gov/read returns all data as JSON. - # Here, the stored data is a uint8 array, therefore it - # is returned as an array of integers. - cert_kv_der = bytes(r.body.json()) - cert_kv_pem = infra.crypto.cert_der_to_pem(cert_kv_der) - assert infra.crypto.are_certs_equal( - cert_pem, cert_kv_pem - ), "stored cert not equal to input cert" + if cert_pem is None: + assert r.status_code == 400, r.status_code + else: + assert r.status_code == 200, r.status_code + # Note that /gov/read returns all data as JSON. + # Here, the stored data is a uint8 array, therefore it + # is returned as an array of integers. + cert_kv_der = bytes(r.body.json()) + cert_kv_pem = infra.crypto.cert_der_to_pem(cert_kv_der) + assert infra.crypto.are_certs_equal( + cert_pem, cert_kv_pem + ), "stored cert not equal to input cert" + + LOG.info("Start OpenID endpoint server") + jwks = create_jwks(kid, cert_pem) + with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks): + LOG.info("Add JWT issuer with auto-refresh") + with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: + json.dump( + {"issuer": issuer, "auto_refresh": True, "ca_cert_name": ca_cert_name}, metadata_fp + ) + metadata_fp.flush() + network.consortium.set_jwt_issuer(primary, metadata_fp.name) + + LOG.info("Wait for key refresh to happen") + # Note: refresh interval is set to 1s, see network args below. + time.sleep(2) + + LOG.info("Check that keys got refreshed") + check_kv_jwt_key_matches(kid, cert_pem) + + LOG.info("Simulate OpenID endpoint server downtime") + time.sleep(3) + # TODO check failure metrics + + LOG.info("Restart OpenID endpoint server with new keys") + kid2 = "my_kid_2" + key2_priv_pem, _ = infra.crypto.generate_rsa_keypair(2048) + cert2_pem = infra.crypto.generate_cert(key2_priv_pem, cn=issuer_host) + jwks = create_jwks(kid2, cert2_pem) + with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks): + LOG.info("Wait for key refresh to happen") + # Note: refresh interval is set to 1s, see network args below. + time.sleep(2) + + LOG.info("Check that keys got refreshed") + check_kv_jwt_key_matches(kid, None) + check_kv_jwt_key_matches(kid2, cert2_pem) def run(args): From b282b7396bdfc20eb8d0f7f9f4cab1177868144c Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Fri, 20 Nov 2020 11:29:22 +0000 Subject: [PATCH 09/18] error handling --- src/http/http_parser.h | 4 +- src/node/node_state.h | 167 ++++++++++++++++++++------------- src/node/rpc/member_frontend.h | 78 +++++++++------ src/tls/base64.h | 2 +- src/tls/key_pair.h | 2 +- tests/jwt_test.py | 37 ++++++-- 6 files changed, 187 insertions(+), 103 deletions(-) diff --git a/src/http/http_parser.h b/src/http/http_parser.h index 9f575f0a5c47..2bf8d8c829c4 100644 --- a/src/http/http_parser.h +++ b/src/http/http_parser.h @@ -156,7 +156,7 @@ namespace http http_parser_parse_url(url.data(), url.size(), 0, &parser_url); if (err != 0) { - throw std::runtime_error(fmt::format("Error parsing url: {}", err)); + throw std::invalid_argument(fmt::format("Error parsing url: {}", err)); } return std::make_pair( @@ -185,7 +185,7 @@ namespace http http_parser_parse_url(url.data(), url.size(), 0, &parser_url); if (err != 0) { - throw std::runtime_error(fmt::format("Error parsing url: {}", err)); + throw std::invalid_argument(fmt::format("Error parsing url: {}", err)); } return { diff --git a/src/node/node_state.h b/src/node/node_state.h index 283c6a4d26fb..50f1ce2b6ab1 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -599,9 +599,52 @@ namespace ccf std::chrono::milliseconds(config.joining.join_timer)); } + template + void send_refresh_jwt_keys(T msg) + { + auto body = serdes::pack(msg, serdes::Pack::Text); + + http::Request request(fmt::format( + "/{}/{}", ccf::get_actor_prefix(ccf::ActorsType::members), "jwt_keys/refresh")); + request.set_header( + http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON); + request.set_body(&body); + // TODO cannot sign as frontend would deny access + // -> add special skip_verification flag (like is_create_request)? + //http::sign_request(request, node_sign_kp); + auto packed = request.build_request(); + + auto node_session = std::make_shared( + enclave::InvalidSessionId, node_cert.raw()); + auto ctx = enclave::make_rpc_context(node_session, packed); + + const auto actor_opt = http::extract_actor(*ctx); + if (!actor_opt.has_value()) + { + throw std::logic_error("Unable to get actor"); + } + + const auto actor = rpc_map->resolve(actor_opt.value()); + auto frontend_opt = this->rpc_map->find(actor); + if (!frontend_opt.has_value()) + { + throw std::logic_error( + "RpcMap::find returned invalid (empty) frontend"); + } + auto frontend = frontend_opt.value(); + frontend->process(ctx); + } + + void send_refresh_jwt_keys_error() + { + // A message that the endpoint fails to parse, leading to 500. + // This is done purely for exposing errors as endpoint metrics. + auto msg = false; + send_refresh_jwt_keys(msg); + } + void refresh_jwt_keys() { - LOG_DEBUG_FMT("Starting JWT key auto-refresh"); auto tx = network.tables->create_read_only_tx(); auto jwt_issuers_view = tx.get_read_only_view(network.jwt_issuers); auto ca_certs_view = tx.get_read_only_view(network.ca_certs); @@ -610,19 +653,20 @@ namespace ccf if (!metadata.auto_refresh) { LOG_DEBUG_FMT( - "Skipping JWT issuer '{}' during key auto-refresh", + "JWT key auto-refresh: Skipping issuer '{}'", issuer); return true; } - LOG_DEBUG_FMT("Refreshing JWT keys for issuer '{}'", issuer); + LOG_DEBUG_FMT("JWT key auto-refresh: Refreshing keys for issuer '{}'", issuer); auto& ca_cert_name = metadata.ca_cert_name.value(); auto ca_cert_der = ca_certs_view->get(ca_cert_name); if (!ca_cert_der.has_value()) { - LOG_FATAL_FMT( - "CA cert with name '{}' not found while refreshing JWT keys for issuer {}", + LOG_FAIL_FMT( + "JWT key auto-refresh: CA cert with name '{}' for issuer '{}' not found", ca_cert_name, issuer); + send_refresh_jwt_keys_error(); return true; } auto ca = std::make_shared(ca_cert_der.value()); @@ -633,6 +677,7 @@ namespace ccf auto metadata_url_port = !metadata_url.port.empty() ? metadata_url.port : "443"; auto http_client = rpcsessions->create_client(ca_cert); + // Connection errors are not signalled and hence not tracked in endpoint metrics currently. http_client->connect( std::string(metadata_url.host), std::string(metadata_url_port), @@ -643,27 +688,47 @@ namespace ccf if (status != HTTP_STATUS_OK) { LOG_FAIL_FMT( - "An error occurred while requesting the OpenID metadata document: {} {}{}", + "JWT key auto-refresh: Error while requesting OpenID metadata: {} {}{}", status, http_status_str(status), data.empty() ? "" : fmt::format(" '{}'", std::string(data.begin(), data.end()))); - return false; + send_refresh_jwt_keys_error(); + return true; } LOG_DEBUG_FMT( - "Received OpenID metadata document for JWT issuer '{}'", + "JWT key auto-refresh: Received OpenID metadata for issuer '{}'", issuer); - auto metadata = nlohmann::json::parse(data); - auto jwks_url_str = metadata.at("jwks_uri").get(); - - LOG_DEBUG_FMT( - "Extracted 'jwks_uri' from OpenID metadata document for JWT issuer '{}': {}", - issuer, jwks_url_str); - - auto jwks_url = http::parse_url_full(jwks_url_str); + std::string jwks_url_str; + try + { + auto metadata = nlohmann::json::parse(data); + jwks_url_str = metadata.at("jwks_uri").get(); + } + catch (std::exception& e) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse OpenID metadata for issuer '{}': {}", + issuer, e.what()); + send_refresh_jwt_keys_error(); + return true; + } + http::URL jwks_url; + try + { + jwks_url = http::parse_url_full(jwks_url_str); + } + catch (std::invalid_argument& e) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse jwks_uri for issuer '{}': {}", + issuer, jwks_url_str); + send_refresh_jwt_keys_error(); + return true; + } auto jwks_url_port = !jwks_url.port.empty() ? jwks_url.port : "443"; auto http_client = rpcsessions->create_client(ca_cert); @@ -677,58 +742,42 @@ namespace ccf if (status != HTTP_STATUS_OK) { LOG_FAIL_FMT( - "An error occurred while requesting the JWKS document: {} {}{}", + "JWT key auto-refresh: Error while requesting JWKS: {} {}{}", status, http_status_str(status), data.empty() ? "" : fmt::format(" '{}'", std::string(data.begin(), data.end()))); - return false; + send_refresh_jwt_keys_error(); + return true; } LOG_DEBUG_FMT( - "Received JWKS document for JWT issuer '{}'", + "JWT key auto-refresh: Received JWKS for issuer '{}'", issuer); - // call internal endpoint to update keys - auto jwks = nlohmann::json::parse(data).get(); - auto msg = SetJwtPublicSigningKeys{ issuer, jwks }; - auto body = serdes::pack(msg, serdes::Pack::Text); - - http::Request request(fmt::format( - "/{}/{}", ccf::get_actor_prefix(ccf::ActorsType::members), "internal/set_jwt_public_signing_keys")); - request.set_header( - http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON); - request.set_body(&body); - // TODO cannot sign as frontend would deny access - // -> add special skip_verification flag (like is_create_request)? - //http::sign_request(request, node_sign_kp); - auto packed = request.build_request(); - - auto node_session = std::make_shared( - enclave::InvalidSessionId, node_cert.raw()); - auto ctx = enclave::make_rpc_context(node_session, packed); - - const auto actor_opt = http::extract_actor(*ctx); - if (!actor_opt.has_value()) + JsonWebKeySet jwks; + try { - throw std::logic_error("Unable to get actor"); + jwks = nlohmann::json::parse(data).get(); } - - const auto actor = rpc_map->resolve(actor_opt.value()); - auto frontend_opt = this->rpc_map->find(actor); - if (!frontend_opt.has_value()) + catch (std::exception& e) { - throw std::logic_error( - "RpcMap::find returned invalid (empty) frontend"); + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse JWKS for issuer '{}': {}", + issuer, e.what()); + send_refresh_jwt_keys_error(); + return true; } - auto frontend = frontend_opt.value(); - frontend->process(ctx); + + // call internal endpoint to update keys + auto msg = SetJwtPublicSigningKeys{ issuer, jwks }; + send_refresh_jwt_keys(msg); return true; }); LOG_DEBUG_FMT( - "Requesting JWKS document at https://{}:{}{}", + "JWT key auto-refresh: Requesting JWKS at https://{}:{}{}", jwks_url.host, jwks_url_port, jwks_url.path); @@ -741,7 +790,7 @@ namespace ccf }); LOG_DEBUG_FMT( - "Requesting OpenID metadata document at https://{}:{}{}", + "JWT key auto-refresh: Requesting OpenID metadata at https://{}:{}{}", metadata_url.host, metadata_url_port, metadata_url.path); @@ -773,22 +822,14 @@ namespace ccf if (!msg->data.self.consensus->is_primary()) { LOG_DEBUG_FMT( - "Node is not primary, skipping JWT key auto-refresh"); + "JWT key auto-refresh: Node is not primary, skipping"); } else { - try - { - msg->data.self.refresh_jwt_keys(); - } - catch (std::exception& e) - { - LOG_FATAL_FMT( - "Unexpected error happened during JWT key auto-refresh: {}", e.what()); - } + msg->data.self.refresh_jwt_keys(); } LOG_DEBUG_FMT( - "Scheduling JWT key auto-refresh in {}s", msg->data.jwt_key_refresh_interval); + "JWT key auto-refresh: Scheduling in {}s", msg->data.jwt_key_refresh_interval); auto delay = std::chrono::seconds(msg->data.jwt_key_refresh_interval); threading::ThreadMessaging::thread_messaging.add_task_after( @@ -798,7 +839,7 @@ namespace ccf jwt_key_refresh_interval); LOG_DEBUG_FMT( - "Scheduling JWT key auto-refresh in {}s", jwt_key_refresh_interval); + "JWT key auto-refresh: Scheduling in {}s", jwt_key_refresh_interval); threading::ThreadMessaging::thread_messaging.add_task_after( std::move(refresh_msg), std::chrono::seconds(jwt_key_refresh_interval)); diff --git a/src/node/rpc/member_frontend.h b/src/node/rpc/member_frontend.h index 6dd6cf68918d..3fb1cfe64759 100644 --- a/src/node/rpc/member_frontend.h +++ b/src/node/rpc/member_frontend.h @@ -410,10 +410,12 @@ namespace ccf auto key_issuer = tx.get_view(this->network.jwt_public_signing_key_issuer); + auto log_prefix = proposal_id != INVALID_ID ? fmt::format("Proposal {}", proposal_id) : "JWT key auto-refresh"; + // add keys if (jwks.keys.empty()) { - LOG_FAIL_FMT("Proposal {}: JWKS has no keys", proposal_id); + LOG_FAIL_FMT("{}: JWKS has no keys", log_prefix, proposal_id); return false; } std::map> new_keys; @@ -422,19 +424,28 @@ namespace ccf if (keys->has(jwk.kid) && key_issuer->get(jwk.kid).value() != issuer) { LOG_FAIL_FMT( - "Proposal {}: key id {} already added for different issuer", - proposal_id, + "{}: key id {} already added for different issuer", + log_prefix, jwk.kid); return false; } if (jwk.x5c.empty()) { - LOG_FAIL_FMT("Proposal {}: JWKS is invalid (empty x5c)", proposal_id); + LOG_FAIL_FMT("{}: JWKS is invalid (empty x5c)", log_prefix); return false; } auto& der_base64 = jwk.x5c[0]; - auto der = tls::raw_from_b64(der_base64); + ccf::Cert der; + try + { + der = tls::raw_from_b64(der_base64); + } + catch(const std::invalid_argument& e) + { + LOG_FAIL_FMT("{}: Could not parse x5c of key id {}: {}", log_prefix, jwk.kid, e.what()); + return false; + } std::map> claims; bool has_key_policy_sgx_claims = @@ -458,9 +469,9 @@ namespace ccf claims.empty()) { LOG_INFO_FMT( - "Proposal {}: Skipping JWT signing key with kid {} (not OE " + "{}: Skipping JWT signing key with kid {} (not OE " "attested)", - proposal_id, + log_prefix, jwk.kid); continue; } @@ -473,8 +484,8 @@ namespace ccf if (claims.find(claim_name) == claims.end()) { LOG_FAIL_FMT( - "Proposal {}: JWKS kid {} is missing the {} SGX claim", - proposal_id, + "{}: JWKS kid {} is missing the {} SGX claim", + log_prefix, jwk.kid, claim_name); return false; @@ -485,8 +496,8 @@ namespace ccf if (expected_claim_val_hex != actual_claim_val_hex) { LOG_FAIL_FMT( - "Proposal {}: JWKS kid {} has a mismatching {} SGX claim", - proposal_id, + "{}: JWKS kid {} has a mismatching {} SGX claim", + log_prefix, jwk.kid, claim_name); return false; @@ -499,27 +510,26 @@ namespace ccf { tls::check_is_cert(der); } - catch (std::exception& exc) + catch (std::invalid_argument& exc) { LOG_FAIL_FMT( - "Proposal {}: JWKS kid {} has an invalid X.509 certificate: " - "{}", - proposal_id, + "{}: JWKS kid {} has an invalid X.509 certificate: {}", + log_prefix, jwk.kid, exc.what()); return false; } } LOG_INFO_FMT( - "Proposal {}: Storing JWT signing key with kid {}", - proposal_id, + "{}: Storing JWT signing key with kid {}", + log_prefix, jwk.kid); new_keys.emplace(jwk.kid, der); } if (new_keys.empty()) { LOG_FAIL_FMT( - "Proposal {}: no keys left after applying filter", proposal_id); + "{}: no keys left after applying filter", log_prefix); return false; } @@ -1846,35 +1856,47 @@ namespace ccf .set_require_client_identity(false) .install(); - auto internal_set_jwt_public_signing_keys = - [this](kv::Tx& tx, CallerId, nlohmann::json&& args) { + // Only called from node. See node_state.h. + auto refresh_jwt_keys = + [this](EndpointContext& args, nlohmann::json&& body) { // TODO check request is coming from own node - - const auto parsed = args.get(); + + // All errors are server errors since the client is the server. - auto issuers = tx.get_view(this->network.jwt_issuers); + SetJwtPublicSigningKeys parsed; + try + { + parsed = body.get(); + } + catch (JsonParseError& e) + { + return make_error( + HTTP_STATUS_INTERNAL_SERVER_ERROR, + "unable to parse body"); + } + + auto issuers = args.tx.get_view(this->network.jwt_issuers); auto issuer_metadata_ = issuers->get(parsed.issuer); if (!issuer_metadata_.has_value()) { return make_error( - HTTP_STATUS_FORBIDDEN, + HTTP_STATUS_INTERNAL_SERVER_ERROR, fmt::format("{} is not a valid issuer", parsed.issuer)); } auto& issuer_metadata = issuer_metadata_.value(); - ObjectId dummy = 0; if (!set_jwt_public_signing_keys( - tx, dummy, parsed.issuer, issuer_metadata, parsed.jwks)) + args.tx, INVALID_ID, parsed.issuer, issuer_metadata, parsed.jwks)) { return make_error( - HTTP_STATUS_FORBIDDEN, + HTTP_STATUS_INTERNAL_SERVER_ERROR, fmt::format("error while storing signing keys for issuer {}", parsed.issuer)); } return make_success(true); }; make_endpoint( - "internal/set_jwt_public_signing_keys", HTTP_POST, json_adapter(internal_set_jwt_public_signing_keys)) + "jwt_keys/refresh", HTTP_POST, json_adapter(refresh_jwt_keys)) .set_require_client_identity(false) .install(); } diff --git a/src/tls/base64.h b/src/tls/base64.h index 70622229b543..c9b5821c4d38 100644 --- a/src/tls/base64.h +++ b/src/tls/base64.h @@ -32,7 +32,7 @@ namespace tls decoded.data(), decoded.size(), &len_written, data, size); if (rc != 0) { - throw std::logic_error( + throw std::invalid_argument( fmt::format("Could not decode base64 string: {}", error_string(rc))); } diff --git a/src/tls/key_pair.h b/src/tls/key_pair.h index 40e741b15f5f..b42934fc58ac 100644 --- a/src/tls/key_pair.h +++ b/src/tls/key_pair.h @@ -890,7 +890,7 @@ namespace tls mbedtls_x509_crt_free(&cert); if (rc != 0) { - throw std::runtime_error(fmt::format( + throw std::invalid_argument(fmt::format( "Failed to parse certificate, mbedtls_x509_crt_parse: {}", tls::error_string(rc))); } diff --git a/tests/jwt_test.py b/tests/jwt_test.py index 224820d2fc94..8273feb6d869 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -264,10 +264,11 @@ class OpenIDProviderServer(AbstractContextManager): def __init__(self, port: int, tls_key_pem: str, tls_cert_pem: str, jwks: dict): host = "localhost" metadata = {"jwks_uri": f"https://{host}:{port}/keys"} - routes = {"/.well-known/openid-configuration": metadata, "/keys": jwks} - + self.jwks = jwks + self_ = self class MyHTTPRequestHandler(BaseHTTPRequestHandler): def do_GET(self): + routes = {"/.well-known/openid-configuration": metadata, "/keys": self_.jwks} body = routes.get(self.path) if body is None: self.send_error(HTTPStatus.NOT_FOUND) @@ -347,9 +348,19 @@ def check_kv_jwt_key_matches(kid, cert_pem): cert_pem, cert_kv_pem ), "stored cert not equal to input cert" + def get_jwt_refresh_endpoint_metrics() -> dict: + with primary.client( + f"member{network.consortium.get_any_active_member().member_id}" + ) as c: + r = c.get("/gov/endpoint_metrics") + m = r.body.json()["metrics"]["jwt_keys/refresh"]["POST"] + assert m["errors"] == 0, m["errors"] # not used in jwt refresh endpoint + m["successes"] = m["calls"] - m["failures"] + return m + LOG.info("Start OpenID endpoint server") jwks = create_jwks(kid, cert_pem) - with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks): + with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks) as server: LOG.info("Add JWT issuer with auto-refresh") with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: json.dump( @@ -364,10 +375,21 @@ def check_kv_jwt_key_matches(kid, cert_pem): LOG.info("Check that keys got refreshed") check_kv_jwt_key_matches(kid, cert_pem) - - LOG.info("Simulate OpenID endpoint server downtime") - time.sleep(3) - # TODO check failure metrics + + LOG.info("Check that JWT refresh endpoint has no failures") + m = get_jwt_refresh_endpoint_metrics() + assert m["failures"] == 0, m["failures"] + assert m["successes"] > 0, m["successes"] + + LOG.info("Serve invalid JWKS") + server.jwks = {"foo": "bar"} + + LOG.info("Wait for key refresh to happen") + time.sleep(2) + + LOG.info("Check that JWT refresh endpoint has some failures") + m = get_jwt_refresh_endpoint_metrics() + assert m["failures"] > 0, m["failures"] LOG.info("Restart OpenID endpoint server with new keys") kid2 = "my_kid_2" @@ -376,7 +398,6 @@ def check_kv_jwt_key_matches(kid, cert_pem): jwks = create_jwks(kid2, cert2_pem) with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks): LOG.info("Wait for key refresh to happen") - # Note: refresh interval is set to 1s, see network args below. time.sleep(2) LOG.info("Check that keys got refreshed") From a051581cfc542c9f96d8b7e2714746a1e2e23be6 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Fri, 20 Nov 2020 11:57:54 +0000 Subject: [PATCH 10/18] cleanup --- src/enclave/interface.h | 4 +- src/host/main.cpp | 6 +- src/node/node_state.h | 256 +++++++++++++++++---------------- src/node/rpc/member_frontend.h | 2 - 4 files changed, 138 insertions(+), 130 deletions(-) diff --git a/src/enclave/interface.h b/src/enclave/interface.h index a2cdf0dadf51..eccf78e5c9f9 100644 --- a/src/enclave/interface.h +++ b/src/enclave/interface.h @@ -80,7 +80,7 @@ struct CCFConfig std::string subject_name; std::vector subject_alternative_names; - size_t jwt_key_refresh_interval; + size_t jwt_key_refresh_interval_s; MSGPACK_DEFINE( consensus_config, @@ -93,7 +93,7 @@ struct CCFConfig joining, subject_name, subject_alternative_names, - jwt_key_refresh_interval); + jwt_key_refresh_interval_s); }; /// General administrative messages diff --git a/src/host/main.cpp b/src/host/main.cpp index 0d3780259f3d..61892d89946c 100644 --- a/src/host/main.cpp +++ b/src/host/main.cpp @@ -318,11 +318,11 @@ int main(int argc, char** argv) "Subject Alternative Name in node certificate. Can be either " "iPAddress:xxx.xxx.xxx.xxx, or dNSName:sub.domain.tld"); - size_t jwt_key_refresh_interval = 1800; + size_t jwt_key_refresh_interval_s = 1800; app .add_option( "--jwt-key-refresh-interval-s", - jwt_key_refresh_interval, + jwt_key_refresh_interval_s, "Interval in seconds for JWT public signing key refresh.") ->capture_default_str(); @@ -661,7 +661,7 @@ int main(int argc, char** argv) ccf_config.subject_name = subject_name; ccf_config.subject_alternative_names = subject_alternative_names; - ccf_config.jwt_key_refresh_interval = jwt_key_refresh_interval; + ccf_config.jwt_key_refresh_interval_s = jwt_key_refresh_interval_s; if (*start) { diff --git a/src/node/node_state.h b/src/node/node_state.h index 50f1ce2b6ab1..f8445c20ea6c 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -609,8 +609,8 @@ namespace ccf request.set_header( http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON); request.set_body(&body); - // TODO cannot sign as frontend would deny access - // -> add special skip_verification flag (like is_create_request)? + // TODO need custom authentication policy that accepts only node certs + // see https://github.com/microsoft/CCF/issues/1904 //http::sign_request(request, node_sign_kp); auto packed = request.build_request(); @@ -643,6 +643,120 @@ namespace ccf send_refresh_jwt_keys(msg); } + void handle_jwt_jwks_response( + const std::string& issuer, + http_status status, std::vector&& data) + { + std::lock_guard guard(lock); + + if (status != HTTP_STATUS_OK) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Error while requesting JWKS: {} {}{}", + status, + http_status_str(status), + data.empty() ? + "" : + fmt::format(" '{}'", std::string(data.begin(), data.end()))); + send_refresh_jwt_keys_error(); + return; + } + + LOG_DEBUG_FMT( + "JWT key auto-refresh: Received JWKS for issuer '{}'", + issuer); + + JsonWebKeySet jwks; + try + { + jwks = nlohmann::json::parse(data).get(); + } + catch (std::exception& e) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse JWKS for issuer '{}': {}", + issuer, e.what()); + send_refresh_jwt_keys_error(); + return; + } + + // call internal endpoint to update keys + auto msg = SetJwtPublicSigningKeys{ issuer, jwks }; + send_refresh_jwt_keys(msg); + } + + void handle_jwt_metadata_response( + const std::string& issuer, std::shared_ptr ca_cert, + http_status status, std::vector&& data) + { + std::lock_guard guard(lock); + + if (status != HTTP_STATUS_OK) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Error while requesting OpenID metadata: {} {}{}", + status, + http_status_str(status), + data.empty() ? + "" : + fmt::format(" '{}'", std::string(data.begin(), data.end()))); + send_refresh_jwt_keys_error(); + return; + } + + LOG_DEBUG_FMT( + "JWT key auto-refresh: Received OpenID metadata for issuer '{}'", + issuer); + + std::string jwks_url_str; + try + { + auto metadata = nlohmann::json::parse(data); + jwks_url_str = metadata.at("jwks_uri").get(); + } + catch (std::exception& e) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse OpenID metadata for issuer '{}': {}", + issuer, e.what()); + send_refresh_jwt_keys_error(); + return; + } + http::URL jwks_url; + try + { + jwks_url = http::parse_url_full(jwks_url_str); + } + catch (std::invalid_argument& e) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse jwks_uri for issuer '{}': {}", + issuer, jwks_url_str); + send_refresh_jwt_keys_error(); + return; + } + auto jwks_url_port = !jwks_url.port.empty() ? jwks_url.port : "443"; + + LOG_DEBUG_FMT( + "JWT key auto-refresh: Requesting JWKS at https://{}:{}{}", + jwks_url.host, + jwks_url_port, + jwks_url.path); + auto http_client = rpcsessions->create_client(ca_cert); + // Note: Connection errors are not signalled and hence not tracked in endpoint metrics currently. + http_client->connect( + std::string(jwks_url.host), + std::string(jwks_url_port), + [this,issuer]( + http_status status, http::HeaderMap&&, std::vector&& data) { + handle_jwt_jwks_response(issuer, status, std::move(data)); + return true; + }); + http::Request r(jwks_url.path, HTTP_GET); + r.set_header(http::headers::HOST, std::string(jwks_url.host)); + http_client->send_request(r.build_request()); + } + void refresh_jwt_keys() { auto tx = network.tables->create_read_only_tx(); @@ -653,7 +767,7 @@ namespace ccf if (!metadata.auto_refresh) { LOG_DEBUG_FMT( - "JWT key auto-refresh: Skipping issuer '{}'", + "JWT key auto-refresh: Skipping issuer '{}', auto-refresh is disabled", issuer); return true; } @@ -676,125 +790,21 @@ namespace ccf auto metadata_url = http::parse_url_full(metadata_url_str); auto metadata_url_port = !metadata_url.port.empty() ? metadata_url.port : "443"; + LOG_DEBUG_FMT( + "JWT key auto-refresh: Requesting OpenID metadata at https://{}:{}{}", + metadata_url.host, + metadata_url_port, + metadata_url.path); auto http_client = rpcsessions->create_client(ca_cert); - // Connection errors are not signalled and hence not tracked in endpoint metrics currently. + // Note: Connection errors are not signalled and hence not tracked in endpoint metrics currently. http_client->connect( std::string(metadata_url.host), std::string(metadata_url_port), [this,issuer,ca_cert]( http_status status, http::HeaderMap&&, std::vector&& data) { - std::lock_guard guard(lock); - - if (status != HTTP_STATUS_OK) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Error while requesting OpenID metadata: {} {}{}", - status, - http_status_str(status), - data.empty() ? - "" : - fmt::format(" '{}'", std::string(data.begin(), data.end()))); - send_refresh_jwt_keys_error(); - return true; - } - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Received OpenID metadata for issuer '{}'", - issuer); - - std::string jwks_url_str; - try - { - auto metadata = nlohmann::json::parse(data); - jwks_url_str = metadata.at("jwks_uri").get(); - } - catch (std::exception& e) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Cannot parse OpenID metadata for issuer '{}': {}", - issuer, e.what()); - send_refresh_jwt_keys_error(); - return true; - } - http::URL jwks_url; - try - { - jwks_url = http::parse_url_full(jwks_url_str); - } - catch (std::invalid_argument& e) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Cannot parse jwks_uri for issuer '{}': {}", - issuer, jwks_url_str); - send_refresh_jwt_keys_error(); + handle_jwt_metadata_response(issuer, ca_cert, status, std::move(data)); return true; - } - auto jwks_url_port = !jwks_url.port.empty() ? jwks_url.port : "443"; - - auto http_client = rpcsessions->create_client(ca_cert); - http_client->connect( - std::string(jwks_url.host), - std::string(jwks_url_port), - [this,issuer]( - http_status status, http::HeaderMap&&, std::vector&& data) { - std::lock_guard guard(lock); - - if (status != HTTP_STATUS_OK) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Error while requesting JWKS: {} {}{}", - status, - http_status_str(status), - data.empty() ? - "" : - fmt::format(" '{}'", std::string(data.begin(), data.end()))); - send_refresh_jwt_keys_error(); - return true; - } - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Received JWKS for issuer '{}'", - issuer); - - JsonWebKeySet jwks; - try - { - jwks = nlohmann::json::parse(data).get(); - } - catch (std::exception& e) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Cannot parse JWKS for issuer '{}': {}", - issuer, e.what()); - send_refresh_jwt_keys_error(); - return true; - } - - // call internal endpoint to update keys - auto msg = SetJwtPublicSigningKeys{ issuer, jwks }; - send_refresh_jwt_keys(msg); - return true; - }); - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Requesting JWKS at https://{}:{}{}", - jwks_url.host, - jwks_url_port, - jwks_url.path); - - http::Request r(jwks_url.path, HTTP_GET); - r.set_header(http::headers::HOST, std::string(jwks_url.host)); - http_client->send_request(r.build_request()); - - return true; }); - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Requesting OpenID metadata at https://{}:{}{}", - metadata_url.host, - metadata_url_port, - metadata_url.path); - http::Request r(metadata_url.path, HTTP_GET); r.set_header(http::headers::HOST, std::string(metadata_url.host)); http_client->send_request(r.build_request()); @@ -804,17 +814,17 @@ namespace ccf void auto_refresh_jwt_keys(const CCFConfig& config) { - auto jwt_key_refresh_interval = config.jwt_key_refresh_interval; + auto jwt_key_refresh_interval_s = config.jwt_key_refresh_interval_s; struct RefreshTimeMsg { - RefreshTimeMsg(NodeState& self_, size_t jwt_key_refresh_interval) : + RefreshTimeMsg(NodeState& self_, size_t jwt_key_refresh_interval_s) : self(self_), - jwt_key_refresh_interval(jwt_key_refresh_interval) + jwt_key_refresh_interval_s(jwt_key_refresh_interval_s) {} NodeState& self; - size_t jwt_key_refresh_interval; + size_t jwt_key_refresh_interval_s; }; auto refresh_msg = std::make_unique>( @@ -829,20 +839,20 @@ namespace ccf msg->data.self.refresh_jwt_keys(); } LOG_DEBUG_FMT( - "JWT key auto-refresh: Scheduling in {}s", msg->data.jwt_key_refresh_interval); + "JWT key auto-refresh: Scheduling in {}s", msg->data.jwt_key_refresh_interval_s); auto delay = - std::chrono::seconds(msg->data.jwt_key_refresh_interval); + std::chrono::seconds(msg->data.jwt_key_refresh_interval_s); threading::ThreadMessaging::thread_messaging.add_task_after( std::move(msg), delay); }, *this, - jwt_key_refresh_interval); + jwt_key_refresh_interval_s); LOG_DEBUG_FMT( - "JWT key auto-refresh: Scheduling in {}s", jwt_key_refresh_interval); + "JWT key auto-refresh: Scheduling in {}s", jwt_key_refresh_interval_s); threading::ThreadMessaging::thread_messaging.add_task_after( std::move(refresh_msg), - std::chrono::seconds(jwt_key_refresh_interval)); + std::chrono::seconds(jwt_key_refresh_interval_s)); } // diff --git a/src/node/rpc/member_frontend.h b/src/node/rpc/member_frontend.h index 3fb1cfe64759..1ff74705ad16 100644 --- a/src/node/rpc/member_frontend.h +++ b/src/node/rpc/member_frontend.h @@ -1859,8 +1859,6 @@ namespace ccf // Only called from node. See node_state.h. auto refresh_jwt_keys = [this](EndpointContext& args, nlohmann::json&& body) { - // TODO check request is coming from own node - // All errors are server errors since the client is the server. SetJwtPublicSigningKeys parsed; From 6b7c08aac4749a21378268c308eaf5b6750a4cc1 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Fri, 20 Nov 2020 12:04:09 +0000 Subject: [PATCH 11/18] formatting --- src/http/http_parser.h | 14 ++-- src/node/jwt.h | 3 +- src/node/node_state.h | 96 +++++++++++++++----------- src/node/rpc/member_frontend.h | 120 ++++++++++++++++++--------------- tests/jwt_test.py | 6 +- 5 files changed, 136 insertions(+), 103 deletions(-) diff --git a/src/http/http_parser.h b/src/http/http_parser.h index 2bf8d8c829c4..f0e3dbaa9364 100644 --- a/src/http/http_parser.h +++ b/src/http/http_parser.h @@ -188,14 +188,12 @@ namespace http throw std::invalid_argument(fmt::format("Error parsing url: {}", err)); } - return { - extract_url_field(parser_url, UF_SCHEMA, url), - extract_url_field(parser_url, UF_HOST, url), - extract_url_field(parser_url, UF_PORT, url), - extract_url_field(parser_url, UF_PATH, url), - extract_url_field(parser_url, UF_QUERY, url), - extract_url_field(parser_url, UF_FRAGMENT, url) - }; + return {extract_url_field(parser_url, UF_SCHEMA, url), + extract_url_field(parser_url, UF_HOST, url), + extract_url_field(parser_url, UF_PORT, url), + extract_url_field(parser_url, UF_PATH, url), + extract_url_field(parser_url, UF_QUERY, url), + extract_url_field(parser_url, UF_FRAGMENT, url)}; } class Parser diff --git a/src/node/jwt.h b/src/node/jwt.h index f90c180e6aaf..8f682714f86c 100644 --- a/src/node/jwt.h +++ b/src/node/jwt.h @@ -49,7 +49,8 @@ namespace ccf DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(JwtIssuerMetadata); DECLARE_JSON_REQUIRED_FIELDS(JwtIssuerMetadata, key_filter); - DECLARE_JSON_OPTIONAL_FIELDS(JwtIssuerMetadata, key_policy, ca_cert_name, auto_refresh); + DECLARE_JSON_OPTIONAL_FIELDS( + JwtIssuerMetadata, key_policy, ca_cert_name, auto_refresh); using JwtIssuer = std::string; using JwtKeyId = std::string; diff --git a/src/node/node_state.h b/src/node/node_state.h index f8445c20ea6c..b5b140664725 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -599,21 +599,23 @@ namespace ccf std::chrono::milliseconds(config.joining.join_timer)); } - template + template void send_refresh_jwt_keys(T msg) { auto body = serdes::pack(msg, serdes::Pack::Text); http::Request request(fmt::format( - "/{}/{}", ccf::get_actor_prefix(ccf::ActorsType::members), "jwt_keys/refresh")); + "/{}/{}", + ccf::get_actor_prefix(ccf::ActorsType::members), + "jwt_keys/refresh")); request.set_header( http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON); request.set_body(&body); - // TODO need custom authentication policy that accepts only node certs - // see https://github.com/microsoft/CCF/issues/1904 - //http::sign_request(request, node_sign_kp); + // Need a custom authentication policy that accepts only node certs. + // See https://github.com/microsoft/CCF/issues/1904 + // http::sign_request(request, node_sign_kp); auto packed = request.build_request(); - + auto node_session = std::make_shared( enclave::InvalidSessionId, node_cert.raw()); auto ctx = enclave::make_rpc_context(node_session, packed); @@ -645,10 +647,11 @@ namespace ccf void handle_jwt_jwks_response( const std::string& issuer, - http_status status, std::vector&& data) + http_status status, + std::vector&& data) { std::lock_guard guard(lock); - + if (status != HTTP_STATUS_OK) { LOG_FAIL_FMT( @@ -663,8 +666,7 @@ namespace ccf } LOG_DEBUG_FMT( - "JWT key auto-refresh: Received JWKS for issuer '{}'", - issuer); + "JWT key auto-refresh: Received JWKS for issuer '{}'", issuer); JsonWebKeySet jwks; try @@ -675,26 +677,30 @@ namespace ccf { LOG_FAIL_FMT( "JWT key auto-refresh: Cannot parse JWKS for issuer '{}': {}", - issuer, e.what()); + issuer, + e.what()); send_refresh_jwt_keys_error(); return; } // call internal endpoint to update keys - auto msg = SetJwtPublicSigningKeys{ issuer, jwks }; + auto msg = SetJwtPublicSigningKeys{issuer, jwks}; send_refresh_jwt_keys(msg); } void handle_jwt_metadata_response( - const std::string& issuer, std::shared_ptr ca_cert, - http_status status, std::vector&& data) + const std::string& issuer, + std::shared_ptr ca_cert, + http_status status, + std::vector&& data) { std::lock_guard guard(lock); - + if (status != HTTP_STATUS_OK) { LOG_FAIL_FMT( - "JWT key auto-refresh: Error while requesting OpenID metadata: {} {}{}", + "JWT key auto-refresh: Error while requesting OpenID metadata: {} " + "{}{}", status, http_status_str(status), data.empty() ? @@ -717,8 +723,10 @@ namespace ccf catch (std::exception& e) { LOG_FAIL_FMT( - "JWT key auto-refresh: Cannot parse OpenID metadata for issuer '{}': {}", - issuer, e.what()); + "JWT key auto-refresh: Cannot parse OpenID metadata for issuer '{}': " + "{}", + issuer, + e.what()); send_refresh_jwt_keys_error(); return; } @@ -731,7 +739,8 @@ namespace ccf { LOG_FAIL_FMT( "JWT key auto-refresh: Cannot parse jwks_uri for issuer '{}': {}", - issuer, jwks_url_str); + issuer, + jwks_url_str); send_refresh_jwt_keys_error(); return; } @@ -743,15 +752,16 @@ namespace ccf jwks_url_port, jwks_url.path); auto http_client = rpcsessions->create_client(ca_cert); - // Note: Connection errors are not signalled and hence not tracked in endpoint metrics currently. + // Note: Connection errors are not signalled and hence not tracked in + // endpoint metrics currently. http_client->connect( std::string(jwks_url.host), std::string(jwks_url_port), - [this,issuer]( + [this, issuer]( http_status status, http::HeaderMap&&, std::vector&& data) { handle_jwt_jwks_response(issuer, status, std::move(data)); return true; - }); + }); http::Request r(jwks_url.path, HTTP_GET); r.set_header(http::headers::HOST, std::string(jwks_url.host)); http_client->send_request(r.build_request()); @@ -762,22 +772,26 @@ namespace ccf auto tx = network.tables->create_read_only_tx(); auto jwt_issuers_view = tx.get_read_only_view(network.jwt_issuers); auto ca_certs_view = tx.get_read_only_view(network.ca_certs); - jwt_issuers_view->foreach([this,&ca_certs_view]( - const JwtIssuer& issuer, const JwtIssuerMetadata& metadata) { + jwt_issuers_view->foreach([this, &ca_certs_view]( + const JwtIssuer& issuer, + const JwtIssuerMetadata& metadata) { if (!metadata.auto_refresh) { LOG_DEBUG_FMT( - "JWT key auto-refresh: Skipping issuer '{}', auto-refresh is disabled", + "JWT key auto-refresh: Skipping issuer '{}', auto-refresh is " + "disabled", issuer); return true; } - LOG_DEBUG_FMT("JWT key auto-refresh: Refreshing keys for issuer '{}'", issuer); + LOG_DEBUG_FMT( + "JWT key auto-refresh: Refreshing keys for issuer '{}'", issuer); auto& ca_cert_name = metadata.ca_cert_name.value(); auto ca_cert_der = ca_certs_view->get(ca_cert_name); if (!ca_cert_der.has_value()) { LOG_FAIL_FMT( - "JWT key auto-refresh: CA cert with name '{}' for issuer '{}' not found", + "JWT key auto-refresh: CA cert with name '{}' for issuer '{}' not " + "found", ca_cert_name, issuer); send_refresh_jwt_keys_error(); @@ -785,25 +799,30 @@ namespace ccf } auto ca = std::make_shared(ca_cert_der.value()); auto ca_cert = std::make_shared(ca); - + auto metadata_url_str = issuer + "/.well-known/openid-configuration"; auto metadata_url = http::parse_url_full(metadata_url_str); - auto metadata_url_port = !metadata_url.port.empty() ? metadata_url.port : "443"; - + auto metadata_url_port = + !metadata_url.port.empty() ? metadata_url.port : "443"; + LOG_DEBUG_FMT( "JWT key auto-refresh: Requesting OpenID metadata at https://{}:{}{}", metadata_url.host, metadata_url_port, metadata_url.path); auto http_client = rpcsessions->create_client(ca_cert); - // Note: Connection errors are not signalled and hence not tracked in endpoint metrics currently. + // Note: Connection errors are not signalled and hence not tracked in + // endpoint metrics currently. http_client->connect( std::string(metadata_url.host), std::string(metadata_url_port), - [this,issuer,ca_cert]( - http_status status, http::HeaderMap&&, std::vector&& data) { - handle_jwt_metadata_response(issuer, ca_cert, status, std::move(data)); - return true; + [this, issuer, ca_cert]( + http_status status, + http::HeaderMap&&, + std::vector&& data) { + handle_jwt_metadata_response( + issuer, ca_cert, status, std::move(data)); + return true; }); http::Request r(metadata_url.path, HTTP_GET); r.set_header(http::headers::HOST, std::string(metadata_url.host)); @@ -831,7 +850,7 @@ namespace ccf [](std::unique_ptr> msg) { if (!msg->data.self.consensus->is_primary()) { - LOG_DEBUG_FMT( + LOG_DEBUG_FMT( "JWT key auto-refresh: Node is not primary, skipping"); } else @@ -839,7 +858,8 @@ namespace ccf msg->data.self.refresh_jwt_keys(); } LOG_DEBUG_FMT( - "JWT key auto-refresh: Scheduling in {}s", msg->data.jwt_key_refresh_interval_s); + "JWT key auto-refresh: Scheduling in {}s", + msg->data.jwt_key_refresh_interval_s); auto delay = std::chrono::seconds(msg->data.jwt_key_refresh_interval_s); threading::ThreadMessaging::thread_messaging.add_task_after( @@ -849,7 +869,7 @@ namespace ccf jwt_key_refresh_interval_s); LOG_DEBUG_FMT( - "JWT key auto-refresh: Scheduling in {}s", jwt_key_refresh_interval_s); + "JWT key auto-refresh: Scheduling in {}s", jwt_key_refresh_interval_s); threading::ThreadMessaging::thread_messaging.add_task_after( std::move(refresh_msg), std::chrono::seconds(jwt_key_refresh_interval_s)); diff --git a/src/node/rpc/member_frontend.h b/src/node/rpc/member_frontend.h index 1ff74705ad16..b9f0869ce0cb 100644 --- a/src/node/rpc/member_frontend.h +++ b/src/node/rpc/member_frontend.h @@ -410,7 +410,9 @@ namespace ccf auto key_issuer = tx.get_view(this->network.jwt_public_signing_key_issuer); - auto log_prefix = proposal_id != INVALID_ID ? fmt::format("Proposal {}", proposal_id) : "JWT key auto-refresh"; + auto log_prefix = proposal_id != INVALID_ID ? + fmt::format("Proposal {}", proposal_id) : + "JWT key auto-refresh"; // add keys if (jwks.keys.empty()) @@ -441,9 +443,13 @@ namespace ccf { der = tls::raw_from_b64(der_base64); } - catch(const std::invalid_argument& e) + catch (const std::invalid_argument& e) { - LOG_FAIL_FMT("{}: Could not parse x5c of key id {}: {}", log_prefix, jwk.kid, e.what()); + LOG_FAIL_FMT( + "{}: Could not parse x5c of key id {}: {}", + log_prefix, + jwk.kid, + e.what()); return false; } @@ -521,15 +527,12 @@ namespace ccf } } LOG_INFO_FMT( - "{}: Storing JWT signing key with kid {}", - log_prefix, - jwk.kid); + "{}: Storing JWT signing key with kid {}", log_prefix, jwk.kid); new_keys.emplace(jwk.kid, der); } if (new_keys.empty()) { - LOG_FAIL_FMT( - "{}: no keys left after applying filter", log_prefix); + LOG_FAIL_FMT("{}: no keys left after applying filter", log_prefix); return false; } @@ -748,41 +751,45 @@ namespace ccf if (!parsed.ca_cert_name.has_value()) { LOG_FAIL_FMT( - "Proposal {}: ca_cert_name is missing but required if auto_refresh is true", - proposal_id); + "Proposal {}: ca_cert_name is missing but required if " + "auto_refresh is true", + proposal_id); return false; } if (!ca_certs->has(parsed.ca_cert_name.value())) { LOG_FAIL_FMT( - "Proposal {}: No CA cert found with name '{}'", - proposal_id, - parsed.ca_cert_name.value()); + "Proposal {}: No CA cert found with name '{}'", + proposal_id, + parsed.ca_cert_name.value()); return false; } http::URL issuer_url; - try { + try + { issuer_url = http::parse_url_full(parsed.issuer); } catch (const std::runtime_error&) { LOG_FAIL_FMT( - "Proposal {}: issuer must be a URL if auto_refresh is true", - proposal_id); + "Proposal {}: issuer must be a URL if auto_refresh is true", + proposal_id); return false; } if (issuer_url.schema != "https") { LOG_FAIL_FMT( - "Proposal {}: issuer must be a URL starting with https:// if auto_refresh is true", - proposal_id); + "Proposal {}: issuer must be a URL starting with https:// if " + "auto_refresh is true", + proposal_id); return false; } if (!issuer_url.query.empty() || !issuer_url.fragment.empty()) { LOG_FAIL_FMT( - "Proposal {}: issuer must be a URL without query/fragment if auto_refresh is true", - proposal_id); + "Proposal {}: issuer must be a URL without query/fragment if " + "auto_refresh is true", + proposal_id); return false; } } @@ -1855,44 +1862,51 @@ namespace ccf make_endpoint("create", HTTP_POST, json_adapter(create)) .set_require_client_identity(false) .install(); - + // Only called from node. See node_state.h. - auto refresh_jwt_keys = - [this](EndpointContext& args, nlohmann::json&& body) { - // All errors are server errors since the client is the server. + auto refresh_jwt_keys = [this]( + EndpointContext& args, nlohmann::json&& body) { + // All errors are server errors since the client is the server. - SetJwtPublicSigningKeys parsed; - try - { - parsed = body.get(); - } - catch (JsonParseError& e) - { - return make_error( - HTTP_STATUS_INTERNAL_SERVER_ERROR, - "unable to parse body"); - } + SetJwtPublicSigningKeys parsed; + try + { + parsed = body.get(); + } + catch (JsonParseError& e) + { + return make_error( + HTTP_STATUS_INTERNAL_SERVER_ERROR, "unable to parse body"); + } - auto issuers = args.tx.get_view(this->network.jwt_issuers); - auto issuer_metadata_ = issuers->get(parsed.issuer); - if (!issuer_metadata_.has_value()) - { - return make_error( - HTTP_STATUS_INTERNAL_SERVER_ERROR, - fmt::format("{} is not a valid issuer", parsed.issuer)); - } - auto& issuer_metadata = issuer_metadata_.value(); + auto issuers = args.tx.get_view(this->network.jwt_issuers); + auto issuer_metadata_ = issuers->get(parsed.issuer); + if (!issuer_metadata_.has_value()) + { + return make_error( + HTTP_STATUS_INTERNAL_SERVER_ERROR, + fmt::format("{} is not a valid issuer", parsed.issuer)); + } + auto& issuer_metadata = issuer_metadata_.value(); - if (!set_jwt_public_signing_keys( - args.tx, INVALID_ID, parsed.issuer, issuer_metadata, parsed.jwks)) - { - return make_error( - HTTP_STATUS_INTERNAL_SERVER_ERROR, - fmt::format("error while storing signing keys for issuer {}", parsed.issuer)); - } + if (!issuer_metadata.auto_refresh) + { + return make_error( + HTTP_STATUS_INTERNAL_SERVER_ERROR, + fmt::format("{} does not have auto_refresh enabled", parsed.issuer)); + } - return make_success(true); - }; + if (!set_jwt_public_signing_keys( + args.tx, INVALID_ID, parsed.issuer, issuer_metadata, parsed.jwks)) + { + return make_error( + HTTP_STATUS_INTERNAL_SERVER_ERROR, + fmt::format( + "error while storing signing keys for issuer {}", parsed.issuer)); + } + + return make_success(true); + }; make_endpoint( "jwt_keys/refresh", HTTP_POST, json_adapter(refresh_jwt_keys)) .set_require_client_identity(false) diff --git a/tests/jwt_test.py b/tests/jwt_test.py index 8273feb6d869..7beaa3dfb655 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -410,9 +410,9 @@ def run(args): args.nodes, args.binary_dir, args.debug_nodes, args.perf_nodes, pdb=args.pdb ) as network: network.start_and_join(args) - #network = test_jwt_without_key_policy(network, args) - #network = test_jwt_with_sgx_key_policy(network, args) - #network = test_jwt_with_sgx_key_filter(network, args) + network = test_jwt_without_key_policy(network, args) + network = test_jwt_with_sgx_key_policy(network, args) + network = test_jwt_with_sgx_key_filter(network, args) network = test_jwt_key_auto_refresh(network, args) From c9bb2f6b2babd3056b11d41fccf289b459d1d858 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Fri, 20 Nov 2020 12:04:46 +0000 Subject: [PATCH 12/18] formatting --- python/ccf/proposal_generator.py | 1 - tests/infra/consortium.py | 4 +++- tests/infra/network.py | 2 +- tests/jwt_test.py | 21 +++++++++++++-------- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/python/ccf/proposal_generator.py b/python/ccf/proposal_generator.py index 9bff97275c0b..1315298f398c 100644 --- a/python/ccf/proposal_generator.py +++ b/python/ccf/proposal_generator.py @@ -18,7 +18,6 @@ from loguru import logger as LOG # type: ignore - def dump_to_file(output_path: str, obj: dict, dump_args: dict): with open(output_path, "w") as f: json.dump(obj, f, **dump_args) diff --git a/tests/infra/consortium.py b/tests/infra/consortium.py index 53e4050f7382..7a2e1310c258 100644 --- a/tests/infra/consortium.py +++ b/tests/infra/consortium.py @@ -390,7 +390,9 @@ def set_jwt_public_signing_keys(self, remote_node, issuer, jwks_path): return self.vote_using_majority(remote_node, proposal, careful_vote) def update_ca_cert(self, remote_node, cert_name, cert_pem_path): - proposal_body, careful_vote = self.make_proposal("update_ca_cert", cert_name, cert_pem_path) + proposal_body, careful_vote = self.make_proposal( + "update_ca_cert", cert_name, cert_pem_path + ) proposal = self.get_any_active_member().propose(remote_node, proposal_body) return self.vote_using_majority(remote_node, proposal, careful_vote) diff --git a/tests/infra/network.py b/tests/infra/network.py index c7ba4cfa549a..8a9366285001 100644 --- a/tests/infra/network.py +++ b/tests/infra/network.py @@ -77,7 +77,7 @@ class Network: "ledger_chunk_bytes", "domain", "snapshot_tx_interval", - "jwt_key_refresh_interval_s" + "jwt_key_refresh_interval_s", ] # Maximum delay (seconds) for updates to propagate from the primary to backups diff --git a/tests/jwt_test.py b/tests/jwt_test.py index 7beaa3dfb655..a20196bce334 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -266,19 +266,23 @@ def __init__(self, port: int, tls_key_pem: str, tls_cert_pem: str, jwks: dict): metadata = {"jwks_uri": f"https://{host}:{port}/keys"} self.jwks = jwks self_ = self + class MyHTTPRequestHandler(BaseHTTPRequestHandler): def do_GET(self): - routes = {"/.well-known/openid-configuration": metadata, "/keys": self_.jwks} + routes = { + "/.well-known/openid-configuration": metadata, + "/keys": self_.jwks, + } body = routes.get(self.path) if body is None: self.send_error(HTTPStatus.NOT_FOUND) return body = json.dumps(body).encode() self.send_response(HTTPStatus.OK) - self.send_header('Content-Length', str(len(body))) + self.send_header("Content-Length", str(len(body))) self.end_headers() self.wfile.write(body) - + def log_message(self, fmt: str, *args): LOG.debug(f"OpenIDProviderServer: {fmt % args}") @@ -348,13 +352,13 @@ def check_kv_jwt_key_matches(kid, cert_pem): cert_pem, cert_kv_pem ), "stored cert not equal to input cert" - def get_jwt_refresh_endpoint_metrics() -> dict: + def get_jwt_refresh_endpoint_metrics() -> dict: with primary.client( - f"member{network.consortium.get_any_active_member().member_id}" - ) as c: + f"member{network.consortium.get_any_active_member().member_id}" + ) as c: r = c.get("/gov/endpoint_metrics") m = r.body.json()["metrics"]["jwt_keys/refresh"]["POST"] - assert m["errors"] == 0, m["errors"] # not used in jwt refresh endpoint + assert m["errors"] == 0, m["errors"] # not used in jwt refresh endpoint m["successes"] = m["calls"] - m["failures"] return m @@ -364,7 +368,8 @@ def get_jwt_refresh_endpoint_metrics() -> dict: LOG.info("Add JWT issuer with auto-refresh") with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: json.dump( - {"issuer": issuer, "auto_refresh": True, "ca_cert_name": ca_cert_name}, metadata_fp + {"issuer": issuer, "auto_refresh": True, "ca_cert_name": ca_cert_name}, + metadata_fp, ) metadata_fp.flush() network.consortium.set_jwt_issuer(primary, metadata_fp.name) From 7850f19da3edb6365f4a0edd71239a783b966550 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Fri, 20 Nov 2020 13:49:03 +0000 Subject: [PATCH 13/18] exclude new endpoint from openapi --- src/node/rpc/endpoint_registry.h | 3 +++ src/node/rpc/member_frontend.h | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/node/rpc/endpoint_registry.h b/src/node/rpc/endpoint_registry.h index a8c0a707923b..d82a54e8f268 100644 --- a/src/node/rpc/endpoint_registry.h +++ b/src/node/rpc/endpoint_registry.h @@ -626,6 +626,9 @@ namespace ccf for (const auto& [path, verb_endpoints] : fully_qualified_endpoints) { + // Special endpoint, can only be called from the node. + if (path == "jwt_keys/refresh") + continue; for (const auto& [verb, endpoint] : verb_endpoints) { add_endpoint_to_api_document(document, endpoint); diff --git a/src/node/rpc/member_frontend.h b/src/node/rpc/member_frontend.h index c808a8797370..c4d4a4835d4c 100644 --- a/src/node/rpc/member_frontend.h +++ b/src/node/rpc/member_frontend.h @@ -1897,7 +1897,8 @@ namespace ccf { return make_error( HTTP_STATUS_INTERNAL_SERVER_ERROR, - fmt::format("{} does not have auto_refresh enabled", parsed.issuer)); + fmt::format( + "{} does not have auto_refresh enabled", parsed.issuer)); } if (!set_jwt_public_signing_keys( From bed63e6cd71ae7298a0c584d18daec5c5634b374 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Mon, 23 Nov 2020 12:40:47 +0000 Subject: [PATCH 14/18] address PR comments --- src/node/jwt_key_auto_refresh.h | 309 ++++++++++++++++++++++++++++++++ src/node/node_state.h | 281 ++--------------------------- src/node/rpc/member_frontend.h | 34 +++- 3 files changed, 354 insertions(+), 270 deletions(-) create mode 100644 src/node/jwt_key_auto_refresh.h diff --git a/src/node/jwt_key_auto_refresh.h b/src/node/jwt_key_auto_refresh.h new file mode 100644 index 000000000000..bd3c1499e65d --- /dev/null +++ b/src/node/jwt_key_auto_refresh.h @@ -0,0 +1,309 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the Apache 2.0 License. +#pragma once + +#include "http/http_builder.h" +#include "http/http_rpc_context.h" +#include "kv/tx.h" +#include "node/jwt.h" +#include "node/rpc/member_frontend.h" +#include "node/rpc/serdes.h" + +#define FMT_HEADER_ONLY +#include +#include + +namespace ccf +{ + class JwtKeyAutoRefresh + { + private: + size_t refresh_interval_s; + NetworkState& network; + std::shared_ptr consensus; + std::shared_ptr rpcsessions; + std::shared_ptr rpc_map; + tls::Pem node_cert; + + public: + JwtKeyAutoRefresh( + size_t refresh_interval_s, + NetworkState& network, + std::shared_ptr consensus, + std::shared_ptr rpcsessions, + std::shared_ptr rpc_map, + tls::Pem node_cert) : + refresh_interval_s(refresh_interval_s), + network(network), + consensus(consensus), + rpcsessions(rpcsessions), + rpc_map(rpc_map), + node_cert(node_cert) + {} + + void start() + { + struct RefreshTimeMsg + { + RefreshTimeMsg(JwtKeyAutoRefresh& self_) : self(self_) {} + + JwtKeyAutoRefresh& self; + }; + + auto refresh_msg = std::make_unique>( + [](std::unique_ptr> msg) { + if (!msg->data.self.consensus->is_primary()) + { + LOG_DEBUG_FMT( + "JWT key auto-refresh: Node is not primary, skipping"); + } + else + { + msg->data.self.refresh_jwt_keys(); + } + LOG_DEBUG_FMT( + "JWT key auto-refresh: Scheduling in {}s", + msg->data.self.refresh_interval_s); + auto delay = std::chrono::seconds(msg->data.self.refresh_interval_s); + threading::ThreadMessaging::thread_messaging.add_task_after( + std::move(msg), delay); + }, + *this); + + LOG_DEBUG_FMT( + "JWT key auto-refresh: Scheduling in {}s", refresh_interval_s); + auto delay = std::chrono::seconds(refresh_interval_s); + threading::ThreadMessaging::thread_messaging.add_task_after( + std::move(refresh_msg), delay); + } + + template + void send_refresh_jwt_keys(T msg) + { + auto body = serdes::pack(msg, serdes::Pack::Text); + + http::Request request(fmt::format( + "/{}/{}", + ccf::get_actor_prefix(ccf::ActorsType::members), + "jwt_keys/refresh")); + request.set_header( + http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON); + request.set_body(&body); + // Need a custom authentication policy that accepts only node certs. + // See https://github.com/microsoft/CCF/issues/1904 + // http::sign_request(request, node_sign_kp); + auto packed = request.build_request(); + + auto node_session = std::make_shared( + enclave::InvalidSessionId, node_cert.raw()); + auto ctx = enclave::make_rpc_context(node_session, packed); + + const auto actor_opt = http::extract_actor(*ctx); + if (!actor_opt.has_value()) + { + throw std::logic_error("Unable to get actor"); + } + + const auto actor = rpc_map->resolve(actor_opt.value()); + auto frontend_opt = this->rpc_map->find(actor); + if (!frontend_opt.has_value()) + { + throw std::logic_error( + "RpcMap::find returned invalid (empty) frontend"); + } + auto frontend = frontend_opt.value(); + frontend->process(ctx); + } + + void send_refresh_jwt_keys_error() + { + // A message that the endpoint fails to parse, leading to 500. + // This is done purely for exposing errors as endpoint metrics. + auto msg = false; + send_refresh_jwt_keys(msg); + } + + void handle_jwt_jwks_response( + const std::string& issuer, + http_status status, + std::vector&& data) + { + if (status != HTTP_STATUS_OK) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Error while requesting JWKS: {} {}{}", + status, + http_status_str(status), + data.empty() ? + "" : + fmt::format(" '{}'", std::string(data.begin(), data.end()))); + send_refresh_jwt_keys_error(); + return; + } + + LOG_DEBUG_FMT( + "JWT key auto-refresh: Received JWKS for issuer '{}'", issuer); + + JsonWebKeySet jwks; + try + { + jwks = nlohmann::json::parse(data).get(); + } + catch (const std::exception& e) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse JWKS for issuer '{}': {}", + issuer, + e.what()); + send_refresh_jwt_keys_error(); + return; + } + + // call internal endpoint to update keys + auto msg = SetJwtPublicSigningKeys{issuer, jwks}; + send_refresh_jwt_keys(msg); + } + + void handle_jwt_metadata_response( + const std::string& issuer, + std::shared_ptr ca_cert, + http_status status, + std::vector&& data) + { + if (status != HTTP_STATUS_OK) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Error while requesting OpenID metadata: {} " + "{}{}", + status, + http_status_str(status), + data.empty() ? + "" : + fmt::format(" '{}'", std::string(data.begin(), data.end()))); + send_refresh_jwt_keys_error(); + return; + } + + LOG_DEBUG_FMT( + "JWT key auto-refresh: Received OpenID metadata for issuer '{}'", + issuer); + + std::string jwks_url_str; + try + { + auto metadata = nlohmann::json::parse(data); + jwks_url_str = metadata.at("jwks_uri").get(); + } + catch (const std::exception& e) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse OpenID metadata for issuer '{}': " + "{}", + issuer, + e.what()); + send_refresh_jwt_keys_error(); + return; + } + http::URL jwks_url; + try + { + jwks_url = http::parse_url_full(jwks_url_str); + } + catch (const std::invalid_argument& e) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: Cannot parse jwks_uri for issuer '{}': {}", + issuer, + jwks_url_str); + send_refresh_jwt_keys_error(); + return; + } + auto jwks_url_port = !jwks_url.port.empty() ? jwks_url.port : "443"; + + LOG_DEBUG_FMT( + "JWT key auto-refresh: Requesting JWKS at https://{}:{}{}", + jwks_url.host, + jwks_url_port, + jwks_url.path); + auto http_client = rpcsessions->create_client(ca_cert); + // Note: Connection errors are not signalled and hence not tracked in + // endpoint metrics currently. + http_client->connect( + std::string(jwks_url.host), + std::string(jwks_url_port), + [this, issuer]( + http_status status, http::HeaderMap&&, std::vector&& data) { + handle_jwt_jwks_response(issuer, status, std::move(data)); + return true; + }); + http::Request r(jwks_url.path, HTTP_GET); + r.set_header(http::headers::HOST, std::string(jwks_url.host)); + http_client->send_request(r.build_request()); + } + + void refresh_jwt_keys() + { + auto tx = network.tables->create_read_only_tx(); + auto jwt_issuers_view = tx.get_read_only_view(network.jwt_issuers); + auto ca_certs_view = tx.get_read_only_view(network.ca_certs); + jwt_issuers_view->foreach([this, &ca_certs_view]( + const JwtIssuer& issuer, + const JwtIssuerMetadata& metadata) { + if (!metadata.auto_refresh) + { + LOG_DEBUG_FMT( + "JWT key auto-refresh: Skipping issuer '{}', auto-refresh is " + "disabled", + issuer); + return true; + } + LOG_DEBUG_FMT( + "JWT key auto-refresh: Refreshing keys for issuer '{}'", issuer); + auto& ca_cert_name = metadata.ca_cert_name.value(); + auto ca_cert_der = ca_certs_view->get(ca_cert_name); + if (!ca_cert_der.has_value()) + { + LOG_FAIL_FMT( + "JWT key auto-refresh: CA cert with name '{}' for issuer '{}' not " + "found", + ca_cert_name, + issuer); + send_refresh_jwt_keys_error(); + return true; + } + auto ca = std::make_shared(ca_cert_der.value()); + auto ca_cert = std::make_shared(ca); + + auto metadata_url_str = issuer + "/.well-known/openid-configuration"; + auto metadata_url = http::parse_url_full(metadata_url_str); + auto metadata_url_port = + !metadata_url.port.empty() ? metadata_url.port : "443"; + + LOG_DEBUG_FMT( + "JWT key auto-refresh: Requesting OpenID metadata at https://{}:{}{}", + metadata_url.host, + metadata_url_port, + metadata_url.path); + auto http_client = rpcsessions->create_client(ca_cert); + // Note: Connection errors are not signalled and hence not tracked in + // endpoint metrics currently. + http_client->connect( + std::string(metadata_url.host), + std::string(metadata_url_port), + [this, issuer, ca_cert]( + http_status status, + http::HeaderMap&&, + std::vector&& data) { + handle_jwt_metadata_response( + issuer, ca_cert, status, std::move(data)); + return true; + }); + http::Request r(metadata_url.path, HTTP_GET); + r.set_header(http::headers::HOST, std::string(metadata_url.host)); + http_client->send_request(r.build_request()); + return true; + }); + } + }; + +} \ No newline at end of file diff --git a/src/node/node_state.h b/src/node/node_state.h index 0ba24b53b108..98f0a7a04560 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -14,6 +14,7 @@ #include "network_state.h" #include "node/progress_tracker.h" #include "node/rpc/serdes.h" +#include "node/jwt_key_auto_refresh.h" #include "node_to_node.h" #include "rpc/frontend.h" #include "rpc/member_frontend.h" @@ -182,6 +183,11 @@ namespace ccf consensus::Index ledger_idx = 0; + // + // JWT key auto-refresh + // + std::shared_ptr jwt_key_auto_refresh; + public: NodeState( ringbuffer::AbstractWriterFactory& writer_factory, @@ -599,280 +605,17 @@ namespace ccf std::chrono::milliseconds(config.joining.join_timer)); } - template - void send_refresh_jwt_keys(T msg) - { - auto body = serdes::pack(msg, serdes::Pack::Text); - - http::Request request(fmt::format( - "/{}/{}", - ccf::get_actor_prefix(ccf::ActorsType::members), - "jwt_keys/refresh")); - request.set_header( - http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON); - request.set_body(&body); - // Need a custom authentication policy that accepts only node certs. - // See https://github.com/microsoft/CCF/issues/1904 - // http::sign_request(request, node_sign_kp); - auto packed = request.build_request(); - - auto node_session = std::make_shared( - enclave::InvalidSessionId, node_cert.raw()); - auto ctx = enclave::make_rpc_context(node_session, packed); - - const auto actor_opt = http::extract_actor(*ctx); - if (!actor_opt.has_value()) - { - throw std::logic_error("Unable to get actor"); - } - - const auto actor = rpc_map->resolve(actor_opt.value()); - auto frontend_opt = this->rpc_map->find(actor); - if (!frontend_opt.has_value()) - { - throw std::logic_error( - "RpcMap::find returned invalid (empty) frontend"); - } - auto frontend = frontend_opt.value(); - frontend->process(ctx); - } - void send_refresh_jwt_keys_error() - { - // A message that the endpoint fails to parse, leading to 500. - // This is done purely for exposing errors as endpoint metrics. - auto msg = false; - send_refresh_jwt_keys(msg); - } - - void handle_jwt_jwks_response( - const std::string& issuer, - http_status status, - std::vector&& data) - { - std::lock_guard guard(lock); - - if (status != HTTP_STATUS_OK) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Error while requesting JWKS: {} {}{}", - status, - http_status_str(status), - data.empty() ? - "" : - fmt::format(" '{}'", std::string(data.begin(), data.end()))); - send_refresh_jwt_keys_error(); - return; - } - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Received JWKS for issuer '{}'", issuer); - - JsonWebKeySet jwks; - try - { - jwks = nlohmann::json::parse(data).get(); - } - catch (std::exception& e) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Cannot parse JWKS for issuer '{}': {}", - issuer, - e.what()); - send_refresh_jwt_keys_error(); - return; - } - - // call internal endpoint to update keys - auto msg = SetJwtPublicSigningKeys{issuer, jwks}; - send_refresh_jwt_keys(msg); - } - - void handle_jwt_metadata_response( - const std::string& issuer, - std::shared_ptr ca_cert, - http_status status, - std::vector&& data) + void auto_refresh_jwt_keys(const CCFConfig& config) { - std::lock_guard guard(lock); - - if (status != HTTP_STATUS_OK) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Error while requesting OpenID metadata: {} " - "{}{}", - status, - http_status_str(status), - data.empty() ? - "" : - fmt::format(" '{}'", std::string(data.begin(), data.end()))); - send_refresh_jwt_keys_error(); - return; - } - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Received OpenID metadata for issuer '{}'", - issuer); - - std::string jwks_url_str; - try + if (!consensus) { - auto metadata = nlohmann::json::parse(data); - jwks_url_str = metadata.at("jwks_uri").get(); - } - catch (std::exception& e) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Cannot parse OpenID metadata for issuer '{}': " - "{}", - issuer, - e.what()); - send_refresh_jwt_keys_error(); + LOG_INFO_FMT("JWT key auto-refresh: consensus not initialized, not starting auto-refresh"); return; } - http::URL jwks_url; - try - { - jwks_url = http::parse_url_full(jwks_url_str); - } - catch (std::invalid_argument& e) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: Cannot parse jwks_uri for issuer '{}': {}", - issuer, - jwks_url_str); - send_refresh_jwt_keys_error(); - return; - } - auto jwks_url_port = !jwks_url.port.empty() ? jwks_url.port : "443"; - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Requesting JWKS at https://{}:{}{}", - jwks_url.host, - jwks_url_port, - jwks_url.path); - auto http_client = rpcsessions->create_client(ca_cert); - // Note: Connection errors are not signalled and hence not tracked in - // endpoint metrics currently. - http_client->connect( - std::string(jwks_url.host), - std::string(jwks_url_port), - [this, issuer]( - http_status status, http::HeaderMap&&, std::vector&& data) { - handle_jwt_jwks_response(issuer, status, std::move(data)); - return true; - }); - http::Request r(jwks_url.path, HTTP_GET); - r.set_header(http::headers::HOST, std::string(jwks_url.host)); - http_client->send_request(r.build_request()); - } - - void refresh_jwt_keys() - { - auto tx = network.tables->create_read_only_tx(); - auto jwt_issuers_view = tx.get_read_only_view(network.jwt_issuers); - auto ca_certs_view = tx.get_read_only_view(network.ca_certs); - jwt_issuers_view->foreach([this, &ca_certs_view]( - const JwtIssuer& issuer, - const JwtIssuerMetadata& metadata) { - if (!metadata.auto_refresh) - { - LOG_DEBUG_FMT( - "JWT key auto-refresh: Skipping issuer '{}', auto-refresh is " - "disabled", - issuer); - return true; - } - LOG_DEBUG_FMT( - "JWT key auto-refresh: Refreshing keys for issuer '{}'", issuer); - auto& ca_cert_name = metadata.ca_cert_name.value(); - auto ca_cert_der = ca_certs_view->get(ca_cert_name); - if (!ca_cert_der.has_value()) - { - LOG_FAIL_FMT( - "JWT key auto-refresh: CA cert with name '{}' for issuer '{}' not " - "found", - ca_cert_name, - issuer); - send_refresh_jwt_keys_error(); - return true; - } - auto ca = std::make_shared(ca_cert_der.value()); - auto ca_cert = std::make_shared(ca); - - auto metadata_url_str = issuer + "/.well-known/openid-configuration"; - auto metadata_url = http::parse_url_full(metadata_url_str); - auto metadata_url_port = - !metadata_url.port.empty() ? metadata_url.port : "443"; - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Requesting OpenID metadata at https://{}:{}{}", - metadata_url.host, - metadata_url_port, - metadata_url.path); - auto http_client = rpcsessions->create_client(ca_cert); - // Note: Connection errors are not signalled and hence not tracked in - // endpoint metrics currently. - http_client->connect( - std::string(metadata_url.host), - std::string(metadata_url_port), - [this, issuer, ca_cert]( - http_status status, - http::HeaderMap&&, - std::vector&& data) { - handle_jwt_metadata_response( - issuer, ca_cert, status, std::move(data)); - return true; - }); - http::Request r(metadata_url.path, HTTP_GET); - r.set_header(http::headers::HOST, std::string(metadata_url.host)); - http_client->send_request(r.build_request()); - return true; - }); - } - - void auto_refresh_jwt_keys(const CCFConfig& config) - { - auto jwt_key_refresh_interval_s = config.jwt_key_refresh_interval_s; - - struct RefreshTimeMsg - { - RefreshTimeMsg(NodeState& self_, size_t jwt_key_refresh_interval_s) : - self(self_), - jwt_key_refresh_interval_s(jwt_key_refresh_interval_s) - {} - - NodeState& self; - size_t jwt_key_refresh_interval_s; - }; - - auto refresh_msg = std::make_unique>( - [](std::unique_ptr> msg) { - if (!msg->data.self.consensus->is_primary()) - { - LOG_DEBUG_FMT( - "JWT key auto-refresh: Node is not primary, skipping"); - } - else - { - msg->data.self.refresh_jwt_keys(); - } - LOG_DEBUG_FMT( - "JWT key auto-refresh: Scheduling in {}s", - msg->data.jwt_key_refresh_interval_s); - auto delay = - std::chrono::seconds(msg->data.jwt_key_refresh_interval_s); - threading::ThreadMessaging::thread_messaging.add_task_after( - std::move(msg), delay); - }, - *this, - jwt_key_refresh_interval_s); - - LOG_DEBUG_FMT( - "JWT key auto-refresh: Scheduling in {}s", jwt_key_refresh_interval_s); - threading::ThreadMessaging::thread_messaging.add_task_after( - std::move(refresh_msg), - std::chrono::seconds(jwt_key_refresh_interval_s)); + jwt_key_auto_refresh = std::make_shared( + config.jwt_key_refresh_interval_s, network, consensus, rpcsessions, rpc_map, node_cert); + jwt_key_auto_refresh->start(); } // diff --git a/src/node/rpc/member_frontend.h b/src/node/rpc/member_frontend.h index c4d4a4835d4c..16bd055c72d3 100644 --- a/src/node/rpc/member_frontend.h +++ b/src/node/rpc/member_frontend.h @@ -1872,12 +1872,39 @@ namespace ccf EndpointContext& args, nlohmann::json&& body) { // All errors are server errors since the client is the server. + if (!consensus) + { + LOG_FAIL_FMT("JWT key auto-refresh: no consensus available"); + return make_error( + HTTP_STATUS_INTERNAL_SERVER_ERROR, "no consensus available"); + } + + auto primary_id = consensus->primary(); + auto nodes_view = args.tx.get_read_only_view(this->network.nodes); + auto info = nodes_view->get(primary_id); + if (!info.has_value()) + { + LOG_FAIL_FMT("JWT key auto-refresh: could not find node info of primary"); + return make_error( + HTTP_STATUS_INTERNAL_SERVER_ERROR, "could not find node info of primary"); + } + + auto primary_cert_pem = info.value().cert; + auto cert_der = args.rpc_ctx->session->caller_cert; + auto caller_cert_pem = tls::cert_der_to_pem(cert_der); + if (caller_cert_pem != primary_cert_pem) + { + LOG_FAIL_FMT("JWT key auto-refresh: request does not originate from primary"); + return make_error( + HTTP_STATUS_INTERNAL_SERVER_ERROR, "request does not originate from primary"); + } + SetJwtPublicSigningKeys parsed; try { parsed = body.get(); } - catch (JsonParseError& e) + catch (const JsonParseError& e) { return make_error( HTTP_STATUS_INTERNAL_SERVER_ERROR, "unable to parse body"); @@ -1887,6 +1914,7 @@ namespace ccf auto issuer_metadata_ = issuers->get(parsed.issuer); if (!issuer_metadata_.has_value()) { + LOG_FAIL_FMT(fmt::format("JWT key auto-refresh: {} is not a valid issuer", parsed.issuer)); return make_error( HTTP_STATUS_INTERNAL_SERVER_ERROR, fmt::format("{} is not a valid issuer", parsed.issuer)); @@ -1895,6 +1923,8 @@ namespace ccf if (!issuer_metadata.auto_refresh) { + LOG_FAIL_FMT(fmt::format( + "JWT key auto-refresh: {} does not have auto_refresh enabled", parsed.issuer)); return make_error( HTTP_STATUS_INTERNAL_SERVER_ERROR, fmt::format( @@ -1904,6 +1934,8 @@ namespace ccf if (!set_jwt_public_signing_keys( args.tx, INVALID_ID, parsed.issuer, issuer_metadata, parsed.jwks)) { + LOG_FAIL_FMT(fmt::format( + "JWT key auto-refresh: error while storing signing keys for issuer {}", parsed.issuer)); return make_error( HTTP_STATUS_INTERNAL_SERVER_ERROR, fmt::format( From 683356deb4c4edb939f06d6eafcfe6cc9f5db3b8 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Mon, 23 Nov 2020 12:54:54 +0000 Subject: [PATCH 15/18] remove explicit wait from e2e test --- tests/jwt_test.py | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/tests/jwt_test.py b/tests/jwt_test.py index a20196bce334..e021f426cfff 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -374,12 +374,9 @@ def get_jwt_refresh_endpoint_metrics() -> dict: metadata_fp.flush() network.consortium.set_jwt_issuer(primary, metadata_fp.name) - LOG.info("Wait for key refresh to happen") - # Note: refresh interval is set to 1s, see network args below. - time.sleep(2) - LOG.info("Check that keys got refreshed") - check_kv_jwt_key_matches(kid, cert_pem) + # Note: refresh interval is set to 1s, see network args below. + with_timeout(lambda: check_kv_jwt_key_matches(kid, cert_pem), timeout=5) LOG.info("Check that JWT refresh endpoint has no failures") m = get_jwt_refresh_endpoint_metrics() @@ -389,12 +386,11 @@ def get_jwt_refresh_endpoint_metrics() -> dict: LOG.info("Serve invalid JWKS") server.jwks = {"foo": "bar"} - LOG.info("Wait for key refresh to happen") - time.sleep(2) - LOG.info("Check that JWT refresh endpoint has some failures") - m = get_jwt_refresh_endpoint_metrics() - assert m["failures"] > 0, m["failures"] + def check_has_failures(): + m = get_jwt_refresh_endpoint_metrics() + assert m["failures"] > 0, m["failures"] + with_timeout(check_has_failures, timeout=5) LOG.info("Restart OpenID endpoint server with new keys") kid2 = "my_kid_2" @@ -402,22 +398,31 @@ def get_jwt_refresh_endpoint_metrics() -> dict: cert2_pem = infra.crypto.generate_cert(key2_priv_pem, cn=issuer_host) jwks = create_jwks(kid2, cert2_pem) with OpenIDProviderServer(issuer_port, key_priv_pem, cert_pem, jwks): - LOG.info("Wait for key refresh to happen") - time.sleep(2) - LOG.info("Check that keys got refreshed") - check_kv_jwt_key_matches(kid, None) + with_timeout(lambda: check_kv_jwt_key_matches(kid, None), timeout=5) check_kv_jwt_key_matches(kid2, cert2_pem) +def with_timeout(fn, timeout): + t0 = time.time() + while True: + try: + return fn() + except: + if time.time() - t0 < timeout: + time.sleep(0.1) + else: + raise + + def run(args): with infra.network.network( args.nodes, args.binary_dir, args.debug_nodes, args.perf_nodes, pdb=args.pdb ) as network: network.start_and_join(args) - network = test_jwt_without_key_policy(network, args) - network = test_jwt_with_sgx_key_policy(network, args) - network = test_jwt_with_sgx_key_filter(network, args) + # network = test_jwt_without_key_policy(network, args) + # network = test_jwt_with_sgx_key_policy(network, args) + # network = test_jwt_with_sgx_key_filter(network, args) network = test_jwt_key_auto_refresh(network, args) From c235fc7e00d7c4c425a3624aecb2841f952e3ebe Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Mon, 23 Nov 2020 12:56:25 +0000 Subject: [PATCH 16/18] formatting --- src/node/node_state.h | 14 ++++++++++---- src/node/rpc/member_frontend.h | 22 +++++++++++++++------- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/node/node_state.h b/src/node/node_state.h index 98f0a7a04560..d92f219ee9e1 100644 --- a/src/node/node_state.h +++ b/src/node/node_state.h @@ -12,9 +12,9 @@ #include "genesis_gen.h" #include "history.h" #include "network_state.h" +#include "node/jwt_key_auto_refresh.h" #include "node/progress_tracker.h" #include "node/rpc/serdes.h" -#include "node/jwt_key_auto_refresh.h" #include "node_to_node.h" #include "rpc/frontend.h" #include "rpc/member_frontend.h" @@ -605,16 +605,22 @@ namespace ccf std::chrono::milliseconds(config.joining.join_timer)); } - void auto_refresh_jwt_keys(const CCFConfig& config) { if (!consensus) { - LOG_INFO_FMT("JWT key auto-refresh: consensus not initialized, not starting auto-refresh"); + LOG_INFO_FMT( + "JWT key auto-refresh: consensus not initialized, not starting " + "auto-refresh"); return; } jwt_key_auto_refresh = std::make_shared( - config.jwt_key_refresh_interval_s, network, consensus, rpcsessions, rpc_map, node_cert); + config.jwt_key_refresh_interval_s, + network, + consensus, + rpcsessions, + rpc_map, + node_cert); jwt_key_auto_refresh->start(); } diff --git a/src/node/rpc/member_frontend.h b/src/node/rpc/member_frontend.h index 16bd055c72d3..2136c9a7290e 100644 --- a/src/node/rpc/member_frontend.h +++ b/src/node/rpc/member_frontend.h @@ -1884,9 +1884,11 @@ namespace ccf auto info = nodes_view->get(primary_id); if (!info.has_value()) { - LOG_FAIL_FMT("JWT key auto-refresh: could not find node info of primary"); + LOG_FAIL_FMT( + "JWT key auto-refresh: could not find node info of primary"); return make_error( - HTTP_STATUS_INTERNAL_SERVER_ERROR, "could not find node info of primary"); + HTTP_STATUS_INTERNAL_SERVER_ERROR, + "could not find node info of primary"); } auto primary_cert_pem = info.value().cert; @@ -1894,9 +1896,11 @@ namespace ccf auto caller_cert_pem = tls::cert_der_to_pem(cert_der); if (caller_cert_pem != primary_cert_pem) { - LOG_FAIL_FMT("JWT key auto-refresh: request does not originate from primary"); + LOG_FAIL_FMT( + "JWT key auto-refresh: request does not originate from primary"); return make_error( - HTTP_STATUS_INTERNAL_SERVER_ERROR, "request does not originate from primary"); + HTTP_STATUS_INTERNAL_SERVER_ERROR, + "request does not originate from primary"); } SetJwtPublicSigningKeys parsed; @@ -1914,7 +1918,8 @@ namespace ccf auto issuer_metadata_ = issuers->get(parsed.issuer); if (!issuer_metadata_.has_value()) { - LOG_FAIL_FMT(fmt::format("JWT key auto-refresh: {} is not a valid issuer", parsed.issuer)); + LOG_FAIL_FMT(fmt::format( + "JWT key auto-refresh: {} is not a valid issuer", parsed.issuer)); return make_error( HTTP_STATUS_INTERNAL_SERVER_ERROR, fmt::format("{} is not a valid issuer", parsed.issuer)); @@ -1924,7 +1929,8 @@ namespace ccf if (!issuer_metadata.auto_refresh) { LOG_FAIL_FMT(fmt::format( - "JWT key auto-refresh: {} does not have auto_refresh enabled", parsed.issuer)); + "JWT key auto-refresh: {} does not have auto_refresh enabled", + parsed.issuer)); return make_error( HTTP_STATUS_INTERNAL_SERVER_ERROR, fmt::format( @@ -1935,7 +1941,9 @@ namespace ccf args.tx, INVALID_ID, parsed.issuer, issuer_metadata, parsed.jwks)) { LOG_FAIL_FMT(fmt::format( - "JWT key auto-refresh: error while storing signing keys for issuer {}", parsed.issuer)); + "JWT key auto-refresh: error while storing signing keys for issuer " + "{}", + parsed.issuer)); return make_error( HTTP_STATUS_INTERNAL_SERVER_ERROR, fmt::format( From 1f65abb250d69d2e0e86e54296276310dafee847 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Mon, 23 Nov 2020 12:57:50 +0000 Subject: [PATCH 17/18] re-enable disabled tests --- tests/jwt_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/jwt_test.py b/tests/jwt_test.py index e021f426cfff..aabbbcc07e4a 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -420,9 +420,9 @@ def run(args): args.nodes, args.binary_dir, args.debug_nodes, args.perf_nodes, pdb=args.pdb ) as network: network.start_and_join(args) - # network = test_jwt_without_key_policy(network, args) - # network = test_jwt_with_sgx_key_policy(network, args) - # network = test_jwt_with_sgx_key_filter(network, args) + network = test_jwt_without_key_policy(network, args) + network = test_jwt_with_sgx_key_policy(network, args) + network = test_jwt_with_sgx_key_filter(network, args) network = test_jwt_key_auto_refresh(network, args) From 26a38fbd7a9e2a020103f8c095651a32829d08f9 Mon Sep 17 00:00:00 2001 From: Maik Riechert Date: Mon, 23 Nov 2020 13:14:55 +0000 Subject: [PATCH 18/18] make pylint happy --- tests/jwt_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/jwt_test.py b/tests/jwt_test.py index aabbbcc07e4a..2acc345d8dea 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -16,8 +16,6 @@ import infra.net import infra.e2e_args import suite.test_requirements as reqs -from infra.proposal import ProposalState -import ccf.proposal_generator from loguru import logger as LOG @@ -283,7 +281,7 @@ def do_GET(self): self.end_headers() self.wfile.write(body) - def log_message(self, fmt: str, *args): + def log_message(self, fmt, *args): # pylint: disable=arguments-differ LOG.debug(f"OpenIDProviderServer: {fmt % args}") with tempfile.NamedTemporaryFile( @@ -387,9 +385,11 @@ def get_jwt_refresh_endpoint_metrics() -> dict: server.jwks = {"foo": "bar"} LOG.info("Check that JWT refresh endpoint has some failures") + def check_has_failures(): m = get_jwt_refresh_endpoint_metrics() assert m["failures"] > 0, m["failures"] + with_timeout(check_has_failures, timeout=5) LOG.info("Restart OpenID endpoint server with new keys") @@ -408,7 +408,7 @@ def with_timeout(fn, timeout): while True: try: return fn() - except: + except Exception: if time.time() - t0 < timeout: time.sleep(0.1) else: