feat(stepfunctions): grant APIs for state machine construct #8486

kaizencc · 2020-06-11T00:08:13Z

This PR closes #5933. It adds additional grant APIs to supplement grantStartExecution().

API additions to state-machine.ts:

grantRead() method that grants read access to a role.
grantTaskResponse() method that grants task response access to a role.
grantExecution() method that allows user to specify what action to map onto all executions.
grant() method that allows user to specify what action to map onto the state machine.

API additions to activity.ts:

grant() method that allows user to specify what action to map onto the activity.

The idea behind these API methods is to mimic the convention of other grant() APIs in other modules. This is slightly more difficult with Step Functions because of the multiple resources that IAM actions can map onto. The rationale is that grant() in state-machine.ts will handle all custom permissions for the state machine, grantExecution() in state-machine.ts will handle all custom permissions for the executions, and grant() in activity.ts will handle all custom permissions for activities.

grantRead() and grantTaskResponse() are convenience APIs that were specified in the original issue for this feature.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation · 2020-06-11T00:29:52Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7e8e82f
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-11T00:54:40Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 10feb2d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-11T00:57:52Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 663decf
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-11T16:01:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 6297b3a
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-11T16:01:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ca60e52
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-11T17:31:36Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 98fe1e8
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-11T17:31:46Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 83a1836
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

shivlaks

awesome start to adding this feature for step functions!!
few initial comments that need a bit of investigation.

packages/@aws-cdk/aws-stepfunctions/README.md

packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

shivlaks · 2020-06-11T17:36:39Z

packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

+        'states:SendTaskFailure',
+        'states:SendTaskHeartbeat',
+      ],
+      resourceArns: ['*'],


does this grant permissions to send task success for any state machine that exists in that account?
can it be scoped down any further? The * implies that success, failures, and heartbeats can be sent to any state machine.

IAM Policy Simulator says this for all three: "This action does not support resource-level permissions. Policies granting access must specify "*" in the resource element."

From the docs it seems it is possible to set resource level permission on these actions.

It seems like the iam policy simulator is not synced with the docs, I suggest we test this and follow up with the step functions team if required.

@kaizen3031593

I tested this on a state machine with an activity and SendTaskSuccess/Failure/Heartbeat worked correctly when mapped onto the activity. It did NOT work when mapped onto the state machine itself. This means that the IAM Policy Simulator is not correct.

I also tested on a state machine with a lambda function and SendTaskSuccess/Failure/Heartbeat worked when mapped onto the state machine.

At any rate, it seems like grantTaskResponse() will also require a resourceArn parameter which makes it very similar to grant(). Thus, I think it is best to omit grantTaskResponse() entirely and move forward with just the grant() API.

that seems reasonable. It would probably be useful to include an example snippet in the README to further guide users who are trying to grant these permissions

I disagree, I think that given how hard it was to discover how to configure this there is still value in the grantTaskResponse () method, additionally it configures all of the Activity-Level Permissions which in itself have value.

My issue is that grantTaskResponse() can take in both a stateMachineArn or an activityArn depending on what is sending the Task Token (i.e. lambdas can send Task Tokens but are not an activity). Perhaps a solution could be to do a grantTaskResponseForStateMachine() and a grantTaskResponseForActivity() method to split it up?

I agree with @NetaNir that we should add support for task responses, but I think it's fine to defer it to a future PR. The reason I think introducing the grant and grantRead APIs are reasonable is because they currently block normal use cases (and also unlock all other use cases).

task response is not universal to any state machine and either requires:

a task that will use the callback pattern

an activity task.

In either case, the grant is slightly different and you can also have a state machine with both.

I'm still in favour of introducing it at a later time (or introduce grant now in a separate PR).
The ability to issue a grant of any sort unlocks use cases that are currently not possible.

An API to enable permissions (grant) is a must. Having a convenience/sugar method for task responses (grantxYZ) is a nice to have.

@shivlaks and I discussed offline and decided to implement a grantTaskResponse() API that takes in both a principal and an optional ResourceArn. The ResourceArn should default to the state machine arn, which is the most obvious use case. When given a ResourceArn it will make sure that the arn is valid (i.e. is a state machine arn or an activity arn).

