Skip to content

Commit

Permalink
Do not return a denyError for DNS resolution failures (stripe#194)
Browse files Browse the repository at this point in the history
* dont return denial errors for dns resolution failures

* fix test

* move DNSError check into net.Error assertion, extend test

* fix integration test
  • Loading branch information
cds2-stripe authored and matt-intercom committed Jan 4, 2024
1 parent 0345168 commit a9b6ba2
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 7 deletions.
2 changes: 1 addition & 1 deletion cmd/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ func validateProxyResponseWithUpstream(t *testing.T, test *TestCase, resp *http.
t.Logf("HTTP Response: %#v", resp)

if test.OverConnect {
a.Contains(err.Error(), "Failed to connect to remote host")
a.Contains(err.Error(), "Failed to resolve remote hostname")
} else {
a.Equal(http.StatusBadGateway, resp.StatusCode)
}
Expand Down
11 changes: 10 additions & 1 deletion pkg/smokescreen/smokescreen.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,11 @@ func rejectResponse(pctx *goproxy.ProxyCtx, err error) *http.Response {
status = "Gateway timeout"
code = http.StatusGatewayTimeout
msg = "Timed out connecting to remote host: " + e.Error()

} else if e, ok := err.(*net.DNSError); ok {
status = "Bad gateway"
code = http.StatusBadGateway
msg = "Failed to resolve remote hostname: " + e.Error()
} else {
status = "Bad gateway"
code = http.StatusBadGateway
Expand Down Expand Up @@ -620,9 +625,13 @@ func handleConnect(config *Config, pctx *goproxy.ProxyCtx) (string, error) {
pctx.Error = denyError{err}
return "", pctx.Error
}

// checkIfRequestShouldBeProxied can return an error if either the resolved address is disallowed,
// or if there is a DNS resolution failure.
sctx.decision, sctx.lookupTime, pctx.Error = checkIfRequestShouldBeProxied(config, pctx.Req, destination)
if pctx.Error != nil {
return "", denyError{pctx.Error}
// DNS resolution failure
return "", pctx.Error
}
if !sctx.decision.allow {
return "", denyError{errors.New(sctx.decision.reason)}
Expand Down
16 changes: 11 additions & 5 deletions pkg/smokescreen/smokescreen_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -406,11 +406,10 @@ func TestHealthcheck(t *testing.T) {

var invalidHostCases = []struct {
scheme string
expectErr bool
proxyType string
}{
{"http", false, "http"},
{"https", true, "connect"},
{"http", "http"},
{"https", "connect"},
}

func TestInvalidHost(t *testing.T) {
Expand All @@ -430,12 +429,19 @@ func TestInvalidHost(t *testing.T) {
client, err := proxyClient(proxySrv.URL)
r.NoError(err)

// This hostname does not exist and should never resolve
resp, err := client.Get(fmt.Sprintf("%s://notarealhost.test", testCase.scheme))
if testCase.expectErr {
r.Contains(err.Error(), "Request rejected by proxy")
if testCase.scheme == "https" {
r.Error(err)
r.Contains(err.Error(), "Bad gateway")
} else {
// Plain HTTP
r.NoError(err)
r.Equal(http.StatusBadGateway, resp.StatusCode)

defer resp.Body.Close()
b, _ := ioutil.ReadAll(resp.Body)
r.Contains(string(b), "Failed to resolve remote hostname")
}

entry := findCanonicalProxyDecision(logHook.AllEntries())
Expand Down

0 comments on commit a9b6ba2

Please sign in to comment.