[common] Refactor Protected Files, part 4

[dimakuv]

Description of the changes

This commit refactors PF code without changing functionality (part 4 in a series of commits). In particular, this commit re-orders variables, struct definitions, struct fields, typedefs, and macros for readability.

Extracted from #1841 at the request of @mkow.

Note that this PR is based on #1866.

How to test this PR?

N/A.

---

This change is 

[mkow]

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

---

common/src/protected_files/protected_files.h line 26 at r2 (raw file):

#define PF_MAC_SIZE 16

/*! Size of the salt used in KDF (Key Derivation Function) */

But this isn't salt, it's key ID? If something, this could be a nonce, but some parts of PF code call it "key ID" instead of nonce?

---

common/src/protected_files/protected_files_format.h line 41 at r2 (raw file):

// these are all defined as relative to node size, so we can decrease node size in tests
// and have deeper tree

This comment sounded useful IMO.

---

common/src/protected_files/protected_files_format.h line 110 at r2 (raw file):

    encrypted_node_t encrypted; // encrypted data from storage (bounce buffer)
    union {                     // decrypted data in private memory (plain buffer)

the structure itself has no control where it's located

Code quote:

in private memory (plain buffer)

---

common/src/protected_files/protected_files_internal.h line 25 at r2 (raw file):

    pf_key_t user_kdk_key; // KDK installed by user of PF (e.g. from Gramine manifest)

    metadata_node_t file_metadata; // plaintext and encrypted metadata from storage (bound buffer)

What does "bound buffer" mean?

---

common/src/protected_files/protected_files_internal.h line 28 at r2 (raw file):

root MHT node is always needed (for files bigger than 3KB)

So, not always? This comment is confusing.

[kailun-qin]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @mkow)

---

common/src/protected_files/protected_files.h line 26 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But this isn't salt, it's key ID? If something, this could be a nonce, but some parts of PF code call it "key ID" instead of nonce?

+1, as the key derivation follows NIST SP 800-108 (as stated in the comment of kdf_input_t), the unique randomized key ID aligns more closely with the nonce in theContext field

---

common/src/protected_files/protected_files_format.h line 34 at r2 (raw file):

static_assert(MD_USER_DATA_SIZE == 3072, "bad struct size");

#define MAX_PAGES_IN_CACHE 48

what about just MAX_NODES_IN_CACHE?

Code quote:

#define MAX_PAGES_IN_CACHE 48

---

common/src/protected_files/protected_files_format.h line 116 at r2 (raw file):

} file_node_t;

// input materials for the KDF construction of NIST-SP800-108

since we mention NIST standard here, can we align all comments of fields w/ its terminologies? e.g., index -- conter

Code quote:

NIST-SP800-108

---

common/src/protected_files/protected_files_format.h line 120 at r2 (raw file):

    uint32_t index;             // always "1"
    char label[MAX_LABEL_SIZE]; // must be NULL terminated
    pf_keyid_t nonce;           // salt for key derivation from KDK, stored in metadata node

similar above

Code quote:

salt

---

common/src/protected_files/protected_files_internal.h line 30 at r2 (raw file):

    file_node_t root_mht; // root MHT node is always needed (for files bigger than 3KB)

    lruc_context_t* cache; // up to MAX_PAGES_IN_CACHE nodes are cached for each file

ditto

Code quote:

MAX_PAGES_IN_CACHE

[mkow]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @kailun-qin)

---

common/src/protected_files/protected_files_format.h line 34 at r2 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

what about just MAX_NODES_IN_CACHE?

+1