docker trust inspect by riyazdf · Pull Request #694 · docker/cli

riyazdf · 2017-11-16T00:52:47Z

Introduces a docker trust inspect command for JSON output with the same information presented in the human-readable docker trust view.

I refactored some of the existing view code so that it could be shared with inspect and added a docs page.

cc @ashfall @eiais for additional review, and @rn to confirm that this information is what he was looking for from the original issue (#659)

shortened and modified example here:

🐳 $ docker trust inspect my-image:purple
[
  {
    "Name": "my-image:purple",
    "SignedTags": [
      {
        "SignedTag": "purple",
        "Digest": "941d3dba358621ce3c41ef67b47cf80f701ff80cdf46b5cc86587eaebfe45557",
        "Signers": [
          "alice",
          "bob",
          "carol"
        ]
      }
    ],
    "Signers": [
      {
        "Name": "alice",
        "Keys": [
                {     
                          "ID": "04dd031411ed671ae1e12f47ddc8646d98f135090b01e54c3561e843084484a3"
                },
                {
                          "ID": "6a11e4898a4014d400332ab0e096308c844584ff70943cdd1d6628d577f45fd8"
                }
        ]
      },
      {
        "Name": "bob",
        "Keys": [
                {
                          "ID": "433e245c656ae9733cdcc504bfa560f90950104442c4528c9616daa45824ccba"
                }
        ]
      },
      {
        "Name": "carol",
        "Keys": [
                {
                          "ID": "d32fa8b5ca08273a2880f455fcb318da3dc80aeae1a30610815140deef8f30d9"
                },
                {
                          "ID":  "9a8bbec6ba2af88a5fad6047d428d17e6d05dbdd03d15b4fc8a9a0e8049cd606"
                }
        ]
      }
    ],
    "AdminstrativeKeys": [
      {
        "Name": "Repository",
        "Keys": [
                {
                          "ID": "27df2c8187e7543345c2e0bf3a1262e0bc63a72754e9a7395eac3f747ec23a44"
                }
        ]
      },
      {
        "Name": "Root",
        "Keys": [
                {
                          "ID": "40b66ccc8b176be8c7d365a17f3e046d1c3494e053dd57cfeacfe2e19c4f8e8f"
                }
        ]
      }
    ]
  }
]

Closes #659

rn · 2017-11-16T08:57:26Z

Thanks. the output looks good to me

dnephin · 2017-11-16T16:21:43Z

+		Args:  cli.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return inspectTrustInfo(dockerCli, args[0])
+		},


I believe all other inspect commands support multiple args for inspecting multiple objects at once, so they return a list instead of a single JSON object.

Other inspect commands also use a formatter in cli/command/formatter to support the --format flag.

I think it might be good to be consistent here.

sounds good. I've added multiple args support. I'd like to hold off on the --format flag for now until we've stabilized the JSON format

eiais · 2017-11-16T19:14:12Z

+
+
+```bash
+$ docker trust inspect alpine:latest | jq


None of the other inspect command's docs show piping to jq

Agreed: can you remove the | jq? We can keep the output as-is (it's documentation, so should be reasonable to output for readability - we may need to add a note / "conventions" section about that somewhere central)

dnephin

LGTM

vdemeester

LGTM 🐸
cc @thaJeztah for docs

thaJeztah

Thanks! See my comments inline 😄

thaJeztah · 2017-11-17T08:58:20Z

+	}, nil
+}
+
+// trustRepo represents consumable information about a trusted repository


nit: can you move these types to the top of this file? (looks like we do so in the other trust commands)

thaJeztah · 2017-11-17T09:10:23Z

+        "SignedTag": "latest",
+        "Digest": "d6bfc3baf615dc9618209a8d607ba2a8103d9c8a405b3bd8741d88b4bef36478",
+        "Signers": [
+          "Repo Admin"


I'm wondering if more information about signers is available, or will be available/needed at some point; basically, I love the simplicity of a []string, but if we anticipate more information is needed at some point, perhaps a []sometype{} ?

Definitely open to input here 👍

if there's more info it will be specified in the top level Signers key struct - do you think that's ok? Let me know if I'm misinterpreting your comment.

thaJeztah · 2017-11-17T09:10:48Z

+      {
+        "Name": "Repository",
+        "Keys": [
+          "5a46c9aaa82ff150bb7305a2d17d0c521c2d784246807b2dc611f436a69041fd"


Same for keys ([]string vs []sometype{})

I'm happy to make the change but don't think we'll need it - notary only exposes []string for Key IDs in its role structs

updated to use a custom type!

ex:

"Signers": [ { "Name": "bob", "Keys": [ { "ID": "B" } ] }, ...

You forgot to update the examples in the docs 😅

thaJeztah · 2017-11-17T09:21:35Z

+
+// lookupTrustInfo returns processed signature and role information about a notary repository.
+// This information is to be pretty printed or serialized into a machine-readable format.
+func lookupTrustInfo(cli command.Cli, remote string) (trustTagRowList, []client.RoleWithSignatures, []data.Role, error) {


lookupTrustInfo() is now shared between inspect and view; perhaps it (and the tests) should be moved to a separate file common.go ? I see there's a helpers.go, but not sure this qualifies as a "helper".

Same for:

getDelegationRoleToKeyMap()

matchReleasedSignatures()

(Possibly same for trustTagKey and the other types defined at the top of this file)

thaJeztah · 2017-11-17T09:56:38Z

+If signers are set up for the repository via other `docker trust` commands, `docker trust inspect` includes a `Signers` key:
+
+```bash
+


Can you remove this line?

thaJeztah · 2017-11-17T11:19:43Z

+### Get details about signatures for multiple images
+
+```bash
+$ docker trust inspect alpine ubuntu | jq


drop | jq

thaJeztah · 2017-11-17T11:20:08Z

+```
+
+### Get details about signatures for all image tags in a repository
+


Can you add a short introduction here? Just to lead in the example

👍 added!

thaJeztah · 2017-11-17T11:20:15Z

+
+
+### Get details about signatures for multiple images
+


Can you add a short introduction here? Just to lead in the example

thaJeztah · 2017-11-17T11:24:38Z

+	cmd := &cobra.Command{
+		Use:   "inspect IMAGE[:TAG] [IMAGE[:TAG]...]",
+		Short: "Return low-level information about keys and signatures",
+		Args:  cli.RequiresMinArgs(1),


We should either disallow specifying multiple repositories, or add the repository name to the output. The output as it is, is not useful if you specify multiple repositories;

$ docker trust inspect alpine hello-world | jq

Produces (removed some entries to keep it short):

[ { "SignedTags": [ { "SignedTag": "2.6", "Digest": "9ace551613070689a12857d62c30ef0daa9a376107ec0fff0e34786cedb3399b", "Signers": [ "Repo Admin" ] }, { "SignedTag": "latest", "Digest": "d6bfc3baf615dc9618209a8d607ba2a8103d9c8a405b3bd8741d88b4bef36478", "Signers": [ "Repo Admin" ] } ], "AdminstrativeKeys": [ { "Name": "Root", "Keys": [ "a2489bcac7a79aa67b19b96c4a3bf0c675ffdf00c6d2fabe1a5df1115e80adce" ] }, { "Name": "Repository", "Keys": [ "5a46c9aaa82ff150bb7305a2d17d0c521c2d784246807b2dc611f436a69041fd" ] } ] }, { "SignedTags": [ { "SignedTag": "latest", "Digest": "0e06ef5e1945a718b02a8c319e15bae44f47039005530bc617a5d071190ed3fc", "Signers": [ "Repo Admin" ] }, { "SignedTag": "linux", "Digest": "452733afb5319f624b88aec51006a077bccf036a87167fd657057e2e31f42736", "Signers": [ "Repo Admin" ] }, ], "AdminstrativeKeys": [ { "Name": "Root", "Keys": [ "ff544c7f4ffa0a61b2fb836fb581182181c971acd58a7023c9a3b049dc952edd" ] }, { "Name": "Repository", "Keys": [ "8d2aaa7461b4305f74dc80bb946ccad962829a57bd0412a480e17a2413b18ec9" ] } ] } ]

With that information alone, I don't know which repository the :latest tag belongs to

sounds good, I added Name as the first field 👍

thaJeztah · 2017-11-17T11:25:27Z

+
+// trustRepo represents consumable information about a trusted repository
+type trustRepo struct {
+	SignedTags        trustTagRowList `json:",omitempty"`


I think this type needs a property for the repo-name (see my comment for "multiple repositories")

thaJeztah

Left some notes for the docs, but LGTM otherwise

thaJeztah · 2017-11-21T15:19:34Z

+      {
+        "Name": "Repository",
+        "Keys": [
+          "5a46c9aaa82ff150bb7305a2d17d0c521c2d784246807b2dc611f436a69041fd"


You forgot to update the examples in the docs 😅

thaJeztah · 2017-11-21T15:21:19Z

+                ]
+            },
+            {
+                "SignedTag": "3.6",


When updating for the new format, can you also shorten the output of this example a bit? Only 1 .. 2 tags should be needed to show what it does 😄

thaJeztah

LGTM, but may need some squashing 👍

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

thaJeztah

LGTM, thanks!

riyazdf requested review from mdlinville, thaJeztah and vdemeester as code owners November 16, 2017 00:52

GordonTheTurtle added the status/0-triage label Nov 16, 2017

dnephin added status/2-code-review and removed status/0-triage labels Nov 16, 2017

dnephin reviewed Nov 16, 2017

View reviewed changes

eiais reviewed Nov 16, 2017

View reviewed changes

mdlinville approved these changes Nov 16, 2017

View reviewed changes

riyazdf force-pushed the trust-inspect branch from 5d88927 to 4aa8de2 Compare November 16, 2017 22:11

dnephin approved these changes Nov 16, 2017

View reviewed changes

vdemeester approved these changes Nov 17, 2017

View reviewed changes

thaJeztah added the impact/changelog label Nov 17, 2017

thaJeztah requested changes Nov 17, 2017

View reviewed changes

thaJeztah requested changes Nov 21, 2017

View reviewed changes

thaJeztah approved these changes Nov 29, 2017

View reviewed changes

riyazdf added 3 commits November 29, 2017 10:52

add docker trust inspect command for JSON viewing

cd38d39

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

support multiple arguments to trust inspect

1eb87cc

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

Use default inspect formatting, remove omitempty, update docs

a942828

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

riyazdf force-pushed the trust-inspect branch from 7002e90 to a942828 Compare November 29, 2017 18:52

thaJeztah approved these changes Nov 29, 2017

View reviewed changes

thaJeztah merged commit d921d5c into docker:master Nov 29, 2017

GordonTheTurtle added this to the 17.12.0 milestone Nov 29, 2017

thaJeztah added the kind/experimental label Nov 29, 2017

riyazdf deleted the trust-inspect branch November 29, 2017 19:46

		If signers are set up for the repository via other `docker trust` commands, `docker trust inspect` includes a `Signers` key:

		```bash

		```

		### Get details about signatures for all image tags in a repository

Uh oh!

Conversation

riyazdf commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rn commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eiais Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

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!

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!

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!

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!

thaJeztah left a comment

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!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

riyazdf commented Nov 16, 2017 •

edited

Loading

eiais Nov 16, 2017 •

edited

Loading