-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add a built-in AWS_IAM authorizer for HTTP APIs #1878
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -397,34 +397,45 @@ def _construct_alias_target(self, domain): | |
|
||
def _add_auth(self): | ||
""" | ||
Add Auth configuration to the OAS file, if necessary | ||
Add Auth configuration to the OAS file. In order to support the built-in AWS_IAM authorizer it is always added. | ||
""" | ||
if not self.auth: | ||
return | ||
|
||
if self.auth and not self.definition_body: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Why remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my response above. |
||
if not self.definition_body: | ||
raise InvalidResourceException( | ||
self.logical_id, "Auth works only with inline OpenApi specified in the 'DefinitionBody' property." | ||
) | ||
|
||
# Make sure keys in the dict are recognized | ||
if not all(key in AuthProperties._fields for key in self.auth.keys()): | ||
raise InvalidResourceException(self.logical_id, "Invalid value for 'Auth' property") | ||
|
||
if not OpenApiEditor.is_valid(self.definition_body): | ||
raise InvalidResourceException( | ||
self.logical_id, | ||
"Unable to add Auth configuration because 'DefinitionBody' does not contain a valid OpenApi definition.", | ||
) | ||
|
||
open_api_editor = OpenApiEditor(self.definition_body) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking back at this line I'm now wondering if this line populates the if not "AWS_IAM" in open_api_editor.security_schemes {
open_api_editor.security_schemes["AWS_IAM"] = {
...
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure whether this information helps or not - but in my pursuit of trying to work how to enable AWS_IAM via OpenAPI this morning, (obviously I did not succeed (edit: I did later)), I did the following.
Under security schemes, it generated this-
The affected path now looked like this
Similarly I got errors when I tried to recreate using the exported template. So it seems like by default it calls it an IAM authorizer in the scheme as sigv4, with the awsSigv4 type. EDIT: I just tried attaching an IAM authorizer via OpenAPI and it worked - so it likes like you can configure an IAM authorizer by using Open API. I'm not sure what I did differently to this morning.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep, you can do it that way. This PR is proposing a way to do it without having to touch OpenAPI. |
||
auth_properties = AuthProperties(**self.auth) | ||
authorizers = self._get_authorizers(auth_properties.Authorizers, auth_properties.DefaultAuthorizer) | ||
|
||
# authorizers is guaranteed to return a value or raise an exception | ||
open_api_editor.add_authorizers_security_definitions(authorizers) | ||
self._set_default_authorizer( | ||
open_api_editor, authorizers, auth_properties.DefaultAuthorizer, auth_properties.Authorizers | ||
) | ||
# To remain backwards compatible add the built-in "AWS_IAM" security scheme _before_ adding the authorizers defined | ||
# in the template so that if the template already has an authorized named "AWS_IAM" it will override the built-in one. | ||
Comment on lines
+415
to
+416
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why wouldn't we add this only if a customer wants AWS_IAM auth? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The goal here is that if a customer somehow already has an authorizer called |
||
open_api_editor.security_schemes["AWS_IAM"] = { | ||
"type": "apiKey", | ||
"name": "Authorization", | ||
"in": "header", | ||
"x-amazon-apigateway-authtype": "awsSigv4", | ||
} | ||
|
||
# If auth is defined in the template validate it, set the default authorizer, and add any specified authorizers. | ||
if self.auth: | ||
# Make sure keys in the dict are recognized | ||
if not all(key in AuthProperties._fields for key in self.auth.keys()): | ||
raise InvalidResourceException(self.logical_id, "Invalid value for 'Auth' property") | ||
|
||
auth_properties = AuthProperties(**self.auth) | ||
# authorizers is guaranteed to return a value or raise an exception | ||
authorizers = self._get_authorizers(auth_properties.Authorizers, auth_properties.DefaultAuthorizer) | ||
|
||
open_api_editor.add_authorizers_security_definitions(authorizers) | ||
self._set_default_authorizer( | ||
open_api_editor, authorizers, auth_properties.DefaultAuthorizer, auth_properties.Authorizers | ||
) | ||
|
||
self.definition_body = open_api_editor.openapi | ||
|
||
def _add_tags(self): | ||
|
@@ -468,7 +479,8 @@ def _set_default_authorizer(self, open_api_editor, authorizers, default_authoriz | |
if not default_authorizer: | ||
return | ||
|
||
if not authorizers.get(default_authorizer): | ||
# The AWS_IAM authorizer is built-in and does not need to be defined in the template as an authorizer. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We require this for all other Auths. Why should AWS_IAM be any different? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because AWS_IAM is a "built in" auth type. It should not be customized and there only ever needs to be a single one of them. That said I did take the approach you are suggesting in my alternate implementation: #1876 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we already have a precedent for another built-in authorizer: |
||
if not authorizers.get(default_authorizer) and default_authorizer != "AWS_IAM": | ||
raise InvalidResourceException( | ||
self.logical_id, | ||
"Unable to set DefaultAuthorizer because '" | ||
|
@@ -488,8 +500,10 @@ def _get_authorizers(self, authorizers_config, default_authorizer=None): | |
:param default_authorizer: name of the default authorizer | ||
""" | ||
authorizers = {} | ||
|
||
if not isinstance(authorizers_config, dict): | ||
# The AWS_IAM authorizer is built-in and does not need to be defined in the template as an authorizer. | ||
if default_authorizer == "AWS_IAM": | ||
return authorizers | ||
raise InvalidResourceException(self.logical_id, "Authorizers must be a dictionary.") | ||
|
||
for authorizer_name, authorizer in authorizers_config.items(): | ||
|
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.
Removing this changes behavior. That is the
self.definition_body = open_api_editor.openapi
is set, which will risk breaking existing customers.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.
My goal here was to allow customers to specify the
IAM_AUTH
authorizer even if they didn't have any auth defined in their serverless http api. If you look at my first example, in the converstaion tab, I illustrated turning on IAM auth for a serverless function using the default serverless http api:Now, we could keep the
if not self.auth
checks. If we did that customers that wanted to use the IAM authorizer would need to define any other authorizer and potentially leave it unused to gain access to the IAM authorizer. That seems fairly hacky to me which I'd like to avoid.