Generation of TOTP secret now uses crypto/rand instead of math/rand and is unit-tested

commands/commandimpls/tfa.go

@@ -2,13 +2,15 @@ package commandimpls
 
 import (
 	"fmt"
+	"io"
+
+	"github.com/Bios-Marcel/discordgo"
+
 	"github.com/Bios-Marcel/cordless/commands"
 	"github.com/Bios-Marcel/cordless/config"
 	"github.com/Bios-Marcel/cordless/ui"
 	"github.com/Bios-Marcel/cordless/ui/tviewutil"
 	"github.com/Bios-Marcel/cordless/util/text"
-	"github.com/Bios-Marcel/discordgo"
-	"io"
 )
 
 const tfaHelpPage = `[::b]NAME
@@ -139,7 +141,10 @@ func (cmd *TFAEnableCmd) Execute(writer io.Writer, parameters []string) {
 	if cmd.session.MFA {
 		fmt.Fprintln(writer, "TFA is already enabled on this account.")
 	} else {
-		cmd.window.ShowTFASetup()
+		tfaError := cmd.window.ShowTFASetup()
+		if tfaError != nil {
+			commands.PrintError(writer, "error showing tfa gui", tfaError.Error())
+		}
 	}
 }
 

ui/window.go

@@ -3,19 +3,26 @@ package ui
 import (
 	"bytes"
 	"fmt"
-	"github.com/Bios-Marcel/cordless/util/text"
-	"github.com/mattn/go-runewidth"
-	"github.com/mdp/qrterminal/v3"
 	"log"
 	"strings"
 	"time"
 	"unicode"
 
+	"github.com/mattn/go-runewidth"
+	"github.com/mdp/qrterminal/v3"
+
+	"github.com/Bios-Marcel/cordless/util/text"
+
 	"github.com/Bios-Marcel/discordemojimap"
 	"github.com/Bios-Marcel/goclipimg"
 
 	"github.com/atotto/clipboard"
 
+	"github.com/Bios-Marcel/discordgo"
+	"github.com/Bios-Marcel/tview"
+	"github.com/gdamore/tcell"
+	"github.com/gen2brain/beeep"
+
 	"github.com/Bios-Marcel/cordless/commands"
 	"github.com/Bios-Marcel/cordless/config"
 	"github.com/Bios-Marcel/cordless/discordutil"
@@ -26,10 +33,6 @@ import (
 	"github.com/Bios-Marcel/cordless/ui/tviewutil"
 	"github.com/Bios-Marcel/cordless/util/fuzzy"
 	"github.com/Bios-Marcel/cordless/util/maths"
-	"github.com/Bios-Marcel/discordgo"
-	"github.com/Bios-Marcel/tview"
-	"github.com/gdamore/tcell"
-	"github.com/gen2brain/beeep"
 )
 
 const (
@@ -2127,8 +2130,12 @@ func (window *Window) ExecuteCommand(input string) {
 // ShowTFASetup generates a new TFA-Secret and shows a QR-Code. The QR-Code can
 // be scanned and the resulting TFA-Token can be entered into cordless and used
 // to enable TFA on this account.
-func (window *Window) ShowTFASetup() {
-	tfaSecret := text.GenerateBase32Key()
+func (window *Window) ShowTFASetup() error {
+	tfaSecret, secretError := text.GenerateBase32Key()
+	if secretError != nil {
+		return secretError
+	}
+
 	qrURL := fmt.Sprintf("otpauth://totp/%s?secret=%s&issuer=Discord", window.session.State.User.Email, tfaSecret)
 	qrCodeText := text.GenerateQRCode(qrURL, qrterminal.M)
 	qrCodeImage := tview.NewTextView().SetText(qrCodeText).SetTextAlign(tview.AlignCenter)
@@ -2207,6 +2214,8 @@ func (window *Window) ShowTFASetup() {
 	qrCodeView.AddItem(tviewutil.CreateCenteredComponent(tokenInput, 68), 3, 0, false)
 	window.app.SetRoot(qrCodeView, true)
 	window.app.SetFocus(tokenInput)
+
+	return nil
 }
 
 func (window *Window) startEditingMessage(message *discordgo.Message) {

util/text/totp.go

@@ -1,12 +1,11 @@
 package text
 
 import (
+	"crypto/rand"
 	"errors"
 	"fmt"
-	"math/rand"
 	"strconv"
 	"strings"
-	"time"
 )
 
 // ParseTFACodes takes an arbitrary string and checks whether it's a valid 6
@@ -32,16 +31,22 @@ func ParseTFACode(text string) (string, error) {
 }
 
 // GenerateBase32Key generates a 16 character key containing 2-7 and A-Z.
-func GenerateBase32Key() string {
-	tfaSecretRaw := make([]rune, 16, 16)
+// The implementation uses crypto/rand.
+func GenerateBase32Key() (string, error) {
+	randomBytes := make([]byte, 16, 16)
+	_, err := rand.Read(randomBytes)
+	if err != nil {
+		return "", err
+	}
+
 	availableCharacters := [...]rune{
 		'2', '3', '4', '5', '6', '7',
 		'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
 	}
-	rand.Seed(time.Now().Unix())
-	for i := 0; i < 16; i++ {
-		tfaSecretRaw[i] = availableCharacters[rand.Int31n(int32(len(availableCharacters)))]
+	tfaSecretRaw := make([]rune, 16, 16)
+	for index, randomByte := range randomBytes {
+		tfaSecretRaw[index] = availableCharacters[rune(int(randomByte)%len(availableCharacters))]
 	}
 
-	return string(tfaSecretRaw)
+	return string(tfaSecretRaw), nil
 }

util/text/totp_test.go

@@ -74,3 +74,42 @@ func TestParseTFACode(t *testing.T) {
 		})
 	}
 }
+
+// TestGenerateBase32Key tests whether 100 unique keys can be generated
+// generated in a row. This is simple in order to make sure that the keys
+// aren't always the same. On top of that, keys are checked against the
+// valid base 32 characters and a length of 16.
+func TestGenerateBase32Key(t *testing.T) {
+	availableCharacters := [...]rune{
+		'2', '3', '4', '5', '6', '7',
+		'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
+	}
+
+	iterations := 50
+	generated := make(map[string]bool, iterations)
+	for i := 0; i < iterations; i++ {
+		newKey, keyError := GenerateBase32Key()
+		if keyError != nil {
+			t.Errorf("Error generating key: %s", keyError)
+		}
+		_, ok := generated[newKey]
+		if ok {
+			t.Errorf("Duplicated key: %s", newKey)
+		}
+		generated[newKey] = true
+
+		if len(newKey) != 16 {
+			t.Errorf("Keylength is invalid: %d", len(newKey))
+		}
+
+	OUTER_LOOP:
+		for _, char := range []rune(newKey) {
+			for _, validChar := range availableCharacters {
+				if validChar == char {
+					continue OUTER_LOOP
+				}
+			}
+			t.Errorf("Generated key contains invalid character: %c", char)
+		}
+	}
+}