-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add Azure vTPM certificate verification during TDX attestation #47
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
base: main
Are you sure you want to change the base?
Conversation
| // Fetch the CA certificate if the AK cert has IssuingCertificateURL extension | ||
| var caCertDER []byte | ||
| if len(cert.IssuingCertificateURL) > 0 { | ||
| i.log.Info(fmt.Sprintf("Downloading CA certificate from: %s", cert.IssuingCertificateURL[0])) | ||
| caCert, err := downloadCACertificate(ctx, cert.IssuingCertificateURL) | ||
| if err != nil { | ||
| i.log.Warn(fmt.Sprintf("Failed to download CA certificate: %v", err)) | ||
| // Don't fail here - validator can still verify directly against root | ||
| } else { | ||
| // Use the parsed certificate's Raw field to ensure clean DER encoding | ||
| caCertDER = caCert.Raw | ||
| i.log.Info(fmt.Sprintf("Successfully downloaded CA certificate: %s", caCert.Subject.String())) | ||
| } | ||
| } else { | ||
| i.log.Info("No IssuingCertificateURL in AK certificate - will verify directly against root CA") | ||
| } |
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.
we can also drop the CA certificate issuer from the instance Info and let the validator always to use the hardcoded ones. However, this is just an optimization to add any intermediate CAs to the pool that the validator uses. It shouldnt impact security though because those would only be added as intermediate certificates and not roots. Roots will always be the Azure's hardcoded ones by the validator
Note: I noticed that the AK certificate in the TPM does not include any IssuingCertificateURL extension and therefore, the validator was failing because it couldn't find the intermediate Azure CAs that signed this AK certificate.
That is why I fetched those certificates from their docs and hardcoded them on the validator side.
Similar approach is done for the TrustedLaunch VMs but they seem to use different AME certificate than TDX CVMs
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 think hardcoding them is a good move.
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.
Since we will be relying on these certs, I'd suggest to consider reading them from a file if one is provided, otherwise using the defaults. I remember this is also how most of the dcap libraries do it
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.
That would be a nice feature request to add an optional flag to read the certs from files. If not provided, just use the hardcoded defaults.
Do you think we should do this in this PR or as a follow-up one to avoid large code pushes in one PR?
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.
Up to you, I think it's fine to leave it out of this one
| var cleanCertDER []byte | ||
| if len(certDERRaw) > 4 && certDERRaw[0] == 0x30 { | ||
| // Parse the DER length to extract exactly the certificate bytes | ||
| var certLen int | ||
| if certDERRaw[1] < 0x80 { | ||
| // Short form: length is in the second byte | ||
| certLen = int(certDERRaw[1]) + 2 | ||
| } else if certDERRaw[1] == 0x82 { | ||
| // Long form with 2 length bytes | ||
| certLen = (int(certDERRaw[2]) << 8) | int(certDERRaw[3]) | ||
| certLen += 4 // Add header bytes | ||
| } else if certDERRaw[1] == 0x81 { | ||
| // Long form with 1 length byte | ||
| certLen = int(certDERRaw[2]) + 3 | ||
| } else { | ||
| return nil, fmt.Errorf("unsupported DER length encoding: 0x%02x", certDERRaw[1]) | ||
| } | ||
|
|
||
| if certLen > 0 && certLen <= len(certDERRaw) { | ||
| cleanCertDER = certDERRaw[:certLen] | ||
| i.log.Info(fmt.Sprintf("Extracted %d bytes certificate from %d bytes TPM data", certLen, len(certDERRaw))) | ||
| } else { | ||
| return nil, fmt.Errorf("invalid certificate length: %d (total data: %d)", certLen, len(certDERRaw)) | ||
| } | ||
| } else { | ||
| return nil, fmt.Errorf("invalid certificate format: does not start with DER SEQUENCE tag") | ||
| } |
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 had to do this manually because just parsing the certificate directly was leading to errors on server side saying trailing data issues.
As written in Azure's docs, the TPM register stores 4096 bytes of data in 0x01C101D0 but the certificate size is much less < 1500bytes. Hence, the rest trailing data are either zeros or garbage causing the function x509.ParseCertificate(cleanCertDER) to fail if passed such malformed data input.
So I needed to fetch the size manually and trunk it to avoid these errors.
|
|
||
| i.log.Info(fmt.Sprintf("Read %d bytes from TPM AK cert index", len(certDERRaw))) | ||
|
|
||
| // The TPM NV index contains trailing data. We need to extract just the certificate. |
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'd suggest to factor this out into a separate function, this one seems a bit unwieldy. Does this encoding have a name, or is it completely proprietary?
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.
Yeah, I will extract this in a separate function.
In regards to the content of the vTPM and how it is encoded, it is from the Azure side that they reserve 4096 byte memory to store the certificate data in it. However, the certificate seems to be ~1010 bytes and the rest is being filled with garbage.
In regards certificate encoding, this is how DER format encoding spec that it starts with 0x30 that defines its sequence tag followed by the size form and value.
|
|
||
| // Helper function to download CA certificate from URLs | ||
| func downloadCACertificate(ctx context.Context, urls []string) (*x509.Certificate, error) { | ||
| client := &http.Client{Timeout: 10 * time.Second} |
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.
That's a lot
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 piece is inspired from the Trustedlaunch code here. It is 1-1 mapping to how the intermediate issuing certificate are downloaded from their URLs
6c6ab11 to
0fc04e3
Compare
| akCert, err := i.readAKCertificateFromTPM(tpm) | ||
| if err != nil { | ||
| i.log.Warn(fmt.Sprintf("Failed to read AK certificate: %v", err)) | ||
| akCert = nil | ||
| } | ||
|
|
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.
We dont return an error here on purpose to allow backward compatibility as well as leave it up to the validator to handle this.
If the validator wants to only check the attestation quote, then it would still pass. If the validator wants to also verify certificates, then this will be checked accordingly
4832d50 to
679eadb
Compare
This pull request enhances the Azure TDX attestation flow by adding robust validation for the vTPM Attestation Key (AK) certificate. It ensures that the attestation key is genuinely signed by Azure's certificate authority, preventing forgery attacks. The changes include reading and parsing the AK certificate from the TPM, downloading and using the correct CA certificates, and verifying the certificate chain and public key match during validation.
Attestation Key Certificate Handling:
Issuernow reads the vTPM AK certificate from the TPM NV index and parses out the DER-encoded certificate. Debug logging is added for certificate extraction and CA download events. (internal/attestation/azure/tdx/issuer.go). Note: currently: it seems that the AK certificate doesn't include URLs to the intermediate certificate that signs it to fetch. Those have to be pulled manually from the Azure's trusted launch docs. Therefore, I omitted the step of fetching those URLs and downloading them for simplicity and avoid unnecessary unused code.InstanceInfostruct is expanded to include the extracted AK certificate for use in validation. (internal/attestation/azure/tdx/tdx.go). I left out the CA certificate since we are not pulling the intermediate CA certificates from any embedded URLs. Remember, those URLs do not exist in the Azure's vTPM certificate in TDX CVM instances.Certificate Chain Verification:
Validatornow includes hardcoded root and intermediate CA certificates used by Azure for TDX vTPM attestation, sourced from official documentation and Microsoft. (internal/attestation/azure/tdx/validator.go)Security Improvements:
These changes collectively strengthen the security and correctness of Azure TDX attestation by tightly binding the attestation process to Azure's trusted certificate authorities.
Note:
These changes are backward compatible and safe guarded by a new operational flag
verify-ak-certificatethat can be passed to proxy-client or attested-get to do the extra Azure vTPM certificate verification.