doc: creating the spec for inspect command by vaninrao10 · Pull Request #490 · notaryproject/notation

vaninrao10 · 2022-12-20T22:53:32Z

Signed-off-by: vaninrao10vaninrao@amazon.com
Use Case / Scenarios:

```
Troubleshooting
```
```
Auditing
```

If verification of the image digest / signature digest fails due to wrong certificate configuration, user will execute the inspect command to extract the certificate thumb print, a hash of a certificate which is a unique identifier for certificates.

yizha1

Thanks @vaninrao10. My main concern is the human readable form of attributes, the current proposal doesn't seem fit into the tree view.

specs/commandline/inspect.md

yizha1 · 2022-12-22T01:21:00Z

specs/commandline/inspect.md

+## Outline
+
+```text
+Inspect artifacts and display the details of the signatures for all the listed signatures and its associated certificate properties.


Inspect all the signatures associated with the signed artifact.

yizha1 · 2022-12-22T01:22:14Z

specs/commandline/inspect.md

+   -p, --password string password for registry operations (default to $NOTATION_PASSWORD if not specified)
+    --plain-http registry access via plain HTTP
+   -u, --username string username for registry operations (default to $NOTATION_USERNAME if not specified)
+   -o, --output on command line sets the output to json


-o flag should be placed before -p flag.

-o, --output string output format, options: "json"

Do we need -o flag with inspect command? Are we expecting inspect command to be invoked programmatically ? If so we will also need to define output format for json output.

yizha1 · 2022-12-22T01:22:58Z

specs/commandline/inspect.md

+Flags:
+   -h, --help for describing the signature
+   -p, --password string password for registry operations (default to $NOTATION_PASSWORD if not specified)
+    --plain-http registry access via plain HTTP


The indentation is not correct. Should be aligned with --password.

yizha1 · 2022-12-22T01:24:43Z

specs/commandline/inspect.md

+Usage:
+    notation inspect [flags] <reference>
+
+Aliases:


we can remove Aliases if no alias is available.

specs/commandline/inspect.md

yizha1 · 2022-12-22T01:48:47Z

specs/commandline/inspect.md

+└── application/vnd.cncf.notary.signature
+    ├── sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+      ├──"Protected Attributes":
+      {


The indentation doesn't look right. The attributes were displayed in json format, however the whole tree view is in human readable form. It's a mix of two types of formats. We may need a new human readable form for attributes.

I would say instead of json use the indentation/format we are using for other commands

specs/commandline/inspect.md

yizha1 · 2022-12-24T03:40:07Z

Besides protected header, unprotected header and cert properties, we could also consider inspecting the payload property according to the signature spec

It is used to compare the signed payload against the descriptor of the remote artifact for troubleshooting purpose.

@vaninrao10 @iamsamirzon @priteshbandi @toddysm

specs/commandline/inspect.md

priteshbandi · 2022-12-28T07:18:40Z

specs/commandline/inspect.md

+   -p, --password string password for registry operations (default to $NOTATION_PASSWORD if not specified)
+    --plain-http registry access via plain HTTP
+   -u, --username string username for registry operations (default to $NOTATION_USERNAME if not specified)
+   -o, --output on command line sets the output to json


Do we need -o flag with inspect command? Are we expecting inspect command to be invoked programmatically ? If so we will also need to define output format for json output.

priteshbandi · 2022-12-28T08:13:00Z

specs/commandline/inspect.md

+└── application/vnd.cncf.notary.signature
+    ├── sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+      ├──"Protected Attributes":
+      {


I would say instead of json use the indentation/format we are using for other commands

priteshbandi · 2022-12-28T08:15:41Z

specs/commandline/inspect.md

+  ├──<digest_of_signature_manifest>
+    ├──<Protected Attributes...>
+    ├──<UnProtected Attributes...>
+    ├──<Cert Properties...>


I would just call it certificates

Suggested change

├──<Cert Properties...>

├──<certificates>

codecov-commenter · 2022-12-28T18:08:40Z

Codecov Report

Merging #490 (c5f63f0) into main (f83a48b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #490   +/-   ##
=======================================
  Coverage   29.57%   29.57%           
=======================================
  Files          26       26           
  Lines        1515     1515           
=======================================
  Hits          448      448           
  Misses       1050     1050           
  Partials       17       17

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

specs/notation-cli.md

specs/commandline/inspect.md

priteshbandi · 2022-12-28T19:21:34Z

specs/commandline/inspect.md

+   ├──<signed attributes>
+   ├──<unsigned attributes>
+   ├──<certificates>
+   ├──<payload>


Indentation is off

specs/commandline/inspect.md

priteshbandi · 2022-12-28T19:34:29Z

specs/commandline/inspect.md

+    ├──<user-defined attributes>
+    ├──<unsigned Attributes>
+    ├──<certificates>
+    ├──<payload>


Suggested change

├──<payload>

└──<payload>

Similar changes for other outputs

specs/commandline/inspect.md

priteshbandi · 2022-12-28T19:42:03Z

DCO check if failing

vaninrao10 · 2022-12-29T00:12:47Z

Thanks @vaninrao10. My main concern is the human readable form of attributes, the current proposal doesn't seem fit into the tree view.

Took care of it.

This has been taken care in the inspect spec for the human readable format in the output examples.

iamsamirzon · 2022-12-29T00:14:52Z

LGTM

iamsamirzon · 2022-12-29T01:23:47Z

LGTM

specs/commandline/inspect.md

iamsamirzon · 2022-12-21T16:19:24Z

specs/commandline/inspect.md

+
+Use `notation inspect` command to inspect or describe all the signatures associated to a signed artifact (image) in a human readable format.
+
+Upon successful execution,the digest of the signed artifact and details of all the signatures associated with artifact and its respective certificate properties are displayed as following:


and its respective to "their respective certificate"

specs/commandline/inspect.md

iamsamirzon · 2022-12-21T16:21:50Z

specs/commandline/inspect.md

+localhost:5000/net-monitor@sha256:b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9
+└── application/vnd.cncf.notary.signature
+    ├── sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+      ├──"Protected Attributes":


As I recollect, the idea was to include even the image digest, signature artifact type, digest of signature and protected attribute inside the JSON formatting.

specs/commandline/inspect.md

iamsamirzon · 2022-12-21T16:26:25Z

specs/commandline/inspect.md

+      {
+        "SHA1 Thumbprint":"2f1cc5b8455381cdefac83b4bd305b789cc9c16e"
+      }
+    └── sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb


Same comment as above. As I recollect one of the feedback item was to enclose everything inside JSON formatting

iamsamirzon · 2022-12-21T17:07:42Z

specs/commandline/inspect.md

+         }
+          "Certificate Properties":
+        {
+          "SHA1 Thumbprint":"2f1rr5b8455381frdajc83b4bd305b743cc9513u"


Suggest use a different name like "Certificate Thumbprint 1", "Certificate Thumbprint 2".

FeynmanZhou · 2022-12-30T07:03:10Z

specs/commandline/inspect.md

+            ├──expiry: 2022-10-06T07:01:20Z
+            ├──verification plugin: com.example.nv2plugin     //extended attributes used by Notary v2 to support plugins
+      ├──user-defined attributes:
+            ├──io.wabbit-networks.buildId: 123    //Notary v2 payload annotations is shown here has user defined metadata.


Suggested change

├──io.wabbit-networks.buildId: 123 //Notary v2 payload annotations is shown here has user defined metadata.

├──io.wabbit-networks.buildId: 123 //user-defined payload annotations

specs/commandline/inspect.md

FeynmanZhou · 2022-12-30T08:22:43Z

specs/commandline/inspect.md

+  ├──<digest_of_signature_manifest>
+    ├──<signed attributes>
+    ├──<user-defined attributes>
+    ├──<unsigned Attributes>


Suggested change

├──<unsigned Attributes>

├──<unsigned attributes>

FeynmanZhou

@vaninrao10 Please see my comments above. Thanks!

yizha1

Thanks @vaninrao10, the latest version looks much better. Exciting. New comments were provided.

specs/notation-cli.md

specs/commandline/inspect.md

specs/notation-cli.md

specs/commandline/inspect.md

shizhMSFT · 2023-01-04T08:41:05Z

specs/commandline/inspect.md

+<registry>/<repository>@<digest>
+└──application/vnd.cncf.notary.signature
+  ├──<digest_of_signature_manifest>
+    ├──<signing algorithm>
+    ├──<signed attributes>
+    ├──<user-defined attributes>
+    ├──<unsigned attributes>
+    ├──<certificates>
+    └──<payload>
+  ├──<digest_of_signature_manifest>
+    ├──<signing algorithm>
+    ├──<signed attributes>
+    ├──<unsigned attributes>
+    ├──<certificates>
+    └──<payload>


Suggested change

<registry>/<repository>@<digest>

└──application/vnd.cncf.notary.signature

├──<digest_of_signature_manifest>

├──<signing algorithm>

├──<signed attributes>

├──<user-defined attributes>

├──<unsigned attributes>

├──<certificates>

└──<payload>

├──<digest_of_signature_manifest>

├──<signing algorithm>

├──<signed attributes>

├──<unsigned attributes>

├──<certificates>

└──<payload>

<registry>/<repository>@<digest>

└── application/vnd.cncf.notary.signature

├── <digest_of_signature_manifest>

│ ├── <signing algorithm>

│ ├── <signed attributes>

│ ├── <user-defined attributes>

│ ├── <unsigned attributes>

│ ├── <certificates>

│ └── <payload>

└── <digest_of_signature_manifest>

├── <signing algorithm>

├── <signed attributes>

├── <unsigned attributes>

├── <certificates>

└── <payload>

Similar format should be applied to the examples.

specs/commandline/inspect.md

shizhMSFT · 2023-01-04T09:03:18Z

specs/commandline/inspect.md

+      ├──certificates:
+            ├──SHA1 fingerprint: 2f1cc5b8455381cdefac83b4bd305b789cc9c16e
+            ├──Issued to: Microsoft Root Certificate Authority 2010
+            ├──Issued by: Microsoft Root Certificate Authority 2010


Should this sub tree print a cert chain instead of a single cert?

shizhMSFT · 2023-01-04T09:06:21Z

specs/commandline/inspect.md

+            ├──SHA1 fingerprint: 2f1cc5b8455381cdefac83b4bd305b789cc9c16e
+            ├──Issued to: Microsoft Root Certificate Authority 2010
+            ├──Issued by: Microsoft Root Certificate Authority 2010


Valid time range is also an important field.

shizhMSFT · 2023-01-04T09:07:01Z

specs/commandline/inspect.md

+            ├──SHA1 fingerprint: 2f1cc5b8455381cdefac83b4bd305b789cc9c16e
+            ├──Issued to: Microsoft Root Certificate Authority 2010
+            ├──Issued by: Microsoft Root Certificate Authority 2010
+      ├──payload:                                                           //descriptor of the target artifact manifest that is signed.


Instead of payload, should it be called subject or target artifact?

shizhMSFT · 2023-01-04T09:09:24Z

specs/commandline/inspect.md

+        "signingScheme": "notary.default.x509",
+        "signingTime": "2022-04-06T07:01:20Z",
+        "expiry": "2022-10-06T07:01:20Z",
+        "verification plugin": "com.example.nv2plugin"


Should it be verificationPlugin?

field names should be consistent across the json view.

yizha1

@priteshbandi @vaninrao10 @FeynmanZhou @shizhMSFT I don't see any big issues for this PR, just the flag name whether it is called -o, --output, since we also plan to introduce this kind of flag to other CLI commands, it's better we can decide now. See comments #490 (comment)

specs/commandline/inspect.md

yizha1 · 2023-01-05T06:31:53Z

specs/commandline/inspect.md

+
+Flags:
+   -h, --help for describing the signature
+   -o, --output on command line sets the output to json


Since we need to add this kind of output flag for other notation cli commands, and it is on the user interface, it's better we can align what is the best solution for our users. Here are 3 options so far:

-o, --output json

`--display {tree, json) by default tree

--json (an example in https://clig.dev/#output)

Any other options, Let's vote?

yizha1 · 2023-01-05T06:41:21Z

@vaninrao10 please fix the DCO issues.

shizhMSFT

The DCO check is failing. Could you sign off all the commits?

specs/notation-cli.md

vaninrao10 requested review from FeynmanZhou, iamsamirzon and yizha1 December 20, 2022 22:56

vaninrao10 added this to the RC-2 milestone Dec 20, 2022

vaninrao10 added the documentation Improvements or additions to documentation label Dec 20, 2022

vaninrao10 self-assigned this Dec 20, 2022

vaninrao10 force-pushed the vani-branch-1 branch from 9b49985 to 629a9fe Compare December 20, 2022 23:02

yizha1 previously requested changes Dec 22, 2022

View reviewed changes

priteshbandi reviewed Dec 28, 2022

View reviewed changes

vaninrao10 force-pushed the vani-branch-1 branch 2 times, most recently from 3e9fa2d to 59aa23d Compare December 29, 2022 00:10

vaninrao10 force-pushed the vani-branch-1 branch from f5165b9 to b283e24 Compare December 29, 2022 01:14

iamsamirzon approved these changes Dec 29, 2022

View reviewed changes

FeynmanZhou reviewed Dec 30, 2022

View reviewed changes

specs/commandline/inspect.md Outdated Show resolved Hide resolved

FeynmanZhou reviewed Dec 30, 2022

View reviewed changes

specs/commandline/inspect.md Outdated Show resolved Hide resolved

FeynmanZhou reviewed Dec 30, 2022

View reviewed changes

specs/commandline/inspect.md Outdated Show resolved Hide resolved

FeynmanZhou reviewed Dec 30, 2022

View reviewed changes

yizha1 reviewed Jan 2, 2023

View reviewed changes

vaninrao10 force-pushed the vani-branch-1 branch from 2e3c130 to 469ea15 Compare January 3, 2023 16:36

vaninrao10 force-pushed the vani-branch-1 branch 2 times, most recently from 2aeb76f to ae544e4 Compare January 3, 2023 21:22

shizhMSFT changed the title ~~Creating the spec for inspect command~~ doc: creating the spec for inspect command Jan 4, 2023

shizhMSFT reviewed Jan 4, 2023

View reviewed changes

yizha1 reviewed Jan 5, 2023

View reviewed changes

shizhMSFT reviewed Jan 5, 2023

View reviewed changes

specs/notation-cli.md Outdated Show resolved Hide resolved

vaninrao10 force-pushed the vani-branch-1 branch from 81f0578 to ebf16d6 Compare January 5, 2023 20:31

vaninrao10 closed this Jan 5, 2023

vaninrao10 force-pushed the vani-branch-1 branch from ebf16d6 to c736231 Compare January 5, 2023 20:38

shizhMSFT mentioned this pull request Jan 6, 2023

doc: notation Inspect Command line Spec #500

Merged


		Use `notation inspect` command to inspect or describe all the signatures associated to a signed artifact (image) in a human readable format.

		Upon successful execution,the digest of the signed artifact and details of all the signatures associated with artifact and its respective certificate properties are displayed as following:

	├──io.wabbit-networks.buildId: 123 //Notary v2 payload annotations is shown here has user defined metadata.
	├──io.wabbit-networks.buildId: 123 //user-defined payload annotations

	├──<unsigned Attributes>
	├──<unsigned attributes>

Conversation

vaninrao10 commented Dec 20, 2022

Uh oh!

yizha1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yizha1 commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

priteshbandi commented Dec 28, 2022

Uh oh!

vaninrao10 commented Dec 29, 2022

Uh oh!

iamsamirzon commented Dec 29, 2022

Uh oh!

iamsamirzon commented Dec 29, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FeynmanZhou left a comment

Choose a reason for hiding this comment

Uh oh!

yizha1 commented Dec 24, 2022 •

edited

Loading

codecov-commenter commented Dec 28, 2022 •

edited

Loading

yizha1 left a comment •

edited

Loading