server: optionally serve http traffic when forwarded from https #77343
Conversation
This commit adds a boolean cluster setting: `security.hsts.enabled` which, when enabled attaches standard HSTS headers to http requests originating from CRDB nodes. The headers looke like this: `Strict-Transport-Security: max-age=31536000` When this header is present, most web browsers will automatically upgrade all HTTP connections to HTTPS and *remember* that setting until the expiry of 1 year defined in the header. *Important*: Careless enabling of this feature can result in broken access to the DB Console in the browser. If we instruct the browser to always use HTTPS without having a valid TLS configuration, the browser will no longer fallback to HTTP until the HSTS setting is manually cleared. Resolves cockroachdb#77224 Release justification: opt-in security enhancement Release note (security update, ops change, ui change): users can enable HSTS headers to be set on all HTTP requests which force browsers to upgrade to HTTPS without a redirect. This is controlled by setting the `security.hsts.enabled` cluster setting which is disabled by default.
|
This change is |
This change allows CRDB to serve HTTP traffic with the `server.accept_https_forwarded.enabled` cluster setting is enabled *and* the request contains an `X-Forwarded-Proto` header with the value `https`, or a `Forwarded` header containing the directive: `proto=https`. The aim is to enable easier use of the DB Console behind load balancers. Release justification: opt-in feature that unblocks customers with load balancers using DB Console Release note (ops change, security update, ui change): When an operator configures CRDB to serve HTTP traffic behind a load balancer, often they would like to terminate TLS at the load balancer and have the load balancer use HTTP to talk to the DB since that traffic will sit within their private network instead of the public internet. Enabling the `server.accept_http_forwarded.enabled` cluster setting will instruct CRDB to inspect the `X-Forwarded-Proto` or `Forwarded` headers and allow traffic over HTTP when the protocol is `https`.
dhartunian
force-pushed
the
no-redirect-when-forwarded-https-traffic
branch
from
ef9782c
to
d8b1faf
Compare
There was a problem hiding this comment.
The first commit LGTM and a PR containing only this can be merged right away.
The 2nd commit deserves more attention and IMHO should be pulled into a separate PR. I think we need additional interlocks for it and that deserves a bit more discussion.
Reviewed 3 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @udnay)
pkg/server/server_http.go, line 81 at r1 (raw file):
// of 1 year that the browser should remember to only use HTTPS for // this site. const hstsHeaderValue = "max-age=31536000"
was there a particular reason to use exactly 1 year? is there any need to make this configurable?
There was a problem hiding this comment.
This PR is just for the second commit, I just worked on the two together.
What additional interlocks do you have in mind here? Should I circulate a short doc w/ security and server teams perhaps?
Reviewable status:
|
The issue I currently have with the 2nd commit is that it's not so clear we want to compromise our security model for what is, arguably, an edge case. Note that once this feature is activated, an attacker with access to the HTTP port can bypass HTTPS by providing the new header. We should have a reflection on this and, at a minimum, a security release note that would explain to the operator how to best secure such a setup. But even before then, I don't fully understand why that forwarded http traffic could not forward also over https (while ignoring the server cert on the proxy-server side of the communication). What's so bad about having the back-end of a proxy using https too? If anything, I'd like a review of alternatives. Also, this is a feature-add, not a bug fix, and thus IMHO does not befits stability period. |
|
Thanks for the guidance @knz, upon further reflection I agree with your reasoning here. Perhaps what we really need is a working nginx example for folks to use. I'll close this for now and see if it comes up as a problem again. |
Note: Please ignore the first commit in this PR, I used it as a base since it has a test file already, it will be merged separately here: #77244
Could potentially resolve: cockroachdb/helm-charts#228
This change allows CRDB to serve HTTP traffic with the
server.accept_https_forwarded.enabledcluster setting is enabledand the request contains an
X-Forwarded-Protoheader with the valuehttps, or aForwardedheader containing the directive:proto=https.The aim is to enable easier use of the DB Console behind load balancers.
Release justification: opt-in feature that unblocks customers with load
balancers using DB Console
Release note (ops change, security update, ui change): When an operator
configures CRDB to serve HTTP traffic behind a load balancer, often they
would like to terminate TLS at the load balancer and have the load
balancer use HTTP to talk to the DB since that traffic will sit within
their private network instead of the public internet. Enabling the
server.accept_http_forwarded.enabledcluster setting will instructCRDB to inspect the
X-Forwarded-ProtoorForwardedheaders and allowtraffic over HTTP when the protocol is
https.