Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Key forwarding: part 1 #222

Merged
merged 6 commits into from
Feb 2, 2023
Merged

Key forwarding: part 1 #222

merged 6 commits into from
Feb 2, 2023

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Jan 28, 2023

This PR implements the key forwarding flow as described in src/qos_core/KEY_FORWARDING.MD. Its worth the time to check that every check in step 4 of the routine is covered by fn validate_manifest (which is in src/qos_core/src/protocol/services/key.rs).

This PR contains the following changes:

  • Implement the server side logic for boot_forward_key and request_key (this just leaves server side logic for inject key). The logic lives in ``src/qos_core/src/protocol/services/key.rs`.
  • Add qos_attest as a dependency of qos_core so we can attestation doc validation. Note, that this will bring in a bit of dependencies to qos_core since qos_attest has some extra deps to parse the attestation doc and verify the cert chain.
  • Pulls out qos_core::protocol::attestor to its own crate, qos_nsm, to avoid cyclical dependencies when we include it qos_attest
  • Refactor verify_attestation_doc_against_user_input to not panic and instead return errors.
  • Small updates in src/qos_core/KEY_FORWARDING.MD - mostly type renaming

TODO

  • Finish tests for request_key
  • Add tests for boot_forward_key

Follow up work:

Create qos_nsm crate

Get qos_nsm to compile in workspace

WIP

Refactor verify_attestation_doc_against_user_input to return errors

Refactor cross check manifest

WIP

Factor out key forwarding to its own file

Setup some mock test inputs

WIP tests

Fix broken tests

Clean up linting
@emostov emostov force-pushed the zeke-key-fwd-jan-26 branch from 232cc95 to 9f01eb7 Compare January 28, 2023 23:32
Remove cargo.lock from qos_nsm

WIP

works test outlined

finish unit test for request_key_inner

Add tests for boot_key_forward

Add tests for validate manifest

Remove todo comment
@@ -0,0 +1,942 @@
//! The services involved in the key forwarding flow.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub is collapsing this diff - wanted to highlight since its the most important file

@emostov emostov force-pushed the zeke-key-fwd-jan-26 branch from a03a5dd to fc5c414 Compare January 31, 2023 19:34
@emostov emostov force-pushed the zeke-key-fwd-jan-26 branch from fc5c414 to 5b78f0e Compare January 31, 2023 19:34
},

/// Request a quorum key as part of the "key forwarding" flow.
RequestKeyRequest {
Copy link
Contributor

@timurnkey timurnkey Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: RequestKeyRequest doesn't provide a whole lot of context. If this is related to the key forward flow maybe there's a better name?

BootKeyForwardRequest
BootKeyForwardResponse
BootKeyRequest
BootKeyResponse

?

Copy link
Contributor Author

@emostov emostov Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the Boot* prefix to refer to the route used to first boot an enclave. RequestKey{Request, Response} is the route to literally request a key.

the flow is

  • BootKeyForward (boot a new enclave in key forwarding mode
  • RequestKey (ask an old enclave for a quorum key)
  • InjectKey (inject a quorum key into the new enclave)

Happy to brainstorm more on naming here

request_key_internal(state, new_manifest_envelope, &attestation_doc)
}

// Primary of `request_key` pulled out to make testing easier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if the below functions are only used for testing, I'd move them into the mod test so it keeps the biz logic succinct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is poorly worded. Just updated to "Primary logic of request_key pulled out so it can be unit tested." In other words I extracted most of the logic to an "inner" function that can be easily tested.

The "outer" function is hard to test because it needs a valid attestation doc from a nitro secure module which we don't have the facilities to produce on the fly. So I pulled out all the logic that is not validating/extracting the attestation doc into the inner function. Unit tests are performed on the inner function.

@emostov emostov requested a review from timurnkey February 1, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants