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.

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

Keydata testsuite minor refactors and improvements and hashAlg #284

Merged
merged 7 commits into from
Apr 4, 2024

Conversation

sespiros
Copy link
Collaborator

This addresses:

Also makes hashAlg public as it will be used in a follow-up PR which will slightly rework the mockPlatformKeyDataHandler to use GCM and associated data to check expected values.

@sespiros sespiros changed the title Keydata test cleanup Keydata testsuite minor refactors and improvements and hashAlg Feb 20, 2024
@chrisccoulson chrisccoulson self-requested a review March 14, 2024 00:09
Copy link
Collaborator

@chrisccoulson chrisccoulson left a comment

Choose a reason for hiding this comment

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

Thanks - this looks ok, although it does need a rebase on master now to resolve some merge conflicts.

I notice that xerrors.Errorf is replaced with fmt.Errorf in a few places - that's ok, but I think it's probably worth another PR to drop the xerrors dependency entirely given that we depend on go1.18 which has the required functionality in the standard library.

keydata.go Outdated
@@ -189,24 +189,24 @@ type KeyDataReader interface {
ReadableName() string
}

// hashAlg corresponds to a digest algorithm.
type hashAlg crypto.Hash
// HashAlg corresponds to a digest algorithm.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth just adding a note as to why this exists (it's just a crypto.Hash that can be serialed to JSON and DER)

@sespiros sespiros force-pushed the keydata-test-cleanup branch from 03573fe to f9ce5f9 Compare March 14, 2024 15:12
@sespiros sespiros marked this pull request as ready for review March 14, 2024 15:12
@sespiros sespiros requested a review from chrisccoulson March 14, 2024 15:46
@sespiros
Copy link
Collaborator Author

Thanks - this looks ok, although it does need a rebase on master now to resolve some merge conflicts.

Rebased on latest master.

I notice that xerrors.Errorf is replaced with fmt.Errorf in a few places - that's ok, but I think it's probably worth another PR to drop the xerrors dependency entirely given that we depend on go1.18 which has the required functionality in the standard library.

I did a cleanup for xerrors in secboot (that I will push in another PR) but it is still added as an indirect dependency. I can do the same for efilib (which also uses go 1.18). go-tpm2 also has it in its go.sum although it's not using it. Any reason why go-tpm2 is still in go 1.9?

@sespiros sespiros mentioned this pull request Mar 14, 2024
@sespiros
Copy link
Collaborator Author

@chrisccoulson I also opened canonical/go-efilib#13

Copy link
Collaborator

@chrisccoulson chrisccoulson left a comment

Choose a reason for hiding this comment

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

Thanks, this looks fine to me.

@chrisccoulson chrisccoulson merged commit f53e2d4 into canonical:master Apr 4, 2024
2 checks passed
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.

2 participants