Add support for upstream SSL

[yanokwa]

This builds on the PR at #182 from @aiqui. Fixes #213.

It'd be nice to have this functionality for ODK Cloud so we can manage SSL at the load balancer rather than within nginx.

Changes

• I don't love hiding variables from people, so instead of none, which suggests to people that they don't need SSL, I call it upstream. Open to changing the name and open to hiding it.
• Updated output message to reflect the above

Additional, unrelated change:

• Moved variables to .env.template to make it easier to evolve without triggering merge conflicts. I believe this will not delete existing .env files that have changes, but I haven't verified this.

[issa-tseng]

will probably cause some diff unhappiness but i guess we'll deal w it

[matthew-white]

All deployments will have a merge conflict after this change, right? It seems like it can't be avoided though, given the new option for SSL_TYPE.

[issa-tseng]

we could maybe try to decontrol the file and provide a default some other way instead? i'm not sure.

[yanokwa]

I discussed this with @lognaturel a bit and she doesn't love diff unhappiness. I've moved the variables to a template file instead and run git rm --cached .env to make sure .env isn't in the repo.

I believe this will not delete existing .env files that have changes, but I haven't verified this.

[yanokwa]

I have tried a lot of git things and I haven't been able to figure out how to remove an ignored file from the repo and keep it in place on everyone's filesystems. Maybe we should not have tracked .env to begin with?

My best solution so far is that we create .env.template and delete .env. Then in the docs, we add an Upgrading to Central 1.2 section where we ask users to do the following.

1. Before git pull, move .env .env.backup.
2. Run git pull, then move .env.backup to .env.
3. Add HTTP/HTTPS port

Then going forward, anything we can put whatever we want in .env.template, but users accordingly, must edit their .env to add those new variables. Or maybe we write a script that puts the defaults in there.

I don't love any of this, but the alternative is that users are dealing with merge conflicts. Editing a text file seems way easier to deal with.

[issa-tseng]

yeah, we shouldn't have versioned .env. i was trying to save install steps.
i don't have any better ideas but one may exist.

[matthew-white]

It seems like the compose file allows you to specify a default value when you use variable substitution: https://docs.docker.com/compose/compose-file/compose-file-v3/#variable-substitution

Using that, we could do something like:

ports:
  - "${HTTP_PORT:-80}:80"
  - "${HTTPS_PORT:-443}:443"

An advantage of that is that current users wouldn't need to add $HTTP_PORT and $HTTPS_PORT to their existing .env file.

[yanokwa]

Thanks Matt! I don't know how I missed that. I've pushed a change that uses defaults for SSL_TYPE, HTTP_PORT, and HTTPS_PORT. I think we should still have people set DOMAIN and SYSADMIN_EMAIL in .env.

[matthew-white]

Just to check, this will go out as part of v1.2, right?

[yanokwa]

@matthew-white Yes, it will, but I'll wait a few days before merging. I want to test the PR a little bit more.

[matthew-white]

OK, sounds good! Since merging deploys immediately to production, I can also merge later when we release v1.2.

[lognaturel]

I can also merge later when we release v1.2.

I think that makes the most sense and we should make sure that there is documentation in place at that time.

It seems like the compose file allows you to specify a default value

YAY. I was very sad when @yanokwa said that was not possible.

[matthew-white]

During today's call, @issa-tseng said that this PR is ✅, so I'll plan to merge this when we release v1.2. I think that @issa-tseng is going to add instructions to the docs for upgrading to v1.2 related to moving .env. We can also highlight those instructions in the release announcement on the forum. @yanokwa, are there any other to-dos for this PR? Is there more testing that you want to do?

[yanokwa]

I'm happy with merging. Thanks for all the great feedback, team!

[issa-tseng]

i wonder if we should enforce that if upstream is set, we require incoming requests to carry X-Forwarded-Proto: https.

[yanokwa]

@issa-tseng Not sure the implications. In my testing setup, the proxy_pass happens over HTTP (and the Central installs are also listening via HTTP on port 80). If you can send me the config you are proposing, I'm glad to test it.

server {
    server_name odk-central.example.com;
    location / {
            proxy_pass http://0.0.0.0:8080;
    }
    listen [::]:443 ssl;
    listen 443 ssl;
    ssl_certificate /etc/ssl/certs/server.crt;
    ssl_certificate_key /etc/ssl/certs/server.key;
    ssl_trusted_certificate /etc/ssl/certs/server.pem;
}
server {
    server_name odk-central-testing.example.com;
    location / {
            proxy_pass http://0.0.0.0:9080;
    }
    listen [::]:443 ssl;
    listen 443 ssl;
    ssl_certificate /etc/ssl/certs/server.crt;
    ssl_certificate_key /etc/ssl/certs/server.key;
    ssl_trusted_certificate /etc/ssl/certs/server.pem;
}
server {
    if ($host = odk-central.example.com) {
        return 301 https://$host$request_uri;
    }
    if ($host = odk-central-testing.example.com) {
        return 301 https://$host$request_uri;
    }
    listen 80 ;
    listen [::]:80 ;
    server_name odk-central.example.com odk-central-testing.example.com;
    return 404;
}

[issa-tseng]

i'm not, yet. i'm wondering if that kind of enforcement would even have support amongst y'all.

[yanokwa]

Doesn't seem like a priority to me. Most of Central won't work properly without SSL anyway, right? I'd rather we just put "Central will not work properly without HTTPS" in the docs and if people want to try to get around that, that's their prerogative.

[yanokwa]

I've force pushed a change to set X-Forwarded-Proto https when you set SSL_TYPE=upstream.

• Pros: This is that it matches the intent of someone who sets upstream without them
• Cons: It's not safe? Maybe we remove the header entirely?

I took a quick look at @matthew-white's suggestion that we only set if the header if it isn't set already, but this suggests it isn't possible.

[matthew-white]

I think as long as we're operating under the assumption that the upstream SSL is working, this seems like a reasonable change. It'd be great to hear what @issa-tseng thinks.

Maybe we remove the header entirely?

I believe that we do need the header for cookie auth to work.

I took a quick look at @matthew-white's suggestion that we only set if the header if it isn't set already, but this suggests it isn't possible.

I actually think the second answer is promising. It'd be something like:

map $http_x_forwarded_proto $forwarded_proto {
  ~. $http_x_forwarded_proto;
  default $scheme;
}

# ...

proxy_set_header X-Forwarded-Proto $forwarded_proto;

I don't think map can go inside a server block, but maybe it can go before the server block in the Central config file? (Not sure.)

[matthew-white]

I just noticed that this has a similar suggestion (though without the regular expression).

[yanokwa]

The suggestion to map works as far as the syntax, but it doesn't solve the redirect problem.