Improved return values of pcr_read. #281

Superhepper · 2021-10-18T17:12:19Z

This fixes #277 by doing the following:

Changed pcr_read to return a DigestList instead of PcrData.
Moved PcrData to abstractions and made it possible to construct
PcrData from rust native types.
Added subtract to PcrSelectionList.
Added a pcr_read_all functiion to the pcr module
in abstractions. This function tries to read all the
values in a PcrSelectionList.
Crate tests that verifies that the new functionality actually works
Improve the docuemntation

Signed-off-by: Jesper Brynolf [email protected]

ionut-arm · 2021-10-19T14:58:54Z

I have one fairly generic question which is: why not put the contents of pcr.rs in pcr/mod.rs?

Superhepper · 2021-10-20T05:56:56Z

I have one fairly generic question which is: why not put the contents of pcr.rs in pcr/mod.rs?

It is just a one of my own coding style rules to only have documentation and mod statements in mod.rs files. So if I need to create a mod.rs files with code in it I use the other way to create modules.

wiktor-k · 2021-10-20T10:04:26Z

For the record it Rust 2018 seems to be leaning on the side of avoiding generic mod.rs and prefer module names in file names: https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html#no-more-modrs

ionut-arm · 2021-10-20T10:53:13Z

Hmm, fair, maybe we should keep it consistent at least, but it's not that critical. I think despite that 2018 edition guide, even the standard library seems to alternate between the two ways of dealing with this, and so do the really popular crates. I guess in the end it'll just end up as whatever each developer prefers, don't think I'm very keen on adding rules about that for our repos.

Superhepper · 2021-10-20T11:12:11Z

I do not think its a critical thing and I do not think I would create any remark for it in a code review either because it is my personal style. Though I agree with you, I would like for it to be consistent.

ionut-arm

Thanks for the patch, left a few comments, apologies if I'm misunderstanding stuff - I'm not that knowledgeable on PCRs

tss-esapi/src/structures/lists/pcr_selection.rs

tss-esapi/src/abstraction/pcr/data.rs

ionut-arm

The fixes look good, I'm ok with the changes overall.

tss-esapi/tests/integration_tests/context_tests/tpm_commands/integrity_collection_pcr_tests.rs

Superhepper · 2021-10-25T17:01:17Z

Cool, but I need to write some more tests before I am pleased with it.

Superhepper · 2021-10-30T08:42:15Z

There is currently a bug in this that makes the pcr_bank not getting extended properly when data is added to the PcrData. And that is why the tests are failing.

Superhepper · 2021-10-30T10:12:07Z

There is currently a bug in this that makes the pcr_bank not getting extended properly when data is added to the PcrData. And that is why the tests are failing.

This has now been fixed.

Superhepper · 2021-10-31T17:16:37Z

There is currently a bug in the PR when subtracting method of the PcrSelectionList and possible also in the subtract method in the PcrSelection.

Superhepper · 2021-11-01T08:40:26Z

There is currently a bug in the PR when subtracting method of the PcrSelectionList and possible also in the subtract method in the PcrSelection.

This has now been fixed.

I am still in the process of writing more tests. Please feel free to come up with any suggestions.

ionut-arm · 2021-11-04T15:00:49Z

I am still in the process of writing more tests. Please feel free to come up with any suggestions.

I can't say I have enough knowledge on this to come up with some good test cases, maybe @lkatalin has some ideas?

Superhepper · 2021-11-16T09:10:28Z

I have not been able to come up with any additional tests and I have not heard of any bugs so far so I moved this out of WIP status.

lkatalin · 2021-11-16T15:48:32Z

Sorry I missed the ping on this and in general apologies for the silence. I have been endeavoring to get more time on this so that it does not continue to sit unmerged. I'm giving it another shot but still blocked on the ActivateCredential error at the moment.

ionut-arm · 2021-11-16T15:52:57Z

@lkatalin - apologies, similarly blocked on other things and couldn't get to poking Keylime more, to get to the bottom of the ActivateCredential issue :(

lkatalin · 2021-11-17T19:46:43Z

@ionut-arm Thanks so much for the fix in #292 ! With that out of the way, I should be able to test this tomorrow.

lkatalin · 2021-11-18T23:34:16Z

I'm able to make progress now that the ActivateCredential error is resolved, but testing this is requiring me to make some non-trivial changes to our code - I should have something working and have actual feedback by sometime next week. But if you need to merge in the meantime, please go ahead.

ionut-arm · 2021-11-26T16:27:29Z

@Superhepper - can this be merged after the other PRs for adding other abstractions? I would've thought so, just wanted to make sure

Superhepper · 2021-11-26T17:02:33Z

Yeah, I think so. If nothing comes up that it contains a major bug or something.

ionut-arm

I'm ok with this being merged as is, and if any bug reports/feedback comes back we can fold that in as well.

ionut-arm · 2021-11-16T15:56:40Z

tss-esapi/src/structures/pcr/selection.rs

+    /// OBS! This is not the same as how many [PcrSlot]
+    /// there are in the selection.


Could you expand this to describe what it does mean?

Better now?

lkatalin · 2021-12-03T02:56:08Z

@ionut-arm @Superhepper Finally finished rebasing our code on this and can confirm at least with some preliminary commands this works on our end. 👍 Huge thanks for this work and sorry for the wait.

wiktor-k

Given the comments on the other PR shouldn't we consider AsRef/Deref on a couple of types (e.g. DigestList)? Thoughts? CC @ionut-arm

tss-esapi/src/abstraction/pcr/bank.rs

tss-esapi/src/context/tpm_commands/enhanced_authorization_ea_commands.rs

tss-esapi/src/structures/pcr/selection.rs

- Changed pcr_read to return a DigestList instead of PcrData. - Moved PcrData to abstractions and made it possible to construct PcrData from rust native types. - Added subtract to PcrSelectionList. - Added a pcr_read_all functiion to the pcr module in abstractions. This function tries to read all the values in a PcrSelectionList. - Removed lower limit for DigestList. - Improved linage between the read pcr list and the read pcr digests in tests. Signed-off-by: Jesper Brynolf <[email protected]>

wiktor-k

LGTM 👍 thanks!

wiktor-k · 2021-12-03T12:07:30Z

tss-esapi/src/abstraction/pcr/bank.rs

+
+    /// Returns true if the [PcrBank] is empty
+    pub fn is_empty(&self) -> bool {
+        self.bank.is_empty()


I think it's okay here but too bad that now we've got some types that Deref to their inner types and some that don't. But Vecs are quite a lot simpler than BTreeMaps I guess.

ionut-arm · 2021-12-03T13:55:15Z

@lkatalin - please be advised that there has been quite a rush to clean up the Context interface of FFI types, and this might mean that you'd need to make further changes when the 7.0.0 release will eventually come. The release isn't "imminent", but it's worth keeping that in mind. You can, of course, try compiling with the tip of the main branch, and that should give you a fairly accurate view.

lkatalin · 2021-12-03T14:50:27Z

@lkatalin - please be advised that there has been quite a rush to clean up the Context interface of FFI types, and this might mean that you'd need to make further changes when the 7.0.0 release will eventually come. The release isn't "imminent", but it's worth keeping that in mind. You can, of course, try compiling with the tip of the main branch, and that should give you a fairly accurate view.

@ionut-arm That's very good to know ahead of time, thank you for telling me.

Add Patrick to the contributor list

Superhepper added the enhancement New feature or request label Oct 18, 2021

Superhepper self-assigned this Oct 18, 2021

Superhepper force-pushed the pcr_read_return_types_change branch from 4850d16 to 1584a1e Compare October 19, 2021 08:48

lkatalin mentioned this pull request Oct 19, 2021

Multiple PCR reads necessary when many PCRs requested keylime/rust-keylime#234

Closed

ionut-arm reviewed Oct 20, 2021

View reviewed changes

tss-esapi/src/structures/lists/pcr_selection.rs Outdated Show resolved Hide resolved

tss-esapi/src/abstraction/pcr/data.rs Outdated Show resolved Hide resolved

tss-esapi/src/abstraction/pcr/data.rs Outdated Show resolved Hide resolved

Superhepper force-pushed the pcr_read_return_types_change branch 4 times, most recently from 5bea2ea to f9d71d1 Compare October 24, 2021 21:51

Superhepper linked an issue Oct 25, 2021 that may be closed by this pull request

Re-implement subtract functionality for PcrSelectionList #259

Closed

ionut-arm reviewed Oct 25, 2021

View reviewed changes

tss-esapi/tests/integration_tests/context_tests/tpm_commands/integrity_collection_pcr_tests.rs Show resolved Hide resolved

Superhepper force-pushed the pcr_read_return_types_change branch 2 times, most recently from 9c996e1 to 6b2b860 Compare October 25, 2021 21:53

Superhepper force-pushed the pcr_read_return_types_change branch 2 times, most recently from 13c85fc to b710312 Compare October 30, 2021 10:10

Superhepper force-pushed the pcr_read_return_types_change branch from b710312 to 895b721 Compare October 30, 2021 17:51

Superhepper force-pushed the pcr_read_return_types_change branch 2 times, most recently from f2906c5 to 33f6e52 Compare October 31, 2021 21:53

lkatalin mentioned this pull request Nov 4, 2021

Manually construct PcrData? #277

Closed

ionut-arm mentioned this pull request Nov 15, 2021

Steps to a stable 7.0.0 #290

Closed

16 tasks

Superhepper marked this pull request as ready for review November 16, 2021 09:08

Superhepper requested review from ionut-arm, puiterwijk and hug-dev November 16, 2021 09:11

Superhepper force-pushed the pcr_read_return_types_change branch from 33f6e52 to 350e227 Compare November 17, 2021 19:43

Superhepper force-pushed the pcr_read_return_types_change branch from 350e227 to 81b8d33 Compare November 20, 2021 14:32

Superhepper force-pushed the pcr_read_return_types_change branch 2 times, most recently from 9563aae to f63200e Compare November 30, 2021 16:58

ionut-arm approved these changes Dec 2, 2021

View reviewed changes

Superhepper force-pushed the pcr_read_return_types_change branch from f63200e to a7a9837 Compare December 3, 2021 06:33

Superhepper requested a review from wiktor-k December 3, 2021 06:37

wiktor-k reviewed Dec 3, 2021

View reviewed changes

Superhepper force-pushed the pcr_read_return_types_change branch from a7a9837 to afbe0e9 Compare December 3, 2021 11:12

wiktor-k self-requested a review December 3, 2021 12:03

wiktor-k approved these changes Dec 3, 2021

View reviewed changes

Superhepper merged commit 468aefb into parallaxsecond:main Dec 3, 2021

Superhepper deleted the pcr_read_return_types_change branch July 3, 2022 09:47

tgonzalezorlandoarm pushed a commit to tgonzalezorlandoarm/rust-tss-esapi that referenced this pull request Mar 14, 2024

Merge pull request parallaxsecond#281 from puiterwijk/contrib_patrick

e0d7f40

Add Patrick to the contributor list

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved return values of pcr_read. #281

Improved return values of pcr_read. #281

Superhepper commented Oct 18, 2021 •

edited

Loading

ionut-arm commented Oct 19, 2021

Superhepper commented Oct 20, 2021

wiktor-k commented Oct 20, 2021

ionut-arm commented Oct 20, 2021 •

edited

Loading

Superhepper commented Oct 20, 2021

ionut-arm left a comment

ionut-arm left a comment

Superhepper commented Oct 25, 2021

Superhepper commented Oct 30, 2021 •

edited

Loading

Superhepper commented Oct 30, 2021

Superhepper commented Oct 31, 2021

Superhepper commented Nov 1, 2021

ionut-arm commented Nov 4, 2021

Superhepper commented Nov 16, 2021

lkatalin commented Nov 16, 2021

ionut-arm commented Nov 16, 2021

lkatalin commented Nov 17, 2021

lkatalin commented Nov 18, 2021

ionut-arm commented Nov 26, 2021

Superhepper commented Nov 26, 2021

ionut-arm left a comment

ionut-arm Nov 16, 2021

Superhepper Dec 3, 2021

lkatalin commented Dec 3, 2021

wiktor-k left a comment

wiktor-k left a comment

wiktor-k Dec 3, 2021

ionut-arm commented Dec 3, 2021

lkatalin commented Dec 3, 2021

		/// OBS! This is not the same as how many [PcrSlot]
		/// there are in the selection.

Improved return values of pcr_read. #281

Improved return values of pcr_read. #281

Conversation

Superhepper commented Oct 18, 2021 • edited Loading

ionut-arm commented Oct 19, 2021

Superhepper commented Oct 20, 2021

wiktor-k commented Oct 20, 2021

ionut-arm commented Oct 20, 2021 • edited Loading

Superhepper commented Oct 20, 2021

ionut-arm left a comment

Choose a reason for hiding this comment

ionut-arm left a comment

Choose a reason for hiding this comment

Superhepper commented Oct 25, 2021

Superhepper commented Oct 30, 2021 • edited Loading

Superhepper commented Oct 30, 2021

Superhepper commented Oct 31, 2021

Superhepper commented Nov 1, 2021

ionut-arm commented Nov 4, 2021

Superhepper commented Nov 16, 2021

lkatalin commented Nov 16, 2021

ionut-arm commented Nov 16, 2021

lkatalin commented Nov 17, 2021

lkatalin commented Nov 18, 2021

ionut-arm commented Nov 26, 2021

Superhepper commented Nov 26, 2021

ionut-arm left a comment

Choose a reason for hiding this comment

ionut-arm Nov 16, 2021

Choose a reason for hiding this comment

Superhepper Dec 3, 2021

Choose a reason for hiding this comment

lkatalin commented Dec 3, 2021

wiktor-k left a comment

Choose a reason for hiding this comment

wiktor-k left a comment

Choose a reason for hiding this comment

wiktor-k Dec 3, 2021

Choose a reason for hiding this comment

ionut-arm commented Dec 3, 2021

lkatalin commented Dec 3, 2021

Superhepper commented Oct 18, 2021 •

edited

Loading

ionut-arm commented Oct 20, 2021 •

edited

Loading

Superhepper commented Oct 30, 2021 •

edited

Loading