kbs: ita: Set hash algorithm based on TEE type #491

jodh-intel · 2024-09-06T15:44:02Z

If the TEE specifies the hash algorithms it can use [1], add the appropriate hash algorithm to the returned Challenge [2].

For backwards compatibility, do not return the selected hash algorithm if the TEE does not provide the list of hash algorithms it can use.

Partially-fixes: #242.

[1] - In the optional extra-params.supported-hash-algorithms list.
[2] - In extra-params.selected-hash-algorithm.

jodh-intel · 2024-09-06T15:57:35Z

Once I've taken it out of draft mode, this PR should land with confidential-containers/guest-components#712.

Xynnn007 · 2024-09-09T04:06:25Z

kbs/src/attestation/intel_trust_authority/mod.rs

+        if let Some(hash_algorithms_found) = tee_parameters.get(SUPPORTED_HASH_ALGORITHMS_JSON_KEY)
+        {
+            if let Some(algorithms) = hash_algorithms_found.as_array() {
+                let hash_algorithms: Vec<String> = algorithms
+                    .iter()
+                    .filter_map(|s| s.as_str())
+                    .map(|s| s.to_lowercase())
+                    .collect();
+
+                supported_hash_algorithms.append(&mut hash_algorithms.clone());
+            } else {
+                return Err(anyhow!(
+                    "Intel Trust Authority: expected array, found {hash_algorithms_found:?}"
+                ));
+            }
+        } else {
+            log::info!("ITA: generate_challenge: no TEE hash parameters, so falling back to legacy behaviour");
+
+            return generic_generate_challenge(tee, tee_parameters).await;
+        }


We could use

let Some(xxx) = yyy() else { bail!("....."); } // do next steps with xxx

to avoid nested { ... }s .

Xynnn007 · 2024-09-09T04:14:22Z

kbs/src/attestation/intel_trust_authority/mod.rs

