Merge branch 'ameshkov:master' into master

.github/workflows/build.yaml

@@ -23,7 +23,7 @@ jobs:
     steps:
       - uses: actions/checkout@master
 
-      - uses: actions/setup-go@v2
+      - uses: actions/setup-go@v3
         with:
           go-version: 1.x
 
@@ -32,7 +32,7 @@ jobs:
           go test -race -v -bench=. -coverprofile=coverage.txt -covermode=atomic ./...
 
       - name: Upload coverage
-        uses: codecov/codecov-action@v1
+        uses: codecov/codecov-action@v3
         if: "success() && matrix.os == 'ubuntu-latest'"
         with:
           token: ${{ secrets.CODECOV_TOKEN }}
@@ -47,7 +47,7 @@ jobs:
     steps:
       - uses: actions/checkout@master
 
-      - uses: actions/setup-go@v2
+      - uses: actions/setup-go@v3
         with:
           go-version: 1.x
 

.github/workflows/lint.yaml

@@ -17,10 +17,10 @@ jobs:
           - ubuntu-latest
           - macos-latest
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v2.3.0
         with:
           # This field is required.  Dont set the patch version to always use
           # the latest patch version.
-          version: v1.48.0
+          version: v1.51.2

.golangci.yml

@@ -12,32 +12,19 @@ run:
   # autogenerated files. If it's not please let us know.
   skip-files:
     - ".*generated.*"
-    - ".*_test.go"
 
 # all available settings of specific linters
 linters-settings:
   gocyclo:
     min-complexity: 20
-  lll:
-    line-length: 200
-  depguard:
-    list-type: blacklist
-    include-go-root: false
-    packages:
-      - golang.org/x/net/context # we use context
-      - log # we use github.com/AdguardTeam/golibs/log
 
 linters:
   enable:
-    - deadcode
     - errcheck
     - govet
     - ineffassign
     - staticcheck
-    - structcheck
     - unused
-    - varcheck
-    - bodyclose
     - depguard
     - dupl
     - gocyclo
@@ -47,30 +34,19 @@ linters:
     - misspell
     - stylecheck
     - unconvert
-    - depguard
   disable-all: true
-  fast: true
-
 
 issues:
   exclude-use-default: false
 
   # List of regexps of issue texts to exclude, empty list by default.
-  # But independently from this option we use default exclude patterns,
+  # But independently of this option we use default exclude patterns,
   # it can be disabled by `exclude-use-default: false`. To list all
   # excluded by default patterns execute `golangci-lint run --help`
   exclude:
-    # errcheck defer Close
-    - error return value not checked \(defer .*\.Close()\)
-    # errcheck: Almost all programs ignore errors on these functions and in most cases it's ok
-    - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*printf?|os\.(Un)?Setenv). is not checked
-    # gosec: Duplicated errcheck checks
-    - G104
-    # gosec: Expect file permissions to be 0600 or less
-    - G302
-    # gosec: Use of weak random number generators
+    # gosec: Potential file inclusion via variable
+    # Exclude as it is required it in the command-line tool.
+    - G304
+    # gosec: Use of weak random number generator
+    # Used in tests.
     - G404
-    # errcheck defer Close
-    - error return value not checked \(defer .*\.Close()\)
-    # gosec: False positive is triggered by 'src, err := os.ReadFile(filename)'
-    - Potential file inclusion via variable

client.go

