diff --git a/.github/workflows/pr-and-merge.yaml b/.github/workflows/pr-and-merge.yaml new file mode 100644 index 0000000..5ce4b8f --- /dev/null +++ b/.github/workflows/pr-and-merge.yaml @@ -0,0 +1,34 @@ +--- +name: CI +on: + pull_request: + +jobs: + golangci-lint: + name: golangci-lint + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + - name: Install Go + uses: actions/setup-go@v5 + with: + go-version: "1.23" + - name: Run golangci-lint + uses: golangci/golangci-lint-action@v6 + with: + version: v1.63.4 + + tests: + name: tests + needs: golangci-lint + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + - name: Install Go + uses: actions/setup-go@v5 + with: + go-version: "1.23" + - name: Test code + run: go test -race -v ./... diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..aea335b --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,74 @@ +--- +linters-settings: + stylecheck: + # STxxxx checks in https://staticcheck.io/docs/configuration/options/#checks + # ST1000 Incorrect or missing package comment. + checks: ["all", "-ST1000"] + misspell: + locale: US + gci: + sections: + - standard # Standard section: captures all standard packages. + - default # Default section: contains all imports that could not be matched to another section type. + - localmodule # Local module section: contains all local packages. This section is not present unless explicitly enabled. + +linters: + disable-all: true + enable: + - asasalint + - asciicheck + - bidichk + - errcheck + - errchkjson + - errorlint + - exhaustive + - fatcontext + - gci + - gocritic + - godox + - gofmt + - gofumpt + - goimports + - gosec + - govet + - ineffassign + - misspell + - nolintlint + - predeclared + - reassign + - sloglint + - staticcheck + - stylecheck + - unparam + - unused + - usestdlibvars + - wastedassign + - whitespace + - wsl + +issues: + exclude-use-default: false + exclude-rules: + - path: '_test\.go' + linters: + - errcheck + + # TLS is not needed in tests. + - text: "G402: TLS InsecureSkipVerify set true" + path: '_test\.go' + linters: + - gosec + + # Ignoring errors of .Close() is ok. + - text: "Error return value of `.*\\.Close` is not checked" + linters: + - errcheck + + # Unhandled errors are checked by errcheck. + - text: G104 + linters: + - gosec + + # Ensure that we can see all issues at once. + max-issues-per-linter: 0 + max-same-issues: 0 diff --git a/README.md b/README.md index 3292f66..4801048 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,6 @@ server := &Server{ IdleTimeout: 350 * time.Second, WriteTimeout: 2 * time.Minute, ReadTimeout: 10 * time.Second, - ErrorLog: log, // whichever log you prefer that fit the interface MaxMessageSize: 1000, } diff --git a/go.mod b/go.mod index 6e4b35e..e96efcf 100644 --- a/go.mod +++ b/go.mod @@ -1,11 +1,10 @@ module github.com/dotse/epp-lib -go 1.18 +go 1.23 require ( - github.com/beevik/etree v1.1.0 - github.com/pkg/errors v0.9.1 - github.com/stretchr/testify v1.8.0 + github.com/beevik/etree v1.5.0 + github.com/stretchr/testify v1.10.0 ) require ( diff --git a/go.sum b/go.sum index a6e50c7..9b31847 100644 --- a/go.sum +++ b/go.sum @@ -1,19 +1,12 @@ -github.com/beevik/etree v1.1.0 h1:T0xke/WvNtMoCqgzPhkX2r4rjY3GDZFi+FjpRZY2Jbs= -github.com/beevik/etree v1.1.0/go.mod h1:r8Aw8JqVegEf0w2fDnATrX9VpkMcyFeM0FhwO62wh+A= -github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/beevik/etree v1.5.0 h1:iaQZFSDS+3kYZiGoc9uKeOkUY3nYMXOKLl6KIJxiJWs= +github.com/beevik/etree v1.5.0/go.mod h1:gPNJNaBGVZ9AwsidazFZyygnd+0pAU38N4D+WemwKNs= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= -github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= -github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk= -github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= +github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/logger.go b/logger.go deleted file mode 100644 index ac2d85d..0000000 --- a/logger.go +++ /dev/null @@ -1,28 +0,0 @@ -package epplib - -import "fmt" - -// Logger hold all logging functions needed. -type Logger interface { - Debugf(string, ...interface{}) - Errorf(string, ...interface{}) - Infof(string, ...interface{}) -} - -// DummyLogger for the logger interface used for testing. -type DummyLogger struct{} - -// Errorf for DummyLogger fmt.Printf the information. -func (*DummyLogger) Errorf(format string, args ...interface{}) { - fmt.Printf(format, args...) //nolint:forbidigo // dummy logger -} - -// Infof for DummyLogger fmt.Printf the information. -func (*DummyLogger) Infof(format string, args ...interface{}) { - fmt.Printf(format, args...) //nolint:forbidigo // dummy logger -} - -// Debugf for DummyLogger fmt.Printf the information. -func (*DummyLogger) Debugf(format string, args ...interface{}) { - fmt.Printf(format, args...) //nolint:forbidigo // dummy logger -} diff --git a/mux.go b/mux.go index 76079fa..7eff00a 100644 --- a/mux.go +++ b/mux.go @@ -3,6 +3,7 @@ package epplib import ( "context" "io" + "log/slog" "github.com/beevik/etree" ) @@ -11,8 +12,6 @@ import ( type CommandMux struct { greetingCommand CommandFunc handlers []handler - - Logger Logger } // GetGreeting returns a greeting. @@ -27,7 +26,10 @@ func (c *CommandMux) Handle(ctx context.Context, rw *ResponseWriter, cmd io.Read _, err := doc.ReadFrom(cmd) if err != nil { - c.Logger.Infof("could not read command, err: %s", err) + slog.InfoContext(ctx, "could not read command", + slog.Any("err", err), + ) + rw.CloseAfterWrite() return @@ -40,7 +42,7 @@ func (c *CommandMux) Handle(ctx context.Context, rw *ResponseWriter, cmd io.Read } } - c.Logger.Infof("unknown command") + slog.InfoContext(ctx, "unknown command") rw.CloseAfterWrite() } diff --git a/mux_test.go b/mux_test.go index 67d1d35..516e329 100644 --- a/mux_test.go +++ b/mux_test.go @@ -32,9 +32,7 @@ func TestMux_Handle(t *testing.T) { barCalled bool ) - cm := &CommandMux{ - Logger: &DummyLogger{}, - } + cm := &CommandMux{} cm.Bind( "//foo[namespace-uri()='urn:ietf:params:xml:ns:epp-1.0']", @@ -115,12 +113,15 @@ func TestMux_Bind(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { cm := &CommandMux{} + if tc.wantPanic { assert.Panics(t, func() { cm.Bind(tc.path, tc.handler) }) + return } + cm.Bind(tc.path, tc.handler) assert.Equal(t, len(cm.handlers), tc.wantLength) }) diff --git a/read_write.go b/read_write.go index 7d95ca1..a3765ae 100644 --- a/read_write.go +++ b/read_write.go @@ -4,10 +4,9 @@ import ( "bytes" "encoding/binary" "errors" + "fmt" "io" "math" - - pkgerrors "github.com/pkg/errors" ) // ErrMessageSize represent an error where the incoming message size is bigger than allowed. @@ -16,9 +15,15 @@ var ErrMessageSize = errors.New("message size exceeds limit") // MessageReader returns an io.Reader that reads one message according to the // message size header. Blocks until a message size header is read. // The message limit is in bytes. -func MessageReader(r io.Reader, msgLimit int64) (io.Reader, error) { +func MessageReader(r io.Reader, msgLimit uint32) (io.Reader, error) { // Get the size of the message we are going to read in the future. // https://tools.ietf.org/html/rfc5734#section-4 + // + // Note:The RFC doesn't explicitly state "signed" or "unsigned", but based + // on its description the field is clearly meant to represent a + // non-negative total length. Since a length can't be negative, it's best + // to interpret the 32 bits as an unsigned integer, which is what we have + // done historically. var totalSize uint32 err := binary.Read(r, binary.BigEndian, &totalSize) @@ -26,19 +31,18 @@ func MessageReader(r io.Reader, msgLimit int64) (io.Reader, error) { return nil, err } - // The size that the client sent is the size including the bytes that tells - // the size so we need to subtract that to get the actual message size. - messageSize := int64(totalSize - uint32(binary.Size(totalSize))) - - if messageSize <= 0 || (msgLimit != 0 && messageSize > msgLimit) { - return nil, pkgerrors.Wrapf(ErrMessageSize, "incoming message size %d", messageSize) + // The length of the EPP XML instance is determined by subtracting four + // octets from the total length of the data unit. The four octets are the + // 32bit length field. + if totalSize <= 4 || (msgLimit > 0 && totalSize > msgLimit) { + return nil, fmt.Errorf("%w: incoming message size %d", ErrMessageSize, totalSize) } // Since we know the message size of the future message we can create a // reader that will read exactly that size and then return an EOF. That way // reading from the connection will always read the number of bytes that // the client said the message is. - return io.LimitReader(r, messageSize), nil + return io.LimitReader(r, int64(totalSize-4)), nil } // ResponseWriter is an io.Writer that buffers response data before writing it @@ -69,20 +73,18 @@ type MessageBuffer struct { // FlushTo flushes the buffer to dst after writing the message size header. func (mb *MessageBuffer) FlushTo(dst io.Writer) error { - if mb.Len() == 0 { - // Nothing to write. - return nil - } // Begin by writing the len(b) as Big Endian uint32, including the // size of the content length header. // https://tools.ietf.org/html/rfc5734#section-4 contentSize := mb.Len() - headerSize := binary.Size(uint32(contentSize)) - totalSize := contentSize + headerSize + if contentSize == 0 { + // Nothing to write. + return nil + } - // Bounds check. - if totalSize > math.MaxUint32 { - return errors.New("content is too large") + totalSize := contentSize + 4 // 4 bytes for the content size header. + if totalSize <= 4 || totalSize > math.MaxUint32 { + return errors.New("invalid message size") } err := binary.Write(dst, binary.BigEndian, uint32(totalSize)) diff --git a/server.go b/server.go index 42acc1b..f6761fc 100644 --- a/server.go +++ b/server.go @@ -4,8 +4,8 @@ import ( "context" "crypto/tls" "errors" - "fmt" "io" + "log/slog" "net" "os" "strings" @@ -51,11 +51,11 @@ type Server struct { // MaxMessageSize if set will return an error if the incoming request // is bigger than the set size in bytes. 0 indicates no limit. - MaxMessageSize int64 + MaxMessageSize uint32 // Logger logs errors when accepting connections, unexpected behavior // from handlers and underlying connection errors. - Logger Logger + Logger *slog.Logger // We keep track of our active connections here. This is guarded by mu. activeConn map[*eppConn]struct{} @@ -78,6 +78,10 @@ func (s *Server) Serve(listener Listener) error { panic("Handler and Greeting is required") } + if s.Logger == nil { + s.Logger = slog.Default() + } + s.listenerMu.Lock() s.listener = listener s.listenerMu.Unlock() @@ -184,13 +188,19 @@ func (s *Server) serveConn(conn net.Conn) { err := setDeadlines(c.conn, s.ReadTimeout, s.WriteTimeout) if err != nil { - s.logError("handshake deadlines", err) + s.Logger.ErrorContext(ctx, "failed to set handshake deadlines", + slog.Any("error", err), + ) + return } err = tlsConn.Handshake() if err != nil { - s.logDebug("handshake", err) + s.Logger.DebugContext(ctx, "handshake failed", + slog.Any("error", err), + ) + return } @@ -210,7 +220,10 @@ func (s *Server) serveConn(conn net.Conn) { err = setDeadlines(c.conn, s.ReadTimeout, s.WriteTimeout) if err != nil { - s.logError("set deadlines for greeting", err) + s.Logger.ErrorContext(ctx, "failed to set greeting deadlines", + slog.Any("error", err), + ) + return } @@ -219,7 +232,10 @@ func (s *Server) serveConn(conn net.Conn) { err = rw.FlushTo(c.conn) if err != nil { - s.logError("flush greeting", err) + s.Logger.ErrorContext(ctx, "failed to flush greeting", + slog.Any("error", err), + ) + return } @@ -237,13 +253,16 @@ func (s *Server) serveConn(conn net.Conn) { err := c.conn.SetDeadline(deadline) if err != nil { - s.logError("set deadlines for await message", err) + s.Logger.ErrorContext(ctx, "failed to set deadlines for await message", + slog.Any("error", err), + ) + return } // Wait for a message to appear on the connection. cmd, err := c.AwaitMessage() - if err != nil { // nolint:nestif // Needed. + if err != nil { if errors.Is(err, os.ErrDeadlineExceeded) { // We have reached the deadline for this session, we now need // to disconnect. @@ -264,15 +283,18 @@ func (s *Server) serveConn(conn net.Conn) { } if errors.Is(err, io.ErrUnexpectedEOF) { - // We don't want to turn this of entirely - s.logInfo("await message", err) + // We don't want to turn this off entirely + s.Logger.InfoContext(ctx, "await message failed, unexpected EOF") return } if errors.Is(err, ErrMessageSize) { // Client has told us that the incoming message is larger than // our supported max size of a message. - s.logInfo(fmt.Sprintf("Message limit exceeded from %q", c.conn.RemoteAddr()), err) + s.Logger.InfoContext(ctx, "await message failed, size limit exceeded", + slog.Any("error", err), + slog.String("remote_addr", c.conn.RemoteAddr().String()), + ) return } @@ -284,19 +306,29 @@ func (s *Server) serveConn(conn net.Conn) { // handshake is complete, just closing the connection by sending a // close_notify is more appropriate. This alert should be followed // by a close_notify. This message is generally a warning. - s.logInfo(fmt.Sprintf("handshake was canceled by client %q", c.conn.RemoteAddr()), err) + s.Logger.InfoContext(ctx, "await message failed, handshake was canceled by client", + slog.Any("error", err), + slog.String("remote_addr", c.conn.RemoteAddr().String()), + ) + return } // We have some other error - s.logError(fmt.Sprintf("await message from %q", c.conn.RemoteAddr()), err) + slog.ErrorContext(ctx, "await message failed", + slog.Any("error", err), + slog.String("remote_addr", c.conn.RemoteAddr().String()), + ) return } err = setDeadlines(c.conn, s.ReadTimeout, s.WriteTimeout) if err != nil { - s.logError("set deadlines for command read/write", err) + s.Logger.ErrorContext(ctx, "failed to set deadlines for command read/write", + slog.Any("error", err), + ) + return } @@ -308,7 +340,11 @@ func (s *Server) serveConn(conn net.Conn) { if err != nil { if errors.Is(err, syscall.EPIPE) { // The client has closed the connection. I.e. "broken pipe". - s.logInfo("flush response", err) + s.Logger.InfoContext(ctx, + "failed to flush response, client has closed connection, broken pipe", + slog.Any("error", err), + ) + return } @@ -317,11 +353,18 @@ func (s *Server) serveConn(conn net.Conn) { // Wierdly enough this is not caught by a errors.Is(err, sycall.ECONNRESET) // like is done above in awaitMessage. Therefore we will info log here in- // case catch to broadly here. - s.logInfo("flush response", err) + s.Logger.InfoContext( + ctx, + "failed to flush response, client has closed connection, connection reset by peer", + slog.Any("error", err), + ) + return } - s.logError("flush response", err) + s.Logger.InfoContext(ctx, "failed to flush response", + slog.Any("error", err), + ) return } @@ -349,24 +392,6 @@ func (s *Server) CloseConnection(conn *tls.Conn) error { return nil } -func (s *Server) logError(prefix string, err error) { - if s.Logger != nil { - s.Logger.Errorf("epp: %s: %+v", prefix, err) - } -} - -func (s *Server) logInfo(prefix string, err error) { - if s.Logger != nil { - s.Logger.Infof("epp: %s: %+v", prefix, err) - } -} - -func (s *Server) logDebug(prefix string, err error) { - if s.Logger != nil { - s.Logger.Debugf("epp: %s: %v", prefix, err) - } -} - func setDeadlines(conn net.Conn, readTimeout, writeTimeout time.Duration) error { err := conn.SetReadDeadline(deadlineFromTimeout(readTimeout)) if err != nil { @@ -418,7 +443,7 @@ type eppConn struct { // causes AwaitMessage to return net.ErrClosed immediately when called. stopAwaitMsg int32 - maxMessageSize int64 + maxMessageSize uint32 } // AwaitMessage blocks until a message header is read from the underlying diff --git a/server_test.go b/server_test.go index 7c8161d..da1ff48 100644 --- a/server_test.go +++ b/server_test.go @@ -50,7 +50,6 @@ func TestListenAndServeClose(t *testing.T) { s := Server{ HandleCommand: func(ctx context.Context, rw *ResponseWriter, cmd io.Reader) {}, Greeting: func(ctx context.Context, rw *ResponseWriter) {}, - Logger: &DummyLogger{}, TLSConfig: tls.Config{ InsecureSkipVerify: true, Certificates: []tls.Certificate{generateCertificate()},