Use github.com/pquerna/otp to allow using the key period (#2278)

* Use github.com/pquerna/otp to allow using the key period

RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes #2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Address review comments.

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Implement digits and algorithm parameter parsing

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Use proper formatting and add logging

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Make linters happy

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

.github/workflows/build.yml

@@ -21,7 +21,7 @@ jobs:
     - name: Set up Go
       uses: actions/setup-go@v3
       with:
-        go-version: 1.18
+        go-version: 1.18.3
     - uses: actions/cache@v3
       with:
         path: ~/go/pkg/mod

.github/workflows/grype.yml

@@ -20,7 +20,7 @@ jobs:
     - name: Set up Go
       uses: actions/setup-go@v3
       with:
-        go-version: 1.18
+        go-version: 1.18.3
     - uses: actions/cache@v3
       with:
         path: ~/go/pkg/mod

.golangci.yml

@@ -47,6 +47,7 @@ linters:
   - maligned
   - nilerr
   - noctx
+  - nosnakecase
   - revive
   - rowserrcheck
   - scopelint

go.mod

@@ -30,6 +30,7 @@ require (
 	github.com/mitchellh/go-ps v1.0.0
 	github.com/muesli/crunchy v0.4.0
 	github.com/nbutton23/zxcvbn-go v0.0.0-20210217022336-fa2cb2858354
+	github.com/pquerna/otp v1.3.0
 	github.com/schollz/closestmatch v0.0.0-20190308193919-1fbe626be92e
 	github.com/skip2/go-qrcode v0.0.0-20200617195104-da1b6568686e
 	github.com/stretchr/testify v1.8.0
@@ -46,6 +47,7 @@ require (
 
 require (
 	filippo.io/edwards25519 v1.0.0 // indirect
+	github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc // indirect
 	github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/golang/protobuf v1.5.2 // indirect

go.sum

@@ -11,6 +11,8 @@ github.com/atotto/clipboard v0.1.4 h1:EH0zSVneZPSuFR11BlR9YppQTVDbh5+16AmcJi4g1z
 github.com/atotto/clipboard v0.1.4/go.mod h1:ZY9tmq7sm5xIbd9bOK4onWV4S6X0u6GY7Vn0Yu86PYI=
 github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM=
 github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=
+github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc h1:biVzkmvwrH8WK8raXaxBx6fRVTlJILwEwQGL1I/ByEI=
+github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8=
 github.com/caspr-io/yamlpath v0.0.0-20200722075116-502e8d113a9b h1:2K3B6Xm7/lnhOugeGB3nIk50bZ9zhuJvXCEfUuL68ik=
 github.com/caspr-io/yamlpath v0.0.0-20200722075116-502e8d113a9b/go.mod h1:4rP9T6iHCuPAIDKdNaZfTuuqSIoQQvFctNWIAUI1rlg=
 github.com/cenkalti/backoff v2.2.1+incompatible h1:tNowT99t7UNflLxfYYSlKYsBpXdEet03Pg2g16Swow4=
@@ -102,6 +104,8 @@ 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/pquerna/otp v1.3.0 h1:oJV/SkzR33anKXwQU3Of42rL4wbrffP4uvUf1SvS5Xs=
+github.com/pquerna/otp v1.3.0/go.mod h1:dkJfzwRKNiegxyNb54X/3fLwhCynbMspSyWKnvi1AEg=
 github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc=
 github.com/rogpeppe/go-internal v1.8.1-0.20210923151022-86f73c517451 h1:d1PiN4RxzIFXCJTvRkvSkKqwtRAl5ZV4lATKtQI0B7I=
 github.com/rogpeppe/go-internal v1.8.1-0.20210923151022-86f73c517451/go.mod h1:JeRgkft04UBgHMgCIwADu4Pn6Mtm5d4nPKWu0nJ5d+o=

internal/action/otp.go

@@ -4,25 +4,26 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/url"
+	"strconv"
+	"strings"
 	"time"
 
 	"github.com/gopasspw/gopass/internal/action/exit"
 	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/internal/store"
 	"github.com/gopasspw/gopass/pkg/clipboard"
 	"github.com/gopasspw/gopass/pkg/ctxutil"
+	"github.com/gopasspw/gopass/pkg/debug"
 	"github.com/gopasspw/gopass/pkg/otp"
 	"github.com/gopasspw/gopass/pkg/termio"
 	"github.com/mattn/go-tty"
+	potp "github.com/pquerna/otp"
+	"github.com/pquerna/otp/hotp"
+	"github.com/pquerna/otp/totp"
 	"github.com/urfave/cli/v2"
 )
 
-const (
-	// we might want to replace this with the currently un-exported step value
-	// from twofactor.FromURL if it gets ever exported.
-	otpPeriod = 30
-)
-
 // OTP implements OTP token handling for TOTP and HOTP.
 func (s *Action) OTP(c *cli.Context) error {
 	ctx := ctxutil.WithGlobalFlags(c)
@@ -102,24 +103,61 @@ func (s *Action) otp(ctx context.Context, name, qrf string, clip, pw, recurse bo
 		go waitForKeyPress(ctx, cancel)
 	}
 
+	// only used for the HOTP case as a fallback
+	var counter uint64 = 1
+	if sv, found := sec.Get("counter"); found && sv != "" {
+		if iv, err := strconv.ParseUint(sv, 10, 64); iv != 0 && err == nil {
+			counter = iv
+		}
+	}
 	for {
 		select {
 		case <-ctx.Done():
 			return nil
 		default:
 		}
-		two, label, err := otp.Calculate(name, sec)
+
+		two, err := otp.Calculate(name, sec)
 		if err != nil {
 			return exit.Error(exit.Unknown, err, "No OTP entry found for %s: %s", name, err)
 		}
-		token := two.OTP()
+
+		var token string
+		switch two.Type() {
+		case "totp":
+			token, err = totp.GenerateCodeCustom(two.Secret(), time.Now(), totp.ValidateOpts{
+				Period:    uint(two.Period()),
+				Skew:      1,
+				Digits:    parseDigits(two.URL()),
+				Algorithm: parseAlgorithm(two.URL()),
+			})
+			if err != nil {
+				return exit.Error(exit.Unknown, err, "Failed to compute OTP token for %s: %s", name, err)
+			}
+		case "hotp":
+			token, err = hotp.GenerateCodeCustom(two.Secret(), counter, hotp.ValidateOpts{
+				Digits:    parseDigits(two.URL()),
+				Algorithm: parseAlgorithm(two.URL()),
+			})
+			if err != nil {
+				return exit.Error(exit.Unknown, err, "Failed to compute OTP token for %s: %s", name, err)
+			}
+			counter++
+			sec.Set("counter", strconv.Itoa(int(counter)))
+			if err := s.Store.Set(ctx, name, sec); err != nil {
+				out.Errorf(ctx, "Failed to persist counter value: %s", err)
+			}
+			debug.Log("Saved counter as %d", counter)
+		}
 
 		now := time.Now()
-		expiresAt := now.Add(otpPeriod * time.Second).Truncate(otpPeriod * time.Second)
+		expiresAt := now.Add(time.Duration(two.Period()) * time.Second).Truncate(time.Duration(two.Period()) * time.Second)
 		secondsLeft := int(time.Until(expiresAt).Seconds())
 		bar := termio.NewProgressBar(int64(secondsLeft))
 		bar.Hidden = skip
 
+		debug.Log("OTP period: %ds", two.Period())
+
 		if clip {
 			if err := clipboard.CopyTo(ctx, fmt.Sprintf("token for %s", name), []byte(token), s.cfg.ClipTimeout); err != nil {
 				return exit.Error(exit.IO, err, "failed to copy to clipboard: %s", err)
@@ -145,7 +183,7 @@ func (s *Action) otp(ctx context.Context, name, qrf string, clip, pw, recurse bo
 		}
 
 		if qrf != "" {
-			return otp.WriteQRFile(two, label, qrf)
+			return otp.WriteQRFile(two, qrf)
 		}
 
 		// let us wait until next OTP code:.
@@ -182,3 +220,58 @@ func (s *Action) otpHandleError(ctx context.Context, name, qrf string, clip, pw,
 
 	return nil
 }
+
+// parseDigits and parseAlgorithm can be replaced if https://github.com/pquerna/otp/pull/74 is merged.
+func parseDigits(ku string) potp.Digits {
+	u, err := url.Parse(ku)
+	if err != nil {
+		debug.Log("Failed to parse key URL: %s", err)
+
+		// return the most common value
+		return potp.DigitsSix
+	}
+
+	q := u.Query()
+	iv, err := strconv.ParseUint(q.Get("digits"), 10, 64)
+	if err != nil {
+		debug.Log("Failed to parse digits param: %s", err)
+
+		// return the most common value
+		return potp.DigitsSix
+	}
+
+	switch iv {
+	case 6:
+		return potp.DigitsSix
+	case 8:
+		return potp.DigitsEight
+	default:
+		debug.Log("Unsupported digits value: %d", iv)
+
+		// return the most common value
+		return potp.DigitsSix
+	}
+}
+
+func parseAlgorithm(ku string) potp.Algorithm {
+	u, err := url.Parse(ku)
+	if err != nil {
+		debug.Log("Failed to parse key URL: %s", err)
+
+		// return the most common value
+		return potp.AlgorithmSHA1
+	}
+
+	q := u.Query()
+	a := strings.ToLower(q.Get("algorithm"))
+	switch a {
+	case "md5":
+		return potp.AlgorithmMD5
+	case "sha256":
+		return potp.AlgorithmSHA256
+	case "sha512":
+		return potp.AlgorithmSHA512
+	default:
+		return potp.AlgorithmSHA1
+	}
+}

pkg/otp/otp.go

@@ -1,19 +1,21 @@
 package otp
 
 import (
+	"bytes"
 	"fmt"
+	"image/png"
 	"os"
 	"strings"
 
-	"github.com/gokyle/twofactor"
 	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/pkg/debug"
 	"github.com/gopasspw/gopass/pkg/gopass"
+	"github.com/pquerna/otp"
 )
 
 // Calculate will compute a OTP code from a given secret.
 //nolint:ireturn
-func Calculate(name string, sec gopass.Secret) (twofactor.OTP, string, error) {
+func Calculate(name string, sec gopass.Secret) (*otp.Key, error) {
 	otpURL, found := sec.Get("otpauth")
 	if found && strings.HasPrefix(otpURL, "//") {
 		otpURL = "otpauth:" + otpURL
@@ -31,61 +33,65 @@ func Calculate(name string, sec gopass.Secret) (twofactor.OTP, string, error) {
 	if otpURL != "" {
 		debug.Log("found otpauth url: %s", out.Secret(otpURL))
 
-		return twofactor.FromURL(otpURL) //nolint:wrapcheck
+		return otp.NewKeyFromURL(otpURL) //nolint:wrapcheck
 	}
 
 	// check yaml entry and fall back to password if we don't have one
-	label := name
 
-	secKey, found := sec.Get("totp")
-	if !found {
-		debug.Log("no totp secret found, falling back to password")
-
-		secKey = sec.Password()
+	// TOTP
+	if secKey, found := sec.Get("totp"); found {
+		return parseOTP("totp", secKey)
 	}
 
-	if strings.HasPrefix(secKey, "otpauth://") {
-		return twofactor.FromURL(secKey) //nolint:wrapcheck
+	// HOTP
+	if secKey, found := sec.Get("hotp"); found {
+		return parseOTP("hotp", secKey)
 	}
 
-	otp, err := twofactor.NewGoogleTOTP(twofactor.Pad(secKey))
-	if err != nil {
-		return otp, label, fmt.Errorf("invalid OTP secret %q: %w", secKey, err)
-	}
+	debug.Log("no totp secret found, falling back to password")
 
-	return otp, label, nil
+	return parseOTP("totp", sec.Password())
 }
 
-// WriteQRFile writes the given OTP code as a QR image to disk.
-func WriteQRFile(otp twofactor.OTP, label, file string) error {
-	var qr []byte
-
-	var err error
+func parseOTP(typ string, secKey string) (*otp.Key, error) {
+	if strings.HasPrefix(secKey, "otpauth://") {
+		debug.Log("parsing otpauth:// URL %q", out.Secret(secKey))
 
-	switch otp.Type() {
-	case twofactor.OATH_HOTP:
-		hotp, ok := otp.(*twofactor.HOTP)
-		if !ok {
-			return fmt.Errorf("Type assertion failed on twofactor.HOTP: %w", ErrType)
+		k, err := otp.NewKeyFromURL(secKey)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse otpauth URL: %w", err)
 		}
 
-		qr, err = hotp.QR(label)
-	case twofactor.OATH_TOTP:
-		totp, ok := otp.(*twofactor.TOTP)
-		if !ok {
-			return fmt.Errorf("Type assertion failed on twofactor.TOTP: %w", ErrType)
-		}
+		return k, nil
+	}
 
-		qr, err = totp.QR(label)
-	default:
-		err = ErrOathOTP
+	debug.Log("assembling otpauth URL from secret only (%q), using defaults", out.Secret(secKey))
+
+	// otpauth://totp/Example:alice@google.com?secret=JBSWY3DPEHPK3PXP&issuer=Example
+	key, err := otp.NewKeyFromURL(fmt.Sprintf("otpauth://%s/new?secret=%s&issuer=gopass", typ, secKey))
+	if err != nil {
+		debug.Log("failed to parse OTP: %s", out.Secret(secKey))
+
+		return nil, fmt.Errorf("invalid OTP secret: %w", err)
 	}
 
+	return key, nil
+}
+
+// WriteQRFile writes the given OTP code as a QR image to disk.
+func WriteQRFile(key *otp.Key, file string) error {
+	// Convert TOTP key into a QR code encoded as a PNG image.
+	var buf bytes.Buffer
+	img, err := key.Image(200, 200)
 	if err != nil {
-		return fmt.Errorf("failed to write qr file: %w", err)
+		return fmt.Errorf("failed to encode qr code: %w", err)
+	}
+
+	if err := png.Encode(&buf, img); err != nil {
+		return fmt.Errorf("failed to encode as png: %w", err)
 	}
 
-	if err := os.WriteFile(file, qr, 0o600); err != nil {
+	if err := os.WriteFile(file, buf.Bytes(), 0o600); err != nil {
 		return fmt.Errorf("failed to write QR code: %w", err)
 	}
 

pkg/otp/otp_test.go

@@ -6,8 +6,8 @@ import (
 	"path/filepath"
 	"testing"
 
-	"github.com/gokyle/twofactor"
 	"github.com/gopasspw/gopass/pkg/gopass/secrets/secparse"
+	"github.com/pquerna/otp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -37,7 +37,7 @@ func TestCalculate(t *testing.T) {
 
 			s, err := secparse.Parse(tc)
 			require.NoError(t, err)
-			otp, _, err := Calculate("test", s)
+			otp, err := Calculate("test", s)
 			assert.NoError(t, err, string(tc))
 			assert.NotNil(t, otp, string(tc))
 		})
@@ -56,7 +56,7 @@ func TestWrite(t *testing.T) {
 
 	tf := filepath.Join(td, "qr.png")
 
-	otp, label, err := twofactor.FromURL(totpURL)
+	key, err := otp.NewKeyFromURL(totpURL)
 	assert.NoError(t, err)
-	assert.NoError(t, WriteQRFile(otp, label, tf))
+	assert.NoError(t, WriteQRFile(key, tf))
 }