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

fix: deterministic encode cose_sign / cose_sign1 sig_signature #135

Merged
merged 6 commits into from
Mar 10, 2023

Conversation

shizhMSFT
Copy link
Contributor

fix #119

@shizhMSFT shizhMSFT requested a review from a team February 20, 2023 12:39
@shizhMSFT shizhMSFT added this to the v1.1.0 milestone Feb 20, 2023
@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Merging #135 (ad6c208) into main (c6971fb) will increase coverage by 0.35%.
The diff coverage is 93.61%.

@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
+ Coverage   92.12%   92.48%   +0.35%     
==========================================
  Files          10       10              
  Lines         978     1024      +46     
==========================================
+ Hits          901      947      +46     
- Misses         51       52       +1     
+ Partials       26       25       -1     
Impacted Files Coverage Δ
cbor.go 87.50% <91.17%> (+3.28%) ⬆️
sign.go 90.47% <100.00%> (+1.54%) ⬆️
sign1.go 89.36% <100.00%> (+2.50%) ⬆️
headers.go 92.14% <0.00%> (-0.91%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shizhMSFT shizhMSFT marked this pull request as ready for review February 20, 2023 15:19
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Comment on lines +118 to +121
case 27:
if data[1] != 0 || data[2] != 0 || data[3] != 0 || data[4] != 0 {
return data, nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This path cannot be fully tested since it requires more than 4 GiB memory to test with, which is not allowed by GitHub Action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Captured the issue in #139

Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

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

LGTM!

@SteveLasker SteveLasker merged commit 556edd4 into veraison:main Mar 10, 2023
@shizhMSFT shizhMSFT deleted the fix_119 branch March 14, 2023 08:52
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.

Sign1 messages with a non-minimal protected header length cannot be verified
5 participants