feat(stepfunctions): grant APIs for state machine construct #8486
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| 'states:SendTaskFailure', | ||
| 'states:SendTaskHeartbeat', | ||
| ], | ||
| resourceArns: ['*'], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions/test/state-machine-resources.test.ts
Outdated
Show resolved
Hide resolved
| ], | ||
| resourceArns: [`${this.executionArn}:*`], | ||
| }); | ||
| return iam.Grant.addToPrincipal({ |
There was a problem hiding this comment.
This part is confusing, can we just return identity and have the three grant blocks look the same?
There was a problem hiding this comment.
awslint tells me that grant methods must return iam.Grant
| 'states:SendTaskFailure', | ||
| 'states:SendTaskHeartbeat', | ||
| ], | ||
| resourceArns: ['*'], |
There was a problem hiding this comment.
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/test/state-machine-resources.test.ts
Show resolved
Hide resolved
Co-authored-by: Neta Nir <neta1nir@gmail.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| }); | ||
|
|
||
| activity.grant(role, 'states:SendTaskSuccess'); | ||
| ``` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
StateMachineandActivityare 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.allis 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
allmight 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’d counter myself here by saying it might be okay since using
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...) |
|
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). |
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
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 thatgrant()instate-machine.tswill handle all custom permissions for the state machine,grantExecution()instate-machine.tswill handle all custom permissions for the executions, andgrant()inactivity.tswill handle all custom permissions for activities.grantRead()andgrantTaskResponse()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