feat: Timestamp #418

Two-Hearts · 2024-06-27T01:06:10Z

This PR adds timestamping support.

It depends on PRs: notaryproject/notation-core-go#207

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

codecov · 2024-06-27T01:08:07Z

Codecov Report

Attention: Patch coverage is 81.73077% with 38 lines in your changes missing coverage. Please review.

Project coverage is 80.46%. Comparing base (9ff1891) to head (5aeef68).
Report is 25 commits behind head on main.

Files	Patch %	Lines
verifier/verifier.go	78.44%	31 Missing and 5 partials ⚠️
verifier/helpers.go	94.11%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #418      +/-   ##
==========================================
+ Coverage   73.43%   80.46%   +7.03%     
==========================================
  Files          24       33       +9     
  Lines        2507     2488      -19     
==========================================
+ Hits         1841     2002     +161     
+ Misses        526      346     -180     
  Partials      140      140

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

shizhMSFT · 2024-07-04T05:08:49Z

.github/workflows/build.yml

Should this part be covered by #419?

shizhMSFT · 2024-07-04T05:10:41Z

example_signWithTimestmap_test.go

+	// exampleRFC3161TSAServer is the URL of a valid example TSA server
+	var exampleRFC3161TSAServer = "http://timestamp.digicert.com"
+
+	// exampleTSARootCertPem is the example TSA's root certificate
+	var exampleTSARootCertPem = `-----BEGIN CERTIFICATE-----
+MIIFkDCCA3igAwIBAgIQBZsbV56OITLiOQe9p3d1XDANBgkqhkiG9w0BAQwFADBi
+MQswCQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3
+d3cuZGlnaWNlcnQuY29tMSEwHwYDVQQDExhEaWdpQ2VydCBUcnVzdGVkIFJvb3Qg
+RzQwHhcNMTMwODAxMTIwMDAwWhcNMzgwMTE1MTIwMDAwWjBiMQswCQYDVQQGEwJV
+UzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3d3cuZGlnaWNlcnQu
+Y29tMSEwHwYDVQQDExhEaWdpQ2VydCBUcnVzdGVkIFJvb3QgRzQwggIiMA0GCSqG
+SIb3DQEBAQUAA4ICDwAwggIKAoICAQC/5pBzaN675F1KPDAiMGkz7MKnJS7JIT3y
+ithZwuEppz1Yq3aaza57G4QNxDAf8xukOBbrVsaXbR2rsnnyyhHS5F/WBTxSD1If
+xp4VpX6+n6lXFllVcq9ok3DCsrp1mWpzMpTREEQQLt+C8weE5nQ7bXHiLQwb7iDV
+ySAdYyktzuxeTsiT+CFhmzTrBcZe7FsavOvJz82sNEBfsXpm7nfISKhmV1efVFiO
+DCu3T6cw2Vbuyntd463JT17lNecxy9qTXtyOj4DatpGYQJB5w3jHtrHEtWoYOAMQ
+jdjUN6QuBX2I9YI+EJFwq1WCQTLX2wRzKm6RAXwhTNS8rhsDdV14Ztk6MUSaM0C/
+CNdaSaTC5qmgZ92kJ7yhTzm1EVgX9yRcRo9k98FpiHaYdj1ZXUJ2h4mXaXpI8OCi
+EhtmmnTK3kse5w5jrubU75KSOp493ADkRSWJtppEGSt+wJS00mFt6zPZxd9LBADM
+fRyVw4/3IbKyEbe7f/LVjHAsQWCqsWMYRJUadmJ+9oCw++hkpjPRiQfhvbfmQ6QY
+uKZ3AeEPlAwhHbJUKSWJbOUOUlFHdL4mrLZBdd56rF+NP8m800ERElvlEFDrMcXK
+chYiCd98THU/Y+whX8QgUWtvsauGi0/C1kVfnSD8oR7FwI+isX4KJpn15GkvmB0t
+9dmpsh3lGwIDAQABo0IwQDAPBgNVHRMBAf8EBTADAQH/MA4GA1UdDwEB/wQEAwIB
+hjAdBgNVHQ4EFgQU7NfjgtJxXWRM3y5nP+e6mK4cD08wDQYJKoZIhvcNAQEMBQAD
+ggIBALth2X2pbL4XxJEbw6GiAI3jZGgPVs93rnD5/ZpKmbnJeFwMDF/k5hQpVgs2
+SV1EY+CtnJYYZhsjDT156W1r1lT40jzBQ0CuHVD1UvyQO7uYmWlrx8GnqGikJ9yd
+SeuMIW59mdNOj6PWTkiU0TryF0Dyu1Qen1iIQqAyHNm0aAFYF/opbSnr6j3bTWc
+fFqK1qI4mfN4i/RN0iAL3gTujJtHgXINwBQy7zBZLq7gcfJW5GqXb5JQbZaNaHqa
+sjYUegbyJLkJEVDXCLG4iXqEI2FCKeWjzaIgQdfRnGTZ6iahixTXTBmyUEFxPT9N
+cCOGDErcgdLMMpSEDQgJlxxPwO5rIHQw0uA5NBCFIRUBCOhVMt5xSdkoF1BN5r5N
+0XWs0Mr7QbhDparTwwVETyw2m+L64kW4I1NsBm9nVX9GtUw/bihaeSbSpKhil9Ie
+4u1Ki7wb/UdKDd9nZn6yW0HQO+T0O/QEY+nvwlQAUaCKKsnOeMzV6ocEGLPOr0mI
+r/OSmbaz5mEP0oUA51Aa5BuVnRmhuZyxm7EAHu/QD09CbMkKvO5D+jpxpchNJqU1
+/YldvIViHTLSoCtU7ZpXwdv6EM8Zt4tKG48BtieVU+i2iW1bvGjUI+iLUaJW+fCm
+gKDWHrO8Dw9TdSmq6hN35N6MgSGtBxBHEa2HPQfRdbzP82Z+
+-----END CERTIFICATE-----`


Can you move this block of code out of the example?

shizhMSFT · 2024-07-04T05:11:35Z

example_signWithTimestmap_test.go

+// user trusted TSA root certificate
+func Example_signWithTimestamp() {
+	// exampleRFC3161TSAServer is the URL of a valid example TSA server
+	var exampleRFC3161TSAServer = "http://timestamp.digicert.com"


A mock server should be used. Otherwise, the runnable test depends on the availability of the digicert server.

shizhMSFT · 2024-07-04T05:11:57Z

go.mod

 	github.com/go-asn1-ber/asn1-ber v1.5.5 // indirect
 	github.com/golang-jwt/jwt/v4 v4.5.0 // indirect
 	github.com/google/uuid v1.6.0 // indirect
 	github.com/x448/float16 v0.8.4 // indirect
 	golang.org/x/sync v0.6.0 // indirect
 )
+
+replace github.com/notaryproject/notation-core-go => github.com/Two-Hearts/notation-core-go v0.0.0-20240703022152-7f0c50591e18


Put a comment here as a reminder to remove this line.

shizhMSFT · 2024-07-05T06:37:28Z

signer/signer.go

@@ -154,8 +159,6 @@ func (s *GenericSigner) Sign(ctx context.Context, desc ocispec.Descriptor, opts
 	if err := envelope.ValidatePayloadContentType(&envContent.Payload); err != nil {
 		return nil, nil, err
 	}
-
-	// TODO: re-enable timestamping https://github.com/notaryproject/notation-go/issues/78


Do you want to mention #78 in this PR description?

shizhMSFT · 2024-07-05T06:47:10Z

verifier/verifier.go

+	revocationTimestampClient := verifierOptions.RevocationTimestampClient
+	if revocationTimestampClient == nil {
+		var err error
+		revocationTimestampClient, err = revocation.NewTimestamp(&http.Client{Timeout: 5 * time.Second})


Where does the "5 seconds" come from?

shizhMSFT · 2024-07-05T06:52:59Z

verifier/verifier.go


+	// under signing scheme notary.x509
 	if signerInfo := outcome.EnvelopeContent.SignerInfo; signerInfo.SignedAttributes.SigningScheme == signature.SigningSchemeX509 {


The function verifyAuthenticTimestamp is huge now. Can you at least refactor the code of the path signerInfo.SignedAttributes.SigningScheme == signature.SigningSchemeX509 out?

It will also simplify the error processing.

shizhMSFT · 2024-07-05T06:55:51Z

verifier/verifier.go

+			timestampLowerLimit := time.Now()
+			timestampUpperLimit := timestampLowerLimit


Why do we need two variables with the same value?

shizhMSFT · 2024-07-05T07:03:20Z

verifier/verifier.go

+				Action: outcome.VerificationLevel.Enforcement[trustpolicy.TypeAuthenticTimestamp],
+			}
+		}
+		if len(trustTSACerts) < 1 {


nit: reduce mental strain for reviewers

Suggested change

if len(trustTSACerts) < 1 {

if len(trustTSACerts) == 0 {

shizhMSFT · 2024-07-05T07:06:31Z

verifier/verifier.go

+		}
+		// 5. Check the timestamp against the signing certificate chain
+		logger.Info("Checking the timestamp against the signing certificate chain...")
+		logger.Infof("Timestamp range: [%v, %v]", timestamp.Value.Add(-timestamp.Accuracy), timestamp.Value.Add(timestamp.Accuracy))


Looks like the Timestamp struct needs a String() method.

Two-Hearts added 30 commits December 1, 2023 11:56

initial test

4b42738

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

fix trust store

ca5bba2

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

resolved conflicts

73ec77c

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

Merge branch 'notaryproject:main' into tsa

2937f68

Merge branch 'notaryproject:main' into tsa

f06d289

Merge branch 'notaryproject:main' into tsa

9819ba2

initial commits

8e6bbdb

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

initial commits

b454646

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

Merge branch 'notaryproject:main' into tsa

d058046

Merge branch 'notaryproject:main' into tsa

1df563c

updated to use tspclient-go

9cd5aaf

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

resolved conflicts

ef01821

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

Merge branch 'notaryproject:main' into tsa

e655c04

updated tspclient-go

ea97a99

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

test

fa4eaaf

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

update

dc9c5fe

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

Merge branch 'notaryproject:main' into tsa

00d7dfd

resolved conflicts

62bacd0

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

Merge branch 'notaryproject:main' into tsa

1f4bd5c

update sign with timestamping

3b98cd7

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

resolved conflicts

09810a9

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

update timestamp verification

e598416

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

updated timestmap

be2ea0e

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

updated timestamp

c8f8e4e

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

resolve conflicts

28e0166

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

added tsa cert chain revocation check

200a071

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

Merge branch 'notaryproject:main' into tsa

6ab5a0e

update tsa

53ee15c

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

added tsa ocsp check

e5aaefe

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

updated timestamping

2e674e4

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

Two-Hearts added 8 commits June 25, 2024 14:42

clean up

cfe3428

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

clean up

573f592

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

fixed tests

3ad1246

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

update timestamp

ac78ce1

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

updated log

925801f

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

added exmaple tests for timestamp

c1621e2

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

test

f987d28

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

clean up

12b393e

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

Two-Hearts requested review from gokarnm, JeyJeyGao, NiazFK, priteshbandi, rgnote, shizhMSFT, toddysm, vaninrao10 and yizha1 as code owners June 27, 2024 01:06

Two-Hearts mentioned this pull request Jun 27, 2024

feat: Timestamp notaryproject/notation#978

Open

Two-Hearts changed the title ~~feat: add Timestamp~~ feat: add Timestamping Jun 27, 2024

Two-Hearts changed the title ~~feat: add Timestamping~~ feat: Timestamp Jun 27, 2024

Two-Hearts closed this Jun 27, 2024

Two-Hearts force-pushed the tsa branch from 12b393e to a1825db Compare June 27, 2024 08:55

resolved conflicts

bf74212

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

Two-Hearts reopened this Jun 27, 2024

Two-Hearts added 4 commits June 27, 2024 18:25

fix tests

13006ce

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

updated timestamping

938762e

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

added more tests

d94d7b3

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

fixed build

5aeef68

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

shizhMSFT reviewed Jul 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Timestamp #418

feat: Timestamp #418

Two-Hearts commented Jun 27, 2024 •

edited

Loading

codecov bot commented Jun 27, 2024 •

edited

Loading

shizhMSFT Jul 4, 2024

shizhMSFT Jul 4, 2024

shizhMSFT Jul 4, 2024

shizhMSFT Jul 4, 2024

shizhMSFT Jul 5, 2024

shizhMSFT Jul 5, 2024

shizhMSFT Jul 5, 2024

shizhMSFT Jul 5, 2024

shizhMSFT Jul 5, 2024

shizhMSFT Jul 5, 2024

shizhMSFT Jul 5, 2024


		// under signing scheme notary.x509
		if signerInfo := outcome.EnvelopeContent.SignerInfo; signerInfo.SignedAttributes.SigningScheme == signature.SigningSchemeX509 {

		timestampLowerLimit := time.Now()
		timestampUpperLimit := timestampLowerLimit

feat: Timestamp #418

Are you sure you want to change the base?

feat: Timestamp #418

Conversation

Two-Hearts commented Jun 27, 2024 • edited Loading

codecov bot commented Jun 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Two-Hearts commented Jun 27, 2024 •

edited

Loading

codecov bot commented Jun 27, 2024 •

edited

Loading