Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Documentation #287

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Conversation

fitzthum
Copy link
Member

@fitzthum fitzthum commented Jan 12, 2024

General improvements to KBS documentation

WIP for now.

@bpradipt @tylerfanelli lmk if there is anything you'd like me to add

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Great, this will make the repo more new-comer friendly

README.md Outdated Show resolved Hide resolved
README.md Outdated

[Reference Value Provider Service](attestation-service/rvps)
The RVPS manages reference values used to verify TEE evidence.
This is [mentioned](https://www.ietf.org/archive/id/draft-ietf-rats-architecture-22.html#name-endorser-reference-value-pr)
Copy link
Member

Choose a reason for hiding this comment

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

I am still not sure whether it is accurate here. The document just mentioned the Verifier would need to be trusted Reference Value Provider and Endorsor in some scenarios.

However, RVPS is neither Reference Value Provider nor Endorsor. it is actually a consumer of Reference Value Provider's reference value and Endorsor's Endorsement. It serves AS with formated reference values.

The purpose of RVPS is to shield the formats of different reference value release carriers and then provide an indexable query service. This is more like adding a layer of abstraction in an engineering sense.

I don't have any better suggestions at the moment because I didn't find a suitable description in this RATS document, so I am ok with letting it as the PR now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll change this to say that it is related to that section of the doc.

README.md Outdated Show resolved Hide resolved
README.md Outdated

![](kbs/docs/pictures/cluster.svg)

</div>
Copy link
Member

@bpradipt bpradipt Jan 16, 2024

Choose a reason for hiding this comment

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

I'm thinking if skopeo and encrypted container image is really needed. We can just mention loading of keys in the right path to keep it simple

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I replaced the diagram with a fancy mermaid one. I am using a flowchart, which has some limitations in terms of layout. Mermaid also supports more complex C4 diagrams, but only as an experimental feature. What do you think about the flow chart? cc @Xynnn007

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Member

Choose a reason for hiding this comment

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

Looks cool!

@fitzthum fitzthum force-pushed the improve-docs branch 8 times, most recently from eccb73e to aafa0a8 Compare January 22, 2024 23:18
@@ -134,16 +159,19 @@ Supported Verifier Drivers:

The AS supports modular policy engine, which can be specified through the AS configuration. The currently supported policy engines are:
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a mention that the policy is universal and no support for multiple policies?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is support for multiple policies. I am going to change this sentence around though. I don't think the modularity of the policy engine (i.e. the possibility to implement something other than OPA) is very significant.


# Usage
## Library

The AS can be built and imported as a Rust crate into any project providing attestation services.
Copy link
Member

Choose a reason for hiding this comment

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

Should there be any mention of CoCo AS and Amber/IntelTrustAuthority AS?

Copy link
Member Author

Choose a reason for hiding this comment

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

ITA is supported in the KBS as an alternative to the AS. The AS itself doesn't have any support for it. We do have some support for veraison in the AS, but it's just part of the CCA verifier.

@fitzthum fitzthum force-pushed the improve-docs branch 4 times, most recently from 44e4681 to 6a56d61 Compare January 23, 2024 20:41
@fitzthum fitzthum marked this pull request as ready for review January 23, 2024 20:42
@fitzthum
Copy link
Member Author

Ok, I think this is enough for now. PTAL @Xynnn007 and @bpradipt

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

lgtm. only a nit

README.md Outdated
Attestation Service (AS for short) is a general function set that can verify TEE evidence.
- [Key Broker Service](kbs)
The KBS is a server that facilitates remote attestation and secret delivery.
It's role is similar to that of the [Relying Party](https://www.ietf.org/archive/id/draft-ietf-rats-architecture-22.html#name-relying-party)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It's role is similar to that of the [Relying Party](https://www.ietf.org/archive/id/draft-ietf-rats-architecture-22.html#name-relying-party)
Its role is similar to that of the [Relying Party](https://www.ietf.org/archive/id/draft-ietf-rats-architecture-22.html#name-relying-party)

Copy link
Member

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm

Make the README a little more descriptive and point
to the cluster guide to help new users.

Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
Clarify a few things in the AS README

Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
@fitzthum fitzthum merged commit 3003ced into confidential-containers:main Jan 24, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants