joinservice: cache certificates for Azure SEV-SNP attestation

[msanft]

Context

Caching ASK intermediate certificates is a prerequisite for VLEK-based signer attestation as performed on AWS SEV-SNP, but can also be used on Azure.

Proposed change(s)

• On cluster initialization, request the Milan certificate chain from the AMD KDS in the join service, and save it in a configmap.
• When creating the validator for a joinservice pod, pass the cached certificate to the validator if it exists.
• In the validator, use the cached certificate. On Azure, this is a special case as there is a 3-step precedence in which certificates should be taken:
  • If the ARK and ASK from the THIM response are intact, use them.
  • If the THIM response does not contain the cert chain, use the ARK from the Constellation config and the cached ASK.
  • If neither of these cases applies, retrieve ARK and ASK from AMD KDS.
    

    constellation/internal/attestation/azure/snp/validator.go

    Lines 267 to 271 in d3c9769

    	// attestationWithCerts returns a formatted version of the attestation report and its certificates from the instanceInfo.
    	// Certificates are retrieved in the following precedence:
    	// 1. ASK or ARK from THIM
    	// 2. ASK or ARK from fallbackCerts
    	// 3. ASK or ARK from AMD KDS

Additional info

• E2E Test
• More recent E2E Test
• Even more recent E2E Test

Checklist

• Add labels (e.g., for changelog category)
• Is PR title adequate for changelog?
• Link to Milestone

[thomasten]

This introduces attestation-variant-specific code in otherwise attestation-variant-agnostic code. This can get messy if we add more variants in the future. We should try to reduce this where possible.
Thing that come into my mind:

• Adding a parameter for attestation-variant-specific data (like cfg, but for non-user-defined data) to the Validator func instead of adding a very specific ValidatorWithASKCertCache func.
• Do we need a separate config map for the cached cert? If yes, should it be named more generically for all kinds of (future) attestation-variant-specific data?
• Better separate the attestation-variant-specific code in joinservice's main.go. (At least move it to an own func.)

Not sure if all of this makes sense or if there are better ways. Please think about it.

[msanft]

This introduces attestation-variant-specific code in otherwise attestation-variant-agnostic code. This can get messy if we add more variants in the future. We should try to reduce this where possible.

I definitely see the point here and agree.

Regarding having a ValidatorWithASKCertCache, I discussed this with Leo and we agreed that changing current validator function signatures would come at the cost of a lot of refactoring to do. However, I also think that this might be worth it, in case other attestation-variant-specific features are introduced in the future. Potentially we should consider a builder pattern or something similar here. I also think that wrapping the csp/attestation-specific things behind a returned interface is not a great abstraction necessarily. Regarding the other 2 points, i will think about it.

[thomasten]

changing current validator function signatures would come at the cost of a lot of refactoring to do. However, I also think that this might be worth it, in case other attestation-variant-specific features are introduced in the future.

True. We should discuss that with @daniel-weisse. I think he has designed most of the current architecture.

[daniel-weisse]

What about adding the ASK as an optional value to the Azure attestation config?
While this would still require attestation variant specific code in the join-service, it would keep the function signature intact, and also allows us to easily disable the caching behavior for the CLI (by not setting the optional ASK field).
We already do something similar for the MAA URL on Azure.

[netlify]

✅ Deploy Preview for constellation-docs canceled.

Name	Link
🔨 Latest commit	87f29b2
🔍 Latest deploy log	https://app.netlify.com/sites/constellation-docs/deploys/651695b8d62add0008e28861

[daniel-weisse]

Implementation looks good to me now

[daniel-weisse]

Please run e2e tests before merging.
Otherwise looks good to me

[malt3]

Looks good from a high level. I think Daniel and Thomas are the best reviewers when it comes to the attestation details.

[3u13r]

LGTM

[github-actions]

Coverage report

Package	Old	New	Trend
internal/attestation/azure/snp	46.90%	49.00%	↗️
internal/attestation/choose	85.00%	85.00%	↔️
internal/attestation/variant	[no test files]	[no test files]	🚧
internal/config	80.10%	80.20%	↗️
internal/constants	[no test files]	[no test files]	🚧
internal/crypto	50.00%	69.60%	↗️
joinservice/cmd	[no test files]	[no test files]	🚧
joinservice/internal/certcache	0.00%	73.60%	🆕
joinservice/internal/certcache/amdkds	0.00%	100.00%	🆕
joinservice/internal/kubernetes	8.50%	6.90%	↘️
joinservice/internal/server	76.20%	79.20%	↗️
joinservice/internal/watcher	80.40%	78.90%	↘️

[daniel-weisse]

Suggested change

	// X509CertToPem takes an x.509 certificate and returns it as a PEM-encoded certificate.
	func X509CertToPem(cert *x509.Certificate) ([]byte, error) {
	// X509CertToPEM takes an x.509 certificate and returns it as a PEM-encoded certificate.
	func X509CertToPEM(cert *x509.Certificate) ([]byte, error) {

PEM should be the correct form here