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: enabled cose to verify time in Tag1 Datetime format #95

Merged
merged 10 commits into from
Nov 23, 2022

Conversation

patrickzheng200
Copy link

@patrickzheng200 patrickzheng200 commented Nov 18, 2022

This PR enables COSE to verify time in Tag1 Datetime format.

Some details that worth to be mentioned:

Sign (Encode/MarshalCBOR):
in go-cose, time.Time will be encoded as numerical (in integer) representation of seconds since January 1, 1970 UTC.
For more details: https://github.com/fxamacker/cbor/blob/7704fa5efaf3ef4ac35aff38f50f6ff567793072/encode.go#L75
https://github.com/veraison/go-cose/blob/f72b6bc06b90d1e9c07ab11332df92c7838e3027/cbor.go#L30

Verify (Decode/UnmarshalCBOR):

  1. CBOR times (tag 0 and 1) decode to time.Time. (This applies to COSE signatures provided by PRSS/ESRP.)
  2. decode CBOR uint/int to Go int64. (This applies to signatures signed by Notation Sign. Although, user passes time.Time into signRequest in Notation Sign, after MarshalCBOR (during Sign) and UnmarshalCBOR (during Verify), this input time.Time is actually converted to an int64 by go-cose.)
    For more details: https://github.com/fxamacker/cbor/blob/7704fa5efaf3ef4ac35aff38f50f6ff567793072/decode.go#L52
    https://github.com/veraison/go-cose/blob/f72b6bc06b90d1e9c07ab11332df92c7838e3027/cbor.go#L43

Edit:
After discussion with @shizhMSFT, we decide to enforce Tag1 Datetime on both Sign and Verify. In other words, during Sign, we will create a Tag1 Datetime CBOR object and save it into cbor.ProtectedHeader. During verify, the Tag1 Datetime will be decoded as time.Time in go.

Signed-off-by: Patrick Zheng [email protected]

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #95 (88ad5d0) into main (cd56ef6) will decrease coverage by 0.14%.
The diff coverage is 77.96%.

@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
- Coverage   78.76%   78.61%   -0.15%     
==========================================
  Files          28       28              
  Lines        2067     2114      +47     
==========================================
+ Hits         1628     1662      +34     
- Misses        336      344       +8     
- Partials      103      108       +5     
Impacted Files Coverage Δ
signature/internal/base/envelope.go 97.72% <ø> (ø)
signature/cose/envelope.go 93.54% <77.96%> (-2.69%) ⬇️

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

@patrickzheng200 patrickzheng200 linked an issue Nov 18, 2022 that may be closed by this pull request
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
@yizha1 yizha1 added this to the RC-1 milestone Nov 18, 2022
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 <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Copy link
Contributor

@JeyJeyGao JeyJeyGao 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

@rgnote rgnote left a comment

Choose a reason for hiding this comment

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

LGTM

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 Outdated Show resolved Hide resolved
Signed-off-by: Patrick Zheng <[email protected]>
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
Signed-off-by: Patrick Zheng <[email protected]>
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 <[email protected]>
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

@shizhMSFT shizhMSFT merged commit 9b5de08 into notaryproject:main Nov 23, 2022
@patrickzheng200 patrickzheng200 deleted the signingTime branch November 23, 2022 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

update COSE signingTime/authenticSigningTime/expiry type
8 participants