(#142) Document Certifier APIs in .h files. Refactor code. Add unit-tests. #149
Conversation
| @@ -29,7 +29,8 @@ typedef unsigned char byte; | |||
|
|
|||
| using std::string; | |||
|
|
|||
| // Policy store | |||
| // ------------------------------------------------------------------- | |||
| // Policy store: Manage storage and administration of policies. | |||
There was a problem hiding this comment.
@yelvmw -- We should walk thru each of this comment block and cleanup / improve them. I've tried to put down what seemed to be best suited.
| int max_num_ts_; | ||
| int num_ts_; | ||
| trusted_service_message** ts_; |
There was a problem hiding this comment.
I relocated this to the L50 above, as it made better sense to list the data structure first for which the max/num-related fields appear afterward.
| // that can be tracked. | ||
| policy_store(const string enc_alg, int max_trusted_services, | ||
| int max_trusted_signed_claims, int max_storage_infos, | ||
| int max_claims, int max_keys, int max_blobs); |
There was a problem hiding this comment.
A consistent pattern of diffs in this change set is to align the parameters to a fn()-definition and a fn()-call as seen here.
| @@ -109,40 +146,155 @@ namespace certifier { | |||
| void delete_blob_by_index(int index); | |||
| int get_num_blobs(); | |||
|
|
|||
| // Serialize contents of the policy store. | |||
There was a problem hiding this comment.
How can we explain this better? I think this comment by itself is not very helpful ... .???
| // ------------------------------------------------------------------------ | ||
| // Encrypt an enclave's states for persistent storage outside of an enclave. | ||
| // Encryption is performed using a private seal key, derived from the TEE | ||
| // system the enclave is running on. |
There was a problem hiding this comment.
Description needs review.
| // ------------------------------------------------------------------------ | ||
| // Decrypt an enclave's sealed data using the same key used for sealing. | ||
| // This allows an enclave's state to be restored when the same enclave is | ||
| // subsequently brought back up. |
There was a problem hiding this comment.
Let's review description. ...
| // ------------------------------------------------------------------------ | ||
| // Reprotect an encrypted data with an internally-generated new key that | ||
| // is used for re-protecting the data. (RESOLVE: How does this business of | ||
| // cipher_key_byte_size() in implementation work out.) |
There was a problem hiding this comment.
This interface's description needs a lot of help. Let's walk thru RESOLVEs I've left behind ... and straighten this out.
| @@ -239,6 +397,10 @@ namespace certifier { | |||
| bool run_peer_certificationservice(const string& host_name, int port); | |||
| }; | |||
|
|
|||
| // ------------------------------------------------------------------ | |||
| // Manage attributes of the secured authenticated channel between | |||
| // client and server process ... RESOLVE ??? Clarify ... | |||
There was a problem hiding this comment.
Needs some help here ...
| @@ -116,7 +120,7 @@ int certifier::framework::policy_store::get_num_trusted_services() { | |||
| } | |||
|
|
|||
| const trusted_service_message* certifier::framework::policy_store::get_trusted_service_info_by_index(int n) { | |||
| if (n >= num_ts_) | |||
| if ((n < 0) || (n >= num_ts_)) | |||
There was a problem hiding this comment.
You will see a collection of such changes to _by_index() interfaces, to catch illegal index IDs.
These were an initial set of fixes I ended up with ... and then held back. (There are other bugs in this file, but those may need to be taken up separately.)
| if (memcmp(unencrypted_data, (byte*)secret_data, strlen(secret_data)) != 0) | ||
| return false; | ||
|
|
||
| key_message new_key; | ||
| key_message new_key; // ? RESOLVE: This is uninit'ed. Don't we want to supply a new key here? |
There was a problem hiding this comment.
Let's discuss this one ... I think the interface is wonky ... and not sure why 'key' is even needed.
…ests This commit started off as an exercise to document external APIs of the Certifier Framework library. It ended up as: - Add prologs to core extern APIs. Doc parameters - Refactor code formatting & indentation of extern API interfaces and definitions to conform closer to coding standard - Add new unit-test in store_tests.cc, test_policy_store_signed_claims(), to exercise policy-store interfaces handling signed_claims.
gapisback
force-pushed
the
agurajada/142-core-APIs-coding-stds
branch
from
7027db3 to
3381401
Compare
| @@ -50,23 +51,29 @@ const key_message* GetPublicPolicyKey(); | |||
|
|
|||
| bool PublicKeyFromCert(const string& cert, key_message* k); | |||
|
|
|||
| bool GetParentEvidence(const string& enclave_type, | |||
| const string& parent_enclave_type, | |||
| string* out); | |||
There was a problem hiding this comment.
Refactored long single-line declarations that were going beyond 80-chars or were line-wrapped with one/two tab-stops to be in this multi-line prototype declarations format.
This commit started off as an exercise to document external APIs of the Certifier Framework library. It ended up as:
store_tests.cc,test_policy_store_signed_claims(), to exercise policy-store interfaces handling signed_claims.NOTE: To the reviewer: About re-indentation of function definitions and declarations, I've tried to list parameters indented on the next-line (when needed) below the 1st param.
When line-length was getting way beyond 80-chars, I took the style of indenting params on the next line, at some # of tab stops.
(All this is a bit ad-hoc, but I think still improves overall readability of code.)