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

WIP reject fake host headers #545

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

WIP reject fake host headers #545

wants to merge 3 commits into from

Conversation

chris48s
Copy link
Member

OK. I'm a bit stuck here.

Currently this application allows anything in the host header. This makes the site vulnerable to a range of spoofing attacks. I want to tighten this up.

The changes in f680c9a achieve this. However, they cause another problem.

When we deploy this app, we bring the lambda up and then run some smoke tests against it before we finalise the deploy. Because at the stage we're running the smoke tests, we haven't pointed the DNS at it yet, we have to test this on a direct URL i.e: we have to call https://fig9r88dhk.execute-api.eu-west-2.amazonaws.com/Prod/ rather than https://developers.womblelabs.co.uk/ or whatever.

That means those URLs also have to be in ALLOWED_HOSTS, otherwise we raise 400 Bad Request in the smoke tests.

So my first bash at a fix for this is fe21b96. In theory that should work. Unfortunately, when we try to run that code on lambda I get

ClientError: An error occurred (AccessDeniedException) when calling the GetRestApis operation: User: arn:aws:sts::489559689862:assumed-role/AggregatorApiLambdaExecutionRole/AggregatorApiApp-development-ApiFrontendFunction-KHzbY2EsfvyY is not authorized to perform: apigateway:GET on resource: arn:aws:apigateway:eu-west-2::/restapis because no identity-based policy allows the apigateway:GET action

so in order to get that to work, I need to grant our AggregatorApiLambdaExecutionRoles the ability to perform that action but I'm unclear on how permissions are managed in this application. It looks from the docs like this was probably done as a one-off rather than as part of the deploy process.

The other thing we could consider doing is just setting the IDs as env vars. We can't use the AggregatorApiFrontendFqdn output to set an env var because we have to define the env vars before that output is available. I'm slightly less clear on ServerlessRestApi. I don't think I can really explain where that comes from.

I think this would be a good one for us to catch up on next week @symroe

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

Successfully merging this pull request may close these issues.

1 participant