Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tpm2 version 2 support #75

Closed
wants to merge 1 commit into from
Closed

Add tpm2 version 2 support #75

wants to merge 1 commit into from

Conversation

leonjia0112
Copy link
Contributor

@leonjia0112 leonjia0112 commented Jul 3, 2019 •
edited
Loading

Implemented a new version of TPM2 in Rust.
This has a TPM2 object that has fields for each instance:

  • tpmdata
  • algorithms
  • legacy_tools

By using this object, we can certainly maintain an TPM object instance in the agent server instead of run tpm functions maintain the file data file. This implementation has merged with the main function. Looking for comment/feedback/suggestion. Thanks.

@jetwhiz @lukehinds @mbestavros @frozencemetery

lukehinds
lukehinds requested changes Jul 4, 2019
View reviewed changes
Copy link
Member

@lukehinds lukehinds left a comment

Choose a reason for hiding this comment

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

Looks ok to me @leonjia0112 , I will see if Robbie / Nabil or Charlie have any reviews before merging.

How are you testing this code (outside of unit tests), did you check these against an emulator?

src/tpm2.rs Outdated Show resolved Hide resolved
@leonjia0112
Copy link
Contributor Author

Looks ok to me @leonjia0112 , I will see if Robbie / Nabil or Charlie have any reviews before merging.

How are you testing this code (outside of unit tests), did you check these against an emulator?

I think I am going to design a test suite in an emulator environment.

@leonjia0112 leonjia0112 force-pushed the add_tpm2_v2_support branch from e4691f7 to 2dc3768 Compare July 5, 2019 14:15
frozencemetery
frozencemetery suggested changes Jul 5, 2019
View reviewed changes
Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

CI not passing; will re-review once it does.

@leonjia0112 leonjia0112 force-pushed the add_tpm2_v2_support branch from 2dc3768 to 167cad6 Compare July 8, 2019 17:16
@leonjia0112 leonjia0112 changed the title Add tpm 2 version support Add tpm2 version 2 support Jul 8, 2019
@leonjia0112
Copy link
Contributor Author

CI passed.

@leonjia0112 leonjia0112 mentioned this pull request Jul 9, 2019
Closed
frozencemetery
frozencemetery suggested changes Jul 9, 2019
View reviewed changes
src/crypto.rs Outdated Show resolved Hide resolved
src/main.rs
@@ -23,6 +23,7 @@ mod common;
mod crypto;
mod secure_mount;
mod tpm;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove the tpm mod as part of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ultimately, this should be removed. But if this is removed, for now, the tpm functions in the main function will not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have modules tpm and tpm2. I don't understand why we wouldn't only have one or the other.

Copy link
Member

Choose a reason for hiding this comment

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

@leonjia0112 what are the dependencies from tpm.rs -> main?

I am not sure why you did not resolve them as part o this patch.

src/tpm2.rs Show resolved Hide resolved
src/tpm2.rs Show resolved Hide resolved
src/tpm2.rs Outdated Show resolved Hide resolved
src/tpm2.rs Show resolved Hide resolved
src/tpm2.rs Outdated Show resolved Hide resolved
src/tpm2.rs Outdated Show resolved Hide resolved
src/tpm2.rs Show resolved Hide resolved
src/tpm2.rs Outdated Show resolved Hide resolved
lukehinds
lukehinds requested changes Jul 10, 2019
View reviewed changes
src/tpm2.rs Outdated Show resolved Hide resolved
@frozencemetery
Copy link
Contributor

@leonjia0112 I think you may have neglected to upload a new revision of this PR? The things you have marked as removed/resolved are not removed/resolved in the latest version on here.

@lukehinds lukehinds mentioned this pull request Jul 24, 2019
Closed
10 tasks
@lukehinds lukehinds mentioned this pull request Jul 31, 2019
Closed
10 tasks
@leonjia0112 leonjia0112 force-pushed the add_tpm2_v2_support branch from 167cad6 to 75c9e30 Compare July 31, 2019 14:36
@leonjia0112 leonjia0112 force-pushed the add_tpm2_v2_support branch from 75c9e30 to ded88af Compare July 31, 2019 14:54
@atothRedHat atothRedHat mentioned this pull request Aug 7, 2019
Closed
10 tasks
@lukehinds lukehinds mentioned this pull request Aug 27, 2019
Closed
11 tasks
This was referenced Sep 4, 2019
Closed
Closed
@lukehinds lukehinds mentioned this pull request Sep 18, 2019
Closed
11 tasks
@lukehinds
Copy link
Member

I think with tpm 2.0 being the only version we support, you could rename tpm2 to tpm.

will review other code shortly.

@atothRedHat atothRedHat mentioned this pull request Sep 25, 2019
Closed
11 tasks
@lukehinds lukehinds mentioned this pull request Oct 9, 2019
Closed
10 tasks
lukehinds
lukehinds reviewed Oct 11, 2019
View reviewed changes
Copy link
Member

@lukehinds lukehinds left a comment •
edited
Loading

Choose a reason for hiding this comment

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

void comment (my mistake).

@lukehinds
Copy link
Member

We should wait for this keylime/keylime#188 to finish its course first, so we know the exact syntax to use for tpm2_* commands:

@lukehinds lukehinds mentioned this pull request Oct 16, 2019
Closed
10 tasks
@lukehinds lukehinds mentioned this pull request Oct 23, 2019
Closed
10 tasks
@lukehinds lukehinds mentioned this pull request Jan 22, 2020
Closed
6 tasks
@lukehinds
Copy link
Member

closing as we will now integrate directly with the ESAPI

@lukehinds lukehinds closed this Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukehinds lukehinds lukehinds requested changes

@frozencemetery frozencemetery frozencemetery requested changes

Assignees
No one assigned
Labels
needs revision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leonjia0112 @frozencemetery @lukehinds