fix(node): enforce RTMR3 validation on event checks

[barakeinav1]

fixes #735

[ghost]

Thank you.

[ghost]

Suggested change

	e["event"].as_str() == Some("compose-hash")
	&& e["imr"].as_u64() == Some(RTMR3_INDEX)
	let is_compose_hash_event = log_entry["event"].as_str() == Some("compose-hash");
	let is_rtmr3_measurement = log_entry["imr"].as_u64() == Some(RTMR3_INDEX);
	is_compose_hash_event && is_rtmr3_measurement

[barakeinav1]

I assume you meant

let expected_compose_hash = event_log
.iter()
.find(|e| {
let is_compose_hash_event = e["event"].as_str() == Some("compose-hash");
let is_rtmr3_measurement = e["imr"].as_u64() == Some(RTMR3_INDEX);
is_compose_hash_event && is_rtmr3_measurement
})
.and_then(|e| e["digest"].as_str());

I would think this will have worse performance since both check will be computed each time. but maybe the complier will optimise this.


anyway I don't think it is much clearer.
but if you prefer I can change it.

let me know.

[ghost]

I see what you are saying. I'm not worried about the performance, as it's just indexing a map of values (nothing is being computed), and in the happy case is unaffected as valid rtmrs will always require all checks.

I'd like extracting them as variables because it gives the reader a better understanding of what is being checked. As someone without expertise knowledge of TEE it's not obvious what that check is doing.

[ghost]

Similarly to the suggestion above, can you extract each condition into a variable with a descriptive name? It's not obvious what we are checking for if one is not familiar with TEE.

[ghost]

Ditto

[ghost]

What type are we converting to here in the try_into() call? We should avoid unwraps to not cause panics.

Suggested change

	let replayed_rtmr3 = replay_rtmr(event_log.to_owned(), RTMR3_INDEX.try_into().unwrap());
	let replayed_rtmr3 = replay_rtmr(event_log.to_owned(), RTMR3_INDEX.try_into().unwrap());

[ghost]

replay_rtmr is expecting u8.

Why not have the const RTMR3_INDEX be u8 instead of u64 instead?

[barakeinav1]

serde_json does not have a as_u8 only as_64.
so I'll need to cast either way. this direction seemed a bit better

[ghost]

I see it will be more casting if you cast up from u8 to u64 but it will always give us a strong guarantee at compile time that the conversion never fails, and code won't panic.

It's also obvious to the reader of the code that RTMR3_INDEX is supposed to be a small value by setting it to a u8.

If you cast up to u64 from u8 it will be quite simple, just add as u64 as a suffix.

 e["imr"].as_u64() == Some(RTMR3_INDEX as u64)