credentials validation #1156

joelddiaz · 2019-01-30T20:16:13Z

do a pre-flight check of permissions using cloud-credentials-operator validation to do a check on the creds being used for installation

the initial list of permissions that gathers the AWS actions needed to perform an installation are taken verbatim from the IAM group permissions the hive team has been using to perform installation/uninstallation with (there absolutely could be some excess actions that used to be needed, but may no longer be needed)

note that the permissions checks are done with the assumption of IAM policies consisting of 'Resource: "*"'. so a list of ["ec2:CreateRoute", "ec2:CreateSubnet"] is evaluated as whether we can peform

{
    "Statement": [
        {
            "Action": [
                "ec2:CreateRoute",
                "ec2:CreateSubnet"
            ],
            "Effect": "Allow",
            "Resource": "*"
        }
    ]
}

joelddiaz · 2019-01-31T14:06:39Z

@abhinavdahiya @wking this is the direction i'm heading with this credential validation work. what do you think?
also, that static list of aws actions is probably worth a review with someone familiar with the installer's needs

wking

The approach looks good to me. I've left a few minor nits inline.

Gopkg.lock

pkg/asset/installconfig/platformcredscheck.go

wking · 2019-01-31T22:46:47Z

pkg/asset/installconfig/platformcredscheck.go

+	// EC2 related perms
+	"ec2:AllocateAddress",
+	"ec2:AssociateAddress",
+	"ec2:AssociateDhcpOptions",


Ugh :p. I think annotating our calls with something like 2658145 would be a more sustainable approach towards maintaining this slice. But hard-coding is fine in the short-term.

pkg/asset/installconfig/platformcredscheck.go

wking · 2019-01-31T22:58:42Z

pkg/asset/installconfig/platformcredscheck.go

+	logger := logrus.New()
+	awsClient, err := ccaws.NewClient([]byte(creds.AccessKeyID), []byte(creds.SecretAccessKey))
+	if err != nil {
+		return fmt.Errorf("error building aws client to check credentials against: %v", err)


nit: we generally use errors.Errorf instead of fmt.Errorf.

wking · 2019-01-31T22:59:20Z

pkg/asset/installconfig/platformcredscheck.go

+		return fmt.Errorf("error checking whether we have enough permissions to install: %v", err)
+	}
+	if !canInstall {
+		return fmt.Errorf("current credentials insufficient for performing cluster installation")


nit: this doesn't need string formatting, so it should use errors.New.

wking · 2019-01-31T23:00:23Z

pkg/asset/installconfig/platformcredscheck.go

+	}
+
+	logger := logrus.New()
+	awsClient, err := ccaws.NewClient([]byte(creds.AccessKeyID), []byte(creds.SecretAccessKey))


nit: client is probably sufficiently unique for validateAWSCreds.

joelddiaz · 2019-02-01T20:29:35Z

@wking broke out the vendor into its own commit, and addressed the feedback

staebler · 2019-02-04T15:47:32Z

pkg/asset/installconfig/platformcredscheck.go

@@ -36,6 +36,7 @@ func (a *PlatformCredsCheck) Generate(dependencies asset.Parents) error {
 	switch platform {
 	case aws.Name:
 		_, err = awsconfig.GetSession()
+		err = awsconfig.ValidateAWSCreds()


This will swallow any error from the call to awsconfig.GetSession.

staebler · 2019-02-04T15:48:30Z

pkg/asset/installconfig/aws/permissions.go

 )
+
+var installPermissionsAWS = []string{


The AWS part of installPermissionsAWS is redundant with the package.

staebler · 2019-02-04T15:48:49Z

pkg/asset/installconfig/aws/permissions.go

+// are sufficient to perform an installation, and that they can be used for cluster runtime
+// as either capable of creating new credentials for components that interact with the cloud or
+// being able to be passed through as-is to the components that need cloud credentials
+func ValidateAWSCreds() error {


The AWS part of ValidateAWSCreds is redundant with the package.

staebler · 2019-02-04T15:51:26Z

pkg/asset/installconfig/aws/permissions.go

+// as either capable of creating new credentials for components that interact with the cloud or
+// being able to be passed through as-is to the components that need cloud credentials
+func ValidateAWSCreds() error {
+	ssn, err := GetSession()


I think you should pass in the session as a parameter to ValidateAWSCreds. This will (1) fit in better with the long-term plans of getting the session from an asset in the store and (2) consolidate the 2 calls to GetSession in PlatformCredsChecker.Generate into a single call.

joelddiaz · 2019-02-04T17:06:09Z

@staebler thanks for the review. i've fixed the swallowing of the err and made the other changes as requested

wking · 2019-02-04T19:36:31Z

pkg/asset/installconfig/aws/permissions.go

+
+	client, err := ccaws.NewClient([]byte(creds.AccessKeyID), []byte(creds.SecretAccessKey))
+	if err != nil {
+		return fmt.Errorf("error building aws client to check credentials against: %v", err)


This should be errors.Errorf, and once you make that change you can drop the fmt import.

Actually, no, you're adding context to an existing error. It should be:

return errors.Wrap(err, "initialize cloud-credentials client")

The "to check credentials" bit should be addressed at the call-site in platformcredscheck.go, with something like:

ssn, err := awsconfig.GetSession() if err != nil { return err } err = awsconfig.ValidateCreds(ssn) if err != nil { return errors.Wrap(err, "validate AWS credentials") }

wking · 2019-02-04T19:42:58Z

pkg/asset/installconfig/aws/permissions.go

+	logger := logrus.New()
+	canInstall, err := credvalidator.CheckPermissionsAgainstActions(client, installPermissions, logger)
+	if err != nil {
+		return errors.Errorf("error checking whether we have enough permissions to install: %v", err)


Sorry for my poor earlier advice. When you have an error, you should use errors.Wrap (or Wrapf), not Errorf). See here. This applies to some of your other error-handling blocks as well.

staebler · 2019-02-04T19:54:45Z

pkg/asset/installconfig/platformcredscheck.go

+		if err != nil {
+			return err
+		}
+		err = awsconfig.ValidateCreds(ssn)


This sets the err variable local to this case and not the err variable that is returned from the Generate function. In other words, errors from awsconfig.ValidateCreds will be ignored.

Personally, I would much rather see each of these cases check err and return when there is an error within the case block rather than fill out a shared err variable that is returned at the end of the function.

joelddiaz · 2019-02-05T16:38:31Z

@wking now using the errors.Wrap() where appropriate (tricky behavior when err is actually nil!!)

@staebler fixed the silly error assignment issue with the most recent push

staebler · 2019-02-05T16:40:34Z

pkg/asset/installconfig/aws/permissions.go

+func ValidateCreds(ssn *session.Session) error {
+	creds, err := ssn.Config.Credentials.Get()
+	if err != nil {
+		return err


Wrap this error, too.

staebler · 2019-02-05T16:46:25Z

pkg/asset/installconfig/platformcredscheck.go

-		_, err = awsconfig.GetSession()
+		ssn, err := awsconfig.GetSession()
+		if err != nil {
+			return err


Wrap this error, too, while we're here.

joelddiaz · 2019-02-06T16:05:05Z

/retest

openshift-ci-robot · 2019-02-06T16:19:49Z

@joelddiaz: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-libvirt	`170d41d`	link	`/test e2e-libvirt`
ci/prow/e2e-openstack	`170d41d`	link	`/test e2e-openstack`
ci/prow/launch-aws	`170d41d`	link	`/test launch-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

joelddiaz · 2019-02-11T14:54:13Z

/test e2e-aws

joelddiaz · 2019-02-13T15:08:24Z

@wking @staebler what's the next step for this PR (besides un-WIP)?

staebler · 2019-02-13T15:28:32Z

@wking @staebler what's the next step for this PR (besides un-WIP)?

The works looks good to me. Does this need a feature exception?

joelddiaz · 2019-02-14T01:30:56Z

/hold

joelddiaz · 2019-02-14T19:48:13Z

@staebler @wking we've got the exception request +1d now

will use pieces from the repo for credential validation put some dummy imports in pkg/asset/installconfig/aws/permissions.go then run: dep ensure (version 0.5.0)

do a pre-flight check of permissions using cloud-credentials-operator validation to do a check on the creds being used for installation the initial list of permissions that gathers the AWS actions needed to perform an installation are taken verbatim from the IAM group permissions the hive team has been using to perform installation/uninstallation with (there absolutely could be some excess actions that used to be needed, but may no longer be needed) note that the permissions checks are done with the assumption of IAM policies consisting of 'Resource: "*"'. so a list of ["ec2:CreateRoute", "ec2:CreateSubnet"] is evaluated as whether we can peform ` { "Statement": [ { "Action": [ "ec2:CreateRoute", "ec2:CreateSubnet" ], "Effect": "Allow", "Resource": "*" } ] } `

staebler · 2019-02-14T20:09:59Z

/hold cancel
/lgtm

openshift-ci-robot · 2019-02-14T20:10:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelddiaz, staebler

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [staebler]

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

openshift-ci-robot requested review from aaronlevy and flaper87 January 30, 2019 20:16

joelddiaz force-pushed the creds-validation branch 2 times, most recently from cc3089f to 85eb699 Compare January 31, 2019 13:48

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2019

wking reviewed Jan 31, 2019

View reviewed changes

joelddiaz force-pushed the creds-validation branch from 85eb699 to ec37101 Compare February 1, 2019 20:22

staebler reviewed Feb 4, 2019

View reviewed changes

joelddiaz force-pushed the creds-validation branch from ec37101 to 1dcddc8 Compare February 4, 2019 17:04

wking reviewed Feb 4, 2019

View reviewed changes

staebler reviewed Feb 4, 2019

View reviewed changes

joelddiaz force-pushed the creds-validation branch from 1dcddc8 to 170d41d Compare February 5, 2019 16:34

staebler reviewed Feb 5, 2019

View reviewed changes

joelddiaz force-pushed the creds-validation branch 2 times, most recently from 008dc44 to f80230b Compare February 5, 2019 20:01

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2019

joelddiaz force-pushed the creds-validation branch from f80230b to 9635bc6 Compare February 11, 2019 16:33

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 11, 2019

joelddiaz changed the title ~~WIP: credentials validation~~ credentials validation Feb 13, 2019

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2019

Joel Diaz added 2 commits February 14, 2019 14:51

vendor openshift/cloud-credential-operator

c108005

will use pieces from the repo for credential validation put some dummy imports in pkg/asset/installconfig/aws/permissions.go then run: dep ensure (version 0.5.0)

joelddiaz force-pushed the creds-validation branch from 9635bc6 to 818b350 Compare February 14, 2019 19:51

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2019

openshift-ci-robot assigned staebler Feb 14, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 14, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2019

openshift-merge-robot merged commit 628cb94 into openshift:master Feb 14, 2019

joelddiaz mentioned this pull request Apr 4, 2019

Installer failed to create DNS records when using STS token #1010

Closed

joelddiaz deleted the creds-validation branch January 6, 2020 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

credentials validation #1156

credentials validation #1156

joelddiaz commented Jan 30, 2019

joelddiaz commented Jan 31, 2019

wking left a comment

wking Jan 31, 2019

wking Jan 31, 2019

wking Jan 31, 2019

wking Jan 31, 2019

joelddiaz commented Feb 1, 2019

staebler Feb 4, 2019

staebler Feb 4, 2019

staebler Feb 4, 2019

staebler Feb 4, 2019

joelddiaz commented Feb 4, 2019

wking Feb 4, 2019

wking Feb 4, 2019

wking Feb 4, 2019

staebler Feb 4, 2019

joelddiaz commented Feb 5, 2019

staebler Feb 5, 2019

staebler Feb 5, 2019

joelddiaz commented Feb 6, 2019

openshift-ci-robot commented Feb 6, 2019 •

edited

Loading

joelddiaz commented Feb 11, 2019

joelddiaz commented Feb 13, 2019

staebler commented Feb 13, 2019

joelddiaz commented Feb 14, 2019

joelddiaz commented Feb 14, 2019

staebler commented Feb 14, 2019

openshift-ci-robot commented Feb 14, 2019

credentials validation #1156

credentials validation #1156

Conversation

joelddiaz commented Jan 30, 2019

joelddiaz commented Jan 31, 2019

wking 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

joelddiaz commented Feb 1, 2019

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

joelddiaz commented Feb 4, 2019

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

joelddiaz commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelddiaz commented Feb 6, 2019

openshift-ci-robot commented Feb 6, 2019 • edited Loading

joelddiaz commented Feb 11, 2019

joelddiaz commented Feb 13, 2019

staebler commented Feb 13, 2019

joelddiaz commented Feb 14, 2019

joelddiaz commented Feb 14, 2019

staebler commented Feb 14, 2019

openshift-ci-robot commented Feb 14, 2019

openshift-ci-robot commented Feb 6, 2019 •

edited

Loading