fix: reject non-HTTPS originating requests #1002
Conversation
- CloudFoundry apps (such as the this Cloud Service Broker) only need to
handle HTTP as CloudFoundry handles TLS termination, simplifying the
app
- It was noted that although the broker is always registered as an HTTPS
endpoint, it can still accept HTTP requests, which adds potential for
erroneous configuration and a subsequent plaintext transmission of
service instance credentials.
- This fix changes the CSB to examine the "X-Forwarded-Proto" HTTP
header to ensure that all requests were sent to CloudFoundry as HTTPS,
and if not they are redirected to the appropriate HTTPS endpoint
- Some folks used to run the CSB in Kubernetes or other non-CloudFoundry
environments, and we don't believe anyone does this any more. But if
anyone does they can:
- Disable this behavior by setting `CSB_LISTENER_ALLOW_HTTP`
- Raising an issue, so that the authors are made aware that we broke
you and that you have a non-CloudFoundry use case.
[#187357359](https://www.pivotaltracker.com/story/show/187357359)
pivotal-marcela-campo
force-pushed
the
fix-block-http
branch
from
accf471
to
1eb08b3
Compare
There was a problem hiding this comment.
The code looks fine, however I was trying to get some context on the different contents of the header we are using and came across this page https://http.dev/x-forwarded-proto which mentions that there is now a more standarised way of handling this requests when there are intermediaries. I am not sure this is the case, but makes me wonder if we are always going to get a value for that header.
From here https://docs.cloudfoundry.org/concepts/http-routing.html#http-headers we believe when the header doesn't come we can't infer anything.
| @@ -201,10 +204,11 @@ func startServer(registry pakBroker.BrokerRegistry, db *sql.DB, brokerapi http.H | |||
| } | |||
| }) | |||
|
|
|||
| allowHTTP := len(viper.GetString(apiAllowHTTP)) != 0 | |||
There was a problem hiding this comment.
question: Why not use viper.GetBool instead of viper.GetString? This way, we could rely on Viper to cast the value.
| @@ -201,10 +204,11 @@ func startServer(registry pakBroker.BrokerRegistry, db *sql.DB, brokerapi http.H | |||
| } | |||
| }) | |||
|
|
|||
| allowHTTP := len(viper.GetString(apiAllowHTTP)) != 0 | |||
There was a problem hiding this comment.
question: Is it accurate to say that setting api.allow.http=false will actually be interpreted as true?
| } | ||
|
|
||
| for _, proto := range xForwardProto { | ||
| if proto != "https" { |
There was a problem hiding this comment.
issue: the HTTP specification allows for optional white-space around the comma separator in a list of header field values.
We should trim the string: proto = strings.TrimSpace(proto)
If I add this test it fails. The ServeHTTPCallCount is 0:
It("allows an HTTPS connection with with-spaces", func() {
httpsmiddleware.EnsureHTTPS(fakeNext, logger, false).ServeHTTP(fakeResponseWriter, &http.Request{
Method: http.MethodHead,
URL: &fakeURL,
Header: http.Header{xForwardProto: []string{"https ,https"}},
})
Expect(fakeNext.ServeHTTPCallCount()).To(Equal(1))
Expect(fakeResponseWriter.WriteHeaderCallCount()).To(BeZero())
})
EnsureHTTPS() [It] allows an HTTPS connection with with-spaces
/home/andrea/workspace/csb/cloud-service-broker/internal/httpsmiddleware/ensurehttps_test.go:88
[FAILED] Expected
<int>: 0
to equal
<int>: 1
In [It] at: /home/andrea/workspace/csb/cloud-service-broker/internal/httpsmiddleware/ensurehttps_test.go:95 @ 04/10/24 15:13:23.18
------------------------------
Summarizing 1 Failure:
[FAIL] EnsureHTTPS() [It] allows an HTTPS connection with with-spaces
/home/andrea/workspace/csb/cloud-service-broker/internal/httpsmiddleware/ensurehttps_test.go:95
CSB_LISTENER_ALLOW_HTTP#187357359