Bug 1815219: CO-876: allow defining Conditions in AWS CredentialsRequest #181

Danil-Grigorev · 2020-04-14T12:33:44Z

Added support for conditions in AWS provider

Allows to use the same form as JSON conditions to specify rules to apply - https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html

apiVersion: cloudcredential.openshift.io/v1
kind: CredentialsRequest
.....
      - iam:PassRole
      condition:
        "Null":
          key: "true"
      effect: Allow
      resource: '*'

Danil-Grigorev · 2020-04-14T12:34:12Z

/assign @JoelSpeed

JoelSpeed

If there are any tests covering this, would be good to update the test cases to give a couple of examples of using the Conditions in the tests

JoelSpeed · 2020-04-14T12:46:07Z

pkg/apis/cloudcredential/v1/aws_types.go

+
+// NotEquals appends NotEquals prefix to the condition
+func (c AWSConditionType) NotEquals() AWSConditionType {
+	return c + "NotEquals"


Would a condition type of StringNotEquals unmarshal from JSON correctly?

It totally will, as it will undergo only a raw encoding to internal golang structure, where the types are still raw strings.

JoelSpeed · 2020-04-14T12:47:29Z

pkg/apis/cloudcredential/v1/aws_types.go

+	// String elements that restrict access based on comparing a key to a string value.
+	String AWSConditionType = "String"
+	// Numeric elements that restrict access based on comparing a key to an integer or decimal value.
+	Numeric AWSConditionType = "Numeric"
+	// Date elements that restrict access based on comparing a key to a date/time value.
+	Date AWSConditionType = "Date"
+	// Bool elements that restrict access based on comparing a key to "true" or "false."
+	Bool AWSConditionType = "Bool"
+	// Arn elements that restrict access based on comparing a key to an ARN.
+	Arn AWSConditionType = "Arn"
+	// Null check if a condition key is present at the time of authorization.
+	Null AWSConditionType = "Null"
+	// Binary elements that test key values that are in binary format.
+	Binary AWSConditionType = "Binary"
+	// IPAddress restrict access based on comparing a key to an IPv4 or IPv6 address or range of IP addresses.
+	IPAddress AWSConditionType = "IpAddress"


Might be worth suffixing these with ConditionType? Eg StringConditionType to make it more obvious what they are for and also distinguish them from other potentially exported fields that could be added at a later date

Yeah, makes sense. Will update it, that sounds more cannon.

Danil-Grigorev · 2020-04-14T13:06:09Z

/cc @joelddiaz Here is a PR for https://issues.redhat.com/browse/CO-876

abhinavdahiya · 2020-04-14T17:29:32Z

pkg/apis/cloudcredential/v1/aws_types.go

@@ -38,6 +38,8 @@ type StatementEntry struct {
 	Action []string `json:"action"`
 	// Resource specifies the object(s) this statement should apply to. (or "*" for all)
 	Resource string `json:"resource"`
+	// Condition specifies under which condition StatementEntry will apply
+	Condition AWSCondition `json:"condition,omitempty"`


Condition AWSCondition `json:"condition,omitempty"` -> Condition StatementEntryCondition `json:"condition,omitempty"`

abhinavdahiya · 2020-04-14T17:37:03Z

pkg/apis/cloudcredential/v1/aws_types.go

@@ -49,3 +51,71 @@ type AWSProviderStatus struct {
 	// Policy is the name of the policy attached to the user in AWS.
 	Policy string `json:"policy"`
 }
+
+// AWSCondition - map of condition types, with associated key - value mapping
+type AWSCondition map[AWSConditionType]AWSConditionKeyValue


Suggested change

type AWSCondition map[AWSConditionType]AWSConditionKeyValue

type StatementEntryCondition map[AWSIAMPolicyConditionType]AWSIAMPolicyConditionKeyValue

abhinavdahiya · 2020-04-14T17:37:48Z

pkg/apis/cloudcredential/v1/aws_types.go

+// AWSCondition - map of condition types, with associated key - value mapping
+type AWSCondition map[AWSConditionType]AWSConditionKeyValue
+
+// AWSConditionKeyValue - mapping of values for the chousen type


Suggested change

// AWSConditionKeyValue - mapping of values for the chousen type

// AWSConditionKeyValue - mapping of values for the chosen type

abhinavdahiya · 2020-04-14T17:38:18Z

pkg/apis/cloudcredential/v1/aws_types.go

+type AWSCondition map[AWSConditionType]AWSConditionKeyValue
+
+// AWSConditionKeyValue - mapping of values for the chousen type
+type AWSConditionKeyValue map[string]interface{}


Suggested change

type AWSConditionKeyValue map[string]interface{}

type AWSIAMPolicyConditionKeyValue map[string]interface{}

abhinavdahiya · 2020-04-14T17:38:31Z

pkg/apis/cloudcredential/v1/aws_types.go

+type AWSConditionKeyValue map[string]interface{}
+
+// AWSConditionType provides condition operators to match the condition key and value in the policy against values in the request context
+type AWSConditionType string


Suggested change

type AWSConditionType string

type AWSIAMPolicyConditionType string

abhinavdahiya · 2020-04-14T17:39:09Z

pkg/apis/cloudcredential/v1/aws_types.go

+	return "Not" + c
+}
+
+// IgnoreCase appends IgnoreCase prefix to the condition


Suggested change

// IgnoreCase appends IgnoreCase prefix to the condition

// IgnoreCase appends IgnoreCase suffix to the condition

abhinavdahiya · 2020-04-14T17:39:24Z

pkg/apis/cloudcredential/v1/aws_types.go

+	return c + "IgnoreCase"
+}
+
+// Equals appends Equals prefix to the condition


