Changes to Fix Ledger APIs used to set Contract Enclave attestation policy #467

prakashngit · 2024-02-13T06:08:20Z

This is an enabler PR to improve the current implementation of registration of contract enclave attestation policy with the ledger.

The current register_enclave_attestation_verification_policy member API that is used to register attestation policy with ledger is replaced with two member APIs: set_contract_enclave_check_attestation_flag and set_contract_enclave_expected_sgx_measurements.

set_contract_enclave_check_attestation_flag is invoked as part of ledger start up, and simply specifies whether attestation check should be enabled or not for contract enclaves. The expectation is to disable the flag for SGX_SIM mode and enable the flag for HW mode. If the flag is enabled, the second API set_contract_enclave_expected_sgx_measurements must be subsquently invoked by the member to provide expected values for mrenclave, basename and ias_public_key.

This PR does not specify when/how the second API gets invoked with HW mode. That will be addressed in a separate PR.

bvavala

Minor comments, but it's good and other PRs can go from here.

bvavala · 2024-02-14T19:37:46Z

ledgers/ccf/scripts/set_contract_enclave_check_attestation_flag.py

+ContractHome = os.environ.get("PDO_HOME") or os.path.realpath("/opt/pdo")
+CCF_Keys = os.environ.get("PDO_LEDGER_KEY_ROOT") or os.path.join(ContractHome, "ccf", "keys")


This can be cleaned up. ContractHome is not used and the keys should be in the ledger key root.

line 30 is consistent with other variable definitions, see for example line 29. We use this pattern all over pdo. nothing unique about 30.

Right, because it's old code that gets copy-pasted every time, but it's not used and never cleaned up.

i think it is reasonable to have default values for variables. at best, this should be discussed as separate PR. the pattern is not restricted to ccf scripts.

there is a lot of clean up to be done on these scripts to make them match expectations. suggest we not worry too much about optimization and clarity & focus this PR on flow and function. See issue #469

bvavala · 2024-02-14T19:37:54Z

ledgers/ccf/scripts/set_contract_enclave_check_attestation_flag.py

+
+# -----------------------------------------------------------------
+def Main() :
+    default_output = os.path.join(CCF_Keys, 'ledger_authority.pem')


thanks, will remove.

ledgers/ccf/transaction_processor/pdo_tp.cpp

bvavala · 2024-02-14T19:53:20Z

eservice/setup.py

@@ -36,6 +36,8 @@
 log_dir = os.path.join(install_root_dir, "logs")
 key_dir = os.path.join(install_root_dir, "keys")

+register_sgx_expected_measurements_script = os.path.join(pdo_root_dir, "ledgers/ccf/scripts/set_contract_enclave_expected_sgx_measurements.py")


I would maintain some naming consistency here, either register... or set....
Or maybe simply remove the variable and dump the path directly in datafile below.

thanks, will fix.

i would suggest we address issue #469 with all of the scripts. this is a reasonable start. but its going to be very difficult to manage in general.

cmickeyb

Generally looks like what we need.
At a high level, I think we should address the general problem of installing the ccf python scripts as a high priority task that you can take advantage of.

cmickeyb · 2024-02-15T18:17:09Z

eservice/setup.py

@@ -36,6 +36,8 @@
 log_dir = os.path.join(install_root_dir, "logs")
 key_dir = os.path.join(install_root_dir, "keys")

+register_sgx_expected_measurements_script = os.path.join(pdo_root_dir, "ledgers/ccf/scripts/set_contract_enclave_expected_sgx_measurements.py")


i would suggest we address issue #469 with all of the scripts. this is a reasonable start. but its going to be very difficult to manage in general.

cmickeyb · 2024-02-15T18:19:21Z

ledgers/ccf/scripts/set_contract_enclave_check_attestation_flag.py

+
+# -----------------------------------------------------------------
+# -----------------------------------------------------------------
+Main()


all of these would be a lot cleaner if we could assume that pdo client modules were installed. it would also make everything else a LOT more consistent and easy to maintain. we do not need these for ledger nodes, but they should be installable/runnable in the same way that every other pdo script is run on every other pdo node.

bvavala

I think there are inconsistencies in error handling.
The question is: how does a CCF client recognize errors (success + error string; or error + error string)?

CCF provides the make_success and make_error APIs for that. However, many times the current code seems to return success (instead of error) with an error string.
I would argue that "error + error string" is preferable -- but one way or the other is fine, so long as it's consistent.

bvavala · 2024-02-20T22:05:54Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+            // is returned. (Note that global commit of the flag might be pending, and this is OK).
+            auto check_attestation_flag_check = check_attestation_flag_view->get(PDO_ENCLAVE_CHECK_ATTESTATION_FLAG);
+            if (check_attestation_flag_check.has_value()){
+                return ccf::make_success("Attesation check flag can be set only once");


Shouldn't this be make_error? (besides, it says that an error is returned)

thats right will fix

bvavala · 2024-02-20T22:06:31Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+            auto check_attestation_flag_view = ctx.tx.rw(contract_enclave_check_attestation_flag);
+            auto check_attestation_flag_global = check_attestation_flag_view->get_globally_committed(PDO_ENCLAVE_CHECK_ATTESTATION_FLAG);
+            if (!check_attestation_flag_global.has_value()){
+                return ccf::make_success("Please enable attesation check before providing expected measurements");


(same as above)

thanks, fixed

bvavala · 2024-02-20T22:06:42Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+            }
+            auto check_attestation_flag = check_attestation_flag_global.value();
+            if (!check_attestation_flag.check_attestation){
+                return ccf::make_success("Please enable attesation check before providing expected measurements");


(same as above)

thanks, fixed

bvavala · 2024-02-20T22:08:42Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+            auto check_attestation_flag_view = ctx.tx.rw(contract_enclave_check_attestation_flag);
+            auto check_attestation_flag_global = check_attestation_flag_view->get_globally_committed(PDO_ENCLAVE_CHECK_ATTESTATION_FLAG);
+            if (!check_attestation_flag_global.has_value()){
+                return ccf::make_success("No attestation verification policy has been set. Enclave cannot be registered");


(same as above)

thanks, fixed

bvavala · 2024-02-20T22:08:57Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+                auto expected_sgx_measurements_view = ctx.tx.rw(contract_enclave_expected_sgx_measurements);
+                auto expected_sgx_measurements_global = expected_sgx_measurements_view->get_globally_committed(PDO_ENCLAVE_EXPECTED_SGX_MEASUREMENTS);
+                if (!check_attestation_flag_global.has_value()){
+                    return ccf::make_success("Expected sgx measurents have not been set. Enclave cannot be registered");


(same as above)

thanks, fixed

ledgers/ccf/transaction_processor/pdo_tp.cpp

ledgers/ccf/pdo/ledgers/ccf/scripts/set_contract_enclave_check_attestation_flag.py

ledgers/ccf/pdo/ledgers/ccf/scripts/set_contract_enclave_expected_sgx_measurements.py

ledgers/ccf/pdo/ledgers/ccf/scripts/set_contract_enclave_check_attestation_flag.py

eservice/bin/register-with-ledger.sh

ledgers/ccf/scripts/start_ccf_network.sh

ledgers/ccf/pdo/ledgers/ccf/scripts/set_contract_enclave_expected_sgx_measurements.py

ledgers/ccf/setup.py

bvavala

Minor corrections, I think it's good.

ledgers/ccf/pdo/ledgers/ccf/scripts/set_contract_enclave_check_attestation_flag.py

bvavala · 2024-02-21T20:05:45Z

ledgers/ccf/pdo/ledgers/ccf/scripts/set_contract_enclave_expected_sgx_measurements.py

+    local_options = parser.parse_args(unprocessed_args)
+
+    if (not local_options.mrenclave) or (not local_options.basename) or (not local_options.ias_public_key):
+        parser.print_help()


Suggested change

parser.print_help()

LOG.error('missing parameters')

parser.print_help()

actually the print_help is standard. its an interactive command and any other command line parameter would generate the same response. in general... we are relying far too much on the logs for the response to interactive commands but we can address that separately. this one absolutely should not be logged however.

leaving it as is.

bvavala · 2024-02-21T20:07:43Z

ledgers/ccf/pdo/ledgers/ccf/scripts/set_contract_enclave_expected_sgx_measurements.py

+        set_contract_enclave_expected_sgx_measurements(member_client, local_options)
+    except Exception as e:
+        while e.__context__ : e = e.__context__
+        LOG.error('failed to set contract enclave attestation check flag: {}', str(e))


Suggested change

LOG.error('failed to set contract enclave attestation check flag: {}', str(e))

LOG.error('failed to set contract enclave expected sgx measurements: {}', str(e))

thanks, fixed

bvavala · 2024-02-21T20:11:54Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+            auto check_attestation_flag_global = check_attestation_flag_view->get_globally_committed(PDO_ENCLAVE_CHECK_ATTESTATION_FLAG);
+            if (!check_attestation_flag_global.has_value()){
+                return ccf::make_error(
+                    HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,"Please enable attesation check before providing expected measurements");


Suggested change

HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,"Please enable attesation check before providing expected measurements");

HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,"Please set the check-attestation flag before providing expected measurements");

thanks, fixed

bvavala · 2024-02-21T20:16:24Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

                return ccf::make_error(
-                    HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput, "No attestation policy has been set. Enclave cannot be registered");
+                HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput, "No attestation verification policy has been set. Enclave cannot be registered");


Suggested change

HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput, "No attestation verification policy has been set. Enclave cannot be registered");

HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput, "No check-attestation flag has been set. Enclave cannot be registered");

