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

The HTTPS assumption is not correct in SAM-local environment, it cause integration errors #138

Closed
allinwonder opened this issue Mar 21, 2018 · 5 comments
Assignees
Labels
Milestone

Comments

@allinwonder
Copy link

We ran into integration issues in SAM-local when we try to use the context scheme to determine integration endpoint scheme. In SAM-Local, all endpoints are using HTTP instead HTTPS.

https://github.com/awslabs/aws-serverless-java-container/blob/60e5bfaa4e2463c7678a8c0d84f815c2034abe8a/aws-serverless-java-container-jersey/src/main/java/com/amazonaws/serverless/proxy/jersey/JerseyHandlerFilter.java#L176

@allinwonder allinwonder changed the title The HTTPS assumption is not correct in SAM-local tests and debugging, it cause integration errors The HTTPS assumption is not correct in SAM-local environment, it cause integration errors Mar 21, 2018
@sapessi sapessi self-assigned this Mar 21, 2018
@sapessi sapessi added the bug label Mar 21, 2018
@sapessi sapessi added this to the Release 1.1 milestone Mar 21, 2018
@sapessi
Copy link
Collaborator

sapessi commented Mar 21, 2018

Thanks for the report @allinwonder. I'll look into it, so far as I can tell I have no easy way to tell whether the request came in as HTTP or HTTPS. The only useful field I could find is the CloudFront header: "CloudFront-Forwarded-Proto":"https". However, I cannot trust this field to decide whether it's HTTPS or not since API Gateway itself may host regional endpoints and this header would not be present.

@sapessi
Copy link
Collaborator

sapessi commented Mar 22, 2018

I think I can work around this. However, I have to rely on the X-Forwarded headers. Headers are easily spoofable and I'm not a huge fan of this solution so I've added lots of checking. If you want to enable custom domains, you will have to explicitly declare them in the ContainerConfig object. The ports are hard-coded as either 443 or 3000.

I'm hoping to make all of this go away if/once API Gateway starts sending the full request uri in the proxy event.

@allinwonder
Copy link
Author

@sapessi given the headers are spoofable, developers can override the default behavior easily when they need. This means if the application can allow developers to use an override header for example "X-Serverless-Proto": "http" to define an expected protocol. In this case, this workaround will only be valid in an explicit environment, its config doesn't depend on or influenced by other use cases.

@sapessi
Copy link
Collaborator

sapessi commented Mar 26, 2018

Hey @allinwonder - I'm not as concerned about protocol as I am about the Host and X-Forwarded-Port headers, I'm afraid these would become an attack vector if your code relies on them to make downstream calls. I have asked the API Gateway team to add the full request URI to the context object.

For the time being, my approach is this:

  1. For the protocol, I'll take whatever the header says (I'll send a pull request to SAM local to make sure it includes these headers)
  2. For the host, I'll either allow a default API Gateway endpoint (xxxxxx.execute-api.xx-xxxx-x.amazonaws.com) or I'll let you manually enable custom domains in the ContainerConfig object:
LambdaContainerHandler.getContainerConfig().addCustomDomain("api.myserver.com");
LambdaContainerHandler.getContainerConfig().enableLocalhost(); // shortcut for localhost
  1. For the port, I've taken a similar strategy where I'm allowing only 443, 80, or 3000

Any chance you could share the code you are using to generate those URLs so that I can add a unit test for this?

sapessi added a commit that referenced this issue Apr 5, 2018
…ture, it would be ideal to get the full request uri from API Gateway in the event.
@sapessi
Copy link
Collaborator

sapessi commented Apr 6, 2018

This is resolved in the latest merge. 1.1 will go out shortly. Closing the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants