Added support for attaching Time stamp authority Response in attach command #3001

Mukuls77 · 2023-05-23T12:21:07Z

Summary

The PR provide the support to attach Timestamp authority response for signed signature time stamp in attach command.
using this functionality now we can attach a TSR response for the generated signature and attach that along with the associated certificates and certificate chain. This PR will help specifically in private CA cases where now Private CA can issue short lived certificates and attach the Time stamp of the signature. This will help in checking the validity of the associated certificates using the signature time stamp

Release Note

Following files have been modified for the PR.
cmd/cosign/cli/attach.go
cmd/cosign/cli/attach/sig.go
cmd/cosign/cli/options/attach.go
doc/cosign_attach_signature.md
test/e2e_test.go
test/e2e_test_attach.sh
test/testdata/test_attach_freetsacacert.pem
Add a release note for each of the following conditions:

Documentation

This PR introduce a new parameter that can be used in the Attach command to pass the path of the Time stamp Authority response.
The updated command help is as below

$ ./cosign attach signature --help
Attach signatures to the supplied container image

Usage:
cosign attach signature [flags]

Examples:
cosign attach signature

Flags:
--allow-http-registry whether to allow using HTTP protocol while connecting to registries. Don't use this for anything but testing
--allow-insecure-registry whether to allow insecure connections to registries (e.g., with expired or self-signed TLS certificates). Don't use this for anything but testing
--attachment-tag-prefix [AttachmentTagPrefix]sha256-[TargetImageDigest].[AttachmentName] optional custom prefix to use for attached image tags. Attachment images are tagged as: [AttachmentTagPrefix]sha256-[TargetImageDigest].[AttachmentName]
--certificate string path to the X.509 certificate in PEM format to include in the OCI Signature
--certificate-chain string path to a list of CA X.509 certificates in PEM format which will be needed when building the certificate chain for the signing certificate. Must start with the parent intermediate CA certificate of the signing certificate and end with the root certificate. Included in the OCI Signature
-h, --help help for signature
--k8s-keychain whether to use the kubernetes keychain instead of the default keychain (supports workload identity).
--payload string path to the payload covered by the signature
--signature string path to the signature, or {-} for stdin
--timeStampedSignatureResponse string path to the Time Stamped Signature Response from RFC3161 compliant TSA

Global Flags:
--output-file string log output to a file
-t, --timeout duration timeout for commands (default 3m0s)
-d, --verbose log debug output

Testing

Make , lint , Docgen test were done. also e2e test related to attach command were done. attached are the logs
Build_Lint_Docgen_test_logs.txt

e2e_attach_test_logs.txt

…ommand Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

codecov · 2023-05-23T12:25:03Z

Codecov Report

Merging #3001 (2b4c9b3) into main (f21081a) will increase coverage by 0.70%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3001      +/-   ##
==========================================
+ Coverage   30.25%   30.95%   +0.70%     
==========================================
  Files         151      153       +2     
  Lines        9473     9678     +205     
==========================================
+ Hits         2866     2996     +130     
- Misses       6162     6224      +62     
- Partials      445      458      +13

Impacted Files	Coverage Δ
cmd/cosign/cli/attach.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/options/attach.go	`0.00% <0.00%> (ø)`

... and 11 files with indirect coverage changes

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

haydentherapper

LGTM on the feature, just a comment on flag name and testing.

haydentherapper · 2023-05-30T02:18:28Z

test/e2e_test_attach.sh

@@ -46,6 +47,10 @@ IMAGE_URI_DIGEST=$IMAGE_URI@$SRC_DIGEST

 ## Sign with Leafcert Private Key
 openssl dgst -sha256 -sign ./private_key -out payload.sig payload.json
+## Generate TSR for the signature
+openssl ts -query -data payload.sig -sha256 -cert -out payload.tsq
+curl -H "Content-Type: application/timestamp-query" --data-binary '@payload.tsq' https://freetsa.org/tsr > payload.tsr


Can we use the libraries in the sigstore/timestamp-authority repo to generate a timestamp (see unit tests, there should be some examples), or stand up an instance of the tsa for an e2e test (also there should be examples of such). We shouldn’t call out to a third party service as part of our tests.

I tried to check the instance in the existing e2e test cases where we have used a separate instance of the tsa, but was not able to find any example. In the unit test i could see there are many instance of test cases covering the TSA testing and verification. so i think adding a new unit test case for attach case will not add up much functionality as in unit test case we directly modify the signature structure to include cert, cert-chain and tsa.
If it is ok with you i can remove the changes i did to introduce TSA functionality in attach test case, as it is already covered in unit test of sign and verify.

This test has an example of spinning up a local server - https://github.com/sigstore/cosign/blob/main/test/e2e_test.go#L724

Hi Hayden i have created a new test cases for using TSR in attach command in e2e_test.go file also i have reverted the changes in e2e_test_attach.sh file which were using freetsa for getting TSR. kindly review the changes,

haydentherapper · 2023-05-30T02:19:27Z

cmd/cosign/cli/options/attach.go

@@ -54,6 +55,8 @@ func (o *AttachSignatureOptions) AddFlags(cmd *cobra.Command) {
 			"when building the certificate chain for the signing certificate. "+
 			"Must start with the parent intermediate CA certificate of the "+
 			"signing certificate and end with the root certificate. Included in the OCI Signature")
