Skip to content

Notation CLI baseline#83

Merged
SteveLasker merged 75 commits intonotaryproject:mainfrom
shizhMSFT:main
Sep 16, 2021
Merged

Notation CLI baseline#83
SteveLasker merged 75 commits intonotaryproject:mainfrom
shizhMSFT:main

Conversation

shizhMSFT and others added 30 commits July 31, 2020 13:36
This is a joint commit of
- Shiwei Zhang
- Steve Lasker
- Aviral Takkar

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Based on our [Notary v2 call this week](https://hackmd.io/_vrqBGAOSUC_VWvFzWruZw?view), we agreed to merge this into a `prototype-1` branch.
This will give us a baseline to which we can open issues and submit new PRs.
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Distribution options for persistence & discovery
Renaming the PR and file to capture the design discussions. As this fork represents a prototype, we'll merge this in, clearing the root README.md for the current implementation.
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
return ocispec.Descriptor{}, err
}
insecure := config.IsRegistryInsecure(ref.Registry)
if host, _, _ := net.SplitHostPort(ref.Registry); host == "localhost" {

Choose a reason for hiding this comment

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

Do we need to include 127.0.0.1?

Choose a reason for hiding this comment

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

all loopback interfaces

}

insecure := config.IsRegistryInsecure(hostname)
if host, _, _ := net.SplitHostPort(hostname); host == "localhost" {

Choose a reason for hiding this comment

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

maybe worth having it as util for determining insecure property from hostname

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create an issue to track, and merge?

@SteveLasker
Copy link
Contributor

Per the notation call today, we've agreed to merge this as a baseline to create issues and continue iterations.

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
@shizhMSFT shizhMSFT changed the title Notation CLI Alpha Notation CLI baseline Sep 13, 2021
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

Some high-level comments:

  • For better readability, we need to add some documentation for packages and exported names.
  • We should also add unit tests. At minimum we need test which exercises positive scenario(successful signing and verification) to avoid breaking changes.
  • The Commit history doesn't look right; also, can we please update the commit message?
  • We need better error messages
  • The cli should exit with proper exit code(non zero for failures) Ref
  • The package structure should be reorganized for better readability.
cmd
  |->docker-generate --> contains cmd to generates docker artifact
  |->docker-notation --> 
       |->crypto     --> not a cli file but a implementation of signer and verifier; shouldn’t be inside cmd
       |->docker     --> contains docker related helper method; shouldn’t be inside cmd
       |->notation   --> contains docker pull,push,etc commands
internal
  |->docker          --> it’s a cli helper; should go to cmd
  |->io
  |->os
  |->version         --> it’s a cli helper; should go to cmd
pkg                  --> What's the difference between internal and pkg? 
  |->cache           --> contains signature related methods; why its named cache?
  |->config          --> contains user config
  |->docker          --> Docker related helper to get image; should this go to registry?
  |->registry        --> Docker related helper

Tracking issue #93

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
@SteveLasker SteveLasker mentioned this pull request Sep 14, 2021
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
@@ -0,0 +1,40 @@
package crypto
Copy link
Contributor

Choose a reason for hiding this comment

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

should service.go be added as default implementation in notation-lib?

Comment on lines +12 to +17
passwordFlag = &cli.StringFlag{
Name: "password",
Aliases: []string{"p"},
Usage: "password for generic remote access",
EnvVars: []string{"NOTATION_PASSWORD"},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking password from cli in plain text is a security risk as its visible while typing and can be seen in shell history.

Comment on lines +12 to +14
VerificationCertificates VerificationCertificates `json:"verificationCerts"`
SigningKeys SigningKeys `json:"signingKeys,omitempty"`
InsecureRegistries []string `json:"insecureRegistries"`
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of exporting types can we export routines so that underlying implementation can be changed without affecting callers.

Copy link
Contributor

@sajayantony sajayantony left a comment

Choose a reason for hiding this comment

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

As per morning call we will have issues tracking things called out above by @mnltejaswini @priteshbandi and others.

Copy link
Contributor

@NiazFK NiazFK left a comment

Choose a reason for hiding this comment

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

Approving with comments tracked in issues.

@SteveLasker SteveLasker merged commit 387ae5c into notaryproject:main Sep 16, 2021
@shizhMSFT shizhMSFT deleted the main branch September 16, 2021 08:11
7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025
* Initial prototype
* Distribution api proposal
* Formating fix
* JWT based system (notaryproject#16)
* Fix paging api link
* Resolve spelling errors
* Fix examples to include full descriptor
* Use library implementation
* refactor network transport
* add makefile
* update dependency
* push signature support
* pull signature support
* integrate sign and push
* no output on sign-then-push by default
* better ux for signature info
* make install better
* update notary library
* update dependencies
* Multiple signatures support (notaryproject#52)
* pull signatures when pulling images (notaryproject#54)
* Explicit add original references when signing (notaryproject#57)
* create parent folders before create config file (notaryproject#59)
* Add git clone steps to building.md
* Ci 2 (notaryproject#49)
* Add basic workflow that compiles the binary
* Add dependabot for Go dependencies
* Add dependabot for github-actions
* Update workflow to use Makefile
* extract `list` from `pull --peek`
* rename digest to sigDigests for code readability
* update doc for building
* refine doc for building
* plain http in config
* no default trust on generated cert
* clean up media type
* refine docker-notation
* cached pull
* refactor notation code
* fix reference print
* refactor docker-notation code
* compare registry host case insensitively
* refine docker-notation pull output
* safer prune
* prepare for v0.5.2 release
* update notation-go-lib dependency
* update release for go releaser
* update notation-go-lib dependency
* make github workflow consistent
* output stderr for docker-generate
* move implementations to the correct package
* Include .editorconfig compatible with go fmt
* Run CI on go 1.16
* Since go 1.16 some ioutil functions are deprecated and moved into the io package. Therefore can't build with previous versions of Go anymore
* Add help task to Makefile
* Add CI status badge
* Notation CLI Alpha
* bump notation-go-lib
* prepare for 0.5.1 release
* refine notation sign
* Bump actions/cache from 2.1.4 to 2.1.6
* Bumps [actions/cache](https://github.com/actions/cache) from 2.1.4 to 2.1.6.
* [Release notes](https://github.com/actions/cache/releases)
* [Commits](actions/cache@v2.1.4...v2.1.6)

This is a joint commit of
- Shiwei Zhang
- Aviral Takkar
- Sajay Antony
- Steve Lasker
- Marco Franssen


Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

8 participants

Comments