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

Add support for AWS authentication #347

Merged
merged 1 commit into from Jul 12, 2017

Conversation

Projects
None yet
3 participants
@focusaurus
Contributor

focusaurus commented Jul 6, 2017

What

  • Add AWS Auth type support (Closes #344)
  • Screenshots here
  • Multifactor authentication (MFA) not yet supported

Why

Allow end users to easily send requests to Amazon Web Services APIs. Although this authentication scheme is non-standard, the service is widely used and difficult to use without good support from developer tools.

Details

Follows closely existing patterns for other authentication schemes.

Notes

More work would be required to support Multifactor authentication, which requires an additional field AWS_SESSION_TOKEN. I can do that as well but would like your guidance on the UI.

Related Issue: #344

@gschier

Nice work @focusaurus! A few minor comments, then one about supporting other body types.

host: parsedUrl.hostname,
headers: {}
};
const body = req.body && req.body.text;

This comment has been minimized.

@gschier

gschier Jul 8, 2017

Collaborator

This only works with "Text" body types like JSON/XML/etc. Other body types, like form application/x-www-form-urlencoded, have a body object that looks different:

{
  mimeType: 'application/x-www-form-urlencoded', 
  params: [{name: 'foo', value: 'bar'}]
}

The final body for these content-types is computed a few lines above, making it pretty easy to support application/x-www-form-urlencoded. However, binary uploads and multipart/form-data are handled by Curl so we don't ever know the final result of those. It would be possible to handle those other two cases ourselves but would take more work (maybe not worth it).

For the cases we don't support, it's probably worth throwing an error. The following code will throw an error for anything that isn't Text or application/x-www-form-urlencoded.

const isForm = body.mimeType === 'multipart/form-data';
const isFile = body.fileName;
const isUnsupported = isForm || isFile;
if (isUnsupported) {
  throw new Error('AWS authentication not supported for provided body type');
}

This comment has been minimized.

@focusaurus

focusaurus Jul 10, 2017

Contributor

For x-www-form-urlencoded if it's all just name/contents pairs I can convert that to a final string so aws4 can add it to the signature. But if there's file uploads I'm not sure how to get that to work properly.

This comment has been minimized.

@focusaurus

focusaurus Jul 10, 2017

Contributor

I've added the code for an exception for unsupported bodies. Here's my remaining questions:

  • Do you want me to convert the x-www-form-urlencoded array of key/value objects which we pass to curl to a finalized and encoded string so it can be passed into aws4 to be signed?
  • When running under npm run dev I can see the unsupported body exception is thrown but the UI doesn't seem to indicate this anywhere.
  • I'm seeing my entered auth values disappear as I navigate off the AWS tab. Is that expected? Is there something about the new code that's not right that's causing that?
const parsedUrl = urlParse(url);
const options = {
path: parsedUrl.path,
host: parsedUrl.hostname,

This comment has been minimized.

@gschier

gschier Jul 8, 2017

Collaborator

Not sure what AWS expects here, but should this be parsedUrl.host instead of parsedUrl.hostname? I think the only difference is that host would include the port if it were specified.

This comment has been minimized.

@focusaurus

focusaurus Jul 10, 2017

Contributor

I think parsedUrl.hostname is correct. I tried explicitly including :443 in my URL and using host but AWS gave me a 403 for that. Since in all normal cases AWS services are accessed via HTTPS using the default port, I think omitting the port is correct.

This comment has been minimized.

@gschier

gschier Jul 10, 2017

Collaborator

Nice 👍 Might be worth adding a comment beside it so someone doesn't ask the same question again?

const options = {
path: parsedUrl.path,
host: parsedUrl.hostname,
headers: {}

This comment has been minimized.

@gschier

gschier Jul 8, 2017

Collaborator

Should we not be passing req.headers into this?

This comment has been minimized.

@focusaurus

focusaurus Jul 10, 2017

Contributor

I don't think so. We could, but first we'd need to convert them to a node format object instead of an array of name/value objects. aws4 only looks at a few key headers (Content-Type, Content-Length, Host) plus the AWS-specific ones.

const signature = aws4.sign(options, credentials);
return Object.keys(signature.headers)
// Don't use the inferred content type aws4 provides
.filter((name) => name !== 'content-type')

This comment has been minimized.

@gschier

gschier Jul 8, 2017

Collaborator

I assume this wouldn't be needed if we passed the content-type header in the initial options above?

This comment has been minimized.

@focusaurus

focusaurus Jul 10, 2017

Contributor

Yes I think you're correct here. Refactoring.

This comment has been minimized.

@focusaurus

focusaurus Jul 10, 2017

Contributor

Actually without this we end up sending a duplicate Content-Type header and the signature won't verify.

This comment has been minimized.

@gschier

gschier Jul 11, 2017

Collaborator

Ok, I think this is good now 👍

@gschier

A few more comments @focusaurus. And, I noticed you hade some questions on an outdated diff (I can't reply anymore), so I'll answer them here.

Do you want me to convert the x-www-form-urlencoded array of key/value objects which we pass to curl to a finalized and encoded string so it can be passed into aws4 to be signed?

I need to do some refactoring with how the body building is done for the new plugin system, so I can handle this part later. For this PR, I'm fine with only handling text bodies.

When running under npm run dev I can see the unsupported body exception is thrown but the UI doesn't seem to indicate this anywhere.

I noticed this today as well while working on another branch. I pulled the fix into develop so you can pull it into this branch.

I'm seeing my entered auth values disappear as I navigate off the AWS tab. Is that expected? Is there something about the new code that's not right that's causing that?

One of me comments below points out the cause of this. It was an easy one to miss.

return {
type,
disabled: oldAuth.disabled || false,
accessKeyId: oldAuth.username || '',

This comment has been minimized.

@gschier

gschier Jul 11, 2017

Collaborator

These two lines should be:

accessKeyId: oldAuth.accesKeyId || '',
secretAccessKey: oldAuth.secretAccessKey || ''
const signature = aws4.sign(options, credentials);
return Object.keys(signature.headers)
// Don't use the inferred content type aws4 provides
.filter((name) => name !== 'content-type')

This comment has been minimized.

@gschier

gschier Jul 11, 2017

Collaborator

Ok, I think this is good now 👍

} = this.props;
const pairs = [{
name: authentication.username || '',

This comment has been minimized.

@gschier

gschier Jul 11, 2017

Collaborator

This should be

name: authentication.accesKeyId || '',
value: authentication.secretAccessKey || ''

This explains why you are seeing your values disappear from the UI.

Add support for AWS authentication
* Implements #344
* Multifactor authentication (MFA) not yet supported

@focusaurus focusaurus force-pushed the focusaurus:feature/aws-auth branch from 3b99cec to a09d35d Jul 11, 2017

@focusaurus

This comment has been minimized.

Contributor

focusaurus commented Jul 11, 2017

OK thanks for the help. I've made your most recent batch of changes and rebased to clean up the noise. I think this might be ready to merge now. Still room to make the features more thorough in the future, but this at least allows me to call AWS API Gateway endpoints with text bodies, which otherwise is pretty much only possible programmatically as even curl and httpie make it exceedingly difficult.

@gschier

Nice! Merging this in 😄

@gschier gschier merged commit d2c677c into getinsomnia:develop Jul 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gschier

This comment has been minimized.

Collaborator

gschier commented Jul 12, 2017

Thanks @focusaurus! Will try to get an update out by the end of the week with this included.

@cludden

This comment has been minimized.

cludden commented Jul 29, 2017

Thanks @focusaurus this is awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment