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

adding identity token as an approximate alias of password #1040

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 29 additions & 8 deletions cmd/oras/internal/option/remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
Username string
PasswordFromStdin bool
Password string
IdentityToken string
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this new value, the --registry-token can be applied to Password directly. The tricky part is whether we need --registry-token-stdin? Please wait for #742 (comment) to be answered?

Copy link
Author

Choose a reason for hiding this comment

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

I see. I will remove the field and would implement identity-token-stdin instead.

Comment on lines 56 to +58
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 having IdentityToken, we can have something like Secret and SecretFromStdin.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, So should I add a field secret that will hold the Identity Token, Instead of Password?


resolveFlag []string
applyDistributionSpec bool
Expand Down Expand Up @@ -87,14 +88,15 @@
// Commonly used for non-unary remote targets.
func (opts *Remote) ApplyFlagsWithPrefix(fs *pflag.FlagSet, prefix, description string) {
var (
shortUser string
shortPassword string
shortHeader string
flagPrefix string
notePrefix string
qweeah marked this conversation as resolved.
Show resolved Hide resolved
shortUser string
shortPassword string
shortIdentityToken string
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not enable shorthand for --identity-token

shortHeader string
flagPrefix string
notePrefix string
)
if prefix == "" {
shortUser, shortPassword = "u", "p"
shortUser, shortPassword, shortIdentityToken = "u", "p", "i"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not enable shorthand for --identity-token

Copy link
Author

Choose a reason for hiding this comment

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

Sure will do it

shortHeader = "H"
}
flagPrefix, notePrefix = applyPrefix(prefix, description)
Expand All @@ -104,6 +106,7 @@
}
fs.StringVarP(&opts.Username, flagPrefix+"username", shortUser, "", notePrefix+"registry username")
fs.StringVarP(&opts.Password, flagPrefix+"password", shortPassword, "", notePrefix+"registry password or identity token")
fs.StringVarP(&opts.IdentityToken, flagPrefix+"identity-token", shortIdentityToken, "", notePrefix+"identity token for registry")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not enable shorthand for --identity-token

Suggested change
fs.StringVarP(&opts.IdentityToken, flagPrefix+"identity-token", shortIdentityToken, "", notePrefix+"identity token for registry")
fs.StringVarP(&opts.IdentityToken, flagPrefix+"identity-token", "", "", notePrefix+"identity token for registry")

fs.BoolVarP(&opts.Insecure, flagPrefix+"insecure", "", false, "allow connections to "+notePrefix+"SSL registry without certs")
plainHTTPFlagName := flagPrefix + "plain-http"
plainHTTP := fs.Bool(plainHTTPFlagName, false, "allow insecure connections to "+notePrefix+"registry without SSL check")
Expand All @@ -124,10 +127,16 @@
return opts.readPassword()
}

// readPassword tries to read password with optional cmd prompt.
// readPassword tries to read password and identity-token with optional cmd prompt.
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: make line length less than 80

Suggested change
// readPassword tries to read password and identity-token with optional cmd prompt.
// readPassword tries to read password and identity-token with optional cmd
// prompt.

func (opts *Remote) readPassword() (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with this, but there is a Cobra feature for mutually exclusive

Copy link
Author

Choose a reason for hiding this comment

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

Oh, thanks for the info. I will check it and get back on this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW identity-token is exclusive to both password and username

if opts.Password != "" && opts.IdentityToken != "" {
return fmt.Errorf("both --password and --identity-token cannot be used together")
}

Check warning on line 134 in cmd/oras/internal/option/remote.go

View check run for this annotation

Codecov / codecov/patch

cmd/oras/internal/option/remote.go#L133-L134

Added lines #L133 - L134 were not covered by tests

if opts.Password != "" {
fmt.Fprintln(os.Stderr, "WARNING! Using --password via the CLI is insecure. Use --password-stdin.")
} else if opts.IdentityToken != "" {
fmt.Fprintln(os.Stderr, "WARNING! Using --identity-token via the CLI is insecure.")

Check warning on line 139 in cmd/oras/internal/option/remote.go

View check run for this annotation

Codecov / codecov/patch

cmd/oras/internal/option/remote.go#L139

Added line #L139 was not covered by tests
} else if opts.PasswordFromStdin {
// Prompt for credential
password, err := io.ReadAll(os.Stdin)
Expand Down Expand Up @@ -267,7 +276,19 @@

// Credential returns a credential based on the remote options.
func (opts *Remote) Credential() auth.Credential {
return credential.Credential(opts.Username, opts.Password)
if opts.IdentityToken != "" {
// If IdentityToken is provided, use it as the credential without a username
return auth.Credential{
Username: "",
Password: opts.IdentityToken,
}

Check warning on line 284 in cmd/oras/internal/option/remote.go

View check run for this annotation

Codecov / codecov/patch

cmd/oras/internal/option/remote.go#L280-L284

Added lines #L280 - L284 were not covered by tests
} else {
// If IdentityToken is not provided, use the username and password as credentials
return auth.Credential{
Username: opts.Username,
Password: opts.Password,
}
}
Comment on lines +279 to +291
Copy link
Contributor

Choose a reason for hiding this comment

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

This block of code should be reverted since credential.Credential() already handles everything.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, will fix it.

}

func (opts *Remote) handleWarning(registry string, logger logrus.FieldLogger) func(warning remote.Warning) {
Expand Down
22 changes: 17 additions & 5 deletions cmd/oras/root/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@
Example - Log in with username and password from command line flags:
oras login -u username -p password localhost:5000

Example - Log in with identity token from the command line flags:
oras login -i token localhost:5000

Example - Log in with username and password from stdin:
oras login -u username --password-stdin localhost:5000

Expand Down Expand Up @@ -79,25 +82,34 @@
func runLogin(ctx context.Context, opts loginOptions) (err error) {
ctx, logger := opts.WithContext(ctx)

// prompt for credential
if opts.Password == "" {
// Check if both '--username' and '--identity-token' are provided
if opts.Username != "" && opts.IdentityToken != "" {
return fmt.Errorf("both --username and --identity-token cannot be used together")
}

Check warning on line 88 in cmd/oras/root/login.go

View check run for this annotation

Codecov / codecov/patch

cmd/oras/root/login.go#L87-L88

Added lines #L87 - L88 were not covered by tests

// If IdentityToken is provided, use it as the credential without a username
if opts.IdentityToken != "" {
opts.Username = "" // Set the username to empty since it's not required when using identity token
opts.Password = opts.IdentityToken // Use the identity token as the password

Check warning on line 93 in cmd/oras/root/login.go

View check run for this annotation

Codecov / codecov/patch

cmd/oras/root/login.go#L92-L93

Added lines #L92 - L93 were not covered by tests
} else if opts.Password == "" {
Comment on lines +90 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

Those part of code should be reverted if we introduce opts.Secret.

Copy link
Author

Choose a reason for hiding this comment

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

Oh Okay. I will revert them. Let me know should I go with Secrets and SecretFromStdin or identity-token-stdin and password.

// If both '--password' and '--username' are not provided, prompt for credentials
if opts.Username == "" {
// prompt for username
// Prompt for username
username, err := readLine("Username: ", false)
if err != nil {
return err
}
opts.Username = strings.TrimSpace(username)
}
if opts.Username == "" {
// prompt for token
// Prompt for token
if opts.Password, err = readLine("Token: ", true); err != nil {
return err
} else if opts.Password == "" {
return errors.New("token required")
}
} else {
// prompt for password
// Prompt for password
if opts.Password, err = readLine("Password: ", true); err != nil {
return err
} else if opts.Password == "" {
Expand Down
Loading