BCDA-471: Refactor BCDA http server to handle https connections

[em1]

Fixes BCDA-471

The BCDA http server must be refactored in order to accept https connections.

Proposed changes:

• Add ability to connect over HTTPS

Change Details

• New environment variables BCDA_TLS_CERT, BCDA_TLS_KEY, HTTP_ONLY
• servicemux.go checks the env vars in Serve() and routes to serveHTTPS() or serveHTTP() depending on whether HTTP_ONLY is true. Defaults to using HTTPS and panics if certificate and key paths are not provided.
• serveHTTPS() loads the certificate and key and creates a TLS listener, and adds TLSConfig with Certificates to each server to enable new IsHTTPS() function
• Note that tests are not included in this PR, but there is a separate ticket (BCDA-509) to create them in the next sprint

Security Implications

More secure with TLS!

Acceptance Validation

Able to connect via https with self-signed certificate locally.

Feedback Requested

Any. Do we need to test in dev or sbx to validate before approval?

[msnook]

Do we need a follow-on ticket to this to (1) disable http within the app altogether, and (2) update ops scripts to close down 80 and open up 443 ?

[em1]

For local development, I think we want to keep the http option. As far as I know, we don't have certificates for any environments right now, so not sure how soon there would need to be ops changes.

[msnook]

Good point on the local development. Maybe we need a FORCE_HTTPS feature flag which defaults to false, and we can configure it to TRUE in AWS when we are ready?

[StewGoin]

Similar to the encryption flags, I don't mind at all having this be an environment variable / flag, but default it to the Prod requirement if possible and make the explicit exception be required for Dev (you want to avoid https? you have to do it on purpose, otherwise you get https).

[tbellj]

looks good to me, but is there a way to test this locally?

[rnagle]

This is a good start, though it doesn't quite work front-to-back.

The problem I'm seeing is that a request for bulk data sends back the Content-Location header with an http URL. This happens because http.Request.TLS is not available here. This is a noted limitation of cmux.

Consider our test client, which uses the api's responses to step through the data request workflow automagically. When it tries to use the http URL from the Content-Location header, it will be unable to continue. 😞

So, I did a little experimenting and have an idea for you --

Consider this helper func:

func IsHTTPS(r *http.Request) bool {
	srv := r.Context().Value(http.ServerContextKey).(*http.Server)
	return srv.TLSConfig.Certificates != nil
}

This works based on the assumption that a populated http.Server.TLSConfig.Certificates field is fundamentally required in order to handle https requests.

With it, you can replace instances of this:

scheme := "http"
if r.TLS != nil {
  scheme = "https"
}

With:

scheme := "http"
if IsHTTPS(r) == true {
  scheme = "https"
}

Caveat is that you must set http.Server.TLSConfig on each of your ServiceMux.Servers using the tls.Config you set up here.

In the comments below, I mention adding a catch-all handler to redirect any http requests to their https equivalents. I think it'd be smart to add this if it's not a crazy change of scope. It should help in cases where bad/ill-informed clients mix http and https protocols in their requests.

[rnagle]

It'd be nice if we could set PreferServerCipherSuites and CurvePreferences in the tls.Config per the recommendations here.

[rnagle]

Side note: with this configuration, when https is enabled, we only support https. Should we also accept http requests and simply redirect them to their https equivalent? My instincts are telling me yes, it would be smart to add this in. This is another question that comes straight from link in my previous comment.

Made requested changes and created ticket for http -> https redirect.

[rnagle]

Tested locally and both http and https configurations work as expected. One minor change, one question in PR comments.

Before merging, we'll need to set HTTP_ONLY=true for our deployment environments so that the app functions properly until we get our DNS and certificates set up. Let me know when you're ready to merge and I can help with that.

[rnagle]

Please move this to docker-compose.yml.

[rnagle]

Any reason not to set srv.TLSConfig = config?

[rnagle]

Great work. 👍