facebookarchive / CommunityCellularManager

Tool for deploying, managing and controlling your Community Cellular Networks
Other
87 stars 36 forks source link

Devel nginx https #25

Closed matt9j closed 7 years ago

matt9j commented 7 years ago

This pull request address #23 and has been tested in the uw deployment of CCM. Please advise us on commit structure, upstreaming procedures, etc. if there's anything else we should add/change! Cheers, -mj

facebook-github-bot commented 7 years ago

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented 7 years ago

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

9muir commented 7 years ago

The problem of supporting multi-term conditionals in nginx seems relatively common, so there are probably various ways you could go to address the POST case. Or you could just split this into two separate PRs and we can merge the 'rewrite -> return 301' fix separately.

Here's a quick stab at this that seems to mostly work:

    if ($http_x_forwarded_proto != "https") {
        set $cond "!https";
    }
    if ($request_method = "POST") {
        set $cond "${cond}:post";
    }
    if ($cond = "!https:post") {
       return 405 'Allow "GET" always';
    }

[Shamelessly taken from https://www.digitalocean.com/community/questions/conditional-within-an-if-in-nginx]

9muir commented 7 years ago

One other thing that I think will be useful is an exception for localhost, since we don't (at least in AWS) terminate SSL locally. That probably then rolls up into the general 'non-https' redirect conditional, something like this for the PR as a whole:

    set $cond "";
    if ($http_x_forwarded_proto != "https") {
        set $cond "!https";
    }
    if ($request_method = "POST") {
        set $cond "${cond}:post";
    }
    if ($remote_addr = "127.0.0.1") {
        set $cond "${cond}:localhost";
    }
    # the variable $cond encapsulates the 3 factors we care about in determining
    # how to handle non-https messages:
    # 1) accept only if they come from localhost - the suffix 'localhost' in the
    # $cond variable causes the two conditionals below to fail and be skipped.
    # 2) send standard 301 to any non-POST request (no 'post' suffix)
    # 3) send a 405 reponse to POST requests, since otherwise clients will turn
    # them into GET on receiving the 301 response.
    if ($cond = "!https") {
      return 301 https://$host$request_uri;
    }
    if ($cond = "!https:post") {
       return 405;
    }

Can you test against the UW env please?

facebook-github-bot commented 7 years ago

@matt-9-johnson updated the pull request - view changes - changes since last import

matt9j commented 7 years ago

Can you test against the UW env please?

This passes a basic sniff test in the UW environment-- after reloading the configuration nginx runs fine and returns the appropriate values for the different conditions. I tested with curl that I got the expected 405 for an http remote post, 301 for an http remote get, and no server errors for localhost or https. I also did a quick sanity check of the site itself to make sure my instance was running stably.

facebook-github-bot commented 7 years ago

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.