Implementation of Enclave Key Refresh #358

manju956 · 2020-03-26T08:34:26Z

This feature initiates refresh of enclave encryption key pair based on timer value set in the config file. A new pair of encryption key is generated in the enclave and the updated enclave signup details are stored in the KvStorage in workers table.
Additioanlly, encryption key signature is also generated during enclave initialization and its made part of PublicEnclaveData. This encryption key signature is re-computed when encryption key gets refreshed.

When a worker key gets refreshed during the work order submission, a specific error code is returned to client to indicate worker key refresh happened at the enclave. On receiving this error code, client retrieves the updated worker details and does work order submission again.

common/python/error_code/error_status.py

examples/apps/generic_client/generic_client.py

listener/avalon_listener/tcs_work_order_handler.py

enclave_manager/avalon_enclave_manager/enclave_manager.py

tc/sgx/trusted_worker_manager/enclave/signup_enclave.cpp

tc/sgx/trusted_worker_manager/enclave_untrusted/enclave_bridge/signup.cpp

tc/sgx/trusted_worker_manager/enclave/enclave_utils.cpp

danintel

Approved after you make corrections I suggested.

common/python/error_code/enclave_error.py

manojgop · 2020-03-27T06:24:21Z

common/python/error_code/error_status.py

    PROCESSING = 7
    BUSY = 8
    UNKNOWN_ERROR = 9
+    WORKER_KEY_REFRESHED = 10


WORKER_KEY_REFRESHED -> WORKER_ENCRYPTION_KEY_REFRESHED

enclave_manager/avalon_enclave_manager/enclave_manager.py

manojgop · 2020-03-27T06:33:52Z

enclave_manager/avalon_enclave_manager/enclave_manager.py

+        enclave_manager.enclave_id).encode("UTF-8")
+    # Calculate sha256 of worker id to get 32 bytes. The TC spec proxy
+    # model contracts expect byte32. Then take a hexdigest for hex str.
+    worker_id = hashlib.sha256(worker_id).hexdigest()


Create a function generate_worker_id() and call it at multiple places instead of duplicating this

manojgop · 2020-03-27T07:05:13Z

enclave_manager/avalon_enclave_manager/enclave_manager.py

            enclave_manager.process_work_orders(kv_helper)
            logger.info("Enclave manager sleeping for %d secs", sleep_interval)
            time.sleep(sleep_interval)
+            key_refresh_time = key_refresh_time + sleep_interval


Decouple key refresh from work order execution. You could use asyncio coroutine to create key refresh as a separate task. That's will also avoid using sleep interval which is used for work order polling.

I will explore asyncio and use it to do key refresh

manojgop · 2020-03-27T07:09:09Z

examples/apps/generic_client/generic_client.py

+            logger.error("\n Unable to get worker \n")
+            sys.exit(-1)
+
+    _start_client(config, options, jrpc_req_id, worker_id, worker_registry)


Any specific reason why prepare worker is done outside start_client ?

Moved worker prepare code to start_client function.

manojgop · 2020-03-27T09:19:58Z

tc/sgx/trusted_worker_manager/enclave/enclave_data.h

    // to do this; for now, the constant will be good enough.
    static const size_t cMaxSealedDataSize = 8192;
-    static const size_t cMaxPublicDataSize = 4096;
+    //static const size_t cMaxPublicDataSize = 4096;


Please remove commented line if it's not needed

manojgop · 2020-03-27T09:20:51Z

tc/sgx/trusted_worker_manager/enclave/enclave_data.h

+    //static const size_t cMaxPublicDataSize = 4096;
+
+    // size of RSA key signature is 2048
+    static const size_t cMaxPublicDataSize = 6144;


How did we arrive at 6144 ?

If you need a larger buffer because of base64, the formula is
(bytes * 4 / 3) + 2 +1
The +2 is because of the 0 to 2 "=" padding bytes. This can be computed exactly if needed.
The +1 is if there is a NUL terminator byte
So I come up with 2048*4/3 +2 + 1 = 2733

I added 2048 bytes(to account encryptionKeySignature) to cMaxPublicDataSize(before 4096 bytes) which sum up to 6144.
the base64 encoded version of the signature was less than 2048, hence i chose to add 2048 to the cMaxPublicDataSize buffer size.
@dan yes we can allot max buffer size equals to 2733 bytes as you suggested. thanks for the suggestion.

manojgop · 2020-03-27T09:22:29Z

tc/sgx/trusted_worker_manager/enclave/signup.edl

            [out] size_t* outSealedEnclaveDataSize,
            [out] sgx_report_t* outEnclaveReport
            );
