-
Notifications
You must be signed in to change notification settings - Fork 9
Trust domain intro 00002 #3
Trust domain intro 00002 #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a few typos and had a few questions. Otherwise this looks good and informative!
4cdd407
to
568f680
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of suggested changes, hope they help.
Very interesting stuff, one thing i think is missing is a bit of a higher-level abstraction of what trust domains are, maybe as a general introduction. Something along the lines of trust domains: what falls within the realm of things you need to consider in term of trust relationships, and what doesn't.
This should also make it easier to help people grok the power of trust domains as a tool for considering not only what is and out of the domain but also what moves in and out of the domains.
Quick note, as per the RFC process (which is now live), this RFC should use the number of its PR, so it should be RFC 00003 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes reviewed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reading through the RFCs now. However, there are three major process issues that need to be resolved:
- You can't file more than one RFC in a single PR.
- The RFC MUST have the number of the PR that adds it.
- The file name format is incorrect.
Please follow the instructions detailed here:
https://github.com/enarx/rfcs/blob/master/contributing.md#how-to-propose-an-rfc
Having now read them, I think the best way forward is to combine them. The first RFC feels too light and should just be a preface to the second one. After merging them, please use RFC#00003. |
This was my first PR. Won't do it again.
Includes or adds? I think we've established (in this thread: #19) that this doesn't work if we want to have consecutive RFCs, because of how the numbering of PRs is assigned.
See below.
This was not complete at time of submission, which is why it was wrong. Will change the file name format, however. |
Disagree. #2 stands on its own, and I expect to reference it from other RFCs. #3 is one implementation (the default), but not the only one. |
This version is a lot more clear than previously. 🎉 There are still issues with consistent capitalization in the lists in |
typo Co-Authored-By: Lily Sturmann <[email protected]>
capitalisation Co-Authored-By: Lily Sturmann <[email protected]>
capitalisation Co-Authored-By: Lily Sturmann <[email protected]>
capitalisation Co-Authored-By: Lily Sturmann <[email protected]>
capitalisation Co-Authored-By: Lily Sturmann <[email protected]>
capitalisation Co-Authored-By: Lily Sturmann <[email protected]>
capitalisation Co-Authored-By: Lily Sturmann <[email protected]>
capitalisation Co-Authored-By: Lily Sturmann <[email protected]>
capitalisation Co-Authored-By: Lily Sturmann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good aside from a couple of markdown rendering issues as noted below.
I assume the commits will be squashed at some point as well.
- entities running processes on the host (includes other workloads | ||
and compromised processes) | ||
- man-in-the-middle entities | ||
Clearly, the first two are the key actors - the others are included for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shows up as part of the "man-in-the-middle" bullet point in markdown.
- Provisioned Keep | ||
- tenant workload | ||
- Other workloads | ||
Not all of these exist at all points in the process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line also needs a space in order to render properly in markdown.
Not all of these exist at all points in the process. | |
Not all of these exist at all points in the process. |
|
||
## Future Possibilities | ||
|
||
- Threat models should be documented and published as RFCs [Issue 15](https://github.com/enarx/rfcx/issues/15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Threat models should be documented and published as RFCs [Issue 15](https://github.com/enarx/rfcx/issues/15) | |
- Threat models should be documented and published as RFCs [Issue #15 ](https://github.com/enarx/rfcx/issues/15) |
For consistency with the below bullet point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I have no qualms with the content of these RFCs (indeed, it's really well done), it needs some organizational love, including:
- a rebase
- organizing the READMEs according to the criteria outlined in RFC process improvements #22 (admittedly, yet to be merged)
Additionally, this RFC currently contains two READMEs. If the tests allow it, this might be fine -- but it might also be prudent to either split them into separate PRs, or combine them.
I suggest closing this PR as our process has been refined since we started off and his PR doesn't fit. |
Closing per @axelsimon |
Initial drafts for rfc#00002 and rfc#00003 (related topics).