[Feature] support notation login by binbin-li · Pull Request #218 · notaryproject/notation

binbin-li · 2022-06-30T09:39:00Z

What?

Add support for logging in/out. Related Issue: #119

Work need to be done:

Add new CMDs that supports login and logout.
For login CMD, validate the input credentials through ORAS, and store it in the credentials store.
For logout CMD, remove the credential from the credentials store
For other CMDs that need authentication, we should get the credentials saved in the credentials store.
Create a submodule that interacts with the credentials store and validate credentials via ORAS.
Integrate with the new Directory pkg to manipulate the config file once it's ready to be use
Add parser that parse the config.json into the required creds config(support configs for both docker and notation)

Test?

Manual test (done):

test plan:

run login cmd
run logout cmd
run other CMDs, e.g. list/push/pull
3.1 user types username/password explicitly. [expect: user-typed credential is used]
3.2 user doesn't type username/password. [expect: saved credential is used]
~/.notaion/config.json is set up
4.1 both credHelpers and credsStore are configured. [expect: only credHeplers is honored]
4.2 only credsStore is configured. [expect: credsStore is honored]
~/.docker/config.json is set up while ~/.notaion/config.json not
5.1 both credHelpers and credsStore are configured. [expect: only credHeplers is honored]
5.2 only credsStore is configured. [expect: credsStore is honored]

Will create another PR to complete the unit tests

Signed-off-by: Binbin Li libinbin@microsoft.com

internal/trace/transport.go

cmd/notation/registry.go

internal/crypto/certificate.go

internal/trace/transport.go

pkg/dockerconfig/config.go

pkg/auth/creds_config.go

pkg/auth/native_passport.go

pkg/auth/registry_remote.go

patrickzheng200 · 2022-07-06T04:08:35Z

@shizhMSFT @binbin-li maybe we could split long PR like this one into multiple smaller PRs in the future to enhance the readability? (there are 20 files been added/updated in this PR) Please suggest if you think otherwise.

binbin-li · 2022-07-06T04:11:10Z

@shizhMSFT @binbin-li maybe we could split long PR like this one into multiple smaller PRs in the future to enhance the readability? (there are 20 files been added/updated in this PR) Please suggest if you think otherwise.

You're right. I didn't expect this PR would be so large at the beginning. I would split long PRs next time.

cmd/notation/registry.go

pkg/dockerconfig/config.go

shizhMSFT · 2022-07-06T04:57:09Z

pkg/dockerconfig/config.go

@@ -0,0 +1,115 @@
+package dockerconfig


We can merge the dockerconfig package into a single file docker.go in the config package.

shizhMSFT · 2022-07-07T16:33:36Z

go.mod

 	github.com/opencontainers/go-digest v1.0.0
 	github.com/urfave/cli/v2 v2.10.3
-	oras.land/oras-go/v2 v2.0.0-20220620164807-8b2a54608a94
+	oras.land/oras-go/v2 v2.0.0-alpha


We need to upgrade oras-go to rc.1 in the next PR.

Sure, I'll bump it up in a seperate PR

cmd/notation/registry.go

cmd/notation/login.go

pkg/auth/types.go

pkg/config/docker_config.go

pkg/auth/types.go

pkg/auth/creds_config.go

cmd/notation/registry.go

shizhMSFT

LGTM

SteveLasker · 2022-07-11T21:13:51Z

I updated PR #119 to reflect the latest.
We should probably complete, then merge #119 before merging this one.

binbin-li · 2022-07-12T01:37:10Z

I updated PR #119 to reflect the latest. We should probably complete, then merge #119 before merging this one.

When referring to PR #119 , do you mean #223 ? @SteveLasker

shizhMSFT · 2022-07-13T14:05:58Z

I updated PR #119 to reflect the latest. We should probably complete, then merge #119 before merging this one.

When referring to PR #119 , do you mean #223 ? @SteveLasker

#119 is an issue. It must be a typo and refers to #223.

@binbin-li Could you update this PR to reflect #223?

binbin-li · 2022-07-14T03:08:13Z

I updated PR #119 to reflect the latest. We should probably complete, then merge #119 before merging this one.

When referring to PR #119 , do you mean #223 ? @SteveLasker

#119 is an issue. It must be a typo and refers to #223.

@binbin-li Could you update this PR to reflect #223?

Working on it.

binbin-li · 2022-07-14T08:01:57Z

Updated as #223 defined.

Moved --plain-http to the global options.
Add --password-stdin option to login cmd.
Add examples.

SteveLasker

thanks @binbin-li
LGTM

gokarnm

LGTM!

Signed-off-by: Binbin Li <libinbin@microsoft.com>