+lazy_static! {
+    /// The hash algorithm used for Intel SGX.
+    static ref SGX_HASH_ALGORITHM: String = HashAlgorithm::Sha256.to_string().to_lowercase();
+
+    /// The hash algorithm used for Intel TDX.
+    static ref TDX_HASH_ALGORITHM: String = HashAlgorithm::Sha512.to_string().to_lowercase();
+
+    static ref ERR_NO_TDX_ALGORITHM: String = format!(
+        "Intel Trust Authority: {:?} not supported by TDX TEE",
+        *TDX_HASH_ALGORITHM
+    );
+    static ref ERR_NO_SGX_ALGORITHM: String = format!(
+        "Intel Trust Authority: {:?} not supported by SGX TEE",
+        *TDX_HASH_ALGORITHM
+    );


We might not need these consts. For SGX_HASH_ALGORITHM, we can

enum HashAlgorithm { #[strum(serialize = "sha256")] Sha256, ... } ... if supported_hash_algorithms.contains(HashAlgorithm::Sha256.as_ref()) { ... }

For errors, I suggest that directly use .context("...") for extra error context.

jodh-intel · 2024-09-09T18:07:09Z

@Xynnn007 - Thanks for reviewing! Branch updated.

/cc @mythi.

mythi · 2024-09-10T06:34:17Z

kbs/src/attestation/intel_trust_authority/mod.rs

 use crate::token::{
    jwk::JwkAttestationTokenVerifier, AttestationTokenVerifier, AttestationTokenVerifierConfig,
    AttestationTokenVerifierType,
 };
 use anyhow::*;
 use async_trait::async_trait;
+use attestation_service::HashAlgorithm;


I had somehow missed this dependency earlier. This is slightly problematic as it is right now. This makes one AS to depend on another AS code just for the sake of HashAlgorithm enum. Not only that but attestation-service/default makes the build to include all of the CoCo-AS verifier dependencies, such as the vtpm stuff that now makes se CI docker build to fail because the ITA Dockerfile does not have libtss2-dev installed.

Branch updated. I've made a local definition of HashAlgorithm to resolve that issue.

Once this and confidential-containers/guest-components#712 land, I plan to tal at consolidating all the definitions though 😄

mkulke

I wonder if it would be better to move the hash_alg support to the protocol level instead of putting it into the schemaless extra_params field. it can be optional, defaulting to sha348. atm, this seems local to ITA and otherwise unspecified strings supported/selected-hash-algorithms.

Sadly, this would need a (breaking) change on kbs-types, but it would make things a bit more explicit:

Challenge {
   nonce String,
   hash_alg: HashAlgorithm
   extra_params: Value
}

Maybe, if we don't want to touch kbs-types we can also introduce trustee ExtraParams that are more than just Value, but specify the hash_alg field.

mkulke · 2024-09-10T08:59:16Z

kbs/src/attestation/intel_trust_authority/mod.rs

+            .filter_map(|s| s.as_str())
+            .map(|s| s.to_lowercase())


if we're already filter_map'ing why map?

Suggested change

.filter_map(|s| s.as_str())

.map(|s| s.to_lowercase())

.filter_map(|s| s.as_str(). to_lowercase())

It's a bit ugly now, but updated.

mkulke · 2024-09-10T09:02:41Z

kbs/src/attestation/intel_trust_authority/mod.rs

+
+        if tee_parameters.is_null() {
+            log::debug!(
+                "ITA: generate_challenge: no TEE parameters so falling back to legacy behavour"


Suggested change

"ITA: generate_challenge: no TEE parameters so falling back to legacy behavour"

"ITA: generate_challenge: no TEE parameters so falling back to legacy behaviour"

mkulke · 2024-09-10T09:04:48Z

kbs/src/attestation/mod.rs

+pub(crate) async fn generic_generate_challenge(
+    _tee: Tee,
+    _tee_parameters: serde_json::Value,
+) -> Result<Challenge> {


why does this have params if we don't use them? Should we maybe do impl Default for Challenge?

I was trying to make this a minimal change and if you look at the existing code, you'll see the same unused params.

Should we maybe do impl Default for Challenge?

I thought of that, but Challenge is defined in the kbs-types crate, so that can be done at a later time imho.

If the TEE specifies the hash algorithms it can use [1], add the appropriate hash algorithm to the returned `Challenge` [2]. For backwards compatibility, do not return the selected hash algorithm if the TEE does not provide the list of hash algorithms it can use. Partially-fixes: confidential-containers#242. [1] - In the optional `extra-params.supported-hash-algorithms` list. [2] - In `extra-params.selected-hash-algorithm`. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel · 2024-09-10T09:52:12Z

@mkulke - Thanks for the review. I agree that this can all be improved, but since we'd really like this PR and confidential-containers/guest-components#712 to be in the next release, and since testing these takes some time, can we defer some of these comments until after the upcoming release(s)?

mkulke · 2024-09-10T11:30:44Z

@mkulke - Thanks for the review. I agree that this can all be improved, but since we'd really like this PR and confidential-containers/guest-components#712 to be in the next release, and since testing these takes some time, can we defer some of these comments until after the upcoming release(s)?

I see SUPPORTED_HASH_ALGORITHMS is generalized for all TEE on the guest-components PR, but only ITA is using it at the moment. I don't think there's much harm in merging this as-is, but maybe we can have a follow up ticket to make the hash_alg a more explicit part of the AS protocol.

fitzthum

LGTM. This should be part of the protocol imo but we can do that later.

mythi · 2024-09-10T17:02:42Z

@mkulke - Thanks for the review. I agree that this can all be improved, but since we'd really like this PR and confidential-containers/guest-components#712 to be in the next release, and since testing these takes some time, can we defer some of these comments until after the upcoming release(s)?

I see SUPPORTED_HASH_ALGORITHMS is generalized for all TEE on the guest-components PR, but only ITA is using it at the moment. I don't think there's much harm in merging this as-is, but maybe we can have a follow up ticket to make the hash_alg a more explicit part of the AS protocol.

I can create the follow-up ticket with additional thoughts. hash_alg is just one part of the puzzle. What/how things are hashed requires thinking too.

Xynnn007

Overall lgtm. Only one nit. Thanks! @jodh-intel

Xynnn007 · 2024-09-11T07:47:25Z

kbs/src/attestation/intel_trust_authority/mod.rs

+const ERR_NO_TEE_ALGOS: &str = "ITA: TEE does not support any hash algorithms";
+const ERR_INVALID_TEE: &str = "ITA: Unknown TEE specified";


Let me do a refactoring about error information assertion later. Now it's good to me.

Xynnn007 · 2024-09-11T07:48:32Z

kbs/src/attestation/intel_trust_authority/mod.rs

+            return Err(anyhow!(
+                "ITA: expected array, found {hash_algorithms_found:?}"
+            ));


Suggested change

return Err(anyhow!(

"ITA: expected array, found {hash_algorithms_found:?}"

));

bail!(

"ITA: expected array, found {hash_algorithms_found:?}"

);

Xynnn007 · 2024-09-11T07:50:19Z

Btw, shall we cut a release after ITA is supported on both gc and trustee side?

cc @fitzthum @mkulke @mythi @ChengyuZhu6

mythi · 2024-09-11T07:54:50Z

Btw, shall we cut a release after ITA is supported on both gc and trustee side?

I'm about to submit a PR for kustomization (kbs/config/kubernetes) still once we get the final validation with these PRs completed.

mythi · 2024-09-11T09:33:55Z

@Xynnn007 we have this one also fully tested so it's fine to merge. We will rebase #494 on top of HEAD after that.

fidencio · 2024-09-11T09:40:25Z

I've triggered the e2e tests as well, so let's wait for them to finish.

fidencio · 2024-09-11T09:57:04Z

Merged, thanks @jodh-intel for the work and all the reviewers for the reviews! :-)

Xynnn007 reviewed Sep 9, 2024

View reviewed changes

jodh-intel force-pushed the kbs-ita-set-tee-hash-algorithm branch 2 times, most recently from f0c55f9 to 666d2c2 Compare September 9, 2024 12:53

mythi reviewed Sep 10, 2024

View reviewed changes

jodh-intel force-pushed the kbs-ita-set-tee-hash-algorithm branch 2 times, most recently from 767a039 to f8c34e9 Compare September 10, 2024 08:18

mkulke reviewed Sep 10, 2024

View reviewed changes

jodh-intel force-pushed the kbs-ita-set-tee-hash-algorithm branch from f8c34e9 to 12da724 Compare September 10, 2024 09:46

mkulke approved these changes Sep 10, 2024

View reviewed changes

jodh-intel marked this pull request as ready for review September 10, 2024 13:51

jodh-intel requested a review from a team as a code owner September 10, 2024 13:51

mythi mentioned this pull request Sep 10, 2024

ita: add support for Azure attestation using dedicated API #494

Merged

fitzthum approved these changes Sep 10, 2024

View reviewed changes

Xynnn007 approved these changes Sep 11, 2024

View reviewed changes

fidencio added test_e2e Authorize TEE e2e test run labels Sep 11, 2024

fidencio merged commit 5d0f0bc into confidential-containers:main Sep 11, 2024
28 checks passed

mythi mentioned this pull request Sep 10, 2024

KBS (protocol) enhancements to reportdata generation #242

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kbs: ita: Set hash algorithm based on TEE type #491

kbs: ita: Set hash algorithm based on TEE type #491

jodh-intel commented Sep 6, 2024

jodh-intel commented Sep 6, 2024

Xynnn007 Sep 9, 2024

Xynnn007 Sep 9, 2024

jodh-intel commented Sep 9, 2024

mythi Sep 10, 2024

jodh-intel Sep 10, 2024

mkulke left a comment

mkulke Sep 10, 2024

jodh-intel Sep 10, 2024

mkulke Sep 10, 2024

jodh-intel Sep 10, 2024

mkulke Sep 10, 2024

jodh-intel Sep 10, 2024

jodh-intel commented Sep 10, 2024

mkulke commented Sep 10, 2024

fitzthum left a comment •

edited

Loading

mythi commented Sep 10, 2024

Xynnn007 left a comment

Xynnn007 Sep 11, 2024

Xynnn007 Sep 11, 2024

Xynnn007 commented Sep 11, 2024 •

edited

Loading

mythi commented Sep 11, 2024

mythi commented Sep 11, 2024

fidencio commented Sep 11, 2024

fidencio commented Sep 11, 2024

	.filter_map(\|s\| s.as_str())
	.map(\|s\| s.to_lowercase())
	.filter_map(\|s\| s.as_str(). to_lowercase())

	"ITA: generate_challenge: no TEE parameters so falling back to legacy behavour"
	"ITA: generate_challenge: no TEE parameters so falling back to legacy behaviour"

		const ERR_NO_TEE_ALGOS: &str = "ITA: TEE does not support any hash algorithms";
		const ERR_INVALID_TEE: &str = "ITA: Unknown TEE specified";

kbs: ita: Set hash algorithm based on TEE type #491

kbs: ita: Set hash algorithm based on TEE type #491

Conversation

jodh-intel commented Sep 6, 2024

jodh-intel commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkulke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Sep 10, 2024

mkulke commented Sep 10, 2024

fitzthum left a comment • edited Loading

Choose a reason for hiding this comment

mythi commented Sep 10, 2024

Xynnn007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 commented Sep 11, 2024 • edited Loading

mythi commented Sep 11, 2024

mythi commented Sep 11, 2024

fidencio commented Sep 11, 2024

fidencio commented Sep 11, 2024

fitzthum left a comment •

edited

Loading

Xynnn007 commented Sep 11, 2024 •

edited

Loading