Skip to content
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 overwriting STS config in ChainableTemporaryCredentials #2803

Conversation

workeitel
Copy link
Contributor

STS uses the global endpoint per default which does not work for
"opt-in" regions. The ChainableTemporaryCredentials wrapper for
fetching credentials via AssumeRole or GetSessionToken does not
allow overriding the endpoint or set a different region.

This change will introduce another optional constructor parameter for
specifying the STS client configuration and forwarding it to the client
constructor.

In addition I remove the incorrect typing for the second
masterCredentials constructor parameter. The parameter needs to be
passed as a nested value in options.

issue: #2673

Checklist
  • npm run test passes
  • .d.ts file is updated
  • changelog is added, npm run add-change

STS uses the global endpoint per default which does not work for
"opt-in" regions. The `ChainableTemporaryCredentials` wrapper for
fetching credentials via `AssumeRole` or `GetSessionToken` does not
allow overriding the endpoint or set a different region.

This change will introduce another optional constructor parameter for
specifying the STS client configuration and forwarding it to the client
constructor.

In addition I remove the incorrect typing for the second
`masterCredentials` constructor parameter. The parameter needs to be
passed as a nested value in `options`.

issue: aws#2673
@@ -3,7 +3,7 @@ import {AWSError} from '../error';
import STS = require('../../clients/sts');

export class ChainableTemporaryCredentials extends Credentials {
constructor(options: ChainableTemporaryCredentials.ChainableTemporaryCredentialsOptions, masterCredentials?: Credentials);
constructor(options: ChainableTemporaryCredentials.ChainableTemporaryCredentialsOptions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class expose the service member which is an STS service cient. Can't you access the service object and update the endpoint directly?
I'm raising this because exposing the client config is not consist with our other credentials providers config interface. For example: SharedIniCredentials only expose the httpOptions config instead of the whole client config.

@AllanZhengYP
Copy link
Contributor

AllanZhengYP commented Aug 8, 2019

@workeitel To update the STS endpoint, you can actually do it as follows:

const stsClient = chainableCredentials.service;
stsClient.setupEventListeners = (request) => {
    request.on('afterBuild', (req) => {
        request.httpRequest.endpoint = //endpoint you want
    })
}

@workeitel
Copy link
Contributor Author

Hey @AllanFly120 . Thanks for the quick reply. I tried multiple approaches but could not get it working:

const credentials = new AWS.ChainableTemporaryCredentials({
  params: {
    DurationSeconds: 900
  }
});

approach 1

As I originally suggested in the ticket #2673

credentials.service.config.endpoint = "sts.us-west-1.amazonaws.com";

did not set the endpoint

approach 2

as you suggested

// @ts-ignore
credentials.service.setupEventListeners = request => {
  // @ts-ignore
  request.on("afterBuild", req => {
    request.httpRequest.endpoint = "sts.us-west-1.amazonaws.com";
  });
};

did not set the endpoint

approach 3

as found somewhere else

// @ts-ignore
credentials.service.setEndpoint("sts.us-west-1.amazonaws.com");

did successfully override the endpoint - but not the region. So I end up with a Credential should be scoped to a valid region, not 'us-east-1'. exception.

I guess I'm doing something wrong.

@AllanZhengYP
Copy link
Contributor

AllanZhengYP commented Aug 9, 2019

Hey @workeitel

Sorry, I didn't make it clear in my previous response. The endpoint in SDK is an object. Here's its interface: https://github.com/aws/aws-sdk-js/blob/master/lib/endpoint.d.ts
So you should set the endpoint like:

const stsClient = chainableCredentials.service;
stsClient.setupEventListeners = (request) => {
    request.on('afterBuild', (req) => {
        request.httpRequest.endpoint.host = "sts.us-west-1.amazonaws.com";
    })
}

Note here your host already contains the region. So setting region separately won't work. You need to insert the region when you construct the host uri, like sts.${region}.amazonaws.com

@workeitel
Copy link
Contributor Author

Thanks @AllanFly120. I tried your version but I assume you mean setupRequestListeners instead:

credentials.service.setupRequestListeners = request => {
  request.on("afterBuild", () => {
    request.httpRequest.endpoint = new AWS.Endpoint(
      "sts.us-west-1.amazonaws.com"
    );
  });
};

But it fails with:

"Host: sts.amazonaws.com. is not in the cert's altnames: DNS:sts.us-west-1.amazonaws.com, DNS:*.sts.us-west-1.vpce.amazonaws.com"

since the Host header is already set. Adding

request.httpRequest.headers.Host = "sts.us-west-1.amazonaws.com"

helps (alternative use build state instead of afterBuild) but then it fails with

"Credential should be scoped to a valid region, not 'us-east-1'. "

I think for signing the request the AWS SigV4 requires the region as well since the region is part of the signature. If we override the endpoint with another region we need to update the region for the signature as well. https://docs.aws.amazon.com/general/latest/gr/sigv4_elements.html#sigv4_elements_auth_params

Overriding the region as well works but is not supported in typings: request.httpRequest.region = "us-west-1";

Final version:

import AWS = require("aws-sdk");

const credentials = new AWS.ChainableTemporaryCredentials({
  params: {
    DurationSeconds: 900,
    RoleArn:
      "arn:aws:iam::62..:role/..."
  }
});

function patchEndpoint(
  creds: AWS.ChainableTemporaryCredentials,
  endpoint: string,
  region: string
) {
  creds.service.setupRequestListeners = request => {
    request.on("build", () => {
      request.httpRequest.endpoint = new AWS.Endpoint(endpoint);

      // @ts-ignore
      request.httpRequest.region = region;
    });
  };
}

patchEndpoint(credentials, "sts.us-west-1.amazonaws.com", "us-west-1");

const client = new AWS.STS({
  region: "us-west-2",
  endpoint: "sts.us-west-2.amazonaws.com",
  credentials
});
client
  .getCallerIdentity()
  .promise()
  .then(console.log, console.log);

But I don't think thats a nice solution. At least the region should be in the httpRequest typings. Or maybe making .service not readonly to allow overriding the STS client?

@AllanZhengYP
Copy link
Contributor

@workeitel I see your point. I'm approving this PR.

@AllanZhengYP AllanZhengYP merged commit 3ffad9c into aws:master Aug 19, 2019
@lock
Copy link

lock bot commented Aug 26, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants