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

plumb AMD certs to workload containers #1549

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Conversation

anmaxvl
Copy link
Contributor

@anmaxvl anmaxvl commented Oct 21, 2022

Add logic to plumb AMD certificates to workload containers. The
assumption is that the certificates will be "fresh enough" for
necessary attestation and key release by the workflow and third
party services.

Signed-off-by: Maksim An maksiman@microsoft.com

@anmaxvl
Copy link
Contributor Author

anmaxvl commented Oct 21, 2022

@KenGordon FYI

Add logic to plumb AMD certificates to workload containers. The
assumption is that the certificates will be "fresh enough" for
necessary attestation and key release by the workflow and third
party services.

Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl anmaxvl marked this pull request as ready for review November 23, 2022 04:45
@anmaxvl anmaxvl requested a review from a team as a code owner November 23, 2022 04:45
@anmaxvl anmaxvl changed the title plumb AMD certs to GCS plumb AMD certs to workload containers Nov 23, 2022
@helsaawy helsaawy self-assigned this Nov 30, 2022
@msscotb msscotb self-assigned this Nov 30, 2022
@@ -42,6 +42,17 @@ func WithSecurityPolicyEnforcer(enforcer string) ConfidentialUVMOpt {
}
}

func base64EncodeContent(filePath string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

base64EncodeFileContents?

return "", nil
}
content, err := os.ReadFile(filePath)
if err != nil && !os.IsNotExist(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could result in an undetected file not found. Callers may end up troubleshooting why the base64 string is "" instead of troubleshooting that the file doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, that's fair. although I should probably revert this change, since the approach to fetching the certs have changed and in previous iterations, I was reading it from the host, thus introduced this utility function. At the moment we always set the default reference file, so under non-SNP deployments the file won't exist, I might need to refactor that a bit, which is probably outside of the scope of this PR. I'll add some logging for now and make the refactoring change in a different PR, if you don't mind.

Signed-off-by: Maksim An <maksiman@microsoft.com>
Copy link
Contributor

@msscotb msscotb left a comment

Choose a reason for hiding this comment

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

lgtm

@anmaxvl anmaxvl merged commit 734a0ed into microsoft:main Dec 6, 2022
@anmaxvl anmaxvl deleted the cert-plumbing branch December 6, 2022 01:05
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
confidential containers: Add AMD cert plumbing

Add logic to plumb AMD certificates to workload containers. The
assumption is that the certificates will be "fresh enough" for
necessary attestation and key release by the workflow and third
party services.

Additionally add error logging when UVM reference info file
is not found

Signed-off-by: Maksim An <maksiman@microsoft.com>
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.

4 participants