This should alleviate @NetaNir's concerns as it addresses activity-level permissions for grantTaskReponse(). It should also encapsulate both activity-level and state-machine-level granularity in one convenience method.

packages/@aws-cdk/aws-stepfunctions/test/integ.state-machine.expected.json

packages/@aws-cdk/aws-stepfunctions/test/state-machine-resources.test.ts

packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

into aws-logs

NetaNir · 2020-06-11T21:42:05Z

packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

+      ],
+      resourceArns: [`${this.executionArn}:*`],
+    });
+    return iam.Grant.addToPrincipal({


This part is confusing, can we just return identity and have the three grant blocks look the same?

awslint tells me that grant methods must return iam.Grant

NetaNir · 2020-06-11T22:04:27Z

packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

+        'states:SendTaskFailure',
+        'states:SendTaskHeartbeat',
+      ],
+      resourceArns: ['*'],


From the docs it seems it is possible to set resource level permission on these actions.

It seems like the iam policy simulator is not synced with the docs, I suggest we test this and follow up with the step functions team if required.

packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

packages/@aws-cdk/aws-stepfunctions/test/state-machine-resources.test.ts

Co-authored-by: Neta Nir <neta1nir@gmail.com>

aws-cdk-automation · 2020-06-12T00:07:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: eefe702
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Pull request has been modified.

aws-cdk-automation · 2020-06-17T23:36:44Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 037fc09
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-17T23:37:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: c92abdd
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-17T23:39:48Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 764b26a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…-cdk into aws-logs

aws-cdk-automation · 2020-06-18T15:51:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: df8db7d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-18T15:53:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: fb6a7ca
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

NetaNir · 2020-06-19T19:34:14Z

packages/@aws-cdk/aws-stepfunctions/README.md

+});
+
+activity.grant(role, 'states:SendTaskSuccess');
+```


Should we have a grantAll() methods? I feel like we are going to save a lot of copy pasta for users. @shivlaks what do you think?

i do see the value in reducing copy-pasta but would say it’s better to revisit the need at a later time. i’d be a little reluctant to add the grantAll mainly because:

StateMachine and Activity are 2 different resources in the Step Functions namespace - activities can be referenced inside of a state machine but both an exist independently, similar to Lambda functions, SQS queues, SNS topics, etc. These resources all offer their own grant APIs and that’s how permissions are managed today.

all is bound to carry a different meaning at a different point in time aside from the context in which it’s running. For example, when a new feature is added, we would need to update all and that new feature might not be pertinent to users.

I’d counter myself here by saying it might be okay since using all might mean the intent is to opt into everything without having to make future code changes. However, we haven’t done this anywhere else in the construct library yet either.

I get the not wanting to use the grantAll() terminology, but I still think we are going to see this code a lot:

activity.grant(role, 'states:SendTaskSuccess') activity.grant(role, 'states:DescribeActivity') activity.grant(role, 'states:DeleteActivity') activity.grant(role, 'states:GetActivityTask') activity.grant(role, 'SendTaskFailure') activity.grant(role, 'states:SendTaskHeartbeat')

Which can be reduced by having a more specific grant method, that being said I do agree we can hold this off until we get some customers feedback.

gotcha, I'd expect it to be a single call to grant with the list of permitted actions as a single parameter i.e. activity.grant(role, ['states:sentTaskSuccess', 'states:DescribeActivity', ...])

or logical groupings of APIs. You're still right that there will likely be a couple of grant calls that we can consolidate down the road.

NetaNir

Looks good.
One general comment, I think the README might be too long with this PR additions, might be worth considering shortening some sections and maybe moving some content to the respective methods documentation. I know this might sound controversial, as generally "more docs are better" but given this is stepfunctions top level README, it might be better to keeping it more focus on use cases.
@shivlaks I leave it to you to decide.

shivlaks · 2020-06-22T14:54:40Z

Looks good.
One general comment, I think the README might be too long with this PR additions, might be worth considering shortening some sections and maybe moving some content to the respective methods documentation. I know this might sound controversial, as generally "more docs are better" but given this is stepfunctions top level README, it might be better to keeping it more focus on use cases.
@shivlaks I leave it to you to decide.

I agree with the thought that we could be a little more succinct. At one point we did decide/try to include 2-3 lines and a simple example for public APIs. I think we can make this one a little tighter but am not strongly opinionated here.

One benefit of having it in the top level README is the translation of the code snippets in package overview for other languages (i.e. Python, Java, etc...)

mergify · 2020-06-22T15:00:00Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

NetaNir

🦄🦄🦄

Pull request has been modified.

aws-cdk-automation · 2020-06-22T16:13:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: af81358
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-22T16:35:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 831f468
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-06-22T16:35:36Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Conroy and others added 3 commits June 10, 2020 19:49

draft of grant APIs in step functions

7e8e82f

Merge branch 'master' into aws-logs

10feb2d

update readme to satisfy linter

663decf

SomayaB assigned shivlaks Jun 11, 2020

Conroy and others added 2 commits June 11, 2020 11:52

added grantRead() method

ca60e52

Merge branch 'master' into aws-logs

6297b3a

Conroy and others added 3 commits June 11, 2020 12:40

modified README and fixed lint issues

2657aeb

merging

83a1836

Merge branch 'master' into aws-logs

98fe1e8

NetaNir requested review from NetaNir and shivlaks June 11, 2020 17:33

NetaNir assigned NetaNir and shivlaks and unassigned shivlaks Jun 11, 2020

shivlaks reviewed Jun 11, 2020

View reviewed changes

Conroy added 2 commits June 11, 2020 15:51

updates

6465d1d

mergeMerge branch 'aws-logs' of https://github.com/kaizen3031593/aws-cdk

7af46e4

into aws-logs

kaizencc requested a review from shivlaks June 11, 2020 21:22

Conroy and others added 2 commits June 11, 2020 19:21

updated readme and privatized method

eefe702

Merge branch 'master' into aws-logs

4966f0d

NetaNir reviewed Jun 11, 2020

View reviewed changes

Update packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

94fd12d

Co-authored-by: Neta Nir <neta1nir@gmail.com>

Merge branch 'master' into aws-logs

c92abdd

Merge branch 'master' into aws-logs

764b26a

Kaizen Conroy and others added 3 commits June 18, 2020 11:04

slight modification to follow conventions

fbfe4a7

mergedMerge branch 'aws-logs' of https://github.com/kaizen3031593/aws…

df8db7d

…-cdk into aws-logs

Merge branch 'master' into aws-logs

fb6a7ca

NetaNir reviewed Jun 19, 2020

View reviewed changes

NetaNir reviewed Jun 21, 2020

View reviewed changes

shivlaks previously approved these changes Jun 22, 2020

View reviewed changes

NetaNir previously approved these changes Jun 22, 2020

View reviewed changes

Merge branch 'master' into aws-logs

af81358

shivlaks previously approved these changes Jun 22, 2020

View reviewed changes

small readme fix

831f468

shivlaks approved these changes Jun 22, 2020

View reviewed changes

mergify bot merged commit fe71364 into aws:master Jun 22, 2020

kaizencc deleted the aws-logs branch June 22, 2020 16:36

peterwoodworth mentioned this pull request May 12, 2023

(aws-stepfunctions): Restrict permissions from grantRead #25561

Open

feat(stepfunctions): grant APIs for state machine construct #8486

feat(stepfunctions): grant APIs for state machine construct #8486

Conversation

kaizencc commented Jun 11, 2020 • edited

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

shivlaks 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

NetaNir Jun 15, 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

aws-cdk-automation commented Jun 12, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 18, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 18, 2020

AWS CodeBuild CI Report

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

NetaNir left a comment

Choose a reason for hiding this comment

shivlaks commented Jun 22, 2020

mergify bot commented Jun 22, 2020

NetaNir left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 22, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 22, 2020

AWS CodeBuild CI Report

mergify bot commented Jun 22, 2020

kaizencc commented Jun 11, 2020 •

edited

NetaNir Jun 15, 2020 •

edited