-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add --explain
#84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
I've created a separate Issue to document the feature once this PR is merged: #90 |
iam-policy-autopilot-policy-generation/src/enrichment/resource_matcher.rs
Show resolved
Hide resolved
iam-policy-autopilot-policy-generation/src/enrichment/resource_matcher.rs
Outdated
Show resolved
Hide resolved
| // Only add if we haven't seen this operation before | ||
| if !operations.contains(additional_op) { | ||
| newly_discovered.insert(additional_op.clone()); | ||
| if !operations.contains(&new_op_with_prov) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we move this condition up to wrap this entire code block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the operation with FAS chain first, to check contains, so we are computing the chain first. We use a custom hash implementation, that just hashes the operation, though. This is a bit ugly, I'll see if I can come up with something better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a new commit, but I didn't change this. Open for suggestions.
iam-policy-autopilot-policy-generation/src/enrichment/resource_matcher.rs
Outdated
Show resolved
Hide resolved
iam-policy-autopilot-policy-generation/src/enrichment/resource_matcher.rs
Outdated
Show resolved
Hide resolved
iam-policy-autopilot-policy-generation/src/enrichment/resource_matcher.rs
Outdated
Show resolved
Hide resolved
iam-policy-autopilot-policy-generation/src/enrichment/resource_matcher.rs
Show resolved
Hide resolved
iam-policy-autopilot-policy-generation/src/enrichment/resource_matcher.rs
Outdated
Show resolved
Hide resolved
eebf9ee to
86b7b64
Compare
7729d9d to
c36dd50
Compare
c36dd50 to
33fae3b
Compare
adpaco-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't forget to add a line to the changelog too.
| #[serde(rename_all = "PascalCase")] | ||
| Provided { | ||
| /// Provided name | ||
| name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OperationView could just be a struct with name, service and optional metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing @mschlaipfer wanted to achieve oneOf.
What about a struct with name and service along with an enum OperationSource: [Extracted, Provided, FasExpansion]?
| /// The reasons this action was added (can have multiple reasons for the same action) | ||
| pub reasons: Vec<Reason>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we end up with the same reason multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not after applying merge, but I think making this a set makes sense.
| /// Result of finding a command/function instantiation with its arguments | ||
| #[derive(Debug, Clone)] | ||
| pub(crate) struct CommandUsage<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused: Why do we need this? Why didn't we need it until now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I've introduced it due to the complex return type of find_command_instantiation_with_args
| #[derive(Debug, Clone, Serialize, PartialEq, Eq, Hash, JsonSchema)] | ||
| #[serde(rename_all = "PascalCase")] | ||
| #[serde(untagged)] | ||
| pub enum OperationView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: OperationSource?
If we name it as OperationSource, can we embed FasInfo inside this enum instead?
| #[serde(rename_all = "PascalCase")] | ||
| Provided { | ||
| /// Provided name | ||
| name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing @mschlaipfer wanted to achieve oneOf.
What about a struct with name and service along with an enum OperationSource: [Extracted, Provided, FasExpansion]?
| name: String, | ||
| resources: Vec<Resource>, | ||
| conditions: Vec<Condition>, | ||
| explanation: Explanation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
| /// metadata to provide complete resource information for IAM policies. | ||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] | ||
| pub(crate) struct Resource { | ||
| /// The resource type name (e.g., "bucket", "object", "*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this confuses me a little bit - is it field modelling resource type or the name?
| /// Represents an operation with its provenance chain (how we reached this operation via FAS expansion) | ||
| #[derive(Debug, Clone)] | ||
| struct OperationWithFasExpansion { | ||
| operation: FasOperation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had GeneratedAction, we don't need these different structs for modeling this variant of operation.
| pub(crate) explanation: Explanation, | ||
| } | ||
|
|
||
| impl Action { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR - I'm starting to feel that we really should have just a struct for all generated operation/action like GeneratedAction/GeneratedOperation, and have fields like source annotating additional information and use this single struct only to model all operations.
Probably will make our code more maintainable and clearer.
What do you think?
Issue #, if available:
Description of changes: This PR adds an experimental explanation feature to the CLI (
--explain) which, for every action in the generated policy, provides a reason for why it has been added, including the expression and code location where IPA found an AWS operation call that led to its inclusion. It also provides information about FAS expansion, if it occurred.We believe that this feature, together with
--service-hints, will help creating policies with fewer unwanted permissions.Example:
{ "Action": "kms:Decrypt", "Reasons": [ { "Operation": { "Name": "get_object", "Service": "s3", "Expr": "s3.get_object(Bucket='my-bucket', Key='my-file.txt')", "Location": "iam-policy-autopilot-cli/tests/resources/test_example.py:7.16-7.68" }, "FAS": { "Explanation": "https://docs.aws.amazon.com/IAM/latest/UserGuide/access_forward_access_sessions.html", "Expansion": [ "s3:GetObject", "kms:Decrypt" ] } }, { "Operation": { "Name": "get_item", "Service": "dynamodb", "Expr": "dynamodb.get_item(\n TableName='my-table',\n Key={'id': {'S': '123'}}\n )", "Location": "iam-policy-autopilot-cli/tests/resources/test_example.py:19.5-22.5" }, "FAS": { "Explanation": "https://docs.aws.amazon.com/IAM/latest/UserGuide/access_forward_access_sessions.html", "Expansion": [ "dynamodb:GetItem", "kms:Decrypt" ] } } ] },This PR adds file and raw expression information and expands location information in multiple places throughout the extraction phase so this data is available in the
SdkMethodCall. There is currently a lot of code duplication, which I have not cleaned up as part of this pull request. See #88.During enrichment it adds the FAS expansion information. The location format follows the one here. It is clickable, so it navigates to the location in VS Code (I have not tested with other editors).
This PR removes the (hidden)
--show-action-mappingscommand which had a similar purpose, but did not support location, expression, or FAS information.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.