-
Notifications
You must be signed in to change notification settings - Fork 448
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
AWS Signature version 4 authentication #586
AWS Signature version 4 authentication #586
Conversation
Closing while fixing tslint errors. Sorry about that. |
Fixed tslint issues |
@@ -344,6 +344,15 @@ Or if you have certificate in `PFX` or `PKCS12` format, setting code can be like | |||
### Azure Active Directory(Azure AD) | |||
Azure AD is Microsoft’s multi-tenant, cloud-based directory and identity management service, you can refer to the [System Variables](#system-variables) section for more details. | |||
|
|||
### AWS Signature v4 |
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.
Also, add the new auth scheme AWS Signature v4
in the Main Features
section like other Authentication schemes.
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.
Done.
README.md
Outdated
AWS Signature version 4 is used to authenticate requests to AWS services. For more information, see [AWS documentation](https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html). `sessionToken` as well as the `X-AWS-Region` and `X-AWS-Service` headers are optional. | ||
```http | ||
GET https://httpbin.org/aws-auth HTTP/1.1 | ||
X-Authentication-Type: AWS accessId accessKey sessionToken | ||
X-AWS-Region: awsRegion | ||
X-AWS-Service: awsService |
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.
@sebastian-fredriksson-bernholtz I prefer the AWS Signature v4 auth mechanism built on the Authorization
header as other Authorization schemas like Basic Auth, Digest Auth, and Azure AD. We don't need to invent these three new headers X-Authentication-Type
, X-AWS-Region
, and X-AWS-Service
.
How about coming up with a syntax for AWS Signature v4
auth scheme like the following:
Authorization: AWS accessKeyId secretAccessKey [token:sessionToken] [region:regionName] [service:serviceName]
And the token
, region
and service
parameters are optional.
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.
The reason I used a new header was that I didn't want it confused with the actual HTTP Authorization Header that will generated and sent. But I realise now that it shouldn't be a concern as the Digest authentication is already functioning like this.
src/utils/httpClient.ts
Outdated
// set AWS authentication | ||
const xAuthentication = extractHeader(options.headers!, 'X-Authentication-Type') as string | undefined; | ||
if (xAuthentication) { | ||
const [ xAuthenticationType, ...rawCredentials ] = xAuthentication.split(" "); | ||
if (xAuthenticationType === 'AWS' ) { | ||
const credentials = { | ||
accessKeyId: rawCredentials[0], | ||
secretAccessKey: rawCredentials[1], | ||
sessionToken: rawCredentials[2] | ||
}; | ||
|
||
const awsScope: { [key: string]: string } = {}; | ||
const region = extractHeader(options.headers!, 'X-AWS-Region') as string | undefined; | ||
if (region) { awsScope.region = region; } | ||
const service = extractHeader(options.headers!, 'X-AWS-Service') as string | undefined; | ||
if (service) { awsScope.service = service; } | ||
|
||
options.hooks!["beforeRequest"] = [ | ||
async options => { | ||
aws4.sign({ ...options, ...awsScope }, credentials); | ||
} | ||
]; | ||
} | ||
} | ||
|
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.
How about move this logic into a separate file under the auth
folder like Digest
auth?
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've changed it to resemble the structure of Digest auth. Let me know what you think. Thanks!
src/utils/auth/awsSignature.ts
Outdated
import got = require('got'); | ||
|
||
export function awsSignature(authorization: string): got.BeforeRequestHook<got.GotBodyOptions<null>> { | ||
const [ , accessKeyId, secretAccessKey ] = authorization.split(" "); |
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 seems not working if the Authorization header contains consecutive spaces like the following:
Authorization: AWS foo bar
In this case the destructed values of accessKeyId
and secretAccessKey
are incorrect.
Maybe here you can replace the splitter with regex /\s+/
, and the code looks like authorization.split(/\s+/)
Or you can use the already split result in httpClient.ts
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.
Thank you so much for your patience. I split the header by any number of whitespace like you suggested.
I was considering using the already split result, but I was hesitant because of the names of the variables (user
and pass
/args
) and didn't want to change too much of the existing code. Maybe it will make more sense to make it more general if there are even more additional authentication types?
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.
👍
@sebastian-fredriksson-bernholtz Merged, thanks. |
Thanks for this awesome extension!
I've added support for AWS Signature version 4 authentication. This resolves #281. I assume that this would be welcomed since that issue is still open.