-
Notifications
You must be signed in to change notification settings - Fork 32
feat(fetch): add fetch crl subcommand
#124
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
Conversation
142b6a3 to
0c4200b
Compare
2f44b99 to
bd771bf
Compare
|
Thank you for reviewing. I have addressed all of the requested changes:
|
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.
Code looks good to me, I just want to know why there are changes in the lock file.
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.
Why are there changes to the lock file?
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.
The changes in the lock file came from an earlier commit when pem-rfc7468 was used. I have cleaned this up and removed those changes.
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.
Thank you!
Adding the feature to fetch Certificate Revocation List (CRL) from AMD KDS. Signed-off-by: Takuma IMAMURA <[email protected]>
bd771bf to
c983aa2
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.
LGTM, one comment.
| snpguest fetch crl der ./certs-kds milan -e vlek | ||
| ``` | ||
| ```bash | ||
| snpguest fetch crl pem ./certs-kds -r attestation-report.bin -e vcek |
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 will only work correctly on Turin+ correct? The attestation report in the previous architectures do not contain the processor model.
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.
It should work on pre-Turin CPUs as well. For report versions < 3, the logic is Turin-only as you said, but for report versions ≥ 3 the CPU family ID is used to determine the processor model.
Lines 114 to 152 in ce3c022
| pub fn get_processor_model(att_report: AttestationReport) -> Result<ProcType> { | |
| if att_report.version < 3 { | |
| if [0u8; 64] == *att_report.chip_id { | |
| return Err(anyhow::anyhow!( | |
| "Attestation report version is lower than 3 and Chip ID is all 0s. Make sure MASK_CHIP_ID is set to 0 or update firmware." | |
| )); | |
| } else { | |
| let chip_id = *att_report.chip_id; | |
| if chip_id[8..64] == [0; 56] { | |
| return Ok(ProcType::Turin); | |
| } else { | |
| return Err(anyhow::anyhow!( | |
| "Attestation report could be either Milan or Genoa. Update firmware to get a new version of the report." | |
| )); | |
| } | |
| } | |
| } | |
| let cpu_fam = att_report | |
| .cpuid_fam_id | |
| .ok_or_else(|| anyhow::anyhow!("Attestation report version 3+ is missing CPU family ID"))?; | |
| let cpu_mod = att_report | |
| .cpuid_mod_id | |
| .ok_or_else(|| anyhow::anyhow!("Attestation report version 3+ is missing CPU model ID"))?; | |
| match cpu_fam { | |
| 0x19 => match cpu_mod { | |
| 0x0..=0xF => Ok(ProcType::Milan), | |
| 0x10..=0x1F | 0xA0..0xAF => Ok(ProcType::Genoa), | |
| _ => Err(anyhow::anyhow!("Processor model not supported")), | |
| }, | |
| 0x1A => match cpu_mod { | |
| 0x0..=0x11 => Ok(ProcType::Turin), | |
| _ => Err(anyhow::anyhow!("Processor model not supported")), | |
| }, | |
| _ => Err(anyhow::anyhow!("Processor family not supported")), | |
| } | |
| } |
I have verified that it works correctly on a Milan CPU with report version 5. Since the report version is determined by the SEV firmware, upgrading the firmware raises the report version and enables the
--report argument to work correctly. This is the same logic already used in the existing fetch ca and fetch vcek subcommands.
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.
So, we basically mandate that one must update their firmware to use this command properly, correct?
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.
Yes, that is correct when using the --report flag to identify the processor model (rather than specifying the processor model explicitly). The same applies to the snpguest fetch ca command.
This PR introduces a new subcommand:
The command fetches Certificate Revocation Lists (CRLs) directly from the AMD Key Distribution Service (KDS). With this change,
snpguestcan now retrieve all attestation collateral (AMD CA certs, VEKs, and CRLs).Usage
See the updated
README.mdfor more details.Test Environment
n2d-highmem-4ubuntu-accelerator-2404-amd64-with-nvidia-570-v202508196.14.0-1016-gcpTest code