-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: extend aws policy configuration #553
base: master
Are you sure you want to change the base?
Conversation
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.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
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.
LGTM!
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 move changes to new version
@Gadam8 looks good Don't forget to update information about SNS and SQS here: |
}, | ||
"principal": { | ||
"description": "The AWS account(s) or resource ARN(s) that this statement applies to.", | ||
"oneOf": [ |
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.
Why not just a string array?
I think my main concern is that object of any property is kind of vague. And we have alot of that already so trying to minimize it, is a priority for me.
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 see where you're coming from, but it can't just be a string array as it needs to be able to take a property and value in some cases https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html
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.
Would it be too much to ask for the objects being defined as in the AWS docs? As OneOf. I think i only see 3 different types (not including basic string). (from quickly glancing)
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 is what I've attempted to do here, according to the AWS docs the principal property can be:
- a single string value of
"*"
- a key/value pair of
AWS/Service: {accountId}/{service}
- a property with an array of string values:
AWS: [123, 456]
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.
from the Docs the actual objects look a bit different.
Youve described them as "any named of pattern property, which is either a string or an array of string.
I would much rather that it is described as its documented by AWS (to leave out any problem with interpretation).
Something like
"oneOf": [
{
"type": "object",
"properties": {
"AWS": {
"type": "array",
"items": {
"type": "string"
}
}
},
"required": ["AWS"],
"additionalProperties": false
},
{
"type": "object",
"properties": {
"AWS": {
"type": "string"
}
},
"required": ["AWS"],
"additionalProperties": false
},
{
"type": "object",
"properties": {
"Federated": {
"type": "string"
}
},
"required": ["Federated"],
"additionalProperties": false
},
{
"type": "object",
"properties": {
"Service": {
"type": "array",
"items": {
"type": "string"
}
}
},
"required": ["Service"],
"additionalProperties": false
},
{
"type": "object",
"properties": {
"Service": {
"type": "string"
}
},
"required": ["Service"],
"additionalProperties": false
}
]
}
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.
Yeah that looks good, I'm not sure the federated
option is needed though? I think that falls under AWS
? We have a slight issue in that the docs allow just *
as a value without a property name https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#principal-anonymous. It is equivalent to AWS: *
, so we can enforce that is used instead for the sake of the structure? However, it is technically an option
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.
@VisualBean is that a thumbs up to enforcing a property name is always provided, even for the "*"
case?
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 think for json and yaml alike *
is a string either way, so it fits under the
"AWS": {
"type": "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.
Sorry I mean, to address the fact that AWS allows both:
Principal: "*"
and
Principal: { "AWS" : "*" }
So I was asking in order to have a consistent structure do we want to enforce a property name (AWS in this case), so we wouldn't accept option one of the above. Does that make sense?
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 think we need to satisfy the "*"
option. So going forward we could have:
`"oneOf": [
{
"type": "string"
},
{
"type": "object",
"properties": {
"AWS": {
"oneOf": [
{
"type": "string"
},
{
"type": "array",
"items": {
"type": "string"
}
}
]
}
},
"required": ["AWS"],
"additionalProperties": false
},
{
"type": "object",
"properties": {
"Service": {
"oneOf": [
{
"type": "string"
},
{
"type": "array",
"items": {
"type": "string"
}
}
]
}
},
"required": ["Service"],
"additionalProperties": false
}
]`
The principal property could then be a custom class which is either a string value or one of the predefined AWS properties (AWS, Service etc). In the same manner as the StringOrStringList
class in the .NET repo currently
Maybe make it an enum for one of them 😅.
Whatever makes the most sense and is the most ‘true’ to the spec i guess.
|
Description
The current AWS policy binding configuration doesn't match what is offered by AWS when configuring IAM policies.
Condition
andResource
are missing andPrincipal
isn't fully fleshed out. This PR addresses this by extending the binding configuration for SNS and SQS.Related issue(s)