Skip to content

Commit

Permalink
fix(server): use RCode=ServFail instead of HTTP 500 for internal errors
Browse files Browse the repository at this point in the history
RFC 8484 Section 4.2.1:
> A successful HTTP response with a 2xx status code (see
> Section 6.3 of RFC7231) is used for any valid DNS response,
> regardless of the DNS response code.  For example, a successful 2xx
> HTTP status code is used even with a DNS message whose DNS response
> code indicates failure, such as SERVFAIL or NXDOMAIN.
https://www.rfc-editor.org/rfc/rfc8484#section-4.2.1
  • Loading branch information
ThinkChaos committed Mar 19, 2024
1 parent 3fcf379 commit 4919ffa
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 10 deletions.
8 changes: 6 additions & 2 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -615,8 +615,12 @@ func (s *Server) OnRequest(
}
}

func (s *Server) resolve(ctx context.Context, request *model.Request) (*model.Response, error) {
var response *model.Response
func (s *Server) resolve(ctx context.Context, request *model.Request) (response *model.Response, rerr error) {
defer func() {
if val := recover(); val != nil {
rerr = fmt.Errorf("panic occurred: %v", val)
}
}()

contextUpstreamTimeoutMultiplier := 100
timeoutDuration := time.Duration(contextUpstreamTimeoutMultiplier) * s.cfg.Upstreams.Timeout.ToDuration()
Expand Down
28 changes: 22 additions & 6 deletions server/server_endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,32 @@ func (s *Server) dohPostRequestHandler(rw http.ResponseWriter, req *http.Request

func (s *Server) processDohMessage(rawMsg []byte, rw http.ResponseWriter, req *http.Request) {
msg := new(dns.Msg)

if err := msg.Unpack(rawMsg); err != nil {
logger().Error("can't deserialize message: ", err)
http.Error(rw, err.Error(), http.StatusBadRequest)

return
}

rw.Header().Set("content-type", dnsContentType)

writeErr := func(err error) {
log.Log().Error(err)

msg := new(dns.Msg)
msg.SetRcode(msg, dns.RcodeServerFailure)

buff, err := msg.Pack()
if err != nil {
return
}

// https://www.rfc-editor.org/rfc/rfc8484#section-4.2.1
rw.WriteHeader(http.StatusOK)

_, _ = rw.Write(buff)
}

clientID := chi.URLParam(req, "clientID")
if clientID == "" {
clientID = extractClientIDFromHost(req.Host)
Expand All @@ -151,22 +169,20 @@ func (s *Server) processDohMessage(rawMsg []byte, rw http.ResponseWriter, req *h

resResponse, err := s.resolve(ctx, r)
if err != nil {
logAndResponseWithError(err, "unable to process query: ", rw)
writeErr(fmt.Errorf("unable to process query: %w", err))

return
}

b, err := resResponse.Res.Pack()
if err != nil {
logAndResponseWithError(err, "can't serialize message: ", rw)
writeErr(fmt.Errorf("can't serialize message: %w", err))

return
}

rw.Header().Set("content-type", dnsContentType)

_, err = rw.Write(b)
logAndResponseWithError(err, "can't write response: ", rw)
log.Log().Error(fmt.Errorf("can't write response: %w", err))
}

func (s *Server) Query(
Expand Down
38 changes: 36 additions & 2 deletions server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,8 +529,35 @@ var _ = Describe("Running DNS server", func() {
Expect(resp).Should(HaveHTTPStatus(http.StatusUnsupportedMediaType))
})
})
When("DNS error occurs", func() {
It("should return 'ServFail'", func() {
msg = util.NewMsgWithQuestion("error.", A)
rawDNSMessage, err := msg.Pack()
Expect(err).Should(Succeed())

resp, err = http.Post(queryURL,
"application/dns-message", bytes.NewReader(rawDNSMessage))
Expect(err).Should(Succeed())
DeferCleanup(resp.Body.Close)

Expect(resp).Should(HaveHTTPStatus(http.StatusOK))

body, err := io.ReadAll(resp.Body)
Expect(err).Should(Succeed())

msg := new(dns.Msg)
Expect(msg.Unpack(body)).Should(Succeed())
Expect(msg.Rcode).Should(Equal(dns.RcodeServerFailure))
})
})
When("Internal error occurs", func() {
It("should return 'Internal server error'", func() {
BeforeEach(func() {
bak := sut.queryResolver
sut.queryResolver = nil // trigger a panic
DeferCleanup(func() { sut.queryResolver = bak })
})

It("should return 'ServFail'", func() {
msg = util.NewMsgWithQuestion("error.", A)
rawDNSMessage, err := msg.Pack()
Expect(err).Should(Succeed())
Expand All @@ -540,7 +567,14 @@ var _ = Describe("Running DNS server", func() {
Expect(err).Should(Succeed())
DeferCleanup(resp.Body.Close)

Expect(resp).Should(HaveHTTPStatus(http.StatusInternalServerError))
Expect(resp).Should(HaveHTTPStatus(http.StatusOK))

body, err := io.ReadAll(resp.Body)
Expect(err).Should(Succeed())

msg := new(dns.Msg)
Expect(msg.Unpack(body)).Should(Succeed())
Expect(msg.Rcode).Should(Equal(dns.RcodeServerFailure))
})
})
})
Expand Down

0 comments on commit 4919ffa

Please sign in to comment.