Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 11, 2023

cli/command: ConfigureAuth: fix terminal state not being restored on error

ConfigureAuth used the readInput() utility to read the username and password.
However, this utility did not return errors it encountered, but instead did
an os.Exit(1). A result of this was that the terminal was not restored if
an error happened. When reading the password, the terminal is configured to
disable echo (i.e. characters are not printed), and failing to restore
the previous state means that the terminal is now "non-functional".

This patch:

  • changes readInput() to return errors it encounters
  • uses a defer() to restore terminal state

cli/command: ConfigureAuth: trim whitespace both for username and password

changes readInput() to trim whitespace. The existing code tried to be
conservative and only trimmed whitespace for username (not for password).
Passwords with leading/trailing whitespace would be very unlikely, and
trimming whitespace is generally accepted.

cli/command/registry: remove intermediate var that collided

This also simplifies the code a bit.

cli/command: replace EncodeAuthToBase64 for registry.EncodeAuthConfig

Replace uses of this function in favor of the implementation in the
API types, so that we have a single, canonical implementation.

cli/command/container/create: pullImage(): use RetrieveAuthTokenFromImage

replace the local code with RetrieveAuthTokenFromImage, which does exactly the same;

cli/cli/command/registry.go

Lines 163 to 188 in 6233560

// RetrieveAuthTokenFromImage retrieves an encoded auth token given a complete image
func RetrieveAuthTokenFromImage(ctx context.Context, cli Cli, image string) (string, error) {
// Retrieve encoded auth token from the image reference
authConfig, err := resolveAuthConfigFromImage(ctx, cli, image)
if err != nil {
return "", err
}
encodedAuth, err := EncodeAuthToBase64(authConfig)
if err != nil {
return "", err
}
return encodedAuth, nil
}
// resolveAuthConfigFromImage retrieves that AuthConfig using the image string
func resolveAuthConfigFromImage(ctx context.Context, cli Cli, image string) (registrytypes.AuthConfig, error) {
registryRef, err := reference.ParseNormalizedNamed(image)
if err != nil {
return registrytypes.AuthConfig{}, err
}
repoInfo, err := registry.ParseRepositoryInfo(registryRef)
if err != nil {
return registrytypes.AuthConfig{}, err
}
return ResolveAuthConfig(ctx, cli, repoInfo.Index), nil
}

cli/command/container/create: pullImage() remove intermediate vars

cli/command/container: pullImage: use DisplayJSONMessagesToStream utility

This utility provides the same logic as was implemented here (and using it
aligns with the "docker pull" equivalent).

Also added a TODO to replace this function with the regular "docker pull"
code.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

@rumpl @laurazard ptal 🤗

@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2023

Codecov Report

Merging #4190 (17df83e) into master (b799ab9) will decrease coverage by 0.01%.
The diff coverage is 39.39%.

❗ Current head 17df83e differs from pull request most recent head 5d856a5. Consider uploading reports for the commit 5d856a5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4190      +/-   ##
==========================================
- Coverage   58.86%   58.85%   -0.01%     
==========================================
  Files         572      572              
  Lines       49576    49492      -84     
==========================================
- Hits        29182    29130      -52     
+ Misses      18624    18598      -26     
+ Partials     1770     1764       -6     

@thaJeztah
Copy link
Member Author

oh! I think I forgot to push some of the changes. let me dig them up

@thaJeztah thaJeztah force-pushed the command_auth_cleanups branch 2 times, most recently from 17df83e to 1b617d6 Compare April 12, 2023 10:44
…error

ConfigureAuth used the readInput() utility to read the username and password.
However, this utility did not return errors it encountered, but instead did
an os.Exit(1). A result of this was that the terminal was not restored if
an error happened. When reading the password, the terminal is configured to
disable echo (i.e. characters are not printed), and failing to restore
the previous state means that the terminal is now "non-functional".

This patch:

- changes readInput() to return errors it encounters
- uses a defer() to restore terminal state

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…sword

changes readInput() to trim whitespace. The existing code tried to be
conservative and only trimmed whitespace for username (not for password).
Passwords with leading/trailing whitespace would be _very_ unlikely, and
trimming whitespace is generally accepted.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Also adds a TODO to verify if this special handling is still needed.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This also simplifies the code a bit.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Replace uses of this function in favor of the implementation in the
API types, so that we have a single, canonical implementation.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…mage

replace the local code with RetrieveAuthTokenFromImage, which does exactly the same;
https://github.com/docker/cli/blob/623356001f6310fef7a8cca0b9ba0ac1b735bb38/cli/command/registry.go#L163-L188

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…lity

This utility provides the same logic as was implemented here (and using it
aligns with the "docker pull" equivalent).

Also added a TODO to replace this function with the regular "docker pull"
code.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the command_auth_cleanups branch from 1b617d6 to 5d856a5 Compare April 12, 2023 19:17
@thaJeztah thaJeztah changed the title cli/command: some cleanups / refactoring related to auth cli/command: some cleanups / refactoring, and fixes related to auth Apr 12, 2023
@thaJeztah
Copy link
Member Author

all green now 👍

@thaJeztah thaJeztah merged commit c25115e into docker:master Apr 13, 2023
@thaJeztah thaJeztah deleted the command_auth_cleanups branch April 13, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants