OSD-22507: New IAM credential, secret rotate util #601

nephomaniac · 2024-07-23T20:55:33Z

First pass at OSD-22507. Attempting to consolidate previous bash, osdctl, additional SOP steps to into a single command/context to help users rotate Osd Admin user(s)' IAM creds and related secrets.

openshift-ci-robot · 2024-07-23T20:55:37Z

@nephomaniac: This pull request references OSD-22507 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

First pass at OSD-22507. Attempting to consolidate previous bash, osdctl, additional SOP steps to into a single command/context to help users rotate Osd Admin user(s)' IAM creds and related secrets.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-07-23T20:56:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nephomaniac
Once this PR has been reviewed and has the lgtm label, please assign iamkirkbater for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

iamkirkbater

Overall this looks very good! Got a few comments, but I don't think any of them are MAJOR changes, like I mentioned at standup some of them are little nits or things like expanding on what something is with a comment because it doesn't make sense at first glance.

iamkirkbater · 2024-07-25T17:07:55Z

cmd/account/iam-cred-rotate.go

+	rotateAWSCredsCmd.Flags().BoolVar(&ops.updateCcsCredsCli, "rotate-ccs-admin", false, "Rotate osdCcsAdmin user credentials. Interactive. Use caution!")
+	rotateAWSCredsCmd.Flags().BoolVar(&ops.describeKeysCli, "describe-keys", false, "Print AWS AccessKey info for osdManagedAdmin and osdCcsAdmin relevant cred rotation, and exit")
+	rotateAWSCredsCmd.Flags().BoolVar(&ops.describeSecretsCli, "describe-secrets", false, "Print AWS CredentialRequests ref'd secrets info relecant to cred rotation, and exit")
+	rotateAWSCredsCmd.Flags().BoolVar(&ops.saveSecretKeyToFile, "save-keys", false, "Save 'newly created' secret access key contents stdout output during execution")


When would an SRE use this portion of the command, or was this for testing purposes?

Removed/replaced the 'save-keys' option with a no-save option to negate saving a yaml copy of the secret/artifact to /tmp upon update() failure.

iamkirkbater · 2024-07-25T17:23:05Z

cmd/account/iam-cred-rotate.go

+			fmt.Println("Invalid input. Expecting (y)es or (N)o or 'all'")
+		}
+	}
+	if userInput == "y" || userInput == "all" {


I don't think we need this check here since a no will return and exit and the default case ends up just restarting the loop for another prompt if there's an invalid input.

If we want to have a bit of explicit-ness here, we could add a continue to the default case in the switch above which would force it to go back to the top of the loop in case someone put something else below that in the block, but since this specific if check is outside of the for loop I think we'll be safe to remove this if statement entirely.

We could further simplify this by changing userInput to be boolean variable named something like rotateAllWithoutPrompts, default it to false and then only in the case "all" we set that to false and check for that on line 515.

Thanks. Removed, cleaned this up in latest commit.

iamkirkbater · 2024-07-25T17:24:03Z

cmd/account/iam-cred-rotate.go

+			fmt.Fprintf(w, "\tSecret Namespace:\t'%s'\n", secret.Namespace)
+			fmt.Fprintf(w, "\tCreate:\t'%v'\n", secret.CreationTimestamp)
+			w.Flush()
+			fmt.Printf("Delete Secret ('%s'), ", secret.Name)


Is there a semantic reason for this not being part of the writer before it's flushed?

Oh, wait - I see this is printing the prompt for the confirmation. Should this be a question then? Or should this only be present if the userInput != "all"?

Thanks, updated this section. Separate print/flush is/was purely for the writer's formatting purposes.

iamkirkbater · 2024-07-25T20:21:25Z

cmd/account/iam-cred-rotate.go

+	// TODO: Can this differ from the login token needed earlier if the cluster
+	// lives in another env (ie integration or staging) (?) If so this needs to allow a 2 step process with
+	// multiple ocm login tokens(?), 1 for each env.


An OCM token should be valid for every environment.

If hive access (prod) is required, this may need more work to allow testing against a cluster running on staging/integration. The utils seem to assume a single ocm config context at the moment (?).

iamkirkbater · 2024-07-25T20:33:24Z

cmd/account/iam-cred-rotate.go

+	}
+	respBody := liveResponse.Body().Resources()
+	if awsAccountClaim, ok := respBody["aws_account_claim"]; ok {
+		var claimJson map[string]interface{}


Might be worth exploring pulling in the Account Claim structs here instead of using a map: https://github.com/openshift/aws-account-operator/blob/master/api/v1alpha1/accountclaim_types.go

thanks pulled in the account Claim struct. This function can/should likely be removed as it's currently only a backup to osdctl's k8s.GetAccountClaimFromClusterID() (added afterward).

iamkirkbater · 2024-07-25T20:45:56Z

cmd/account/iam-cred-rotate.go

+	}
+	var inUseStr string = "unknown?"
+	p := printer.NewTablePrinter(o.IOStreams.Out, 3, 1, 3, ' ')
+	headers := []string{"#", "ACCESS-KEY-ID", "CR-IN-USE", "CREATED", "STATUS", "IAM-USER"}


What is CR-IN-USE supposed to do here? I guess maybe expand on this with a comment?

Added a comment to explain the 'IN-USE' column/attribute indicates the current key referenced by the account CR.

iamkirkbater · 2024-07-25T20:52:04Z

cmd/account/iam-cred-rotate.go

+		return err
+	}
+	// Allow user to interactively delete keys if user has the max.
+	// Todo: Should this be presented when a user has less than max too?


I'd argue no - if there's less than the max we shouldn't need to worry about deleting any.

Then again - I don't think there's even a case to keep two - if we're rotating the creds then either they've been compromised (and therefore we'd want to rotate both out of caution anyway) or we've been asked to as a pre-emptive measure, in which case we should only have one.

I wouldn't change this for now, but let's keep this on our radar of something we may want to come back to later.

iamkirkbater · 2024-07-25T20:54:07Z

cmd/account/iam-cred-rotate.go

+	// Allow user to interactively delete keys if user has the max.
+	// Todo: Should this be presented when a user has less than max too?
+	if len(currKeys.AccessKeyMetadata) >= 2 {
+		// TODO: Should this script provide an option to delete the oldest key for the user? Too risky?


Same answer as above - I think ALL keys should be rotated with this.

iamkirkbater · 2024-07-25T21:07:30Z

cmd/account/iam-cred-rotate.go

+	checkUser, err := o.awsClient.GetUser(&iam.GetUserInput{UserName: awsSdk.String(osdManagedAdminUsername)})
+	var nse *iamTypes.NoSuchEntityException
+	if (err != nil && errors.As(err, &nse)) || checkUser == nil {
+		o.log.Warn(o.ctx, "User Not Found: '%s', trying user:'%s' instead...\n", osdManagedAdminUsername, common.OSDManagedAdminIAM)


Might be worth changing this to be an Info message - it's an edge case but it's not so uncommon to warrant a Warning which may be misinterpreted as something being wrong when in reality it's just the age of the account.

Updated to INFO level

iamkirkbater · 2024-07-25T21:10:32Z

cmd/account/iam-cred-rotate.go

+	// Todo: Make displaying the secret a cli provided arg? If this is useful for error/script failure recovery?
+	if o.saveSecretKeyToFile {
+		err := os.WriteFile(saveFileManaged, []byte(*createAccessKeyOutput.AccessKey.SecretAccessKey), 0600)
+		if err != nil {
+			o.log.Error(o.ctx, "Error! Saving key info to file:'%s', err:%s", saveFileManaged, err)
+			//return err
+		} else {
+			fmt.Printf("\nSaved key to: '%s'\n", saveFileManaged)
+		}
+	}


Maybe this would be better to add after the attempt to update the secret failed?

Actually - what might be better would be to have the secret attempt to be created, and then if it fails we just write the whole secret object's yaml to a tmp file with a random name (instead of being user-provided) and then the SRE would be able to just oc apply -f /tmp/whatever if there's something wrong, the data would already be base64 encoded and ready to run from there.

Then we could exit with an error, similar to when you oc edit [object] without permissions, it gives you an error and says you can re-run this command with 'oc replace -f /tmp/randomfilepath' to apply

Current behavior is to build and save the yaml representation of the secret in /tmp and print the path to this file(s). These can be used for apply -f or just review later. This will only happen if the key(s) have been created and something fails during the secret update().

…review/apply. Allow json, yaml output.

nephomaniac · 2024-08-05T22:10:33Z

Big thanks @iamkirkbater for the review and comments. Most recent commit should contain these suggestions as well as support -o yaml/json to stdout (logs to stderr) for the 'describe' specific commands.

nephomaniac · 2024-08-20T16:59:57Z

/test lint

…put.

cmd/account/iam-cred-rotate.go

clcollins

Generally super small comments/spelling suggestions. I have one concern with saving the keys that might just be me being too paranoid, and a suggestion about how to search for the credentialsRequests.

Otherwise great work! This is WAY more than I expected it to be when I wrote OSD-22507!

clcollins · 2024-08-30T19:07:18Z

cmd/account/iam-cred-rotate.go

+	if !o.updateCcsCredsCli && !o.updateMgmtCredsCli && !o.describeKeysCli && !o.describeSecretsCli {
+		return cmdutil.UsageErrorf(cmd, "must provide one or more actions: ('--rotate-managed-admin' and/or '--rotate-ccs-admin'), or '--describe-secrets', or '--describe-keys'")
+	}
+	if (o.updateCcsCredsCli || o.updateMgmtCredsCli) && (o.describeKeysCli || o.describeSecretsCli) {


Cobra has "Flag Groups" that you can use to mark mutually exclusive options.

clcollins · 2024-08-30T19:08:04Z

cmd/account/iam-cred-rotate.go

+		Example:           examples,
+		DisableAutoGenTag: true,
+		Run: func(cmd *cobra.Command, args []string) {
+			cmdutil.CheckErr(ops.preRunCliChecks(cmd, args))


The ops.preRunCliChecks might make more sense in the context of the Cobra PreRun: or PreRunE:.

clcollins · 2024-08-30T19:14:33Z

cmd/account/iam-cred-rotate.go

+		return nil
+	}
+	// Display cluster info, let user confirm they are on the correct cluster, etc...
+	o.printClusterInfo()


Has the cluster info already been printed by err = o.printClusterInfo() on line 239 already?

clcollins · 2024-08-30T19:15:44Z

cmd/account/iam-cred-rotate.go

+			return err
+		}
+	}
+	//


Suggested change

//

clcollins · 2024-08-30T19:20:06Z

cmd/account/iam-cred-rotate.go

+		o.log.Warn(o.ctx, "fetchCredentialsRequests returned err:'%s'\n", err)
+		return err
+	}
+	fmt.Printf("\nOptions --describe-keys, --describe-secrets can be used to provide additional info/status\n")


You use a mix of the logger and fmt.PrintX for output through here. Is that intentional, given this is a CLI tool and there's not a logfile/external logging.

(Genuinely asking - I have struggled with some decision making in ocm-container along the same lines).

clcollins · 2024-08-30T20:03:49Z

cmd/account/iam-cred-rotate.go

+	// lives in another env (ie integration or staging) (?) If so this needs to allow a 2 step process with
+	// multiple ocm login tokens(?), 1 for each env.
+	// Logging into Hive Shard here (equiv of 'ocm backplane login $HIVESHARD')
+	// Is specifially creating the hive connection here a better/worse option than connecting via 'ocm backplane'


Suggested change

// Is specifially creating the hive connection here a better/worse option than connecting via 'ocm backplane'

// Is specifically creating the hive connection here a better/worse option than connecting via 'ocm backplane'

clcollins · 2024-08-30T20:06:34Z

cmd/account/iam-cred-rotate.go

+	// multiple ocm login tokens(?), 1 for each env.
+	// Logging into Hive Shard here (equiv of 'ocm backplane login $HIVESHARD')
+	// Is specifially creating the hive connection here a better/worse option than connecting via 'ocm backplane'
+	// externally, and using the LazyClient (ie o.kubeCli) instead?


IMO - I think we should do all the hive connections required for this operation via code in these functions, because we can ensure everything (env, logged in clusters, etc) and there's no other interaction with (or need for) an external client.

clcollins · 2024-08-30T20:08:18Z

cmd/account/iam-cred-rotate.go

+	var err error
+
+	if len(o.accountIDSuffixLabel) <= 0 {
+		return fmt.Errorf("doAwsCredRoteate() empty required accountIDSuffixLabel")


Suggested change

return fmt.Errorf("doAwsCredRoteate() empty required accountIDSuffixLabel")

return fmt.Errorf("doAwsCredRotate() empty required accountIDSuffixLabel")

clcollins · 2024-08-30T20:21:50Z

cmd/account/iam-cred-rotate.go

+		o.log.Warn(o.ctx, "%v", err)
+		return "", err
+	}
+	saveFile, err := os.CreateTemp(os.TempDir(), string(*accessKey.UserName))


Is it overkill to add a chmod 600? Probably right? Something about saving the secret locally makes me uncomfortable, but I dunno if that's reasonable or not.

Lemme tractor-beam @wshearn for Team Security's take.

Maybe in the event of an error like this it could be displayed on-screen instead? I dunno, I'm probably being over-paranoid.

Thanks for the input! I agree. Knowing the flow better, I dont think there's a lot of benefit to this upon failure either. I'll remove all of this. In terms of recovering from errors during the key rotation portion, I've added some retry logic around the syncset portion (per potential syncset collisions noted in the original SOP).

clcollins · 2024-08-30T20:27:59Z

cmd/account/iam-cred-rotate.go

+		}
+		o.log.Debug(o.ctx, "Successfully updated secrets for '%s'\n", userName)
+	} else {
+		// Thi is a secondary check, and should have also been performed early on.


Suggested change

// Thi is a secondary check, and should have also been performed early on.

// This is a secondary check, and should have also been performed early on.

…fig builder, validation functions.

openshift-ci · 2024-09-09T01:09:58Z

@nephomaniac: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

OSD-22507: POC first pass at new rotate command

5407426

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 23, 2024

openshift-ci bot requested review from fahlmant and mjlshen July 23, 2024 20:56

nephomaniac marked this pull request as draft July 24, 2024 19:47

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2024

iamkirkbater reviewed Jul 25, 2024

View reviewed changes

OSD-22507: Implement suggestions in PR. Save yaml on fail for manual …

48b61ea

…review/apply. Allow json, yaml output.

nephomaniac marked this pull request as ready for review August 13, 2024 22:54

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2024

openshift-ci bot requested a review from typeid August 13, 2024 22:54

OSD-22507: Remove unused functions, clean up json encoding.

f2ea826

nephomaniac added 2 commits August 29, 2024 10:29

OSD-22507: Allow retry or saving failed syncset obj yaml

14d5a2b

OSD-22507: Update help text, and credreq obj for json,yaml secret out…

b974eb8

…put.

nephomaniac changed the title ~~OSD-22507: POC first pass at new rotate command (draft)~~ OSD-22507: POC first pass at new rotate command Aug 30, 2024

clcollins reviewed Aug 30, 2024

View reviewed changes

cmd/account/iam-cred-rotate.go Outdated Show resolved Hide resolved

clcollins reviewed Aug 30, 2024

View reviewed changes

cmd/account/iam-cred-rotate.go Outdated Show resolved Hide resolved

clcollins reviewed Aug 30, 2024

View reviewed changes

cmd/account/iam-cred-rotate.go Outdated Show resolved Hide resolved

clcollins reviewed Aug 30, 2024

View reviewed changes

cmd/account/iam-cred-rotate.go Outdated Show resolved Hide resolved

OSD-22507: Update typos in comments and help strings per PR review

ea8f5cc

clcollins requested changes Aug 30, 2024

View reviewed changes

OSD-22507: Allow cli provided OCM config for hive connection, and con…

93653d0

…fig builder, validation functions.

nephomaniac changed the title ~~OSD-22507: POC first pass at new rotate command~~ OSD-22507: New IAM credential, secret rotate util Sep 12, 2024

	// Is specifially creating the hive connection here a better/worse option than connecting via 'ocm backplane'
	// Is specifically creating the hive connection here a better/worse option than connecting via 'ocm backplane'

	return fmt.Errorf("doAwsCredRoteate() empty required accountIDSuffixLabel")
	return fmt.Errorf("doAwsCredRotate() empty required accountIDSuffixLabel")

	// Thi is a secondary check, and should have also been performed early on.
	// This is a secondary check, and should have also been performed early on.

OSD-22507: New IAM credential, secret rotate util #601

Are you sure you want to change the base?

OSD-22507: New IAM credential, secret rotate util #601

Conversation

nephomaniac commented Jul 23, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Jul 23, 2024 • edited by openshift-ci bot Loading

openshift-ci bot commented Jul 23, 2024

iamkirkbater left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nephomaniac commented Aug 5, 2024

nephomaniac commented Aug 20, 2024

clcollins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 9, 2024

nephomaniac commented Jul 23, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jul 23, 2024 •

edited by openshift-ci bot

Loading