From 33cad5b79686b0fbaba8a5b98d6151de5557c92c Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 21 May 2024 10:23:56 -0700 Subject: [PATCH 1/3] 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. --- cmd/admin-user-svcacct-add.go | 72 ++++++------ cmd/idp-ldap-accesskey-create-with-login.go | 44 ++++---- cmd/idp-ldap-accesskey-create.go | 116 ++++++++++---------- 3 files changed, 114 insertions(+), 118 deletions(-) diff --git a/cmd/admin-user-svcacct-add.go b/cmd/admin-user-svcacct-add.go index b36f280d3d..748f8d9a07 100644 --- a/cmd/admin-user-svcacct-add.go +++ b/cmd/admin-user-svcacct-add.go @@ -227,12 +227,12 @@ 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) } @@ -240,9 +240,9 @@ func generateCredentials() (accessKey, secretKey string, err error) { } // 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] @@ -250,9 +250,9 @@ func generateCredentials() (accessKey, secretKey string, err error) { 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= 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 index 0449fcd7e7..1ca46267ae 100644 --- 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 index 3ef3c589d4..a78b3c0742 100644 --- 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 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= 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 } - 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 } From fa92333687c812c0ba21011b47839c380657d827 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 23 May 2024 00:59:14 -0700 Subject: [PATCH 2/3] Update cmd/idp-ldap-accesskey-create.go Co-authored-by: Taran Pelkey --- cmd/idp-ldap-accesskey-create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/idp-ldap-accesskey-create.go b/cmd/idp-ldap-accesskey-create.go index a78b3c0742..67f3e56a0b 100644 --- a/cmd/idp-ldap-accesskey-create.go +++ b/cmd/idp-ldap-accesskey-create.go @@ -167,7 +167,7 @@ func accessKeyCreateOpts(ctx *cli.Context, targetUser string) madmin.AddServiceA } if policyPath != "" { - // Validate the policy document and ensure it has at least when statement + // 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") From 88e47815b15a2017766e9d0bbd17182380138122 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 23 May 2024 09:38:36 -0700 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Shubhendu --- cmd/admin-user-svcacct-add.go | 2 +- cmd/idp-ldap-accesskey-create.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/admin-user-svcacct-add.go b/cmd/admin-user-svcacct-add.go index 748f8d9a07..713f4074d3 100644 --- a/cmd/admin-user-svcacct-add.go +++ b/cmd/admin-user-svcacct-add.go @@ -333,7 +333,7 @@ func mainAdminUserSvcAcctAdd(ctx *cli.Context) error { if expiry != "" { location, e := time.LoadLocation("Local") - fatalIf(probe.NewError(e), "unable to load local location verify your local TZ= settings") + fatalIf(probe.NewError(e), "unable to load local location. verify your local TZ= settings") var found bool for _, format := range supportedTimeFormats { diff --git a/cmd/idp-ldap-accesskey-create.go b/cmd/idp-ldap-accesskey-create.go index 67f3e56a0b..f1fc111d67 100644 --- a/cmd/idp-ldap-accesskey-create.go +++ b/cmd/idp-ldap-accesskey-create.go @@ -184,7 +184,7 @@ func accessKeyCreateOpts(ctx *cli.Context, targetUser string) madmin.AddServiceA switch { case expVal != "": location, e := time.LoadLocation("Local") - fatalIf(probe.NewError(e), "unable to load local location verify your local TZ= settings") + fatalIf(probe.NewError(e), "unable to load local location. verify your local TZ= settings") var found bool for _, format := range supportedTimeFormats {