* feat: support notation login * refactor: Refactor for readablity * chore: refactor registry.go * feat: update CMDs to be consistent with specs * chore: refactor load config logic Signed-off-by: Binbin Li <libinbin@microsoft.com>

binbin-li requested a review from a team June 30, 2022 09:39

binbin-li force-pushed the notation-login branch from e3bc8b8 to 9699d69 Compare June 30, 2022 09:49

github-advanced-security bot found potential problems Jun 30, 2022

View reviewed changes

internal/trace/transport.go Fixed Show fixed Hide fixed

binbin-li force-pushed the notation-login branch 6 times, most recently from d0f53d4 to 2f02aa0 Compare July 1, 2022 06:51

binbin-li changed the title ~~[WIP] feat: support notation login~~ feat: support notation login Jul 1, 2022

binbin-li force-pushed the notation-login branch 4 times, most recently from d5bc151 to ccdb1dd Compare July 4, 2022 09:14

shizhMSFT reviewed Jul 5, 2022

View reviewed changes

SteveLasker changed the title ~~feat: support notation login~~ [Feature]: support notation login Jul 5, 2022

SteveLasker changed the title ~~[Feature]: support notation login~~ [Feature] support notation login Jul 5, 2022

binbin-li force-pushed the notation-login branch from ccdb1dd to d67a27d Compare July 6, 2022 03:24

shizhMSFT reviewed Jul 6, 2022

View reviewed changes

binbin-li force-pushed the notation-login branch 8 times, most recently from c97998f to 3caa631 Compare July 7, 2022 11:44

shizhMSFT reviewed Jul 7, 2022

View reviewed changes

binbin-li force-pushed the notation-login branch from 6cc5439 to 5521311 Compare July 8, 2022 03:30

shizhMSFT reviewed Jul 8, 2022

View reviewed changes

cmd/notation/registry.go Outdated Show resolved Hide resolved

shizhMSFT approved these changes Jul 8, 2022

View reviewed changes

shizhMSFT requested review from gokarnm, priteshbandi and rgnote July 8, 2022 04:28

shizhMSFT added this to the RC-1 milestone Jul 8, 2022

binbin-li force-pushed the notation-login branch from 95e8c75 to 7085fc0 Compare July 14, 2022 02:47

binbin-li force-pushed the notation-login branch from 066cfa4 to 7a2547c Compare July 14, 2022 07:58

SteveLasker approved these changes Jul 14, 2022

View reviewed changes

binbin-li force-pushed the notation-login branch 2 times, most recently from 8af7cc5 to 41f9bca Compare July 18, 2022 06:58

gokarnm approved these changes Jul 20, 2022

View reviewed changes

binbin-li added 5 commits July 20, 2022 05:23

feat: support notation login

0f74c2f

Signed-off-by: Binbin Li <libinbin@microsoft.com>

refactor: Refactor for readablity

f2c034a

Signed-off-by: Binbin Li <libinbin@microsoft.com>

chore: refactor registry.go

a828699

Signed-off-by: Binbin Li <libinbin@microsoft.com>

feat: update CMDs to be consistent with specs

e29ad0f

Signed-off-by: Binbin Li <libinbin@microsoft.com>

chore: refactor load config logic

e705d73

Signed-off-by: Binbin Li <libinbin@microsoft.com>

binbin-li force-pushed the notation-login branch from 41f9bca to e705d73 Compare July 20, 2022 05:23

gokarnm merged commit bb5b497 into notaryproject:main Jul 20, 2022

binbin-li mentioned this pull request Jul 20, 2022

test: Add unit tests for notation login #256

Merged

chloeyin mentioned this pull request Jul 25, 2022

notation sign doesn't work on main branch #259

Closed

yizha1 linked an issue Jul 25, 2022 that may be closed by this pull request

[Feature] notation login #119

Closed

4 tasks

dtzar mentioned this pull request Jul 25, 2022

Define local credential experience for Registry Authentication #206

Closed

Comments

Conversation

binbin-li commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Test?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickzheng200 commented Jul 6, 2022

Uh oh!

binbin-li commented Jul 6, 2022

Uh oh!

Uh oh!

Uh oh!

shizhMSFT Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

shizhMSFT Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

binbin-li Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

SteveLasker commented Jul 11, 2022

Uh oh!

binbin-li commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shizhMSFT commented Jul 13, 2022

Uh oh!

binbin-li commented Jul 14, 2022

Uh oh!

binbin-li commented Jul 14, 2022

Uh oh!

SteveLasker left a comment

Choose a reason for hiding this comment

Uh oh!

gokarnm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

binbin-li commented Jun 30, 2022 •

edited

Loading

binbin-li commented Jul 12, 2022 •

edited

Loading