Relax measurements#80
Conversation
There was a problem hiding this comment.
This looks good. However, I think it's headed slightly in the wrong direction.
@flihp's PR for #75 ends up returning the PlatformId with a method on Stream. I think it would be better if we could do the same thing here, and add a bool to Stream when constructed indicating if measurement verification failed. Then we'd add a bool method accessor to Stream. This logically separates the use of PlaftormId and verifications failed at the API level, and allows example code and the like to not even bother looking at whether measurements verified or not. This is usefu,l because eventually, we'll always fail the connection if measurements fail to verify, and we won't have to change the code that isn't worrying about that. I also just prefer the logical separation which helpfully prevents bikeshedding about type names.
|
|
||
| /// The result of measurement appraisal | ||
| #[derive(Clone, Debug, Deserialize)] | ||
| pub enum SprocketsResult { |
There was a problem hiding this comment.
I think a better name would be MeasurementStatus, and then the variants could be Verified and Unverified.
I find it confusing when a type includes Result in the name even though it is not a Result.
| MeasurementsUnverified(PlatformId), | ||
| } | ||
|
|
||
| impl SprocketsResult { |
There was a problem hiding this comment.
Presumably, the purpose of this type is to allow us to check inside omicron whether measurements passed or succeeded. In that case, maybe we should add a method here for ergonomics. Something like:
pub fn measurements_verified() -> bool {
matches!(self, SprocketsResult::MeasurementsVerified(_))
}
Ah I hadn't seen that PR! I like that approach. |
21ee83f to
6059adc
Compare
| pub fn peer_platform_id_unchecked(&self) -> &PlatformId { | ||
| &self.platform_id | ||
| } | ||
|
|
||
| pub fn peer_platform_id(&self) -> Option<&PlatformId> { | ||
| if self.corpus_appraisal_success { | ||
| Some(&self.platform_id) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
I will admit this is a little goofy but I wanted to make it difficult to use the unappraised version without being intentional.
There was a problem hiding this comment.
Won't this practically prevent us from operating with the appraisal turned off for initial trust quorum? Or do we expect that we'll turn it on always by the time TQ is in production?
There was a problem hiding this comment.
My initial thought was to always use peer_platform_id_unchecked for now. I think by the time we want to deploy TQ we should be requiring the appraisal. We could return (PlatformId, bool) indicating the appraisal status but that also allows you to do let (id, _) = stream.peer_platform_id() to skip the appraisal status which maybe is fine? I kind of liked the enum return and requiring the caller to always check the appraisal status but I love my types a lot (maybe too much).
There was a problem hiding this comment.
Got it. I had a different expectation for how we'd always enable appraisal, which I believe is what's causing some confusion here. I figured that once we enabled appraisal, we'd do so by changing the sprockets code to return an error rather than making the caller check via looking at the platform id.
In essence I expected we'd back out this code and go back to returning an error, rather than changing the code at the call sites. Once we have some testing under our belt, the API should not allow a stream to be constructed with a failure ever again. This is how SPDM works, and how I always intended sprockets to work. It makes it foolproof to do the right thing, because you literally can't get a stream object if appraisal fails.
There was a problem hiding this comment.
Okay I see your point. My thought was we could have some intermediate APIs but you're right we eventually want the current behavior.
Currently we require measurement appraisal against a valid corpus set to be successful in order to connect via sprockets. This is what we want in the ideal world. Unfortunately, we don't live in the ideal world and we're still working out how the corpus should be updated. Allow callers to determine how to handle measurement failures should be handled. This still requires a full verifcation of the certificate chain which should always work on hardware.
6059adc to
4109fa5
Compare
andrewjstone
left a comment
andrewjstone
left a comment
There was a problem hiding this comment.
Looks great @labbott. Thanks for bearing with me.
| &self.platform_id | ||
| } | ||
|
|
||
| pub fn appraisal_status(&self) -> bool { |
There was a problem hiding this comment.
nit: Maybe call this appraisal_success to be more clear that true is success and false is not.
Mock up what happens if we have a client who doesn't follow the protocol and just sends messages
4109fa5 to
7c47daa
Compare
2 participants
No description provided.