[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

[dimakuv]

Reviewable status: 0 of 5 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), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)

---

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

Previously, kailun-qin (Kailun Qin) wrote…

+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

Done, using "nonce" now.

"Key ID" is a wrong/unused word in the Protected Files codebase. At least I don't know what it's supposed to mean.

@kailun-qin is right, my term "salt" is also incorrect. The NIST SP 800-108 standard uses the term "nonce", so let's call everything the nonce.

@mkow Please notice that a follow up PR (not yet extracted from #1841, because I'm waiting for this PR to be finalized and merged, otherwise there will be too complex rebases) will fix the rest of the comments in the Protected Files codebase.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

+1

Done.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

This comment sounded useful IMO.

But we never changed the sizes, and we don't plan to change the sizes. I don't see a point in this comment. I think our 4KB node size is now fixed in stone (especially after we modified our Slab allocator to be compatible with 4KB-nodes).

---

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

Previously, mkow (Michał Kowalczyk) wrote…

the structure itself has no control where it's located

Done, rephrased

---

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

Previously, kailun-qin (Kailun Qin) wrote…

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

Done.

---

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

Previously, kailun-qin (Kailun Qin) wrote…

similar above

Done.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

What does "bound buffer" mean?

Done. Typo.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

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

So, not always? This comment is confusing.

Done.

---

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

Previously, kailun-qin (Kailun Qin) wrote…

ditto

Done.

[mkow]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Typo.

Ah, lol.

[kailun-qin]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[dimakuv]

Jenkins, test this please

[dimakuv]

Jenkins, test this please

[mkow]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[kailun-qin]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[mkow]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[kailun-qin]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved