Skip to content

Add checkquote function without using the tpm2#570

Merged
wiktor-k merged 4 commits into
parallaxsecond:mainfrom
brandsimon:sbr/checkquote
Apr 15, 2025
Merged

Add checkquote function without using the tpm2#570
wiktor-k merged 4 commits into
parallaxsecond:mainfrom
brandsimon:sbr/checkquote

Conversation

@brandsimon

Copy link
Copy Markdown
Contributor

No description provided.

@brandsimon

Copy link
Copy Markdown
Contributor Author

I did not update any packages, but got the following error:

error: rustc 1.78.0 is not supported by the following package:
  base64ct@1.7.3 requires rustc 1.81

Is this expected?

@baloo

baloo commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

Not exactly a review but a couple of improvements on this PR: brandsimon#6

@baloo

baloo commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

I did not update any packages, but got the following error:

error: rustc 1.78.0 is not supported by the following package:
  base64ct@1.7.3 requires rustc 1.81

Is this expected?

#572
Something forgotten in #537

@baloo

baloo commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

Here is a followup to fix the rsa pkcs1v15 signature check: brandsimon#7

Comment thread tss-esapi/src/utils/mod.rs Outdated
Comment thread tss-esapi/src/utils/quote.rs Outdated
Comment thread tss-esapi/src/utils/quote.rs Outdated
Comment thread tss-esapi/src/utils/quote.rs
@baloo

baloo commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

You can rebase on main to fix the three tests.

@brandsimon brandsimon force-pushed the sbr/checkquote branch 6 times, most recently from 1889af6 to 2a80db7 Compare April 1, 2025 20:41
@brandsimon

Copy link
Copy Markdown
Contributor Author

@baloo Thank you very very much for your work.
The verification functions are a lot cleaner now!

I did some changes so cargo fmt and cargo clippy succeed. From my side, this is ready to merge.

@wiktor-k wiktor-k left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this looks very nice 👌 just a couple of nits/questions.

Comment thread tss-esapi/src/utils/quote.rs Outdated
Comment thread tss-esapi/src/utils/quote.rs Outdated
Comment thread tss-esapi/src/utils/quote.rs Outdated
Comment thread tss-esapi/src/utils/quote.rs Outdated
Comment thread tss-esapi/src/utils/quote.rs Outdated
@brandsimon

Copy link
Copy Markdown
Contributor Author

@wiktor-k
Thank you, all done!
The clippy failures are also on main and I think because of a new version of clippy.

@brandsimon

Copy link
Copy Markdown
Contributor Author

I fixed the clippy issues in #575
Once merged, I rebase onto main.

@Superhepper Superhepper left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is my personal preference to try to avoid names like common, utils, e.t.c because they tend to end up becoming a garbage can full of "nice to have things". And it has been one of my personal goals to try to get rid of the utils module once and for all.

With that said. I would much rather have this put under abstraction. And if you want to be really specific about it you could even create a sub module in there called no_tpm.

hrmm or maybe a no_tpm module along side abstraction would be best. Because I guess it will contain more things that want to used the TPM structures for compatibility reasons.

@wiktor-k

Copy link
Copy Markdown
Collaborator

It is my personal preference to try to avoid names like common, utils, e.t.c because they tend to end up becoming a garbage can full of "nice to have things". And it has been one of my personal goals to try to get rid of the utils module once and for all.

FWIW I agree with this perspective and also don't like utils, helpers, etc.

brandsimon and others added 3 commits April 12, 2025 09:56
Signed-off-by: Simon Brand <simon.brand@postadigitale.de>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Simon Brand <simon.brand@postadigitale.de>
@brandsimon brandsimon force-pushed the sbr/checkquote branch 2 times, most recently from 2f3d086 to 423ae95 Compare April 13, 2025 10:42

@wiktor-k wiktor-k left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. FWIW you can fix the sign-offs with git rebase --signoff main && git push --force-with-lease... or... well... since it's just last commit I guess git commit --amend --signoff --no-edit would work too :)

Signed-off-by: Simon Brand <simon.brand@postadigitale.de>
@brandsimon

Copy link
Copy Markdown
Contributor Author

@wiktor-k
Thank you, signed-off the last commit

@wiktor-k wiktor-k merged commit dedbdaa into parallaxsecond:main Apr 15, 2025
#[cfg(feature = "sha1")]
HashingAlgorithm::Sha1 => {
let hash = sha1::Sha1::digest(message);
Ok(verifying_key.verify_prehash(&hash, &signature).is_ok())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The use of PrehashVerifier should be swapped with DigestVerifier just so we stay off the hazmat.

RustCrypto/elliptic-curves#1146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants