Worker OCall Abstraction, PoC by murerfel · Pull Request #293 · integritee-network/worker

murerfel · 2021-07-06T10:08:26Z

Proof-of-concept for the abstraction of the OCall API in the untrusted worker. This will be a first step for #287 .

The entire OCall bridge component is still a module of the worker for now. In a later stage, this will be extracted to a separate crate. Implementation&behavior can be injected using the Bridge struct, that internally uses global static state.

Merge Review of OCall API Draft in Enclave #283 first
Rebase on master afterwards

murerfel · 2021-07-06T10:15:12Z

+    // initialize o-call bridge
+    OCallBridge::initialize(Arc::new(OCallBridgeComponentFactoryImpl {}));
+


Here we initialize the component (i.e. global state) and inject the concrete implementation

murerfel · 2021-07-06T10:16:29Z

A lot of the changes in the main file are just formatting changes, automatically applied by cargofmt upon saving

murerfel · 2021-07-06T10:18:35Z

+pub struct RemoteAttestationOCallImpl {
+    // TODO as a member here we need the e-call API trait, so we can use it instead of making the e-call directly
+}


This is the actual implementation of the remote attestation o-calls. This was extracted from the c-functions and turned into interfaces that only use Rust types. Translating from and to C-API types should be the responsibility of the FFI functions.

murerfel · 2021-07-06T10:20:45Z

+
+pub struct Bridge {}
+
+impl Bridge {


The Bridge is the static/global interface to inject concrete implementations (or rather the factories for them) - this is done at startup of the worker. On the other side, it is used by the o-call FFI to retrieve the state and forward calls to their respective implementation.

Is there any reason you're not putting such comments directly into the code? I think (atleast for me) it would be really helpful to have the option to look up such comments also later on.

Good input. This is also helpful for fellow sdk users.

Yes, indeed, there probably should be more comments, especially in the future for all the public facing traits and functions.

I've neglected them in part because of laziness, but also because comments tend to go out of sync with the code VERY fast, and then they're more harmful than anything else. And here we're dealing with the beginning of some substantial refactoring, where everything is very volatile.. so far my excuse. Once we reach a more stable state, we should definitely do more documentation inside the code.

My latest commit added some of the comments you (rightfully) requested

murerfel · 2021-07-06T10:28:09Z

+    let revocation_list: Vec<u8> =
+        unsafe { slice::from_raw_parts(p_sigrl, sigrl_len as usize).to_vec() };
+
+    let report = unsafe { *p_report };
+    let spid = unsafe { *p_spid };
+    let quote_nonce = unsafe { *p_nonce };


Translate parameters from C-API types to Rust types

murerfel · 2021-07-06T10:28:29Z

+    let quote = get_quote_result.2;
+
+    if quote.len() as u32 > maxlen {
+        return sgx_status_t::SGX_ERROR_FAAS_BUFFER_TOO_SHORT;
+    }
+
+    let quote_slice = unsafe { slice::from_raw_parts_mut(p_quote, quote.len()) };
+    quote_slice.clone_from_slice(quote.as_slice());
+
+    unsafe {
+        *p_qe_report = get_quote_result.1;
+        *p_quote_len = quote.len() as u32;
+    };
+
+    get_quote_result.0


Translate results back to the C-API

murerfel · 2021-07-06T10:55:13Z

+    fn init_quote(&self) -> (sgx_status_t, sgx_target_info_t, sgx_epid_group_id_t) {
+        // TODO this translation to unsafe C-API should be moved to the EnclaveApi / ECall API
+        let mut ti: sgx_target_info_t = sgx_target_info_t::default();
+        let mut eg: sgx_epid_group_id_t = sgx_epid_group_id_t::default();
+
+        unsafe {
+            let ret_status = sgx_init_quote(
+                &mut ti as *mut sgx_target_info_t,
+                &mut eg as *mut sgx_epid_group_id_t,
+            );
+            (ret_status, ti, eg)
+        }
+    }


3 of the 4 methods for remote attestation basically just call back right into the enclave, using an e-call. Once we have the e-call API for this, these implementations should be moved to that API, making the implementation here very simple.

murerfel · 2021-07-06T10:57:21Z

+impl OCallBridgeComponentFactory for OCallBridgeComponentFactoryImpl {
+    fn get_ra_api(&self) -> Arc<dyn RemoteAttestationOCall> {
+        Arc::new(RemoteAttestationOCallImpl {})
+    }
+}


Component factory, i.e. root for all ocall components. Once we're filling this up with more components for all the other ocalls, its usefulness will become apparent.

murerfel · 2021-07-06T10:58:41Z

+    #[test]
+    fn init_quote_sets_results() {
+        let mut ra_ocall_api_mock = MockRemoteAttestationOCall::new();
+        ra_ocall_api_mock
+            .expect_init_quote()
+            .times(1)
+            .returning(|| (sgx_status_t::SGX_SUCCESS, dummy_target_info(), [8u8; 4]));
+
+        let mut ti: sgx_target_info_t = sgx_target_info_t::default();
+        let mut eg: sgx_epid_group_id_t = sgx_epid_group_id_t::default();
+
+        let ret_status = sgx_init_quote(
+            &mut ti as *mut sgx_target_info_t,
+            &mut eg as *mut sgx_epid_group_id_t,
+            Arc::new(ra_ocall_api_mock),
+        );
+
+        assert_eq!(ret_status, sgx_status_t::SGX_SUCCESS);
+        assert_eq!(eg, [8u8; 4]);
+    }


Another simple test using the mocking framework to mock the RemoteAttestationOCall trait.

clangenb

Looks in general very good. In general, I don't see any design remarks that could stand in the way of our refactoring goal.

My remarks are only details and rather rust related.

clangenb

Cool. I like the new errors! My two comments are only a little nitpicking. :) Will approve anyhow.

haerdib

LGTM, though I haven't really looked at the overall structure. Leaving this task to @clangenb. :)
I'd really appreciate some more documentation though - especially when it gets as abstract as the bridge struct some reasoning comments (what is it used for, what's the original reasoning behind it...) would be really helpful.

haerdib · 2021-07-07T08:26:51Z

+    }
+}
+
+fn lookup_ipv4(host: &str, port: u16) -> Result<SocketAddr, String> {


Just as an information: This function, respective generally the transformation from a string and / or port to a SocketAdress is used for most ws applications, there's also one within the enclave

Thanks, we should make sure then to share this code in an external crate, shared by both 👍

brenzi

it is unfortunate that this PR is mixed with cargo fmt changes. This makes reviewing painful. Actually I thought that we enforce fmt in CI anyway, but maybe that was dropped when moving to GH actions? We should enfore fmt in CI if we don't do so yet (excluding sgx_runtime, because parity doesn't use fmt)

I'm not yet sure that "bridge" is a good name. In blockchain, a bridge is usually something that allows cross-blockchain interaction. But, actually, we're bridging layer one with layer two somehow, so I'd leave it

murerfel · 2021-07-12T08:58:41Z

I wouldn't mind renaming 'bridge' to something else, especially when that name is already reserved for another concept in the domain. It will never again be as simple as now to do the renaming.. What are the alternatives?
OCall-

Gateway
Link
Facade
... any suggestions are welcome 😄

brenzi · 2021-07-15T16:35:43Z

@mullefel You mentioned that you need to rebase before merging? there seem to be no conflicts, but let me know. I'd be ready to merge as soon as the Jenkins tests have passed

Because we envision to implement an "integritee-bridge" in the future, I think we really should rename the "OcallBridge".
Gateway and Link are ambiguous again. "Facade" could work. What do you think about "Tunnel"?

murerfel · 2021-07-19T05:34:54Z

I find 'Tunnel' an interesting suggestion - it's not something that I've found commonly used yet, but I think it could be a good fit in our case 👍 I will open an issue to re-name the bridges to 'tunnels'. I think it's less work to merge the PRs now and do the re-naming in a separate PR (instead of having to rebase a lot of conflicts because of the renaming)

Edit: created issue #316 for this

…d worker

murerfel requested a review from clangenb July 6, 2021 10:08

murerfel commented Jul 6, 2021

View reviewed changes

Comment thread worker/src/ocall_bridge/bridge_api.rs

murerfel commented Jul 6, 2021

View reviewed changes

Comment thread worker/src/ocall_bridge/bridge_api.rs

murerfel commented Jul 6, 2021

View reviewed changes

Comment thread worker/src/ocall_bridge/ffi/get_ias_socket.rs Outdated

murerfel commented Jul 6, 2021

View reviewed changes

murerfel self-assigned this Jul 6, 2021

clangenb reviewed Jul 6, 2021

View reviewed changes

murerfel marked this pull request as ready for review July 7, 2021 06:18

murerfel commented Jul 7, 2021

View reviewed changes

Comment thread worker/Cargo.toml

murerfel requested a review from clangenb July 7, 2021 07:06

murerfel force-pushed the feature/fm-worker-ocall-abstraction-poc branch from e47ae3d to bab40fe Compare July 7, 2021 07:10

clangenb approved these changes Jul 7, 2021

View reviewed changes

Comment thread worker/src/ocall_bridge/attestation_ocall_impl.rs

Comment thread worker/src/ocall_bridge/attestation_ocall_impl.rs

murerfel requested a review from haerdib July 7, 2021 07:48

haerdib approved these changes Jul 7, 2021

View reviewed changes

murerfel requested a review from brenzi July 7, 2021 13:42

murerfel mentioned this pull request Jul 7, 2021

worker ocall abstraction onchain #299

Merged

brenzi approved these changes Jul 8, 2021

View reviewed changes

murerfel mentioned this pull request Jul 19, 2021

Re-name o-call and e-call **bridge** to **tunnel** #316

Open

murerfel force-pushed the feature/fm-worker-ocall-abstraction-poc branch from 9c163f5 to 2ac8961 Compare July 19, 2021 07:51

Proof-of-concept for the abstraction of the OCall API in the untruste…

6065d03

…d worker

murerfel force-pushed the feature/fm-worker-ocall-abstraction-poc branch from 2ac8961 to 6065d03 Compare July 19, 2021 08:28

murerfel merged commit 2918dde into master Jul 19, 2021

murerfel deleted the feature/fm-worker-ocall-abstraction-poc branch July 19, 2021 09:11

haerdib mentioned this pull request Sep 30, 2021

Create Documentation KnowHowDump #431

Closed

		// initialize o-call bridge
		OCallBridge::initialize(Arc::new(OCallBridgeComponentFactoryImpl {}));

Conversation

murerfel commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murerfel commented Jul 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haerdib Jul 7, 2021 • 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!

Uh oh!

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!

clangenb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clangenb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

haerdib left a comment • 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!

haerdib Jul 7, 2021 • 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!

brenzi left a comment

Choose a reason for hiding this comment

Uh oh!

murerfel commented Jul 12, 2021

Uh oh!

brenzi commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murerfel commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

murerfel commented Jul 6, 2021 •

edited

Loading

haerdib Jul 7, 2021 •

edited

Loading

haerdib left a comment •

edited

Loading

haerdib Jul 7, 2021 •

edited

Loading

brenzi commented Jul 15, 2021 •

edited

Loading

murerfel commented Jul 19, 2021 •

edited

Loading