+        public tcf_err_t ecall_RefreshEnclaveKey(


ecall_RefreshEnclaveKey -> ecall_RefreshWorkerEncryptionKey

manojgop · 2020-03-27T09:34:31Z

tc/sgx/trusted_worker_manager/enclave/work_order_processor.cpp

+            Log(TCF_LOG_ERROR, "error::DecryptMessage - %d - %s", \
+                e.error_code(), e.what());
+            tcf::error::ThrowIf<tcf::error::KeyRefreshError>(
+                true == true, "Failed to decrypt encrypted session key. " \


Use throw tcf::error::KeyRefreshError(msg) instead of tcf::error::ThrowIf(true==true)

manojgop · 2020-03-27T09:36:59Z

tc/sgx/trusted_worker_manager/enclave_untrusted/enclave_bridge/signup.cpp

 }  // tcf::enclave_api::base::CreateSignupData

+// XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
+tcf_err_t tcf::enclave_api::enclave_data::RefreshEnclaveKey(


RefreshEnclaveKey -> RefreshWorkerEncryptionKey

manojgop · 2020-03-27T09:37:38Z

tc/sgx/trusted_worker_manager/enclave_untrusted/enclave_bridge/signup.cpp

+        result = TCF_ERR_UNKNOWN;
+    } catch (...) {
+        tcf::enclave_api::base::SetLastError("Unexpected exception in (RefreshEnclaveKey)");
+        result = TCF_ERR_UNKNOWN;


RefreshEnclaveKey -> RefreshWorkerEncryptionKey

manojgop · 2020-04-07T12:06:04Z

enclave_manager/avalon_enclave_manager/enclave_manager.py

+                     "Setting sleep interval to 10 seconds: %s", str(err))
+        sleep_interval = 10
+    while True:
+        await lock.acquire()


Please add a comment why we need a lock and why we need to synchronize between key refresh and work order processing if its required.

I will add comment to explain the need to synchronization using lock

manojgop · 2020-04-07T12:21:37Z

examples/apps/generic_client/generic_client.py

+            res["error"]["code"] == \
+            WorkOrderStatus.WORKER_ENCRYPT_KEY_REFRESHED:
+        logger.error("Worker Key refreshed. Retrieving latest Worker details")
+        _start_client(config, options, jrpc_req_id, worker_id, worker_registry)


So _start_client() is being called recursively here ? If so, please modify the logs. The logs is only mentioning about retrieving latest worker details. _start_client() also resubmits the work order.

Its not called recursively. Only if work order execution fails due to worker key refresh during the submission of work order, the error scenario is handle by retrieving latest worker details(after key refresh) and submit the work order again.

manojgop · 2020-04-16T09:41:07Z

enclave_manager/avalon_enclave_manager/enclave_manager.py

+                    enclave manager service
+    """
+    try:
+        sleep_interval = int(config["EnclaveManager"]["sleep_interval"])


Need to integrate the change for synchronous processing via ZMQ instead of polling based on the config

Yes, will rebase my PR.

This feature initiates refresh of worker encryption key pair based on number of work orders processed in case of Singleton worker or number of pre-processed work orders in case of KME worker. A new pair of encryption key is generated in the enclave and the updated enclave signup details are stored in the KvStorage in workers table. Worker encryption key signature is re-computed when encryption key gets refreshed. When a worker key gets refreshed during the work order submission, a specific error code is returned to client to indicate worker key refresh. On receiving this error code, client retrieves the updated worker details and does work order submission again. Signed-off-by: manju956 <[email protected]>

lgtm-com · 2020-09-01T10:47:15Z

This pull request introduces 5 alerts when merging 1df89b3 into ed424e7 - view on LGTM.com

new alerts:

4 for Unused import
1 for Syntax error

manju956 · 2020-11-09T09:31:21Z

A new updated PR is raised for worker key refresh policy implementation #671. Hence closing this PR

manju956 requested review from EugeneYYY, Ram-srini, TomBarnes, danintel, dcmiddle, manojgop and pankajgoyal2 as code owners March 26, 2020 08:34

manju956 force-pushed the key_refresh branch 8 times, most recently from 9654e0a to 2c7d403 Compare March 26, 2020 12:29

danintel assigned manju956 Mar 26, 2020

danintel suggested changes Mar 26, 2020

View reviewed changes

danintel reviewed Mar 26, 2020

View reviewed changes

tc/sgx/trusted_worker_manager/enclave/enclave_utils.cpp Outdated Show resolved Hide resolved

danintel approved these changes Mar 26, 2020

View reviewed changes

manju956 force-pushed the key_refresh branch 2 times, most recently from 672f9e3 to 292c2c1 Compare March 27, 2020 06:38

manojgop suggested changes Mar 27, 2020

View reviewed changes

manojgop reviewed Mar 27, 2020

View reviewed changes

manojgop suggested changes Mar 27, 2020

View reviewed changes

manju956 force-pushed the key_refresh branch from 292c2c1 to 7ed902b Compare April 6, 2020 08:11

manju956 closed this Apr 6, 2020

manju956 reopened this Apr 6, 2020

manju956 force-pushed the key_refresh branch 2 times, most recently from f34bf46 to efa7b39 Compare April 6, 2020 10:28

manojgop suggested changes Apr 7, 2020

View reviewed changes

manju956 force-pushed the key_refresh branch 3 times, most recently from e6be4ea to 4e27e62 Compare April 13, 2020 12:46

manojgop reviewed Apr 16, 2020

View reviewed changes

danintel approved these changes Jun 18, 2020

View reviewed changes

manju956 force-pushed the key_refresh branch from 4e27e62 to 1df89b3 Compare September 1, 2020 10:40

manju956 requested review from SriharshaSubbakrishna, karthikamurthy and rranjan3 as code owners September 1, 2020 10:40

manju956 closed this Nov 9, 2020

Implementation of Enclave Key Refresh #358

Implementation of Enclave Key Refresh #358

Uh oh!

Conversation

manju956 commented Mar 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danintel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manojgop Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manju956 Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Sep 1, 2020

Uh oh!

manju956 commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

manojgop Mar 27, 2020 •

edited

Loading

manju956 Mar 27, 2020 •

edited

Loading

manju956 commented Nov 9, 2020 •

edited

Loading