-
Notifications
You must be signed in to change notification settings - Fork 124
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
Allow STS regional requests without 'Host' header #2827
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 |
---|---|---|
|
@@ -54,13 +54,34 @@ def extract_relevant_data(response) | |
|
||
# Call to AWS STS endpoint using the provided authentication header | ||
def attempt_signed_request(signed_headers) | ||
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. Feels like with the multiple forms of region lookup, paired with fallback validation, we should move to region extraction and STS request validation. What do you think about having 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. With this approach would you ignore the 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'd recommend something like: def aws_call(region:, headers:)
host = if region == 'global'
'sts.amazonaws.com'
else
"sts.#{region}.amazonaws.com"
end
aws_request = URI("https://#{host}/?Action=GetCallerIdentity&Version=2011-06-15")
begin
@client.get_response(aws_request, headers)
rescue => e
# Handle any network failures with a generic verification error
raise(Errors::Authentication::AuthnIam::VerificationError, e)
end
end
# Call to AWS STS endpoint using the provided authentication header
def attempt_signed_request(signed_headers)
region = extract_sts_region(signed_headers)
# Attempt check using the discovered region
response = aws_call(region: region, headers: signed_headers)
return response if response.code.to_i == 200
# If the discovered region is `us-east-1`, also check if the request
# was made from the
if region == 'us-east-1'
response = aws_call(region: 'global', headers: signed_headers)
return response if response.code.to_i == 200
end
raise(Errors::Authentication::AuthnIam::VerificationError, 'Signature is invalid')
end
def extract_sts_region(signed_headers)
if signed_headers['host'].present?
if signed_headers['host'] == 'sts.amazonaws.com'
'global'
elsif match = signed_headers['host'].match(%r{sts.([\w\-]+).amazonaws.com})
match.captures.first
end
else
match = signed_headers['authorization'].match(%r{Credential=[^/]+/[^/]+/([^/]+)/})
return match.captures.first if match
raise(Errors::Authentication::AuthnIam::InvalidAWSHeaders, 'Failed to extract AWS region from authorization header')
end
end This separates the following into separate methods:
Note: the above is pseudo code, so please refactor as makes sense. 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. Great feedback, thanks! I've incorporated this but let me know if there's anything else |
||
aws_request = URI("https://#{signed_headers['host']}/?Action=GetCallerIdentity&Version=2011-06-15") | ||
begin | ||
@client.get_response(aws_request, signed_headers) | ||
region = extract_sts_region(signed_headers) | ||
|
||
# Attempt request using the discovered region and return immediately if successful | ||
response = aws_call(region: region, headers: signed_headers) | ||
return response if response.code.to_i == 200 | ||
|
||
# If the discovered region is `us-east-1`, fallback to the global endpoint | ||
if region == 'us-east-1' | ||
@logger.debug(LogMessages::Authentication::AuthnIam::RetryWithGlobalEndpoint.new) | ||
fallback_response = aws_call(region: 'global', headers: signed_headers) | ||
return fallback_response if fallback_response.code.to_i == 200 | ||
end | ||
|
||
# Handle any network failures with a generic verification error | ||
return response | ||
Comment on lines
+59
to
+70
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. Wonder if in the case where 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. Wasn't too sure about this. There isn't a way to definitively know whether the regional endpoint or the global endpoint was used to generate the signed headers even after attempting both requests. AWS throws a fairly generic My thinking was that the built-in retry logic should essentially be obscured from the UX so prioritize the information in original response in this case |
||
end | ||
|
||
def aws_call(region:, headers:) | ||
host = if region == 'global' | ||
'sts.amazonaws.com' | ||
else | ||
"sts.#{region}.amazonaws.com" | ||
end | ||
aws_request = URI("https://#{host}/?Action=GetCallerIdentity&Version=2011-06-15") | ||
begin | ||
@client.get_response(aws_request, headers) | ||
rescue StandardError => e | ||
raise(Errors::Authentication::AuthnIam::VerificationError.new(e)) | ||
# Handle any network failures with a generic verification error | ||
raise(Errors::Authentication::AuthnIam::VerificationError, e) | ||
end | ||
end | ||
|
||
|
@@ -76,6 +97,25 @@ def response_from_signed_request(aws_headers) | |
body.dig('ErrorResponse', 'Error', 'Message').to_s.strip | ||
) | ||
end | ||
|
||
# Extracts the STS region from the host header if it exists. | ||
# If not, we use the authorization header's credential string, i.e.: | ||
# Credential=AKIAIOSFODNN7EXAMPLE/20220830/us-east-1/sts/aws4_request | ||
def extract_sts_region(signed_headers) | ||
host = signed_headers['host'] | ||
gl-johnson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if host == 'sts.amazonaws.com' | ||
return 'global' | ||
end | ||
|
||
match = host&.match(%r{sts.([\w\-]+).amazonaws.com}) | ||
gl-johnson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return match.captures.first if match | ||
gl-johnson marked this conversation as resolved.
Show resolved
Hide resolved
gl-johnson marked this conversation as resolved.
Show resolved
Hide resolved
gl-johnson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
match = signed_headers['authorization']&.match(%r{Credential=[^/]+/[^/]+/([^/]+)/}) | ||
return match.captures.first if match | ||
|
||
raise Errors::Authentication::AuthnIam::InvalidAWSHeaders, 'Failed to extract AWS region from authorization header' | ||
end | ||
end | ||
end | ||
end |
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Lists should be surrounded by blank lines