Skip to content

Verifier/KBS: Use SHA-512 for report data / runtime_data calculation in IBM SEL#1338

Merged
Xynnn007 merged 1 commit into
confidential-containers:mainfrom
BbolroC:use-sha512-report-data-ibm-sel
May 19, 2026
Merged

Verifier/KBS: Use SHA-512 for report data / runtime_data calculation in IBM SEL#1338
Xynnn007 merged 1 commit into
confidential-containers:mainfrom
BbolroC:use-sha512-report-data-ibm-sel

Conversation

@BbolroC
Copy link
Copy Markdown
Member

@BbolroC BbolroC commented May 13, 2026

Use the more secure SHA-512 algorithm for the selected-hash-algorithm field in extra_params for IBM SEL.
Update the verifier validation accordingly. The logic for configuring extra_params follows the pattern used by Intel Trust Authority.

In summary:

  1. Use SHA-512 for report data in IBM SEL via extra_params
  2. Select SHA-512 as runtime data hash algorithm for IBM SE in KBS

Depends-on: confidential-containers/guest-components#1469

Signed-off-by: Hyounggyu Choi Hyounggyu.Choi@ibm.com

Copy link
Copy Markdown
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

We might also need to change grpc-as to use sha512 to align with https://github.com/confidential-containers/guest-components/pull/1469/changes

Comment thread kbs/src/attestation/coco/builtin.rs Outdated
@BbolroC BbolroC force-pushed the use-sha512-report-data-ibm-sel branch from 96e7447 to b3eeb95 Compare May 13, 2026 07:25
@BbolroC BbolroC marked this pull request as draft May 13, 2026 09:26
@BbolroC BbolroC force-pushed the use-sha512-report-data-ibm-sel branch 2 times, most recently from a655349 to 8c35c88 Compare May 13, 2026 10:55
@BbolroC BbolroC marked this pull request as ready for review May 13, 2026 15:06
Copy link
Copy Markdown
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Thanks @BbolroC As IBM SE is a little different from other architectures, I tried to get in detail with the code change. There are some comments

Comment thread attestation-service/src/lib.rs Outdated
Comment on lines +6 to +7
pub const SUPPORTED_HASH_ALGORITHMS_JSON_KEY: &str = "supported-hash-algorithms";
pub const SELECTED_HASH_ALGORITHM_JSON_KEY: &str = "selected-hash-algorithm";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that there are also declarations . Maybe we could move that part under attestation/mod.rs

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, you are right. I will move them to attestation/mod.rs. thanks!

Comment thread attestation-service/src/lib.rs Outdated
SELECTED_HASH_ALGORITHM_JSON_KEY: needed_algorithm
})
})
.unwrap_or_else(|| serde_json::Value::String(String::new()))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will potentially hide the errors when doing the chain calling. Looks like a default value is given whenever there is an error? Note that if a malwared tee_params is given, the function will also return a default value, without telling anything wrong. Please ensure if this is expected.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let me get back to the old logic then. Thanks!

Comment thread attestation-service/src/lib.rs Outdated
/// and selects a hash algorithm based on the TEE type if available.
/// Returns a JSON value with the selected algorithm,
/// or an empty string if negotiation fails or is not applicable.
pub fn generate_extra_params(tee: Tee, tee_params: &serde_json::Value) -> serde_json::Value {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, note that extra_params is a conception for KBS's Challenge struct. See the function and it's proper to move the SE-specific logic to that place.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

True. I didn't realize that the extra_params is determined only with tee and tee_params passed to generate_challenge(). I will update the PR. Thanks!

Comment thread deps/verifier/src/se/ibmse.rs Outdated
// Validate runtime_data_digest if provided
if let ReportData::Value(expected_report_data) = expected_report_data {
let expected_report_data =
regularize_data(expected_report_data, 64, "USER_DATA", "IBM SE");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably the 64 here could also be made into a const.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, let's use a constant. Thanks!

Comment on lines +66 to +69
let runtime_data_hash_algorithm = match evidence.tee {
Tee::Se => HashAlgorithm::Sha512,
_ => HashAlgorithm::Sha384,
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like we need to think about bringing a mechanism to include this parameter via the header parsing from RCAR protocol than harding coding here. This should be a TODO for me and not for this PR. This place in the PR looks good now.

Comment thread protos/attestation.proto Outdated
}
message ChallengeResponse {
string attestation_challenge = 1;
string extra_params = 2;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might not need to change the proto here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, we don't need this. Thanks!

Comment thread attestation-service/src/bin/grpc/mod.rs Outdated
Comment on lines +246 to +255
// Generate extra_params for hash algorithm negotiation
let extra_params = serde_json::from_str::<serde_json::Value>(tee_params)
.ok()
.map(|params| generate_extra_params(tee, &params).to_string())
.unwrap_or_default();

info!("GetAttestationChallenge succeeded.");
let res = ChallengeResponse {
attestation_challenge,
extra_params,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See comment about generate_extra_params, and we do not need the part to be changed.

@BbolroC BbolroC force-pushed the use-sha512-report-data-ibm-sel branch from 8c35c88 to 6693190 Compare May 18, 2026 11:08
Copy link
Copy Markdown
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

LGTM. Although the PR does two things -

  1. use sha512 for report data in IBM SEL
  2. select sha512 as runtime data hash algorithm for IBM SE in KBS

So a updation on commit message, or reorg into two commits would either be ok

@BbolroC
Copy link
Copy Markdown
Member Author

BbolroC commented May 19, 2026

LGTM. Although the PR does two things -

  1. use sha512 for report data in IBM SEL
  2. select sha512 as runtime data hash algorithm for IBM SE in KBS

So a updation on commit message, or reorg into two commits would either be ok

I would go with the 1st. Thanks!

Use the more secure SHA-512 algorithm for the
selected-hash-algorithm field in extra_params for IBM SEL.
Update the verifier validation accordingly.

In summary:
1. Use SHA-512 for report data in IBM SEL via extra_params
2. Select SHA-512 as runtime data hash algorithm for IBM SE in KBS

Signed-off-by: Hyounggyu Choi <Hyounggyu.Choi@ibm.com>
@BbolroC BbolroC force-pushed the use-sha512-report-data-ibm-sel branch from 6693190 to 5d46fe3 Compare May 19, 2026 05:49
@BbolroC BbolroC changed the title builtin-as: use sha512 for report data in IBM SEL builtin-as: Use SHA-512 for report data in IBM SEL May 19, 2026
@Xynnn007 Xynnn007 changed the title builtin-as: Use SHA-512 for report data in IBM SEL Verifier/KBS: Use SHA-512 for report data / runtime_data calculation in IBM SEL May 19, 2026
@Xynnn007 Xynnn007 merged commit 80cf7f7 into confidential-containers:main May 19, 2026
46 checks passed
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.

2 participants