Skip to content

fix: login and logout will leverage docker config and os default store#712

Merged
priteshbandi merged 2 commits intonotaryproject:mainfrom
Wwwsylvia:fix_fallback
Jun 9, 2023
Merged

fix: login and logout will leverage docker config and os default store#712
priteshbandi merged 2 commits intonotaryproject:mainfrom
Wwwsylvia:fix_fallback

Conversation

@Wwwsylvia
Copy link
Contributor

@Wwwsylvia Wwwsylvia commented Jun 8, 2023 •
edited
Loading

Fixes: #696

Behaviors of this PR

  • If the notation config has auth configuration
    • Use the notation config for credentials GET, PUT and DELETE
  • If the notation config does not have auth configuration, or the the notation config file does not exist
    • If the docker config has auth configuration
      • Use the notation config for credentials GET, PUT and DELETE
    • If there is no authentication configured in the docker config, or the the docker config file does not exist
      • If there is a platform-default native keychain installed
        • Use the platform-default native keychain for credentials GET, PUT and DELETE
      • If there is no platform-default native keychain installed
        • GET will always return an empty credential
        • PUT will always fail (ErrPlaintextPutDisabled)
        • DELETE will result in no operation

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2023 •
edited
Loading

Codecov Report

Merging #712 (b612e92) into main (c9d3997) will decrease coverage by 0.26%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #712      +/-   ##
==========================================
- Coverage   63.77%   63.52%   -0.26%     
==========================================
  Files          40       40              
  Lines        2228     2237       +9     
==========================================
  Hits         1421     1421              
- Misses        686      695       +9     
  Partials      121      121
Impacted Files Coverage Δ
cmd/notation/login.go 25.00% <ø> (ø)
internal/auth/credentials.go 0.00% <0.00%> (ø)

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

ghost
ghost reviewed Jun 8, 2023
View reviewed changes
JeyJeyGao
JeyJeyGao reviewed Jun 8, 2023
View reviewed changes
@Wwwsylvia Wwwsylvia mentioned this pull request Jun 8, 2023
Merged
@Wwwsylvia
Copy link
Contributor Author

Wwwsylvia commented Jun 8, 2023 •
edited
Loading

Binary for testing: https://github.com/Wwwsylvia/notation/releases/tag/v1.0.9-sylvia

priteshbandi
priteshbandi reviewed Jun 9, 2023
View reviewed changes
@shizhMSFT
Copy link
Contributor

shizhMSFT commented Jun 9, 2023 •
edited
Loading

It seems that we were on the wrong track as we were figuring out the auth from the perspective of oras instead of notation.

For the consideration of notation, should we think about the following decision table?

ID Is Notation config auth configured? Is Docker config auth configured? Decision
1 No No Detect default native store for GET, PUT, and DELETE
2 Yes No Use Notation config only for GET, PUT, and DELETE. If plaintext cred is configured but cred store is not configured, fail on PUT.
3 No Yes Use docker config only for GET, PUT, and DELETE. If plaintext cred is configured but cred store is not configured, fail on PUT.
4 Yes Yes Use Notation config only for GET, PUT, and DELETE. If plaintext cred is configured but cred store is not configured, fail on PUT.

Here are detailed descriptions:

  1. No auth configured at all. In this case, we should detect and use the default native store for better OOBE.
  2. Notation auth config is configured but docker is not. This is a typical scenario where docker is not installed. In this case, we should use the notation auth config only.
  3. Notation auth config is not configured but docker is. This is a typical scenario where docker is installed and users want to share the docker auth with notation.
  4. Both notation auth config and docker auth config are configured. This is a typical scenario that users want to use notation as an isolated and independent component from docker.

@Wwwsylvia
refactor credentials store
4af9746 
Signed-off-by: Sylvia Lei <lixlei@microsoft.com>
@Wwwsylvia Wwwsylvia marked this pull request as ready for review June 9, 2023 09:28
@Wwwsylvia
update go mod
b08978c 
Signed-off-by: Sylvia Lei <lixlei@microsoft.com>
shizhMSFT
shizhMSFT approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

priteshbandi
priteshbandi approved these changes Jun 9, 2023
View reviewed changes
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.

LGTM

@priteshbandi priteshbandi mentioned this pull request Jun 9, 2023
Closed
@priteshbandi priteshbandi merged commit 0391202 into notaryproject:main Jun 9, 2023
@yizha1 yizha1 mentioned this pull request Jun 10, 2023
Closed
@shizhMSFT shizhMSFT mentioned this pull request Jul 21, 2023
Merged
6 tasks
@Wwwsylvia Wwwsylvia deleted the fix_fallback branch October 16, 2023 06:48
7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025
@Wwwsylvia @7h3-3mp7y-m4n
fix: login and logout will leverage docker config and os default …
f80f31a 
…store (notaryproject#712)

Fixes: notaryproject#696 

Signed-off-by: Sylvia Lei <lixlei@microsoft.com>
FeynmanZhou pushed a commit to FeynmanZhou/notation that referenced this pull request May 15, 2025
@Wwwsylvia @FeynmanZhou
fix: login and logout will leverage docker config and os default …
5299a25 
…store (notaryproject#712)

Fixes: notaryproject#696 

Signed-off-by: Sylvia Lei <lixlei@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@priteshbandi priteshbandi priteshbandi approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@gokarnm gokarnm Awaiting requested review from gokarnm gokarnm is a code owner

@justincormack justincormack Awaiting requested review from justincormack

@NiazFK NiazFK Awaiting requested review from NiazFK NiazFK is a code owner

@rgnote rgnote Awaiting requested review from rgnote rgnote is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker

+2 more reviewers

@JeyJeyGao JeyJeyGao JeyJeyGao left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

desktop.exe credential store is not supported in WSL

5 participants

@Wwwsylvia @codecov-commenter @shizhMSFT @priteshbandi @JeyJeyGao

Comments