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

Update Readme #116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update Readme #116

wants to merge 1 commit into from

Conversation

AdithyaKrishnan
Copy link
Contributor

  • Provided in-depth explanation for tool usage
  • This README is structured similarly to the snpguest README to maintain consistency across the projects.

Copy link
Member

@tylerfanelli tylerfanelli left a comment

Choose a reason for hiding this comment

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

We have documentation for this tool in the docs/snphost.1.adoc. Seems like we're duplicating work here. Why is this necessary?

@AdithyaKrishnan
Copy link
Contributor Author

We have documentation for this tool in the docs/snphost.1.adoc. Seems like we're duplicating work here. Why is this necessary?

Hey @tylerfanelli ,
We also had the same adoc file in place for for snpguest but we did end up creating a readme. My opinion is that having a detailed readme on the landing page of a repo is more user-friendly and on top the syntax, we've added details of supported OSes, dependencies, build instructions, and bug reporting. I can jump on to a slack huddle if you have any further queries.

Copy link
Member

@DGonzalezVillal DGonzalezVillal left a comment

Choose a reason for hiding this comment

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

You are still missing a lot of functionality.

Missing:
ok
fetch crl
show (guests, identifier, tcb, vcek-url, version)
config set
config reset
commit

Also, to @tylerfanelli 's point, just adding simple functionality makes this repetitive to snphost.1.adoc. What I liked from the snpguest README is that we showed people different workflows on what they can do with the different commands.

For example, how to fetch, verify and then import certificates for extended attestation. (i know it's not upstream, but it's really the main reason why people would use these commands on host side)

Or using the config commands and commit to change the snp platform config.

README.md Outdated

### 2. export

Exports the SEV-SNP certificate chain from the AMD Platform Security Processor (PSP) to a specified directory. The user must specify the desired encoding format (`der` or `pem`), the certificate file name, and the target directory path. This command is useful for retrieving the certificate chain for attestation purposes.
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
Exports the SEV-SNP certificate chain from the AMD Platform Security Processor (PSP) to a specified directory. The user must specify the desired encoding format (`der` or `pem`), the certificate file name, and the target directory path. This command is useful for retrieving the certificate chain for attestation purposes.
Deserializes a GHCB formatted cert chain file into individual certificates. The user must specify the desired encoding format (`der` or `pem`) for the certificates, the file where the GHCB cert chain is stored in, and the target directory path where to store the deserialized certs. This command is useful for looking at the individual certs that will be used for an extended attestation.

The functionality changed in the latest iteration of Extended Attestation, the certs are no longer stored in the SP. We will probably see this functionality change again in the future.

README.md Outdated
**Example:**

```bash
snphost export pem certificate.pem ./certs
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
snphost export pem certificate.pem ./certs
snphost export pem ghcb-certs.bin ./certs

README.md Outdated

### 3. import

Imports serialized SEV-SNP certificates to a specified certificate file. This certificate file can then be provided to QEMU to perform extended attestation on guests. Currently, only the ASK, ARK, VCEK, and VLEK certificates are supported for serialization.
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
Imports serialized SEV-SNP certificates to a specified certificate file. This certificate file can then be provided to QEMU to perform extended attestation on guests. Currently, only the ASK, ARK, VCEK, and VLEK certificates are supported for serialization.
Converts a certificate chain into a GHCB formatted file for extended attestation. This formatted file can then be provided to QEMU to perform extended attestation on guests. Currently, only the ASK, ASVK, ARK, VCEK, and VLEK certificates are supported for serialization.

README.md Outdated
**Example:**

```bash
snphost import ./certs certificate.pem
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
snphost import ./certs certificate.pem
snphost import ./certs ghcb-certs.bin

README.md Outdated
Comment on lines 114 to 213
Fetches certificates from the AMD PSP and verifies the certificate chain, ensuring its integrity and authenticity. This command is essential for validating the trustworthiness of the SEV-SNP environment.

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
Fetches certificates from the AMD PSP and verifies the certificate chain, ensuring its integrity and authenticity. This command is essential for validating the trustworthiness of the SEV-SNP environment.
Reads the certificates in a directory and verifies the certificate chain, ensuring its integrity and authenticity. This command is essential for validating the trustworthiness of the certificates that can be then passed to complete attestation.

README.md Outdated
**Usage:**

```bash
snphost verify
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
snphost verify
snphost verify ./certs

@tylerfanelli
Copy link
Member

Also, to @tylerfanelli 's point, just adding simple functionality makes this repetitive to snphost.1.adoc. What I liked from the snpguest README is that we showed people different workflows on what they can do with the different commands.

Exactly, this more resembles documentation you would see in a man page (and our adoc) rather than the README. The snpguest readme shows how some usual commands are linked together for some standard uses of the tool.

Update README.md

- Provided in-depth explanation for tool usage
- This README is structured similarly to the snpguest README to maintain consistency across the projects.

Signed-off-by: Adithya Krishnan Kannan <[email protected]>
Copy link
Member

@DGonzalezVillal DGonzalezVillal left a comment

Choose a reason for hiding this comment

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

Thanks for adding the missing commands. Still some things I'd change to make it more clear.

Again I think a workflow section that shows how to use the commands to successfully provision the SNP machine would be helpful.

The workflows I'd like to see are:

Changing the SNP config:
config set -> commit.

Explain what changing the config means, what values can you pass and what happens when you commit the changes.

config set -> config reset

Changing the config and then resetting it to the previously committed values.

Extended Attestation:
fetch certs -> export -> pass to QEMU

Although this is the previous workflow, I think devel kernel still supports it.

Comment on lines +80 to +83
**Example:**

```
```
Copy link
Member

Choose a reason for hiding this comment

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

If the example is the same as the usage, just delete the example then.

Suggested change
**Example:**
```
```

Comment on lines +150 to +191
**Subcommands:**

#### 1. `guests`
Lists all active guests.

**Usage:**
```sh
snphost show guests
```

#### 2. `identifier`
Displays the SNP identifier.

**Usage:**
```sh
snphost show identifier
```

#### 3. `tcb`
Shows the current Trusted Computing Base (TCB) version.

**Usage:**
```sh
snphost show tcb
```

#### 4. `vcek-url`
Displays the URL for fetching the VCEK certificate.

**Usage:**
```sh
snphost show vcek-url
```

#### 5. `version`

Prints the current version of `snphost`.

**Usage:**
```sh
snphost show version
```
Copy link
Member

@DGonzalezVillal DGonzalezVillal Feb 12, 2025

Choose a reason for hiding this comment

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

Idk if we need to list every single show subcommand this way. Maybe bullets of every show subcommand would be more effective. This just seem bulky to me.

```

### 8. `config`
Manages `snphost` configuration.
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
Manages `snphost` configuration.
Subcommands to manage the host machine's configuration.

**Subcommands**

#### 1. `set`
Sets a configuration parameter.
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
Sets a configuration parameter.
Change the config of the SNP platform. The user can
provide the desired versions of the different TCB parameters they would like to modify.
The command will change the reported values by the PSP.

Copy link
Member

Choose a reason for hiding this comment

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

Look at the snphost.1.adoc explanation for more information. I think you could write out the workflow of changing the config (config set -> commit) and you could give a more detailed explanation there. This is a sensitive command to run, so the more information you can provide here the better.


**Example:**
```sh
snphost config set
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
snphost config set
snphost config set 0 0 0 0 0

The suggestion doesn't show actual values you'd use, but I'm trying to say you should provide a value for the config you are going to change.

Again, try to understand what the command is really doing to provide an accurate example of how you would use it. And I think your example should come with an explanation of what will it will do when you run it. You're not providing any new information that is not already listed in the adoc.

```

#### 2.`reset`
Resets a configuration parameter to its default value.
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
Resets a configuration parameter to its default value.
This command resets the SEV-SNP platform. This will clear all
persistent data managed by the platform and reset the platform configuration
to its last committed version.

Comment on lines +232 to +235
**Example:**
```sh
snphost config reset
```
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
**Example:**
```sh
snphost config reset
```

```

### 7. `commit`
Commits pending configuration changes.
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
Commits pending configuration changes.
This command commits the current firmware and SNP platform config versions to the PSP.
This can't be undone and will not allow rollbacks to older versions.

Very Important that you clear up that this will not allow rollbacks (meaning you won't be able to change the config to a version prior to what has been committed to the ASP.

@AdithyaKrishnan
Copy link
Contributor Author

Thanks for adding the missing commands. Still some things I'd change to make it more clear.

Again I think a workflow section that shows how to use the commands to successfully provision the SNP machine would be helpful.

The workflows I'd like to see are:

Changing the SNP config: config set -> commit.

Explain what changing the config means, what values can you pass and what happens when you commit the changes.

config set -> config reset

Changing the config and then resetting it to the previously committed values.

Extended Attestation: fetch certs -> export -> pass to QEMU

Although this is the previous workflow, I think devel kernel still supports it.

Hey @DGonzalezVillal , I'm working on creating the flowchart for this as a picture to upload similar to the extended attestation flowchart picture that you made for snpguest. I'll get it reviewed by you and upload it as soon as I'm done with my current CI tasks.

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