Suggested change

// Equals appends Equals prefix to the condition

// Equals appends Equals suffix to the condition

abhinavdahiya · 2020-04-14T17:39:39Z

pkg/apis/cloudcredential/v1/aws_types.go

+	return c + "Equals"
+}
+
+// NotEquals appends NotEquals prefix to the condition


Suggested change

// NotEquals appends NotEquals prefix to the condition

// NotEquals appends NotEquals suffix to the condition

abhinavdahiya · 2020-04-14T17:39:47Z

pkg/apis/cloudcredential/v1/aws_types.go

+	return c + "NotEquals"
+}
+
+// Like appends Like prefix to the condition


Suggested change

// Like appends Like prefix to the condition

// Like appends Like suffix to the condition

abhinavdahiya · 2020-04-14T17:39:56Z

pkg/apis/cloudcredential/v1/aws_types.go

+	return c + "Like"
+}
+
+// NotLike appends NotLike prefix to the condition


Suggested change

// NotLike appends NotLike prefix to the condition

// NotLike appends NotLike suffix to the condition

abhinavdahiya · 2020-04-14T17:40:20Z

pkg/apis/cloudcredential/v1/aws_types.go

+	return c + "NotLike"
+}
+
+// LessThan appends LessThan prefix to the condition


Suggested change

// LessThan appends LessThan prefix to the condition

// LessThan appends LessThan suffix to the condition

abhinavdahiya · 2020-04-14T17:40:29Z

pkg/apis/cloudcredential/v1/aws_types.go

+	return c + "LessThan"
+}
+
+// GreaterThan appends GreaterThan prefix to the condition


Suggested change

// GreaterThan appends GreaterThan prefix to the condition

// GreaterThan appends GreaterThan suffix to the condition

joelddiaz

Why all the defining of AWSCondition type constants and all the ConditionType modifiers? CCO doesn't really parse or populate these fields as they are simply passed on to the AWS IAM API calls. And these functions allow building invalid text (DateNotEqualsGreaterThan).

I would have done something more simple like:
type AWSIAMPolicyConditionKeyValue map[string]interface{}
and
type AWSIAMCondition map[string]AWSIAMPolicyConditionKeyValue

Many of the keys for conditions will end up with extra qualifiers when defining a set of conditions.

"Condition": {
                "ForAllValues:DateNotEquals": {
                    "aws:EpochTime": "123"
                },
                "ForAllValues:StringEquals": {
                    "aws:PrincipalAccount": "abcd",
                    "aws:PrincipalArn": "12345"
                },
                "StringEquals": {
                    "aws:userid": "usera"
                }
            }

TL;DR: What is the benefit of adding all these formal definitions of the AWS IAM condition types? It feels like CCO will be out of step with AWS if/when new modifiers are added as valid condition modifiers.

Danil-Grigorev · 2020-04-14T21:29:55Z

@joelddiaz Sorry, looking at the PR now, I can't see a reason for presence of AWS IAM condition types, or it will actually be a total overhead for the scope of the PR. Agree with you, going to remove unnecessary parts and change naming.

Danil-Grigorev · 2020-04-15T10:34:25Z

@joelddiaz Tested this PR in my setup, works fine for any combination of conditions. I addressed concerns around constants, and decided to remove them, as it sounded clear that is not the thing needed for a static template provided externally. @JoelSpeed I hope this integration is enough to cover the needs for the conditions. In case the tests are still needed to display the usage, I have a sample part to update the PR on request, but aside from that the tests are of no use here. This PR is sufficient in it's stage, as it is only backfilling missing fields of supported template.

enxebre · 2020-04-15T10:54:42Z

I'm retitling to make sure this is tracked along with the BZ issue. Same need to be done with the upcoming counterpart PR for the machine API Operator.
/retitle Bug 1815219: CO-876: allow defining Conditions in AWS CredentialsRequest

openshift-ci-robot · 2020-04-15T10:55:20Z

@Danil-Grigorev: This pull request references Bugzilla bug 1815219, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1815219: CO-876: allow defining Conditions in AWS CredentialsRequest

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.

abhinavdahiya

LGTM

pkg/apis/cloudcredential/v1/aws_types.go

JoelSpeed · 2020-04-16T15:57:07Z

I've manually verified that this works with openshift/machine-api-operator#557 to create the expected policy on the user

enxebre · 2020-04-17T08:13:14Z

lgtm as well @joelddiaz @dgoodwin you folks good to merge?

joelddiaz

this looks good. can you please squash the committs? then we can get this merged

enxebre · 2020-04-21T08:36:47Z

thanks @Danil-Grigorev ping @joelddiaz :)

joelddiaz

this looks good to me.
@dgoodwin maybe a second set of eyes here before we merge this..

dgoodwin

Just two nits.

dgoodwin · 2020-04-21T15:21:03Z

pkg/aws/actuator/actuator.go

+	Effect    string
+	Action    []string
+	Resource  string
+	Condition minterv1.IAMPolicyCondition `json:",omitempty"`


Should this json tag be here? it looks like it shouldn't.

No, it is necessary, as an empty condition in the template is not accepted by AWS validation

dgoodwin · 2020-04-21T17:16:45Z

pkg/apis/cloudcredential/v1/aws_types.go

@@ -38,6 +38,8 @@ type StatementEntry struct {
 	Action []string `json:"action"`
 	// Resource specifies the object(s) this statement should apply to. (or "*" for all)
 	Resource string `json:"resource"`
+	// Condition specifies under which condition StatementEntry will apply
+	Condition IAMPolicyCondition `json:"condition,omitempty"`


I'd prefer "iamPolicyCondition" or "policyCondition" to "condition" for the json key. Condition collides a little too much with typical status.conditions.

Ok, fixing now.

Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>

dgoodwin · 2020-04-22T13:08:22Z

/lgtm

openshift-ci-robot · 2020-04-22T13:12:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, Danil-Grigorev, dgoodwin

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~~ [dgoodwin]

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

openshift-bot · 2020-04-22T14:38:48Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-22T15:17:48Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-04-22T16:08:59Z

@Danil-Grigorev: Some pull requests linked via external trackers have merged: openshift/cloud-credential-operator#181. The following pull requests linked via external trackers have not merged:

openshift/machine-api-operator#557 is open
Bugzilla bug 1815219 has been moved to the MODIFIED state.

In response to this:

Bug 1815219: CO-876: allow defining Conditions in AWS CredentialsRequest

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.

openshift-ci-robot assigned JoelSpeed Apr 14, 2020

openshift-ci-robot requested review from abutcher and twiest April 14, 2020 12:34

JoelSpeed reviewed Apr 14, 2020

View reviewed changes

abhinavdahiya reviewed Apr 14, 2020

View reviewed changes

joelddiaz reviewed Apr 14, 2020

View reviewed changes

Danil-Grigorev requested a review from joelddiaz April 14, 2020 21:32

Danil-Grigorev changed the title ~~Added support for conditions in AWS provider~~ CO-876: allow defining Conditions in AWS CredentialsRequest Apr 14, 2020

openshift-ci-robot changed the title ~~CO-876: allow defining Conditions in AWS CredentialsRequest~~ Bug 1815219: CO-876: allow defining Conditions in AWS CredentialsRequest Apr 15, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 15, 2020

abhinavdahiya approved these changes Apr 15, 2020

View reviewed changes

JoelSpeed reviewed Apr 16, 2020

View reviewed changes

pkg/apis/cloudcredential/v1/aws_types.go Outdated Show resolved Hide resolved

Danil-Grigorev force-pushed the aws-conditions branch from 39ea1f5 to e2f5f03 Compare April 16, 2020 09:45

JoelSpeed mentioned this pull request Apr 16, 2020

BUG 1815219: Allow machines to have encrypted EBS volumes with non-default key openshift/machine-api-operator#557

Merged

joelddiaz reviewed Apr 20, 2020

View reviewed changes

Danil-Grigorev force-pushed the aws-conditions branch from e2f5f03 to 03c7750 Compare April 20, 2020 15:34

joelddiaz reviewed Apr 21, 2020

View reviewed changes

dgoodwin reviewed Apr 21, 2020

View reviewed changes

Added support for conditions in AWS provider

019d973

Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>

Danil-Grigorev force-pushed the aws-conditions branch from 03c7750 to 019d973 Compare April 22, 2020 13:03

Danil-Grigorev requested review from dgoodwin and joelddiaz April 22, 2020 13:05

openshift-ci-robot assigned dgoodwin Apr 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020

openshift-merge-robot merged commit c442add into openshift:master Apr 22, 2020

	type AWSCondition map[AWSConditionType]AWSConditionKeyValue
	type StatementEntryCondition map[AWSIAMPolicyConditionType]AWSIAMPolicyConditionKeyValue

	// AWSConditionKeyValue - mapping of values for the chousen type
	// AWSConditionKeyValue - mapping of values for the chosen type

	type AWSConditionKeyValue map[string]interface{}
	type AWSIAMPolicyConditionKeyValue map[string]interface{}

	type AWSConditionType string
	type AWSIAMPolicyConditionType string

	// IgnoreCase appends IgnoreCase prefix to the condition
	// IgnoreCase appends IgnoreCase suffix to the condition

	// Equals appends Equals prefix to the condition
	// Equals appends Equals suffix to the condition

	// NotEquals appends NotEquals prefix to the condition
	// NotEquals appends NotEquals suffix to the condition

	// Like appends Like prefix to the condition
	// Like appends Like suffix to the condition

	// NotLike appends NotLike prefix to the condition
	// NotLike appends NotLike suffix to the condition

	// LessThan appends LessThan prefix to the condition
	// LessThan appends LessThan suffix to the condition

	// GreaterThan appends GreaterThan prefix to the condition
	// GreaterThan appends GreaterThan suffix to the condition

Bug 1815219: CO-876: allow defining Conditions in AWS CredentialsRequest #181

Bug 1815219: CO-876: allow defining Conditions in AWS CredentialsRequest #181

Conversation

Danil-Grigorev commented Apr 14, 2020 • edited

Danil-Grigorev commented Apr 14, 2020

JoelSpeed 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

Danil-Grigorev commented Apr 14, 2020

Choose a reason for hiding this comment

abhinavdahiya Apr 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya Apr 14, 2020 • edited

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

joelddiaz left a comment

Choose a reason for hiding this comment

Danil-Grigorev commented Apr 14, 2020

Danil-Grigorev commented Apr 15, 2020

enxebre commented Apr 15, 2020

openshift-ci-robot commented Apr 15, 2020

abhinavdahiya left a comment

Choose a reason for hiding this comment

JoelSpeed commented Apr 16, 2020

enxebre commented Apr 17, 2020 • edited

joelddiaz left a comment

Choose a reason for hiding this comment

enxebre commented Apr 21, 2020

joelddiaz left a comment

Choose a reason for hiding this comment

dgoodwin 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

dgoodwin commented Apr 22, 2020

openshift-ci-robot commented Apr 22, 2020

openshift-bot commented Apr 22, 2020

openshift-bot commented Apr 22, 2020

openshift-ci-robot commented Apr 22, 2020

Danil-Grigorev commented Apr 14, 2020 •

edited

abhinavdahiya Apr 14, 2020 •

edited

abhinavdahiya Apr 14, 2020 •

edited

enxebre commented Apr 17, 2020 •

edited