restructure credential creation with simplified expectations

avoid weird coding style, simplify by using relatively less number of variables and move things around to make code more readable. improve HELP output readability.
minio · May 21, 2024 · d33a187 · d33a187
1 parent b46cf3c
commit d33a187
Show file tree

Hide file tree

Showing 3 changed files with 117 additions and 118 deletions.
diff --git a/cmd/admin-user-svcacct-add.go b/cmd/admin-user-svcacct-add.go
@@ -227,32 +227,32 @@ func (u acctMessage) String() string {
 
 // generateCredentials - creates randomly generated credentials of maximum
 // allowed length.
-func generateCredentials() (accessKey, secretKey string, err error) {
-	readBytes := func(size int) (data []byte, err error) {
+func generateCredentials() (accessKey, secretKey string, err *probe.Error) {
+	readBytes := func(size int) (data []byte, e error) {
 		data = make([]byte, size)
 		var n int
-		if n, err = rand.Read(data); err != nil {
-			return nil, err
+		if n, e = rand.Read(data); e != nil {
+			return nil, e
 		} else if n != size {
 			return nil, fmt.Errorf("Not enough data. Expected to read: %v bytes, got: %v bytes", size, n)
 		}
 		return data, nil
 	}
 
 	// Generate access key.
-	keyBytes, err := readBytes(accessKeyMaxLen)
-	if err != nil {
-		return "", "", err
+	keyBytes, e := readBytes(accessKeyMaxLen)
+	if e != nil {
+		return "", "", probe.NewError(e)
 	}
 	for i := 0; i < accessKeyMaxLen; i++ {
 		keyBytes[i] = alphaNumericTable[keyBytes[i]%alphaNumericTableLen]
 	}
 	accessKey = string(keyBytes)
 
 	// Generate secret key.
-	keyBytes, err = readBytes(secretKeyMaxLen)
-	if err != nil {
-		return "", "", err
+	keyBytes, e = readBytes(secretKeyMaxLen)
+	if e != nil {
+		return "", "", probe.NewError(e)
 	}
 
 	secretKey = strings.ReplaceAll(string([]byte(base64.StdEncoding.EncodeToString(keyBytes))[:secretKeyMaxLen]),
@@ -294,7 +294,7 @@ func mainAdminUserSvcAcctAdd(ctx *cli.Context) error {
 	if len(accessKey) <= 0 || len(secretKey) <= 0 {
 		randomAccessKey, randomSecretKey, err := generateCredentials()
 		if err != nil {
-			fatalIf(probe.NewError(err), "Unable to add a new service account")
+			fatalIf(err, "unable to generate randomized access credentials")
 		}
 		if len(accessKey) <= 0 {
 			accessKey = randomAccessKey
@@ -308,54 +308,48 @@ func mainAdminUserSvcAcctAdd(ctx *cli.Context) error {
 	client, err := newAdminClient(aliasedURL)
 	fatalIf(err, "Unable to initialize admin connection.")
 
-	var policyBytes []byte
+	opts := madmin.AddServiceAccountReq{
+		AccessKey:   accessKey,
+		SecretKey:   secretKey,
+		Name:        name,
+		Description: description,
+		TargetUser:  user,
+	}
+
 	if policyPath != "" {
 		// Validate the policy document and ensure it has at least when statement
-		var e error
-		policyBytes, e = os.ReadFile(policyPath)
-		fatalIf(probe.NewError(e), "Unable to open the policy document.")
+		policyBytes, e := os.ReadFile(policyPath)
+		fatalIf(probe.NewError(e), "unable to read the policy document")
+
 		p, e := policy.ParseConfig(bytes.NewReader(policyBytes))
-		fatalIf(probe.NewError(e), "Unable to parse the policy document.")
+		fatalIf(probe.NewError(e), "unable to parse the policy document")
+
 		if p.IsEmpty() {
-			fatalIf(errInvalidArgument(), "Empty policy documents are not allowed.")
+			fatalIf(errInvalidArgument(), "empty policies are not allowed")
 		}
-	}
 
-	var expiryTime time.Time
-	var expiryPointer *time.Time
+		opts.Policy = policyBytes
+	}
 
 	if expiry != "" {
 		location, e := time.LoadLocation("Local")
-		if e != nil {
-			fatalIf(probe.NewError(e), "Unable to parse the expiry argument.")
-		}
+		fatalIf(probe.NewError(e), "unable to load local location verify your local TZ=<val> settings")
 
-		patternMatched := false
+		var found bool
 		for _, format := range supportedTimeFormats {
 			t, e := time.ParseInLocation(format, expiry, location)
 			if e == nil {
-				patternMatched = true
-				expiryTime = t
-				expiryPointer = &expiryTime
+				found = true
+				opts.Expiration = &t
 				break
 			}
 		}
 
-		if !patternMatched {
-			fatalIf(probe.NewError(fmt.Errorf("expiry argument is not matching any of the supported patterns")), "unable to parse the expiry argument.")
+		if !found {
+			fatalIf(probe.NewError(fmt.Errorf("expiry argument is not matching any of the supported patterns")), "unable to parse the expiry argument")
 		}
 	}
 
-	opts := madmin.AddServiceAccountReq{
-		Policy:      policyBytes,
-		AccessKey:   accessKey,
-		SecretKey:   secretKey,
-		Name:        name,
-		Description: description,
-		TargetUser:  user,
-		Expiration:  expiryPointer,
-	}
-
 	creds, e := client.AddServiceAccount(globalContext, opts)
 	fatalIf(probe.NewError(e).Trace(args...), "Unable to add a new service account.")
 

diff --git a/cmd/idp-ldap-accesskey-create-with-login.go b/cmd/idp-ldap-accesskey-create-with-login.go
@@ -34,7 +34,7 @@ import (
 
 var idpLdapAccesskeyCreateWithLoginCmd = cli.Command{
 	Name:         "create-with-login",
-	Usage:        "log in using LDAP credentials to generate access key pair",
+	Usage:        "login using LDAP credentials to generate access key pair",
 	Action:       mainIDPLdapAccesskeyCreateWithLogin,
 	Before:       setGlobalsFromContext,
 	Flags:        append(idpLdapAccesskeyCreateFlags, globalFlags...),
@@ -51,31 +51,27 @@ FLAGS:
 EXAMPLES:
   1. Create a new access key pair for https://minio.example.com by logging in with LDAP credentials
      {{.Prompt}} {{.HelpName}} https://minio.example.com
+
   2. Create a new access key pair for http://localhost:9000 via login with custom access key and secret key 
      {{.Prompt}} {{.HelpName}} http://localhost:9000 --access-key myaccesskey --secret-key mysecretkey
-	`,
+`,
 }
 
 func mainIDPLdapAccesskeyCreateWithLogin(ctx *cli.Context) error {
-	if len(ctx.Args()) != 1 {
+	if !ctx.Args().Present() {
 		showCommandHelpAndExit(ctx, 1) // last argument is exit code
 	}
 
-	args := ctx.Args()
-	url := args.Get(0)
-
-	opts := accessKeyCreateOpts(ctx, "")
-
 	isTerminal := term.IsTerminal(int(os.Stdin.Fd()))
 	if !isTerminal {
-		e := fmt.Errorf("login flag cannot be used with non-interactive terminal")
-		fatalIf(probe.NewError(e), "Invalid flags.")
+		e := fmt.Errorf("login flag cannot be used with a non-interactive terminal")
+		fatalIf(probe.NewError(e), "unable to read from STDIN")
 	}
 
-	client := loginLDAPAccesskey(url)
+	client, opts := loginLDAPAccesskey(ctx)
 
 	res, e := client.AddServiceAccountLDAP(globalContext, opts)
-	fatalIf(probe.NewError(e), "Unable to add service account.")
+	fatalIf(probe.NewError(e), "unable to add service account")
 
 	m := ldapAccesskeyMessage{
 		op:          "create",
@@ -91,32 +87,34 @@ func mainIDPLdapAccesskeyCreateWithLogin(ctx *cli.Context) error {
 	return nil
 }
 
-func loginLDAPAccesskey(URL string) *madmin.AdminClient {
+func loginLDAPAccesskey(ctx *cli.Context) (*madmin.AdminClient, madmin.AddServiceAccountReq) {
+	urlStr := ctx.Args().First()
+
 	console.SetColor(cred, color.New(color.FgYellow, color.Italic))
 	reader := bufio.NewReader(os.Stdin)
 
 	fmt.Printf("%s", console.Colorize(cred, "Enter LDAP Username: "))
 	value, _, e := reader.ReadLine()
-	fatalIf(probe.NewError(e), "Unable to read username")
+	fatalIf(probe.NewError(e), "unable to read username")
 	username := string(value)
 
-	fmt.Printf("%s", console.Colorize(cred, "Enter Password: "))
+	fmt.Printf("%s", console.Colorize(cred, "Enter LDAP Password: "))
 	bytePassword, e := term.ReadPassword(int(os.Stdin.Fd()))
-	fatalIf(probe.NewError(e), "Unable to read password")
+	fatalIf(probe.NewError(e), "unable to read password")
 	fmt.Printf("\n")
 	password := string(bytePassword)
 
-	ldapID, e := credentials.NewLDAPIdentity(URL, username, password)
-	fatalIf(probe.NewError(e), "Unable to initialize LDAP identity.")
+	stsCreds, e := credentials.NewLDAPIdentity(urlStr, username, password)
+	fatalIf(probe.NewError(e), "unable to initialize LDAP identity")
 
-	u, e := url.Parse(URL)
-	fatalIf(probe.NewError(e), "Unable to parse server URL.")
+	u, e := url.Parse(urlStr)
+	fatalIf(probe.NewError(e), "unable to parse server URL")
 
 	client, e := madmin.NewWithOptions(u.Host, &madmin.Options{
-		Creds:  ldapID,
+		Creds:  stsCreds,
 		Secure: u.Scheme == "https",
 	})
-	fatalIf(probe.NewError(e), "Unable to initialize admin connection.")
+	fatalIf(probe.NewError(e), "unable to initialize admin connection")
 
-	return client
+	return client, accessKeyCreateOpts(ctx, username)
 }
diff --git a/cmd/idp-ldap-accesskey-create.go b/cmd/idp-ldap-accesskey-create.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2015-2023 MinIO, Inc.
+// Copyright (c) 2015-2024 MinIO, Inc.
 //
 // This file is part of MinIO Object Storage stack
 //
@@ -19,6 +19,7 @@ package cmd
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"os"
 	"time"
@@ -84,13 +85,16 @@ FLAGS:
 EXAMPLES:
   1. Create a new access key pair with the same policy as the authenticated user
      {{.Prompt}} {{.HelpName}} local/
+
   2. Create a new access key pair with custom access key and secret key
      {{.Prompt}} {{.HelpName}} local/ --access-key myaccesskey --secret-key mysecretkey
+
   4. Create a new access key pair for user with username "james" that expires in 1 day
      {{.Prompt}} {{.HelpName}} local/ james --expiry-duration 24h
+
   5. Create a new access key pair for authenticated user that expires on 2021-01-01
      {{.Prompt}} {{.HelpName}} --expiry 2021-01-01
-	`,
+`,
 }
 
 func mainIDPLdapAccesskeyCreate(ctx *cli.Context) error {
@@ -128,77 +132,80 @@ func mainIDPLdapAccesskeyCreate(ctx *cli.Context) error {
 }
 
 func accessKeyCreateOpts(ctx *cli.Context, targetUser string) madmin.AddServiceAccountReq {
-	accessVal := ctx.String("access-key")
-	secretVal := ctx.String("secret-key")
 	name := ctx.String("name")
-	description := ctx.String("description")
+	expVal := ctx.String("expiry")
 	policyPath := ctx.String("policy")
-
+	accessKey := ctx.String("access-key")
+	secretKey := ctx.String("secret-key")
+	description := ctx.String("description")
 	expDurVal := ctx.Duration("expiry-duration")
-	expVal := ctx.String("expiry")
+
+	// generate access key and secret key
+	if len(accessKey) <= 0 || len(secretKey) <= 0 {
+		randomAccessKey, randomSecretKey, err := generateCredentials()
+		if err != nil {
+			fatalIf(err, "unable to generate randomized access credentials")
+		}
+		if len(accessKey) <= 0 {
+			accessKey = randomAccessKey
+		}
+		if len(secretKey) <= 0 {
+			secretKey = randomSecretKey
+		}
+	}
 
 	if expVal != "" && expDurVal != 0 {
-		e := fmt.Errorf("Only one of --expiry or --expiry-duration can be specified")
-		fatalIf(probe.NewError(e), "Invalid flags.")
+		fatalIf(probe.NewError(errors.New("Only one of --expiry or --expiry-duration can be specified")), "invalid flags")
 	}
 
-	var exp time.Time
-	if expVal != "" {
-		location, e := time.LoadLocation("Local")
-		if e != nil {
-			fatalIf(probe.NewError(e), "Unable to parse the expiry argument.")
+	opts := madmin.AddServiceAccountReq{
+		TargetUser:  targetUser,
+		AccessKey:   accessKey,
+		SecretKey:   secretKey,
+		Name:        name,
+		Description: description,
+	}
+
+	if policyPath != "" {
+		// Validate the policy document and ensure it has at least when statement
+		policyBytes, e := os.ReadFile(policyPath)
+		fatalIf(probe.NewError(e), "unable to read the policy document")
+
+		p, e := policy.ParseConfig(bytes.NewReader(policyBytes))
+		fatalIf(probe.NewError(e), "unable to parse the policy document")
+
+		if p.IsEmpty() {
+			fatalIf(errInvalidArgument(), "empty policies are not allowed")
 		}
 
-		patternMatched := false
+		opts.Policy = policyBytes
+	}
+
+	switch {
+	case expVal != "":
+		location, e := time.LoadLocation("Local")
+		fatalIf(probe.NewError(e), "unable to load local location verify your local TZ=<val> settings")
+
+		var found bool
 		for _, format := range supportedTimeFormats {
 			t, e := time.ParseInLocation(format, expVal, location)
 			if e == nil {
-				patternMatched = true
-				exp = t
+				found = true
+				opts.Expiration = &t
 				break
 			}
 		}
 
-		if !patternMatched {
-			e := fmt.Errorf("invalid expiry date format '%s'", expVal)
-			fatalIf(probe.NewError(e), "unable to parse the expiry argument.")
+		if !found {
+			fatalIf(probe.NewError(fmt.Errorf("invalid expiry date format '%s'", expVal)), "unable to parse the expiry argument")
 		}
-	} else if expDurVal != 0 {
-		exp = time.Now().Add(expDurVal)
-	} else {
-		exp = time.Unix(0, 0)
+	case expDurVal != 0:
+		t := time.Now().Add(expDurVal)
+		opts.Expiration = &t
+	default:
+		t := time.Unix(0, 0)
+		opts.Expiration = &t
 	}
 
-	var policyBytes []byte
-	if policyPath != "" {
-		// Validate the policy document and ensure it has at least when statement
-		var e error
-		policyBytes, e = os.ReadFile(policyPath)
-		fatalIf(probe.NewError(e), "Unable to open the policy document.")
-		p, e := policy.ParseConfig(bytes.NewReader(policyBytes))
-		fatalIf(probe.NewError(e), "Unable to parse the policy document.")
-		if p.IsEmpty() {
-			fatalIf(errInvalidArgument(), "Empty policy documents are not allowed.")
-		}
-	}
-
-	accessKey, secretKey, e := generateCredentials()
-	fatalIf(probe.NewError(e), "Unable to generate credentials.")
-
-	// If access key and secret key are provided, use them instead
-	if accessVal != "" {
-		accessKey = accessVal
-	}
-	if secretVal != "" {
-		secretKey = secretVal
-	}
-	return madmin.AddServiceAccountReq{
-		Policy:      policyBytes,
-		TargetUser:  targetUser,
-		AccessKey:   accessKey,
-		SecretKey:   secretKey,
-		Name:        name,
-		Description: description,
-		Expiration:  &exp,
-	}
+	return opts
 }