From eed11026fc1c45ac33cf26416049fa984da44e68 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Mon, 18 Sep 2023 15:19:02 +0000 Subject: [PATCH 1/6] enclave.file is no longer required, CLI option replaces it --- doc/host_config_schema/cchost_config.json | 4 ++-- src/host/main.cpp | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/doc/host_config_schema/cchost_config.json b/doc/host_config_schema/cchost_config.json index b49457a4be6c..c1f5593dd17f 100644 --- a/doc/host_config_schema/cchost_config.json +++ b/doc/host_config_schema/cchost_config.json @@ -25,7 +25,7 @@ } }, "description": "This section includes configuration for the enclave application launched by this node", - "required": ["file"], + "required": [], "additionalProperties": false }, "network": { @@ -680,6 +680,6 @@ "minimum": 0 } }, - "required": ["enclave", "network", "command"], + "required": ["network", "command"], "additionalProperties": false } diff --git a/src/host/main.cpp b/src/host/main.cpp index 13a7511744b8..885b51febc6b 100644 --- a/src/host/main.cpp +++ b/src/host/main.cpp @@ -112,6 +112,10 @@ int main(int argc, char** argv) "Logging level for the enclave code") ->transform(CLI::CheckedTransformer(log_level_options, CLI::ignore_case)); + std::string enclave_file_path; + app.add_option( + "--enclave-file", enclave_file_path, "Path to enclave application"); + try { app.parse(argc, argv); @@ -262,8 +266,17 @@ int main(int argc, char** argv) config.slow_io_logging_threshold; // create the enclave + if (enclave_file_path.empty() && !config.enclave.file.empty()) + { + LOG_FAIL_FMT( + "Enclave path was not passed on CLI! Using path specified in config " + "instead ({}). This fallback is deprecated and the CLI option will be " + "required in a future release.", + config.enclave.file); + enclave_file_path = config.enclave.file; + } host::Enclave enclave( - config.enclave.file, config.enclave.type, config.enclave.platform); + enclave_file_path, config.enclave.type, config.enclave.platform); // messaging ring buffers const auto buffer_size = config.memory.circuit_size; From 00c785fd3b00fee8bf6f8fbe32628002ac583d2c Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Mon, 18 Sep 2023 16:35:17 +0000 Subject: [PATCH 2/6] Remove aggressively, placating some tests --- src/host/configuration.h | 9 ++++++--- src/host/main.cpp | 7 +++++++ tests/config.jinja | 1 - tests/e2e_operations.py | 5 ++++- tests/infra/remote.py | 8 +++++++- 5 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/host/configuration.h b/src/host/configuration.h index a2339798be30..1d340a07c171 100644 --- a/src/host/configuration.h +++ b/src/host/configuration.h @@ -64,6 +64,8 @@ namespace host std::string file; EnclaveType type; EnclavePlatform platform; + + bool operator==(const Enclave&) const = default; }; Enclave enclave = {}; @@ -166,8 +168,8 @@ namespace host }; DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(CCHostConfig::Enclave); - DECLARE_JSON_REQUIRED_FIELDS(CCHostConfig::Enclave, type, file); - DECLARE_JSON_OPTIONAL_FIELDS(CCHostConfig::Enclave, platform); + DECLARE_JSON_REQUIRED_FIELDS(CCHostConfig::Enclave); + DECLARE_JSON_OPTIONAL_FIELDS(CCHostConfig::Enclave, file, type, platform); DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(CCHostConfig::OutputFiles); DECLARE_JSON_REQUIRED_FIELDS(CCHostConfig::OutputFiles); @@ -223,9 +225,10 @@ namespace host CCHostConfig::Command, service_certificate_file, start, join, recover); DECLARE_JSON_TYPE_WITH_BASE_AND_OPTIONAL_FIELDS(CCHostConfig, CCFConfig); - DECLARE_JSON_REQUIRED_FIELDS(CCHostConfig, enclave, command); + DECLARE_JSON_REQUIRED_FIELDS(CCHostConfig, command); DECLARE_JSON_OPTIONAL_FIELDS( CCHostConfig, + enclave, tick_interval, slow_io_logging_threshold, node_client_interface, diff --git a/src/host/main.cpp b/src/host/main.cpp index 885b51febc6b..521c57b9328d 100644 --- a/src/host/main.cpp +++ b/src/host/main.cpp @@ -275,6 +275,13 @@ int main(int argc, char** argv) config.enclave.file); enclave_file_path = config.enclave.file; } + + if (enclave_file_path.empty()) + { + LOG_FATAL_FMT("No enclave file path specified"); + return static_cast(CLI::ExitCodes::ValidationError); + } + host::Enclave enclave( enclave_file_path, config.enclave.type, config.enclave.platform); diff --git a/tests/config.jinja b/tests/config.jinja index 03d738915ccc..7db010bcf6c2 100644 --- a/tests/config.jinja +++ b/tests/config.jinja @@ -1,6 +1,5 @@ { "enclave": { - "file": "{{ enclave_file }}", "type": "{{ enclave_type }}", "platform": "{{ enclave_platform }}" }, diff --git a/tests/e2e_operations.py b/tests/e2e_operations.py index 6f967bd47e70..91a54faffe25 100644 --- a/tests/e2e_operations.py +++ b/tests/e2e_operations.py @@ -372,7 +372,8 @@ def run_config_timeout_check(args): # works as intended. It is difficult to do with the existing framework # as is because of the indirections and the fact that start() is a # synchronous call. - start_node_path = network.nodes[0].remote.remote.root + node = network.nodes[0] + start_node_path = node.remote.remote.root # Remove ledger and pid file to allow a restart shutil.rmtree(os.path.join(start_node_path, "0.ledger")) os.remove(os.path.join(start_node_path, "node.pid")) @@ -398,6 +399,8 @@ def run_config_timeout_check(args): "0.config.json", "--config-timeout", f"{config_timeout}s", + "--enclave-file", + node.remote.enclave_file, ], cwd=start_node_path, env=env, diff --git a/tests/infra/remote.py b/tests/infra/remote.py index b637b7bb5b3d..3f13e2722762 100644 --- a/tests/infra/remote.py +++ b/tests/infra/remote.py @@ -761,7 +761,7 @@ def __init__( t = t_env.get_template(self.TEMPLATE_CONFIGURATION_FILE) output = t.render( start_type=start_type.name.title(), - enclave_file=self.enclave_file, + enclave_file=self.enclave_file, # Ignored by current jinja, but passed for LTS compat enclave_type=enclave_type.title(), enclave_platform=enclave_platform.title() if enclave_platform == "virtual" @@ -842,6 +842,12 @@ def __init__( enclave_log_level, ] + if v is None or v >= Version("4.0.9"): + cmd += [ + "--enclave-file", + self.enclave_file, + ] + if start_type == StartType.start: members_info = kwargs.get("members_info") if not members_info: From 384b06bc291b37b1b104eb945671d28eef10ca8a Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Tue, 19 Sep 2023 09:36:53 +0000 Subject: [PATCH 3/6] Clean up your own mess --- tests/e2e_operations.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/e2e_operations.py b/tests/e2e_operations.py index 91a54faffe25..b3a5fb5e3b46 100644 --- a/tests/e2e_operations.py +++ b/tests/e2e_operations.py @@ -296,6 +296,7 @@ def run_file_operations(args): network.stop_all_nodes(skip_verification=True) test_split_ledger_on_stopped_network(primary, args) + args.common_read_only_ledger_dir = None # Reset for future tests def run_tls_san_checks(args): @@ -306,7 +307,6 @@ def run_tls_san_checks(args): args.perf_nodes, pdb=args.pdb, ) as network: - args.common_read_only_ledger_dir = None # Reset from previous test network.start_and_open(args) network.verify_service_certificate_validity_period( args.initial_service_cert_validity_days @@ -366,7 +366,6 @@ def run_config_timeout_check(args): args.perf_nodes, pdb=args.pdb, ) as network: - args.common_read_only_ledger_dir = None # Reset from previous test network.start_and_open(args) # This is relatively direct test to make sure the config timeout feature # works as intended. It is difficult to do with the existing framework @@ -435,7 +434,6 @@ def run_sighup_check(args): args.perf_nodes, pdb=args.pdb, ) as network: - args.common_read_only_ledger_dir = None # Reset from previous test network.start_and_open(args) network.nodes[0].remote.remote.hangup() time.sleep(1) @@ -476,7 +474,6 @@ def run_preopen_readiness_check(args): args.perf_nodes, pdb=args.pdb, ) as network: - args.common_read_only_ledger_dir = None # Reset from previous test network.start(args) primary, _ = network.find_primary() with primary.client() as c: @@ -500,7 +497,6 @@ def run_pid_file_check(args): args.perf_nodes, pdb=args.pdb, ) as network: - args.common_read_only_ledger_dir = None # Reset from previous test network.start_and_open(args) LOG.info("Check that pid file exists") node = network.nodes[0] From 0db5933df9e562555f51771704f0931fe2af42f5 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Tue, 19 Sep 2023 10:29:12 +0000 Subject: [PATCH 4/6] CHANGELOG --- CHANGELOG.md | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11ce54923f27..f764c133eb21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,12 +5,6 @@ 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-dev3] - -[5.0.0-dev3]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.0-dev3 - -- Updated Intel SGX PSW from 2.17 to 2.20 (#5616) - ## [5.0.0-dev2] [5.0.0-dev2]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.0-dev2 @@ -20,6 +14,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Updated QCBOR from `1.1` to `1.2`. - Updated `nghttp2` from `1.51.0` to `1.55.1`. - Converted SNP attestation UVM endorsements from integer to arbitrary string. +- Updated Intel SGX PSW from 2.17 to 2.20 (#5616) +- Path to the enclave file should now be passed as `--enclave-file` CLI argument to `cchost`, rather than `enclave.file` entry within configuration file. The configuration entry is deprecated, and will be removed in a future release. ## [5.0.0-dev1] From c2e0bccceaaeb766f45b978304997ef53ef4e1f8 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Tue, 19 Sep 2023 10:40:06 +0000 Subject: [PATCH 5/6] Take wing --- .daily_canary | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.daily_canary b/.daily_canary index ba9dbcc8e24a..14115e54561c 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------>> From 410af74ba3bf8af7bf51a00a1729de8456be9a53 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Tue, 19 Sep 2023 13:28:15 +0000 Subject: [PATCH 6/6] Feedback, inline documentation of security-critical params --- CHANGELOG.md | 2 +- src/host/main.cpp | 28 +++++++++++++++++++--------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f764c133eb21..1e33e98b170f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Updated `nghttp2` from `1.51.0` to `1.55.1`. - Converted SNP attestation UVM endorsements from integer to arbitrary string. - Updated Intel SGX PSW from 2.17 to 2.20 (#5616) -- Path to the enclave file should now be passed as `--enclave-file` CLI argument to `cchost`, rather than `enclave.file` entry within configuration file. The configuration entry is deprecated, and will be removed in a future release. +- Path to the enclave file should now be passed as `--enclave-file` CLI argument to `cchost`, rather than `enclave.file` entry within configuration file. This is to ensure the path to the application file is attested on Confidential Containers/SNP, even if the configuration itself is provided from un-attested storage. The configuration entry is deprecated, and will be removed in a future release. ## [5.0.0-dev1] diff --git a/src/host/main.cpp b/src/host/main.cpp index 521c57b9328d..10bd492be771 100644 --- a/src/host/main.cpp +++ b/src/host/main.cpp @@ -78,7 +78,13 @@ int main(int argc, char** argv) { // ignore SIGPIPE signal(SIGPIPE, SIG_IGN); - CLI::App app{"ccf"}; + CLI::App app{ + "CCF Host launcher. Runs a single CCF node, based on the given " + "configuration file.\n" + "Some parameters are marked \"(security critical)\" - these must be passed " + "on the CLI rather than within a configuration file, so that (on relevant " + "platforms) their value is captured in an attestation even if the " + "configuration file itself is unattested.\n"}; std::string config_file_path = "config.json"; app.add_option( @@ -109,12 +115,14 @@ int main(int argc, char** argv) .add_option( "--enclave-log-level", enclave_log_level, - "Logging level for the enclave code") + "Logging level for the enclave code (security critical)") ->transform(CLI::CheckedTransformer(log_level_options, CLI::ignore_case)); std::string enclave_file_path; app.add_option( - "--enclave-file", enclave_file_path, "Path to enclave application"); + "--enclave-file", + enclave_file_path, + "Path to enclave application (security critical)"); try { @@ -266,14 +274,16 @@ int main(int argc, char** argv) config.slow_io_logging_threshold; // create the enclave - if (enclave_file_path.empty() && !config.enclave.file.empty()) + if (!config.enclave.file.empty()) { LOG_FAIL_FMT( - "Enclave path was not passed on CLI! Using path specified in config " - "instead ({}). This fallback is deprecated and the CLI option will be " - "required in a future release.", - config.enclave.file); - enclave_file_path = config.enclave.file; + "DEPRECATED: Enclave path was specified in config file! This should be " + "removed from the config, and passed directly to the CLI instead"); + + if (enclave_file_path.empty()) + { + enclave_file_path = config.enclave.file; + } } if (enclave_file_path.empty())