restructure credential creation with simplified expectations (#4939)

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 24, 2024 · a8fdcbe · a8fdcbe
1 parent d4b7b00
commit a8fdcbe
Show file tree

Hide file tree

Showing 3 changed files with 114 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,77 @@ 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 one 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.")
-		}
-	} else if expDurVal != 0 {
-		exp = time.Now().Add(expDurVal)
-	} else {
-		exp = time.Unix(0, 0)
-	}
-
-	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.")
+		if !found {
+			fatalIf(probe.NewError(fmt.Errorf("invalid expiry date format '%s'", expVal)), "unable to parse the expiry argument")
 		}
+	case expDurVal != 0:
+		t := time.Now().Add(expDurVal)
+		opts.Expiration = &t
 	}
 
-	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
 }