Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 9, 2023

Conversation

Wwwsylvia
Copy link
Contributor

@Wwwsylvia Wwwsylvia commented Jun 8, 2023

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

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

go.mod Outdated Show resolved Hide resolved
@Wwwsylvia
Copy link
Contributor Author

Wwwsylvia commented Jun 8, 2023

@shizhMSFT
Copy link
Contributor

shizhMSFT commented Jun 9, 2023

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.

Signed-off-by: Sylvia Lei <[email protected]>
Signed-off-by: Sylvia Lei <[email protected]>
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

desktop.exe credential store is not supported in WSL
6 participants