-
Notifications
You must be signed in to change notification settings - Fork 106
[RTE-659] Adding TDX Collateral collection in the dcap-artifact-retrieval and pcs crate
#855
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: master
Are you sure you want to change the base?
Conversation
intel-sgx/dcap-artifact-retrieval/src/provisioning_client/intel.rs
Outdated
Show resolved
Hide resolved
intel-sgx/dcap-artifact-retrieval/src/provisioning_client/intel.rs
Outdated
Show resolved
Hide resolved
intel-sgx/dcap-artifact-retrieval/src/provisioning_client/mod.rs
Outdated
Show resolved
Hide resolved
47752ee to
87b2500
Compare
intel-sgx/dcap-artifact-retrieval/src/provisioning_client/azure.rs
Outdated
Show resolved
Hide resolved
c70c94e to
a3ad696
Compare
intel-sgx/dcap-artifact-retrieval/src/provisioning_client/azure.rs
Outdated
Show resolved
Hide resolved
intel-sgx/dcap-artifact-retrieval/src/provisioning_client/intel.rs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| pub trait PlatformTypeWithTcbDeserializer<T: PlatformType> : PlatformType { | ||
| fn deserialize<'de, D>(deserializer: D) -> Result<TcbData<T, Unverified>, D::Error> |
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.
PlatformType is already a super trait of PlatformTypeWithTcbDeserializer so why do you need a generic type here?
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 is to introduce the T of PlatformType type so it can be passed as generic to TcbData<T, Unverified> in the Result type. Then the making it super trait allows to simplify the other user codes.
Instead of using the generic constraint PlatformType + PlatformTypeDeserializer<T>, the relevant code can just use a single PlatformTypeWithDeserializer<T> which will also definitely requires the type to have PlatformType trait.
|
I am thinking about it again, I think the |
8bd7c6c to
1b361ee
Compare
intel-sgx/dcap-artifact-retrieval/src/provisioning_client/intel.rs
Outdated
Show resolved
Hide resolved
| fn tcbinfo(&self, fmspc: &Fmspc, evaluation_data_number: Option<u16>) -> Result<TcbInfo, Error>; | ||
| fn tcbinfo(&self, fmspc: &Fmspc, evaluation_data_number: Option<u16>) -> Result<TcbInfo<platform::SGX>, Error>; | ||
|
|
||
| fn tcbinfo_tdx(&self, fmspc: &Fmspc, evaluation_data_number: Option<u16>) -> Result<TcbInfo<platform::TDX>, Error>; |
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.
Could you use the generic in the function call as well?
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 will check again about it, I thought because the generic is not used in the type parameter, it would not work.
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.
Checked, this is impossible as it requires the entire Client, Fetcher type to be parameterized as well. We have to do it like this.
|
|
||
| fn qe_identity(&self, evaluation_data_number: Option<u16>) -> Result<QeIdentitySigned, Error>; | ||
|
|
||
| fn tdqe_identity(&self, evaluation_data_number: Option<u16>) -> Result<QeIdentitySigned, Error>; |
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.
Could you use the generic in the function call as well?
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.
Same as above
| } | ||
|
|
||
| pub trait PlatformType : Into<Platform> + Display + Clone { | ||
| fn new() -> Self; |
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.
Is this used?
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 for default instantiation of the generic type in custom serializer/deserializer
intel-sgx/pcs/src/pckcrt.rs
Outdated
| /// TCB component as specified in the Intel PCKCrt API v3 and v4 | ||
| #[derive(Serialize, Deserialize, Debug, Default)] | ||
| struct IntelSgxTcbComponentsV3 { | ||
| pub struct IntelSgxTcbComponentsV3 { |
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 does this need to be pub now?
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.
Because the type is used in function signature of a pub trait
|
|
||
| impl<'de> Deserialize<'de> for TcbData<Unverified> { | ||
| fn deserialize<D>(deserializer: D) -> Result<TcbData<Unverified>, D::Error> | ||
| impl PlatformTypeForTcbInfo<platform::SGX> for platform::SGX { |
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 you can do this simpler by parsing with a "unified" struct (using resulting in default impl where fields are missing). And ensuring that you have different impl for each generic. For example: TcbInfo<TDX> has a tdx_module function, that TcbInfo<SGX> simply does not have).
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.
But then there will be unwrap as we may need to use Option type or the likes for that. It was previously written like that until your last comment to avoid unwrap in the accessor function.
6ec7fbd to
37b25fc
Compare
| pub fn tcb_evaluation_data_numbers() { | ||
| let root_ca = include_bytes!("../../tests/data/root_SGX_CA_der.cert"); | ||
| let root_cas = [&root_ca[..]]; | ||
| let client = make_client(PcsVersion::V4); | ||
| let eval_numbers = client.tcb_evaluation_data_numbers().unwrap(); | ||
|
|
||
| let eval_numbers2 = serde_json::ser::to_vec(&eval_numbers) | ||
| .and_then(|v| serde_json::from_slice::<RawTcbEvaluationDataNumbers>(&v)) | ||
| .unwrap(); | ||
| assert_eq!(eval_numbers, eval_numbers2); | ||
|
|
||
| let fmspc = Fmspc::try_from("90806f000000").unwrap(); | ||
| let eval_numbers: TcbEvaluationDataNumbers = | ||
| eval_numbers.verify(&root_cas, Platform::SGX).unwrap(); | ||
| for number in eval_numbers.numbers().map(|n| n.number()) { | ||
| // TODO(#811): Since PCCS is cache service and not able to cache the | ||
| // `Gone` response mentioned below from Intel PCS, We need to change | ||
| // the test behavior to call QE ID API with update=standard to get the | ||
| // smallest TcbEvaluationDataNumber that's still available. | ||
| // | ||
| // Here, we temporarily fix this be hardcoding. | ||
| if number < 18 { | ||
| continue; | ||
| } | ||
| let qe_identity = match client.qe_identity(Some(number)) { | ||
| Ok(id) => id, | ||
| // API query with update="standard" will return QE Identity with TCB Evaluation Data Number M. | ||
| // A 410 Gone response is returned when the inputted TCB Evaluation Data Number is < M, | ||
| // so we ignore these TCB Evaluation Data Numbers. | ||
| Err(super::Error::PCSError(status_code, _)) if status_code == super::StatusCode::Gone => continue, | ||
| res @Err(_) => res.unwrap(), | ||
| }; | ||
| let verified_qe_id = qe_identity | ||
| .verify(&root_cas, EnclaveIdentity::QE) | ||
| .unwrap(); | ||
| assert_eq!(verified_qe_id.tcb_evaluation_data_number(), u64::from(number)); | ||
|
|
||
| let tcb_info = client | ||
| .tcbinfo(&fmspc, Some(number)) | ||
| .unwrap() | ||
| .verify(&root_cas, Platform::SGX, 2) | ||
| .unwrap(); | ||
| assert_eq!(tcb_info.tcb_evaluation_data_number(), u64::from(number)); | ||
| } | ||
| } |
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 remove this test because it is exactly the same as the intel provider unit test and the pccs provider actually uses the intel provider instead. I assume currently PCCS server does not directly support this API.
398e733 to
6422572
Compare
intel-sgx/pcs/src/pckcrt.rs
Outdated
| /// Selects the highest matching TCB level | ||
| /// see <https://api.portal.trustedservices.intel.com/documentation#pcs-tcb-info-v2> | ||
| pub fn find_tcb_state<V: VerificationType>(&self, tcb_data: &TcbData<V>) -> Option<TcbLevel> { | ||
| pub fn find_tcb_state<V: VerificationType>(&self, tcb_data: &TcbData<platform::SGX, V>) -> Option<TcbLevel<platform::SGX>> { |
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.
When checking our code that is doing verification, I think this method should accept any TCB data, not specifically only SGX TCB data. We found out that SGX and TDX TCB may have different SVN results, which I currently don't know which one that is embedded into the PCK Certificate.
I may have to refactor this again to reflect this function accepting TCB data for any T, not only for platform::SGX
5b0f8e4 to
aae5f90
Compare
bac2790 to
e1d480a
Compare
…d TDX separately - `pcs` crate now introduces a static type to indicate if `TcbInfo` and `TcbComponent` is designated for SGX or TDX - Adjust `PckCert` to only allow SGX-type TCB components - QE Identity type now properly handle other than SGX QE, including TDQE, QVE, QAE altogether, especially for file handling - `dcap-artifact-retrieval` crate now supports obtaining TDX collateral from Intel services. Testing for PCCS server is pending update to the test server. - File name for TDX TcbInfo data has extra `.tdx` prefix - Applying TDX specific data on TcbComponent
- Consistency in `store` and `restore` function - Renaming `TcbComponents` variants that is used for backward compatibility - Addressing some warnings from linter - Reverting `QeIdentity` changes
…SGX TCB without TDX fields
e1d480a to
064efb0
Compare
| filename: &str, | ||
| ) -> Result<Option<PathBuf>, Error> { | ||
| /// Write given object in json to given filename under given dir. | ||
| pub fn write_to_file<T: serde::ser::Serialize>(obj: &T, dir: &str, filename: &str, options: WriteOptions) -> Result<Option<PathBuf>, Error> { |
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.
Could we keep the rustdoc as detailed as old code ?
| } | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
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 these dead_code and unused should be able to remove since they should be coverred by your tests?
|
|
||
| #[derive(Clone, Debug, Default, Eq, PartialEq)] | ||
| pub struct TcbComponentsV4<T: PlatformTypeForTcbComponent<T>> { | ||
| sgxtcbcomponents: [TcbComponentEntry; 16], |
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.
typo sgxtcbcomponents ? I assume this should be generic to both sgx and tdx?
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.
could we add tdx test cases
Adding the support for TDX collateral data and extending the existing
TcbInfotypes to be able to distinguish over SGX or TDX collateral types.The PCCS functionality has no unit test yet awaiting the confirmation of the update on Fortanix PCCS server.