thanks, fixed

bvavala · 2024-02-21T21:09:48Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+            auto check_attestation_flag_check = check_attestation_flag_view->get(PDO_ENCLAVE_CHECK_ATTESTATION_FLAG);
+            if (check_attestation_flag_check.has_value()){
+                return ccf::make_error(
+                    HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,"Attesation check flag can be set only once");


Suggested change

HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,"Attesation check flag can be set only once");

HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,"Check-attestation flag can be set only once");

ledgers/ccf/transaction_processor/pdo_tp.cpp

…policy. Changes to address Review Comments have been squashed into the same commit. Signed-off-by: Prakash Narayana Moorthy <[email protected]>

cmickeyb

I would still like a lot more documentation on what each of these scripts does. Please plan to provide updates to the readme (or use the docstrings in the python files) to provide a human readable (as in something that could be understood by someone who might want to use the script, NOT someone who wants to develop it).

prakashngit requested review from cmickeyb and bvavala February 13, 2024 06:08

prakashngit assigned bvavala Feb 13, 2024

prakashngit added the enhancement New feature or request label Feb 13, 2024

bvavala reviewed Feb 14, 2024

View reviewed changes

cmickeyb reviewed Feb 15, 2024

View reviewed changes

bvavala mentioned this pull request Feb 16, 2024

Consolidation of register-with-ledger step #415

Closed

cmickeyb mentioned this pull request Feb 19, 2024

Significant update to build and install process for ccf ledger #470

Merged

bvavala reviewed Feb 20, 2024

View reviewed changes

prakashngit force-pushed the Prakash.Fix_Ledger_APIs_for_attestation_policy_registration branch from 3366b95 to 68430c2 Compare February 21, 2024 05:03

prakashngit marked this pull request as ready for review February 21, 2024 05:04

cmickeyb requested changes Feb 21, 2024

View reviewed changes

bvavala requested changes Feb 21, 2024

View reviewed changes

Changes to Fix Ledger APIs used to set Contract Enclave atttestation …

99a7478

…policy. Changes to address Review Comments have been squashed into the same commit. Signed-off-by: Prakash Narayana Moorthy <[email protected]>

prakashngit force-pushed the Prakash.Fix_Ledger_APIs_for_attestation_policy_registration branch from 2dad0bd to 99a7478 Compare February 22, 2024 20:36

prakashngit requested review from cmickeyb and bvavala February 22, 2024 20:37

bvavala approved these changes Feb 23, 2024

View reviewed changes

cmickeyb approved these changes Feb 23, 2024

View reviewed changes

bvavala merged commit 955e2b7 into hyperledger-labs:main Feb 26, 2024
4 checks passed

		ContractHome = os.environ.get("PDO_HOME") or os.path.realpath("/opt/pdo")
		CCF_Keys = os.environ.get("PDO_LEDGER_KEY_ROOT") or os.path.join(ContractHome, "ccf", "keys")

	parser.print_help()
	LOG.error('missing parameters')
	parser.print_help()

	LOG.error('failed to set contract enclave attestation check flag: {}', str(e))
	LOG.error('failed to set contract enclave expected sgx measurements: {}', str(e))

	HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,"Please enable attesation check before providing expected measurements");
	HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,"Please set the check-attestation flag before providing expected measurements");

Changes to Fix Ledger APIs used to set Contract Enclave attestation policy #467

Changes to Fix Ledger APIs used to set Contract Enclave attestation policy #467

Conversation

prakashngit commented Feb 13, 2024

bvavala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmickeyb Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmickeyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvavala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvavala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmickeyb Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmickeyb left a comment

Choose a reason for hiding this comment

cmickeyb Feb 15, 2024 •

edited

Loading

cmickeyb Feb 21, 2024 •

edited

Loading