+	cmd.Flags().StringVar(&o.TimeStampedSig, "timeStampedSignatureResponse", "",


Can we use an acronym “—tsr” for the flag name instead?

Agree i will change the flag name from --timeStampedSignatureResponse to --tsr

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

…cosign into Mukuls77-AddCmd-Branch

malancas

This LGTM now that the test uses sigstore/timestamp-authority libraries but I'll wait for @haydentherapper to give this another look as well.

Mukuls77 · 2023-06-03T03:54:44Z

Thanks @malancas for the review @haydentherapper can you pls also review the PR changes

haydentherapper · 2023-06-05T16:44:25Z

test/testdata/test_attach_freetsacacert.pem

Can you delete this cert?

haydentherapper · 2023-06-05T16:47:49Z

test/e2e_test.go

+	pemleafRef := mkfile(string(pemLeaf), td, t)
+	pemrootRef := mkfile(string(pemRoot), td, t)
+
+	certchainRef := mkfile(string(appendSlices([][]byte{pemSub, pemRoot})), td, t)


nit: Can you just inline the concatenation of these two, string(append(pemSub, pemRoot...)) and remove appendSlices given it's not used elsewhere?

haydentherapper

Looks great! Just two small comments, thanks for updating the tests.

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

Mukuls77 · 2023-06-06T04:45:53Z

Thanks @haydentherapper for the review i have done the required changes pls check

Added support for attaching Time stamp authority Response in attach c…

0d0e92e

…ommand Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

Mukuls77 mentioned this pull request May 23, 2023

Enhancing cosing attach signature command to include RFC3161 Timestamp response #2979

Closed

MukulSharma77 added 2 commits May 23, 2023 18:28

Fixed white space issue

b251413

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

Fixed Lint issue gofmt -s error

6c5a0f8

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

haydentherapper reviewed May 30, 2023

View reviewed changes

MukulSharma77 added 5 commits June 1, 2023 12:51

updated timestampResponse argument to --tsr

bdb68c4

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

updated timestampResponse argument to --tsr

10034ab

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

added e2e test case for attach with rfc3161 time stamp response

11febb0

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

reverted the changes for using freetsa in attach test

7df1a7f

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

Merge branch 'Mukuls77-AddCmd-Branch' of https://github.com/Mukuls77/…

1ccfce0

…cosign into Mukuls77-AddCmd-Branch

malancas reviewed Jun 1, 2023

View reviewed changes

haydentherapper reviewed Jun 5, 2023

View reviewed changes

test/testdata/test_attach_freetsacacert.pem Outdated

Copy link

Contributor

haydentherapper Jun 5, 2023

Choose a reason for hiding this comment

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

Can you delete this cert?

haydentherapper reviewed Jun 5, 2023

View reviewed changes

removed unused cert and implemented review comments

2b4c9b3

Signed-off-by: Mukuls77 <mukul.sharma@nokia.com>

haydentherapper approved these changes Jun 6, 2023

View reviewed changes

haydentherapper merged commit 1894263 into sigstore:main Jun 6, 2023
26 checks passed

github-actions bot added this to the v1.14.0 milestone Jun 6, 2023

Mukuls77 deleted the Mukuls77-AddCmd-Branch branch June 6, 2023 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for attaching Time stamp authority Response in attach command #3001

Added support for attaching Time stamp authority Response in attach command #3001

Mukuls77 commented May 23, 2023

codecov bot commented May 23, 2023 •

edited

haydentherapper left a comment

haydentherapper May 30, 2023

Mukuls77 May 30, 2023 •

edited

haydentherapper May 30, 2023

Mukuls77 Jun 1, 2023

haydentherapper May 30, 2023

Mukuls77 May 30, 2023

malancas left a comment •

edited

Mukuls77 commented Jun 3, 2023

haydentherapper Jun 5, 2023

haydentherapper Jun 5, 2023

haydentherapper left a comment

Mukuls77 commented Jun 6, 2023

Added support for attaching Time stamp authority Response in attach command #3001

Added support for attaching Time stamp authority Response in attach command #3001

Conversation

Mukuls77 commented May 23, 2023

Summary

Release Note

Documentation

Testing

codecov bot commented May 23, 2023 • edited

Codecov Report

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper May 30, 2023

Choose a reason for hiding this comment

Mukuls77 May 30, 2023 • edited

Choose a reason for hiding this comment

haydentherapper May 30, 2023

Choose a reason for hiding this comment

Mukuls77 Jun 1, 2023

Choose a reason for hiding this comment

haydentherapper May 30, 2023

Choose a reason for hiding this comment

Mukuls77 May 30, 2023

Choose a reason for hiding this comment

malancas left a comment • edited

Choose a reason for hiding this comment

Mukuls77 commented Jun 3, 2023

haydentherapper Jun 5, 2023

Choose a reason for hiding this comment

haydentherapper Jun 5, 2023

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

Mukuls77 commented Jun 6, 2023

codecov bot commented May 23, 2023 •

edited

Mukuls77 May 30, 2023 •

edited

malancas left a comment •

edited