From 583fab2039990d4535a152d77016998e78d84f7c Mon Sep 17 00:00:00 2001 From: Amaury Chamayou Date: Tue, 14 Nov 2023 16:02:18 +0000 Subject: [PATCH] Authenticate recovery share submission (#5832) --- .daily_canary | 2 +- CHANGELOG.md | 6 ++++ python/utils/submit_recovery_share.sh | 44 +++++++++++++++------------ src/node/gov/handlers/recovery.h | 28 +++++++++++------ tests/infra/member.py | 25 ++++++++++++--- 5 files changed, 72 insertions(+), 33 deletions(-) diff --git a/.daily_canary b/.daily_canary index bdbdf905bf22..dbaa880daab0 100644 --- a/.daily_canary +++ b/.daily_canary @@ -1,4 +1,4 @@ -^- ___ ___ (- -) (= =) | Y & +--? ( V ) / . \ | +---=---' -/--x-m- /--n-n---xXx--/--yY------>>>----<<<>>]]{{}}--||- +/--x-m- /--n-n---xXx--/--yY------>>>----<<<>>]]{{}}---||- diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f668b788f39..5b3aa2ef169c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [5.0.0-dev7] + +[5.0.0-dev7]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.0-dev7 + +- `POST /recovery/members/{memberId}:recover` is now authenticated by COSE Sign1, making it consistent with the other `POST` endpoints in governance, and avoiding a potential denial of service where un-authenticated and un-authorised clients could submit invalid shares repeatedly. The `submit_recovery_share.sh` script has been amended accordingly, and now takes a `--member-id-privk` and `--member-id-cert` (#5821). + ## [5.0.0-dev6] [5.0.0-dev6]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.0-dev6 diff --git a/python/utils/submit_recovery_share.sh b/python/utils/submit_recovery_share.sh index 98332910703d..927af0db9d39 100755 --- a/python/utils/submit_recovery_share.sh +++ b/python/utils/submit_recovery_share.sh @@ -7,7 +7,7 @@ set -e function usage() { echo "Usage:""" - echo " $0 https:// --member-enc-privk /path/to/member_enc_privk.pem --api-version api_version --cert /path/to/member_cert.pem [CURL_OPTIONS]" + echo " $0 https:// --member-enc-privk /path/to/member_enc_privk.pem --api-version api_version --member-id-privk /path/to/member_id_privk.pem ----member-id-cert /path/to/member_cert.pem [CURL_OPTIONS]" echo "Retrieves the encrypted recovery share for a given member, decrypts the share and submits it for recovery." echo "" echo "A sufficient number of recovery shares must be submitted by members to initiate the end of recovery procedure." @@ -35,6 +35,12 @@ while [ "$1" != "" ]; do --member-enc-privk) member_enc_privk="$2" ;; + --member-id-privk) + member_id_privk="$2" + ;; + --member-id-cert) + member_id_cert="$2" + ;; --api-version) api_version="$2" ;; @@ -45,31 +51,30 @@ while [ "$1" != "" ]; do shift done -# Loop through all arguments and find cert -next_is_cert=false - -for item in "$@" ; do - if [ "$next_is_cert" == true ]; then - cert=$item - next_is_cert=false - fi - if [ "$item" == "--cert" ]; then - next_is_cert=true - fi -done - if [ -z "${member_enc_privk}" ]; then echo "Error: No member encryption private key in arguments (--member-enc-privk)" exit 1 fi -if [ -z "${cert}" ]; then - echo "Error: No user certificate in arguments (--cert)" +if [ -z "${member_id_privk}" ]; then + echo "Error: No member identity private key in arguments (--member-id-privk)" exit 1 fi +if [ -z "${member_id_cert}" ]; then + echo "Error: No member identity cert in arguments (--member-id-cert)" + exit 1 +fi + +if [ ! -f "env/bin/activate" ] + then + python3.8 -m venv env +fi +source env/bin/activate +pip install -q ccf + # Compute member ID, as the SHA-256 fingerprint of the signing certificate -member_id=$(openssl x509 -in "$cert" -noout -fingerprint -sha256 | cut -d "=" -f 2 | sed 's/://g' | awk '{print tolower($0)}') +member_id=$(openssl x509 -in "$member_id_cert" -noout -fingerprint -sha256 | cut -d "=" -f 2 | sed 's/://g' | awk '{print tolower($0)}') if [ "${api_version}" == "classic" ]; then get_share_path="gov/encrypted_recovery_share/${member_id}" @@ -89,5 +94,6 @@ encrypted_share=$(curl -sS --fail -X GET "${node_rpc_address}/${get_share_path}" echo "${encrypted_share}" \ | openssl base64 -d \ | openssl pkeyutl -inkey "${member_enc_privk}" -decrypt -pkeyopt rsa_padding_mode:oaep -pkeyopt rsa_oaep_md:sha256 \ - | openssl base64 -A | jq -R '{share: (.)}' \ - | curl -i -sS --fail -H "Content-Type: application/json" -X POST "${node_rpc_address}/${submit_share_path}" "${@}" -d @- \ No newline at end of file + | openssl base64 -A | jq -c -R '{share: (.)}' \ + | ccf_cose_sign1 --ccf-gov-msg-type recovery_share --ccf-gov-msg-created_at "$(date -uIs)" --signing-key "${member_id_privk}" --signing-cert "${member_id_cert}" --content "-" \ + | curl -i -sS --fail -H "Content-Type: application/cose" -X POST "${node_rpc_address}/${submit_share_path}" "${@}" --data-binary @- \ No newline at end of file diff --git a/src/node/gov/handlers/recovery.h b/src/node/gov/handlers/recovery.h index 9da880d9bee2..3e88ecf94b12 100644 --- a/src/node/gov/handlers/recovery.h +++ b/src/node/gov/handlers/recovery.h @@ -106,16 +106,26 @@ namespace ccf::gov::endpoints return; } - const auto& raw_body = ctx.rpc_ctx->get_request_body(); - const nlohmann::json params = nlohmann::json::parse(raw_body); + const auto& cose_ident = + ctx.template get_caller(); - auto raw_recovery_share = - crypto::raw_from_b64(params["share"].get()); + auto params = nlohmann::json::parse(cose_ident.content); + if (cose_ident.member_id != member_id) + { + detail::set_gov_error( + ctx.rpc_ctx, + HTTP_STATUS_BAD_REQUEST, + ccf::errors::InvalidAuthenticationInfo, + fmt::format( + "Member ID from path parameter ({}) does not match " + "member ID from body signature ({}).", + member_id, + cose_ident.member_id)); + return; + } - // Cleanse other copies of secret where possible. Note that this - // leaves a JSON-parsed copy, and potentially others in the TLS/HTTP - // stack. - OPENSSL_cleanse((char*)raw_body.data(), raw_body.size()); + auto raw_recovery_share = + crypto::raw_from_b64(params["share"].template get()); size_t submitted_shares_count = 0; try @@ -195,7 +205,7 @@ namespace ccf::gov::endpoints "/recovery/members/{memberId}:recover", HTTP_POST, api_version_adapter(submit_recovery_share), - ccf::no_auth_required) + detail::active_member_sig_only_policies("recovery_share")) .set_openapi_hidden(true) .install(); } diff --git a/tests/infra/member.py b/tests/infra/member.py index a9365fa232a7..9820c750997c 100644 --- a/tests/infra/member.py +++ b/tests/infra/member.py @@ -363,6 +363,7 @@ def get_and_submit_recovery_share(self, remote_node): help_res.check_returncode() help_out = help_res.stdout.decode() supports_api_version = "--api-version" in help_out + support_member_id_cert = "--member-id-cert" in help_out cmd = [ self.share_script, @@ -371,17 +372,33 @@ def get_and_submit_recovery_share(self, remote_node): os.path.join(self.common_dir, f"{self.local_id}_enc_privk.pem"), ] + # Versions of the script that support --member-id arguments use COSE Sign1 + # to authenticate with the service. + if support_member_id_cert: + cmd += [ + "--member-id-privk", + os.path.join(self.common_dir, f"{self.local_id}_privk.pem"), + "--member-id-cert", + os.path.join(self.common_dir, f"{self.local_id}_cert.pem"), + ] + if supports_api_version: cmd += [ "--api-version", self.gov_api_impl.API_VERSION, ] + # Versions of the script that do not support --member-id arguments use + # client certificates (forward to curl) to authenticate with the service. + if not support_member_id_cert: + cmd += [ + "--key", + os.path.join(self.common_dir, f"{self.local_id}_privk.pem"), + "--cert", + os.path.join(self.common_dir, f"{self.local_id}_cert.pem"), + ] + cmd += [ - "--cert", - os.path.join(self.common_dir, f"{self.local_id}_cert.pem"), - "--key", - os.path.join(self.common_dir, f"{self.local_id}_privk.pem"), "--cacert", os.path.join(self.common_dir, "service_cert.pem"), ]