Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Trust domain intro 00002 #3

Closed

Conversation

MikeCamel
Copy link
Contributor

Initial drafts for rfc#00002 and rfc#00003 (related topics).

@MikeCamel MikeCamel added the documentation Improvements or additions to documentation label Feb 12, 2020
Copy link
Contributor

@lkatalin lkatalin left a 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!

rfc#00002 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
@MikeCamel MikeCamel force-pushed the Trust-domain-intro-00002 branch from 4cdd407 to 568f680 Compare February 13, 2020 21:50
Copy link
Collaborator

@axelsimon axelsimon left a 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.

rfc#00002 Outdated Show resolved Hide resolved
rfc#00002 Outdated Show resolved Hide resolved
rfc#00002 Outdated Show resolved Hide resolved
rfc#00002 Outdated Show resolved Hide resolved
rfc#00002 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
rfc#00003 Outdated Show resolved Hide resolved
@axelsimon
Copy link
Collaborator

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 :)

rfc#00002 Outdated Show resolved Hide resolved
Copy link
Contributor Author

@MikeCamel MikeCamel left a comment

Choose a reason for hiding this comment

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

All changes reviewed.

Copy link
Contributor

@npmccallum npmccallum left a 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:

  1. You can't file more than one RFC in a single PR.
  2. The RFC MUST have the number of the PR that adds it.
  3. 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

@npmccallum
Copy link
Contributor

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.

@MikeCamel
Copy link
Contributor Author

I'm reading through the RFCs now. However, there are three major process issues that need to be resolved:

1. You can't file more than one RFC in a single PR.

This was my first PR. Won't do it again.

2. The RFC MUST have the number of the PR that adds it.

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.

3. The file name format is incorrect.

See below.

Please follow the instructions detailed here:

https://github.com/enarx/rfcs/blob/master/contributing.md#how-to-propose-an-rfc

This was not complete at time of submission, which is why it was wrong. Will change the file name format, however.

@MikeCamel
Copy link
Contributor Author

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.

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.

@lkatalin
Copy link
Contributor

This version is a lot more clear than previously. 🎉 There are still issues with consistent capitalization in the lists in Enarx use case states; not sure how much we care.

MikeCamel and others added 16 commits March 24, 2020 14:43
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]>
Copy link
Contributor

@lkatalin lkatalin left a 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
Copy link
Contributor

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.
Copy link
Contributor

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.

Suggested change
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- 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.

Copy link
Contributor

@mbestavros mbestavros left a 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:

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.

@axelsimon
Copy link
Collaborator

I suggest closing this PR as our process has been refined since we started off and his PR doesn't fit.
PR #47 addresses the Trust Domain Introduction part of this PR.
A new PR will be opened soon for the Trust domains and Enarx part of this PR.

@mbestavros
Copy link
Contributor

Closing per @axelsimon

@mbestavros mbestavros closed this Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants