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

update: validate Tag1 Datetime CBOR object #98

Merged
merged 13 commits into from
Dec 5, 2022

Conversation

patrickzheng200
Copy link

This PR's purpose is to fix #97. Based on our spec, we will only support Tag1 Datetime in notary v2 COSE.

This PR adds validation of signing time and expiry (if exists) to make sure they are Tag1 Datetime CBOR objects. Objects other than Tag1 including Tag0 Datetime CBOR would trigger an error.

Signed-off-by: Patrick Zheng patrickzheng@microsoft.com

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@patrickzheng200 patrickzheng200 self-assigned this Nov 25, 2022
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2022

Codecov Report

Merging #98 (5627a02) into main (9b5de08) will increase coverage by 0.30%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
+ Coverage   78.56%   78.86%   +0.30%     
==========================================
  Files          28       28              
  Lines        2109     2139      +30     
==========================================
+ Hits         1657     1687      +30     
  Misses        344      344              
  Partials      108      108              
Impacted Files Coverage Δ
signature/cose/envelope.go 93.90% <85.71%> (+0.44%) ⬆️

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

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
priteshbandi
priteshbandi previously approved these changes Dec 2, 2022
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

Would rely on review from @shizhMSFT for cose related unmarshalling code.

signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
@priteshbandi priteshbandi dismissed their stale review December 2, 2022 21:23

mistakenly approved PR, there is feedback that needs to be addressed

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Show resolved Hide resolved
signature/cose/envelope.go Show resolved Hide resolved
signature/cose/envelope.go Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
signature/cose/envelope.go Outdated Show resolved Hide resolved
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

return time.Time{}, fmt.Errorf("header %q time value does not have a tag", label)
}
if rawTag.Number != 1 {
return time.Time{}, errors.New("only Tag1 Datetime CBOR object is supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return time.Time{}, errors.New("only Tag1 Datetime CBOR object is supported")
return time.Time{}, errors.New("only Tag `1` Datetime CBOR object is supported")

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

@priteshbandi priteshbandi merged commit 3022517 into notaryproject:main Dec 5, 2022
@patrickzheng200 patrickzheng200 deleted the tag1 branch December 6, 2022 00:08
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.

Tell apart Tag0 and Tag1 Datetime CBOR object during COSE verification
5 participants