@@ -3,10 +3,12 @@ package dnscrypt
 import (
 	"crypto/ed25519"
 	"encoding/binary"
+	"fmt"
 	"net"
 	"strings"
 	"time"
 
+	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/ameshkov/dnsstamps"
 	"github.com/miekg/dns"
@@ -69,6 +71,7 @@ func (c *Client) DialStamp(stamp dnsstamps.ServerStamp) (*ResolverInfo, error) {
 	if err != nil {
 		return nil, err
 	}
+
 	resolverInfo.ResolverCert = cert
 
 	// Compute shared key that we'll use to encrypt/decrypt messages
@@ -83,7 +86,7 @@ func (c *Client) DialStamp(stamp dnsstamps.ServerStamp) (*ResolverInfo, error) {
 // Exchange performs a synchronous DNS query to the specified DNSCrypt server and returns a DNS response.
 // This method creates a new network connection for every call so avoid using it for TCP.
 // DNSCrypt cert needs to be fetched and validated prior to this call using the c.DialStamp method.
-func (c *Client) Exchange(m *dns.Msg, resolverInfo *ResolverInfo) (*dns.Msg, error) {
+func (c *Client) Exchange(m *dns.Msg, resolverInfo *ResolverInfo) (resp *dns.Msg, err error) {
 	network := "udp"
 	if c.Net == "tcp" {
 		network = "tcp"
@@ -91,15 +94,16 @@ func (c *Client) Exchange(m *dns.Msg, resolverInfo *ResolverInfo) (*dns.Msg, err
 
 	conn, err := net.Dial(network, resolverInfo.ServerAddress)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("dialing: %w", err)
 	}
-	defer conn.Close()
+	defer func() { err = errors.WithDeferred(err, conn.Close()) }()
 
-	r, err := c.ExchangeConn(conn, m, resolverInfo)
+	resp, err = c.ExchangeConn(conn, m, resolverInfo)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("exchanging: %w", err)
 	}
-	return r, nil
+
+	return resp, nil
 }
 
 // ExchangeConn performs a synchronous DNS query to the specified DNSCrypt server and returns a DNS response.
@@ -217,7 +221,7 @@ func (c *Client) decrypt(b []byte, resolverInfo *ResolverInfo) (*dns.Msg, error)
 }
 
 // fetchCert loads DNSCrypt cert from the specified server
-func (c *Client) fetchCert(stamp dnsstamps.ServerStamp) (*Cert, error) {
+func (c *Client) fetchCert(stamp dnsstamps.ServerStamp) (cert *Cert, err error) {
 	providerName := stamp.ProviderName
 	if !strings.HasSuffix(providerName, ".") {
 		providerName = providerName + "."
@@ -236,67 +240,87 @@ func (c *Client) fetchCert(stamp dnsstamps.ServerStamp) (*Cert, error) {
 		return nil, ErrFailedToFetchCert
 	}
 
-	var certErr error
 	currentCert := &Cert{}
 	foundValid := false
-
 	for _, rr := range r.Answer {
 		txt, ok := rr.(*dns.TXT)
 		if !ok {
 			continue
 		}
-		var b []byte
-		b, certErr = unpackTxtString(strings.Join(txt.Txt, ""))
-		if certErr != nil {
-			log.Debug("[%s] failed to pack TXT record: %v", providerName, certErr)
+
+		cert, err = parseCert(stamp, currentCert, providerName, strings.Join(txt.Txt, ""))
+		if err != nil {
+			log.Debug("[%s] bad cert: %s", providerName, err)
+		} else if cert == nil {
 			continue
 		}
 
-		cert := &Cert{}
-		certErr = cert.Deserialize(b)
-		if certErr != nil {
-			log.Debug("[%s] failed to deserialize cert: %v", providerName, certErr)
-			continue
-		}
-
-		log.Debug("[%s] fetched certificate %d", providerName, cert.Serial)
-
-		if !cert.VerifyDate() {
-			certErr = ErrInvalidDate
-			log.Debug("[%s] cert %d date is not valid", providerName, cert.Serial)
-			continue
-		}
-
-		if !cert.VerifySignature(stamp.ServerPk) {
-			certErr = ErrInvalidCertSignature
-			log.Debug("[%s] cert %d signature is not valid", providerName, cert.Serial)
-			continue
-		}
-
-		if cert.Serial < currentCert.Serial {
-			log.Debug("[%v] cert %d superseded by a previous certificate", providerName, cert.Serial)
-			continue
-		}
-
-		if cert.Serial == currentCert.Serial {
-			if cert.EsVersion > currentCert.EsVersion {
-				log.Debug("[%v] Upgrading the construction from %v to %v", providerName, currentCert.EsVersion, cert.EsVersion)
-			} else {
-				log.Debug("[%v] Keeping the previous, preferred crypto construction", providerName)
-				continue
-			}
-		}
-
-		// Setting the cert
 		currentCert = cert
 		foundValid = true
 	}
 
 	if foundValid {
 		return currentCert, nil
+	} else if err == nil {
+		err = errors.Error("no valid txt records")
 	}
 
-	return nil, certErr
+	return nil, err
+}
+
+// parseCert parses a certificate from its string form and returns it if it has
+// priority over currentCert.
+func parseCert(
+	stamp dnsstamps.ServerStamp,
+	currentCert *Cert,
+	providerName string,
+	certStr string,
+) (cert *Cert, err error) {
+	certBytes, err := unpackTxtString(certStr)
+	if err != nil {
+		return nil, fmt.Errorf("unpacking txt record: %w", err)
+	}
+
+	cert = &Cert{}
+	err = cert.Deserialize(certBytes)
+	if err != nil {
+		return nil, fmt.Errorf("deserializing cert for: %w", err)
+	}
+
+	log.Debug("[%s] fetched certificate %d", providerName, cert.Serial)
+
+	if !cert.VerifyDate() {
+		return nil, ErrInvalidDate
+	}
+
+	if !cert.VerifySignature(stamp.ServerPk) {
+		return nil, ErrInvalidCertSignature
+	}
+
+	if cert.Serial < currentCert.Serial {
+		log.Debug("[%v] cert %d superseded by a previous certificate", providerName, cert.Serial)
+
+		return nil, nil
+	}
+
+	if cert.Serial > currentCert.Serial {
+		return cert, nil
+	}
+
+	if cert.EsVersion <= currentCert.EsVersion {
+		log.Debug("[%v] keeping the previous, preferred crypto construction", providerName)
+
+		return nil, nil
+	}
+
+	log.Debug(
+		"[%v] upgrading the construction from %v to %v",
+		providerName,
+		currentCert.EsVersion,
+		cert.EsVersion,
+	)
+
+	return cert, nil
 }
 
 func (c *Client) maxQuerySize() int {

client_test.go

@@ -74,7 +74,7 @@ func TestTimeoutOnDialExchange(t *testing.T) {
 
 	// Check error
 	require.NotNil(t, err)
-	require.True(t, os.IsTimeout(err))
+	require.ErrorIs(t, err, os.ErrDeadlineExceeded)
 }
 
 func TestFetchCertPublicResolvers(t *testing.T) {

cmd/main.go

@@ -1,3 +1,4 @@
+// Package main is responsible for the command-line interface.
 package main
 
 import (

encrypted_query.go

@@ -49,7 +49,7 @@ func (q *EncryptedQuery) Encrypt(packet []byte, sharedKey [sharedKeySize]byte) (
 
 	// Step 1: generate nonce
 	binary.BigEndian.PutUint64(q.Nonce[:8], uint64(time.Now().UnixNano()))
-	rand.Read(q.Nonce[8:12])
+	_, _ = rand.Read(q.Nonce[8:12])
 
 	// Unencrypted part of the query:
 	// <client-magic> <client-pk> <client-nonce>

encrypted_response.go

@@ -32,7 +32,7 @@ func (r *EncryptedResponse) Encrypt(packet []byte, sharedKey [sharedKeySize]byte
 	var response []byte
 
 	// Step 1: generate nonce
-	rand.Read(r.Nonce[12:16])
+	_, _ = rand.Read(r.Nonce[12:16])
 	binary.BigEndian.PutUint64(r.Nonce[16:nonceSize], uint64(time.Now().UnixNano()))
 
 	// Unencrypted part of the query:

go.mod

@@ -1,6 +1,6 @@
 module dns.froth.zone/dnscrypt
 
-go 1.18
+go 1.19
 
 require (
 	github.com/AdguardTeam/golibs v0.12.0

server_test.go

@@ -262,8 +262,12 @@ func newTestServer(t require.TestingT, handler Handler) *testServer {
 	srv.udpConn, err = net.ListenUDP("udp", &net.UDPAddr{IP: net.IPv4zero, Port: 0})
 	require.NoError(t, err)
 
-	go s.ServeUDP(srv.udpConn)
-	go s.ServeTCP(srv.tcpListen)
+	go func() {
+		_ = s.ServeUDP(srv.udpConn)
+	}()
+	go func() {
+		_ = s.ServeTCP(srv.tcpListen)
+	}()
 	return srv
 }