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

AA: Add API of CheckInitData #462

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

Xynnn007
Copy link
Member

@Xynnn007 Xynnn007 commented Jan 29, 2024

This new API is for kata-agent to check the data's integrity binding relationship with the hardware evidence.

Close #446

The main work for TDX and SNP platform has been done by @danmihai1 , this PR just refers most of the code.

cc @fitzthum

@Xynnn007 Xynnn007 marked this pull request as draft January 29, 2024 06:19
@Xynnn007 Xynnn007 force-pushed the aa-initdata branch 2 times, most recently from 726160a to 4c06b19 Compare January 29, 2024 06:40
@Xynnn007 Xynnn007 marked this pull request as ready for review January 29, 2024 08:01
attestation-agent/attester/src/lib.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/snp/hostdata.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mrconfigid.rs Outdated Show resolved Hide resolved
attestation-agent/lib/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

Nice.

One thing I think we might have forgotten about when designing: ideally the KBS/AS will end up with the plaintext of the InitData so that it can be validated with a policy. We don't seem to have a mechanism for this. Perhaps if the plaintext is provided via this API it should be saved by the AA so that it can be sent as part of remote attestation. We could probably implement this in a follow-on.

attestation-agent/protos/attestation-agent.proto Outdated Show resolved Hide resolved
attestation-agent/attester/Cargo.toml Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mrconfigid.rs Outdated Show resolved Hide resolved
@Xynnn007 Xynnn007 force-pushed the aa-initdata branch 2 times, most recently from cf585d5 to 90161dd Compare January 30, 2024 06:19
@Xynnn007
Copy link
Member Author

Thanks @mkulke for the suggestions. I left the review conversions which might need to be ensured by you. PTAL

Hi @fitzthum

One thing I think we might have forgotten about when designing: ideally the KBS/AS will end up with the plaintext of the InitData so that it can be validated with a policy. We don't seem to have a mechanism for this. Perhaps if the plaintext is provided via this API it should be saved by the AA so that it can be sent as part of remote attestation. We could probably implement this in a follow-on.

Yes, agreed. Added the enum of initdata for plaintext, which could be relied on. Also some conversions to be determined. PTAL

message InitDataPlaintext {
bytes Content = 1;
string Algorithm = 2;
}
Copy link
Member

@fitzthum fitzthum Jan 30, 2024

Choose a reason for hiding this comment

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

So it's worth noting that this is InitData specific code. If we still haven't reached an agreement there, maybe we should leave support for the Plaintext to a later PR.

fwiw, I think this shows a clear benefit of the InitData approach. The policy does not include any information about how it is hashed and even if it did, we would have to do parsing of OPA to get it. We could add an additional field to pass the hash type from the shim (where the data is first hashed), to the kata agent, to the AA, to the KBS, but it seems nicer to have it simply included in the InitData. cc @mkulke

Copy link
Contributor

Choose a reason for hiding this comment

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

I would opt for adding plaintext checks later if that doesn't clutter the API. At the moment check_init_data() is ambiguous and needs a dead Option<> param. Maybe we can split it into check_init_data_digest and in follow-up PRs introduce check_init_data_body or consolidate both into check_init_data.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind having one endpoint for both even if only the digest is supported at first. I think we will need a plaintext option for the attestation flow to really add up, but this will require some agreement on the format.

Btw, @Xynnn007 this InitDataPlaintext message doesn't quite match up with your RFC spec which contains a version and digest field as well.

Copy link
Member Author

@Xynnn007 Xynnn007 Jan 31, 2024

Choose a reason for hiding this comment

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

Btw, @Xynnn007 this InitDataPlaintext message doesn't quite match up with your RFC spec which contains a version and digest field as well.

Yes. I once hope that the format in RFC is handled inside kata-agent. That means

  • version check
  • data field into files
  • use the digest field & hash alg to match the data field and call AA's CheckInitData() semantically / DIrectly use data field and hash alg to call AA's CheckInitDataPlaintext() semantically, where the InitDataPlaintext

@mkulke I once thought that one API is enough, and to decrease the potential adaptation work when API changes, I used one API to receive different kinds of parameters as they both are "checking the initdata field binding relationship".

But with your opinions I realize that the steps are probably relatively big. Let me add one simple parameter initDataDigest and leave the plaintext extension in future together with the format issue.

This api is used to check the current TEE evidence's initdata field,
like TDX's MRCONFIGID, SNP's HOSTDATA, etc. If the one inside evidence
is different from the one provided, the RPC will raise an error.

The operation of getting current TEE's evidence should be network
independent, because AA usually runs at an early stage of a guest, where
network is usually not prepared.

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007
Copy link
Member Author

Xynnn007 commented Feb 1, 2024

Thanks @fitzthum @mkulke @jialez0 , I fixed the suggestions you've left.

  1. Use pad()
  2. Only check digest for now and API
  3. use tdx_attest::get_evidence rather than impl manually

PTAL

Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

lgtm

In TDX, we use tdx_attest crate to get a raw hardware tdx report to
parse the MRCONFIGID field.

In SNP, we use sev crate to get a hardware report to parse HOSTDATA
field.

The input one should be resize as the evidence field inside the TEE
evidence to compare.

Signed-off-by: Magnus Kulke <[email protected]>
Signed-off-by: Dan Mihai <[email protected]>
Signed-off-by: Xynnn007 <[email protected]>
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM

jialez0
jialez0 approved these changes Feb 2, 2024
@jialez0 jialez0 merged commit d56a2cf into confidential-containers:main Feb 2, 2024
17 checks passed
@Xynnn007 Xynnn007 deleted the aa-initdata branch February 2, 2024 03:57
@jiazhang0
Copy link
Member

@fitzthum @Xynnn007 @jialez0 Let me clarify something for the future decision and this time just let it go.

I think it is an not easy decision to add a new API which may introduces extra attack interface. In other word, there is still the possibility to use an approach to check the validation of init data without adding new API. In addition, CoCo is not the only case where AA will be deployed. In some cases, the proxy is not used.

Furthermore, considering certain security principles such as defense in depth and the principle of least privilege, adding a new API is not the only solution to address the problem and requirement at hand. Even if someone argues that the newly added API has successfully passed penetration testing without any vulnerabilities found, the results of penetration testing can only provide temporary confidence. Past performance is not indicative of future results. Therefore, we cannot exclude the potential for unknown vulnerabilities and security risks with new API.

Security often focuses on the future possibilities that arise when the timeline is extended, and aims to proactively address potential risks and threats that may arise in the future.

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.

AA | Design ideas to verify the initdata
5 participants