Temporarily support missing raw cmd line in the evidence #4958
Conversation
…n only succeeds if the corresponding value is set to skip
proto/attestation/verification.proto
Outdated
| @@ -146,7 +146,7 @@ message KernelLayerData { | |||
| RawDigest kernel_cmd_line = 2 [deprecated = true]; | |||
|
|
|||
| // Command-line that was passed to the kernel during startup. | |||
There was a problem hiding this comment.
Consider adding a comment here stating what the significance is between an absent field and a present-but-empty field.
There was a problem hiding this comment.
+1 in fact I'm not sure I am reading the logic correctly, could you clarify the intention?
There was a problem hiding this comment.
Added a comment, more context for this change in b/325979696
There was a problem hiding this comment.
It may make sense to establish a new pattern, where instead of having a single 'obsolete' evidence copy and a single 'current' evidence copy you accumulate more and more evidence protos over time and simply name them by date (e.g. 'rk_evidence_20240312' etc.) and then always add more tests to verifier_tests.rs as new fields are added or old ones are deprecated, always keeping old tests with old Evidence and old ReferenceValues around to ensure that they continue to pass. That probably would ensure we have a better chance of catching backward-incompatible changes using this unit test.
Not necessarily something you need to address in this CL, but you could consider naming this as 'rk_evidence_20240312' or something similar already.
There was a problem hiding this comment.
Good idea, done. Though I hope that with existing users migrated and our patterns becoming more stable we'll end up with just one version here again :)
| ) | ||
| .context("kernel command line failed verification")?; | ||
| } else { | ||
| // Support missing kernel_cmd_line_regex but only if the corresponding reference |
There was a problem hiding this comment.
FWIW this still feels backwards to me (i.e. we should start from the expected reference value). I know we agreed to clean it up later on anyway :)
proto/attestation/verification.proto
Outdated
| @@ -146,7 +146,7 @@ message KernelLayerData { | |||
| RawDigest kernel_cmd_line = 2 [deprecated = true]; | |||
|
|
|||
| // Command-line that was passed to the kernel during startup. | |||
There was a problem hiding this comment.
+1 in fact I'm not sure I am reading the logic correctly, could you clarify the intention?
| ) | ||
| .context("kernel command line failed verification")?; | ||
| } else { | ||
| // Support missing kernel_cmd_line_regex but only if the corresponding reference |
There was a problem hiding this comment.
Good idea, done. Though I hope that with existing users migrated and our patterns becoming more stable we'll end up with just one version here again :)
proto/attestation/verification.proto
Outdated
| @@ -146,7 +146,7 @@ message KernelLayerData { | |||
| RawDigest kernel_cmd_line = 2 [deprecated = true]; | |||
|
|
|||
| // Command-line that was passed to the kernel during startup. | |||
There was a problem hiding this comment.
Added a comment, more context for this change in b/325979696
| // migrated. | ||
| anyhow::ensure!( | ||
| matches!( | ||
| reference_values.kernel_cmd_line_regex.as_ref().unwrap().r#type.as_ref(), |
There was a problem hiding this comment.
I also just noticed that this unwrap() call will crash/panic if no kernel_cmd_line_regex field is specified in the ReferenceValues, as opposed to generating a (recoverable) error., which seems undesirable given that that field is now optional.
Verification only succeeds if the corresponding value is set to skip