Allow STS regional requests without 'Host' header

[gl-johnson]

Desired Outcome

Currently we assume that the signed request will feature a 'Host' header telling Conjur which STS endpoint to use.

This assumption is not valid for all HTTP clients (unless explicitly added to the request), including the Golang AWS SDK. Instead we should the following prioritized order in determining which endpoint to use:

1. Host header if it exists
2. AWS region (pulled from Authorization header)
3. Global endpoint (only if result from 2 is us-east-1 and the regional endpoint failed)

Implemented Changes

• Implemented above logic for determining STS endpoint + tests
• Minor fixes to dev environment

Connected Issue/Story

CNJR-1904

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

• The CHANGELOG has been updated, or
• This PR does not include user-facing changes and doesn't require a
  CHANGELOG update

Test coverage

• This PR includes new unit and integration tests to go with the code
  changes, or
• The changes in this PR do not require tests

Documentation

• Docs (e.g. READMEs) were updated in this PR
• A follow-up issue to update official docs has been filed here: CNJR-1907
• This PR does not require updating any documentation

Behavior

• This PR changes product behavior and has been reviewed by a PO, or
• These changes are part of a larger initiative that will be reviewed later, or
• No behavior was changed with this PR

Security

• Security architect has reviewed the changes in this PR,
• These changes are part of a larger initiative with a separate security review, or
• There are no security aspects to these changes

[jvanderhoof]

I'd recommend we move the signed header resolution into the extract_sts_host method. This allows us to keep all logic related to the endpoint lookup in one place.

sts_host = signed_headers['host'] || extract_sts_host(signed_headers)

Then we push the logic for checking header value or attempting to extract the region from the credential:

# Extract AWS region from the authorization header's credential string, i.e.:
# Credential=AKIAIOSFODNN7EXAMPLE/20220830/us-east-1/sts/aws4_request
def extract_sts_host(signed_headers)
  return signed_headers['host'] if signed_headers['host'].present?

  region = signed_headers['authorization'].match(%r{Credential=[^/]+/[^/]+/([^/]+)/})&.captures&.first
  raise(Errors::Authentication::AuthnIam::InvalidAWSHeaders, 'Failed to extract AWS region from authorization header') unless region

  "sts.#{region}.amazonaws.com"
end

[jvanderhoof]

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 extract_sts_host return the region? Based on that response, we can build the STS url and validate it while falling back to the global endpoint if the regional validation fails.

[gl-johnson]

With this approach would you ignore the Host header and always rely on the region from the credential string? Otherwise I don't see a benefit to adding another pattern match to extract the STS region from the Host header when the STS url is readily available if it exists

[jvanderhoof]

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:

• Region extraction
• Retry behavior on the global endpoint
• STS lookup

Note: the above is pseudo code, so please refactor as makes sense.

[gl-johnson]

Great feedback, thanks! I've incorporated this but let me know if there's anything else

[codeclimate]

Lists should be surrounded by blank lines

[john-odonnell]

I think if match makes match& irrelevant - you're right to include it because we don't want to return anything if match == nil, but if match should prevent this case.

[john-odonnell]

This applies to line 115 as well

[john-odonnell]

Looking at this again, and even given my suggestion:

return match.captures&.first if match

I wonder if we still need this Safe Navigation Operator.

• .match returns nil if there are no matches found
• .match returns a MatchData object for 1+ matches found

That means:

• if match catches cases with no matches
• Otherwise, there is guaranteed to be at least one match - match.captures will not be nil.

We might be able to do this instead, with no change in behavior and less &s:

return match.captures.first if match

[john-odonnell]

Wonder if in the case where region == 'us-east-1' and fallback_response.code.to_i != 200 we want to return the fallback_response instead of the primary response.

[gl-johnson]

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 The request signature we calculated does not match the signature you provided error message either way

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

[codeclimate]

Code Climate has analyzed commit da7dff4 and detected 8 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2
Style	2
Complexity	4

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 87.1% (-1.2% change).

View more on Code Climate.

[john-odonnell]

LGTM!