Config: do to not store decryption variables globally (#571)

* Config: do to not store decryption variables globally

* save session variables as unexported fields in config
* unexport internal encryption helper functions
* add encryption/decryption tests

* fix yes/no prompt returns flipped boolean

* add a test to ensure prompting for encryption works

* fix possible NPE when failing

* don't return salt if there's error

config/config.go

@@ -1577,6 +1577,7 @@ func GetFilePath(configfile string) (string, error) {
 
 // ReadConfig verifies and checks for encryption and verifies the unencrypted
 // file contains JSON.
+// Prompts for decryption key, if target file is encrypted
 func (c *Config) ReadConfig(configPath string, dryrun bool) error {
 	defaultPath, err := GetFilePath(configPath)
 	if err != nil {
@@ -1599,12 +1600,18 @@ func (c *Config) ReadConfig(configPath string, dryrun bool) error {
 		}
 
 		if c.EncryptConfig == fileEncryptionPrompt {
-			m.Lock()
-			IsInitialSetup = true
-			m.Unlock()
-			if c.PromptForConfigEncryption(configPath, dryrun) {
-				c.EncryptConfig = fileEncryptionEnabled
-				return c.SaveConfig(defaultPath, dryrun)
+			confirm, err := promptForConfigEncryption()
+			if err == nil {
+				if confirm {
+					c.EncryptConfig = fileEncryptionEnabled
+					return c.SaveConfig(defaultPath, dryrun)
+				}
+
+				c.EncryptConfig = fileEncryptionDisabled
+				err := c.SaveConfig(configPath, dryrun)
+				if err != nil {
+					log.Errorf(log.ConfigMgr, "Cannot save config. Error: %s\n", err)
+				}
 			}
 		}
 		return nil
@@ -1615,7 +1622,7 @@ func (c *Config) ReadConfig(configPath string, dryrun bool) error {
 		if errCounter >= maxAuthFailures {
 			return errors.New("failed to decrypt config after 3 attempts")
 		}
-		key, err := PromptForConfigKey(IsInitialSetup)
+		key, err := PromptForConfigKey(false)
 		if err != nil {
 			log.Errorf(log.ConfigMgr, "PromptForConfigKey err: %s", err)
 			errCounter++
@@ -1624,9 +1631,9 @@ func (c *Config) ReadConfig(configPath string, dryrun bool) error {
 
 		var f []byte
 		f = append(f, fileData...)
-		data, err := DecryptConfigFile(f, key)
+		data, err := c.decryptConfigData(f, key)
 		if err != nil {
-			log.Errorf(log.ConfigMgr, "DecryptConfigFile err: %s", err)
+			log.Errorf(log.ConfigMgr, "decryptConfigData err: %s", err)
 			errCounter++
 			continue
 		}
@@ -1645,6 +1652,7 @@ func (c *Config) ReadConfig(configPath string, dryrun bool) error {
 }
 
 // SaveConfig saves your configuration to your desired path
+// prompts for encryption key, if necessary
 func (c *Config) SaveConfig(configPath string, dryrun bool) error {
 	if dryrun {
 		return nil
@@ -1661,17 +1669,21 @@ func (c *Config) SaveConfig(configPath string, dryrun bool) error {
 	}
 
 	if c.EncryptConfig == fileEncryptionEnabled {
-		var key []byte
-
-		if IsInitialSetup {
+		// Ensure we have the key from session or from user
+		if len(c.sessionDK) == 0 {
+			var key []byte
 			key, err = PromptForConfigKey(true)
 			if err != nil {
 				return err
 			}
-			IsInitialSetup = false
+			var sessionDK, storedSalt []byte
+			sessionDK, storedSalt, err = makeNewSessionDK(key)
+			if err != nil {
+				return err
+			}
+			c.sessionDK, c.storedSalt = sessionDK, storedSalt
 		}
-
-		payload, err = EncryptConfigFile(payload, key)
+		payload, err = c.encryptConfigFile(payload)
 		if err != nil {
 			return err
 		}

config/config_encryption.go

@@ -27,33 +27,22 @@ const (
 	errAESBlockSize = "config file data is too small for the AES required block size"
 )
 
-var (
-	storedSalt []byte
-	sessionDK  []byte
-)
-
-// PromptForConfigEncryption asks for encryption key
-func (c *Config) PromptForConfigEncryption(configPath string, dryrun bool) bool {
+// promptForConfigEncryption asks for encryption confirmation
+// returns true if encryption was desired, false otherwise
+func promptForConfigEncryption() (bool, error) {
 	log.Println("Would you like to encrypt your config file (y/n)?")
 
 	input := ""
 	_, err := fmt.Scanln(&input)
 	if err != nil {
-		return false
+		return false, err
 	}
 
-	if !common.YesOrNo(input) {
-		c.EncryptConfig = fileEncryptionDisabled
-		err := c.SaveConfig(configPath, dryrun)
-		if err != nil {
-			log.Printf("Cannot save config. Error: %s\n", err)
-		}
-		return false
-	}
-	return true
+	return common.YesOrNo(input), nil
 }
 
 // PromptForConfigKey asks for configuration key
+// if initialSetup is true, the password needs to be repeated
 func PromptForConfigKey(initialSetup bool) ([]byte, error) {
 	var cryptoKey []byte
 
@@ -94,16 +83,21 @@ func PromptForConfigKey(initialSetup bool) ([]byte, error) {
 // EncryptConfigFile encrypts configuration data that is parsed in with a key
 // and returns it as a byte array with an error
 func EncryptConfigFile(configData, key []byte) ([]byte, error) {
-	var err error
-
-	if len(sessionDK) == 0 {
-		sessionDK, err = makeNewSessionDK(key)
-		if err != nil {
-			return nil, err
-		}
+	sessionDK, salt, err := makeNewSessionDK(key)
+	if err != nil {
+		return nil, err
 	}
+	c := &Config{
+		sessionDK:  sessionDK,
+		storedSalt: salt,
+	}
+	return c.encryptConfigFile(configData)
+}
 
-	block, err := aes.NewCipher(sessionDK)
+// EncryptConfigFile encrypts configuration data that is parsed in with a key
+// and returns it as a byte array with an error
+func (c *Config) encryptConfigFile(configData []byte) ([]byte, error) {
+	block, err := aes.NewCipher(c.sessionDK)
 	if err != nil {
 		return nil, err
 	}
@@ -118,15 +112,21 @@ func EncryptConfigFile(configData, key []byte) ([]byte, error) {
 	stream.XORKeyStream(ciphertext[aes.BlockSize:], configData)
 
 	appendedFile := []byte(EncryptConfirmString)
-	appendedFile = append(appendedFile, storedSalt...)
+	appendedFile = append(appendedFile, c.storedSalt...)
 	appendedFile = append(appendedFile, ciphertext...)
 	return appendedFile, nil
 }
 
 // DecryptConfigFile decrypts configuration data with the supplied key and
-// returns the un-encrypted file as a byte array with an error
+// returns the un-encrypted data as a byte array with an error
 func DecryptConfigFile(configData, key []byte) ([]byte, error) {
-	configData = RemoveECS(configData)
+	return (&Config{}).decryptConfigData(configData, key)
+}
+
+// decryptConfigData decrypts configuration data with the supplied key and
+// returns the un-encrypted data as a byte array with an error
+func (c *Config) decryptConfigData(configData, key []byte) ([]byte, error) {
+	configData = removeECS(configData)
 	origKey := key
 
 	if ConfirmSalt(configData) {
@@ -158,10 +158,11 @@ func DecryptConfigFile(configData, key []byte) ([]byte, error) {
 	stream.XORKeyStream(configData, configData)
 	result := configData
 
-	sessionDK, err = makeNewSessionDK(origKey)
+	sessionDK, storedSalt, err := makeNewSessionDK(origKey)
 	if err != nil {
 		return nil, err
 	}
+	c.sessionDK, c.storedSalt = sessionDK, storedSalt
 
 	return result, nil
 }
@@ -176,8 +177,8 @@ func ConfirmECS(file []byte) bool {
 	return bytes.Contains(file, []byte(EncryptConfirmString))
 }
 
-// RemoveECS removes encryption confirmation string
-func RemoveECS(file []byte) []byte {
+// removeECS removes encryption confirmation string
+func removeECS(file []byte) []byte {
 	return bytes.Trim(file, EncryptConfirmString)
 }
 
@@ -188,17 +189,16 @@ func getScryptDK(key, salt []byte) ([]byte, error) {
 	return scrypt.Key(key, salt, 32768, 8, 1, 32)
 }
 
-func makeNewSessionDK(key []byte) ([]byte, error) {
-	var err error
+func makeNewSessionDK(key []byte) (dk, storedSalt []byte, err error) {
 	storedSalt, err = crypto.GetRandomSalt([]byte(SaltPrefix), SaltRandomLength)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	dk, err := getScryptDK(key, storedSalt)
+	dk, err = getScryptDK(key, storedSalt)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return dk, nil
+	return dk, storedSalt, nil
 }

config/config_encryption_test.go

@@ -1,14 +1,22 @@
 package config
 
 import (
+	"bytes"
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"testing"
 )
 
 func TestPromptForConfigEncryption(t *testing.T) {
 	t.Parallel()
 
-	if Cfg.PromptForConfigEncryption("", true) {
-		t.Error("PromptForConfigEncryption return incorrect bool")
+	confirm, err := promptForConfigEncryption()
+	if confirm {
+		t.Error("promptForConfigEncryption return incorrect bool")
+	}
+	if err == nil {
+		t.Error("Expected error as there is no input")
 	}
 }
 
@@ -32,26 +40,30 @@ func TestEncryptConfigFile(t *testing.T) {
 		t.Fatal("Expected error")
 	}
 
-	sessionDK = []byte("a")
-	_, err = EncryptConfigFile([]byte("test"), nil)
+	c := &Config{
+		sessionDK: []byte("a"),
+	}
+	_, err = c.encryptConfigFile([]byte("test"))
 	if err == nil {
 		t.Fatal("Expected error")
 	}
 
-	sessionDK, err = makeNewSessionDK([]byte("asdf"))
+	sessDk, salt, err := makeNewSessionDK([]byte("asdf"))
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	_, err = EncryptConfigFile([]byte("test"), []byte("key"))
+	c = &Config{
+		sessionDK:  sessDk,
+		storedSalt: salt,
+	}
+	_, err = c.encryptConfigFile([]byte("test"))
 	if err != nil {
 		t.Fatal(err)
 	}
 }
 
 func TestDecryptConfigFile(t *testing.T) {
-	sessionDK = nil
-
 	result, err := EncryptConfigFile([]byte("test"), []byte("key"))
 	if err != nil {
 		t.Fatal(err)
@@ -96,7 +108,7 @@ func TestRemoveECS(t *testing.T) {
 	t.Parallel()
 
 	ECStest := []byte(EncryptConfirmString)
-	isremoved := RemoveECS(ECStest)
+	isremoved := removeECS(ECStest)
 
 	if string(isremoved) != "" {
 		t.Errorf("TestConfirmECS: Error ECS not deleted.")
@@ -106,8 +118,182 @@ func TestRemoveECS(t *testing.T) {
 func TestMakeNewSessionDK(t *testing.T) {
 	t.Parallel()
 
-	_, err := makeNewSessionDK(nil)
+	_, _, err := makeNewSessionDK(nil)
 	if err == nil {
 		t.Fatal("makeNewSessionDK passed with nil key")
 	}
 }
+
+func TestEncryptTwiceReusesSaltButNewCipher(t *testing.T) {
+	c := &Config{}
+	c.EncryptConfig = 1
+	tempDir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("Problem creating temp dir at %s: %s\n", tempDir, err)
+	}
+	defer os.RemoveAll(tempDir)
+
+	// Prepare input
+	passFile, err := ioutil.TempFile(tempDir, "*.pw")
+	if err != nil {
+		t.Fatalf("Problem creating temp file at %s: %s\n", tempDir, err)
+	}
+	passFile.WriteString("pass\npass\n")
+	passFile.Close()
+
+	// Temporarily replace Stdin with a custom input
+	oldIn := os.Stdin
+	defer func() { os.Stdin = oldIn }()
+	os.Stdin, err = os.Open(passFile.Name())
+	if err != nil {
+		t.Fatalf("Problem opening temp file at %s: %s\n", passFile.Name(), err)
+	}
+
+	// Save encrypted config
+	enc1 := filepath.Join(tempDir, "encrypted.dat")
+	err = c.SaveConfig(enc1, false)
+	if err != nil {
+		t.Fatalf("Problem storing config in file %s: %s\n", enc1, err)
+	}
+	// Save again
+	enc2 := filepath.Join(tempDir, "encrypted2.dat")
+	err = c.SaveConfig(enc2, false)
+	if err != nil {
+		t.Fatalf("Problem storing config in file %s: %s\n", enc2, err)
+	}
+	data1, err := ioutil.ReadFile(enc1)
+	if err != nil {
+		t.Fatalf("Problem reading file %s: %s\n", enc1, err)
+	}
+	data2, err := ioutil.ReadFile(enc2)
+	if err != nil {
+		t.Fatalf("Problem reading file %s: %s\n", enc2, err)
+	}
+	// legth of prefix + salt
+	l := len(EncryptConfirmString+SaltPrefix) + SaltRandomLength
+	// Even though prefix, including salt with the random bytes is the same
+	if !bytes.Equal(data1[:l], data2[:l]) {
+		t.Error("Salt is not reused.")
+	}
+	// the cipher text should not be
+	if bytes.Equal(data1, data2) {
+		t.Error("Encryption key must have been reused as cipher texts are the same")
+	}
+}
+
+func TestSaveAndReopenEncryptedConfig(t *testing.T) {
+	c := &Config{}
+	c.Name = "myCustomName"
+	c.EncryptConfig = 1
+	tempDir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("Problem creating temp dir at %s: %s\n", tempDir, err)
+	}
+	defer os.RemoveAll(tempDir)
+
+	// Prepare password
+	passFile, err := ioutil.TempFile(tempDir, "*.pw")
+	if err != nil {
+		t.Fatalf("Problem creating temp file at %s: %s\n", tempDir, err)
+	}
+	passFile.WriteString("pass\npass\n")
+	passFile.Close()
+
+	// Temporarily replace Stdin with a custom input
+	cleanup := setAnswersFile(t, passFile.Name())
+	defer cleanup()
+
+	// Save encrypted config
+	enc := filepath.Join(tempDir, "encrypted.dat")
+	err = c.SaveConfig(enc, false)
+	if err != nil {
+		t.Fatalf("Problem storing config in file %s: %s\n", enc, err)
+	}
+
+	// Prepare password input for decryption
+	passFile, err = os.Open(passFile.Name())
+	if err != nil {
+		t.Fatalf("Problem opening temp file at %s: %s\n", passFile.Name(), err)
+	}
+	defer passFile.Close()
+	os.Stdin = passFile
+
+	// Clean session
+	readConf := &Config{}
+	// Load with no existing state, key is read from the prepared file
+	err = readConf.ReadConfig(enc, true)
+
+	// Verify
+	if err != nil {
+		t.Fatalf("Problem reading config in file %s: %s\n", enc, err)
+	}
+
+	if c.Name != readConf.Name || c.EncryptConfig != readConf.EncryptConfig {
+		t.Error("Loaded conf not the same as original")
+	}
+}
+
+// setAnswersFile sets the given file as the current stdin
+// returns the close function to defer for reverting the stdin
+func setAnswersFile(t *testing.T, answerFile string) func() {
+	oldIn := os.Stdin
+
+	inputFile, err := os.Open(answerFile)
+	if err != nil {
+		t.Fatalf("Problem opening temp file at %s: %s\n", answerFile, err)
+	}
+	os.Stdin = inputFile
+	return func() {
+		inputFile.Close()
+		os.Stdin = oldIn
+	}
+}
+
+func TestReadConfigWithPrompt(t *testing.T) {
+	// Prepare temp dir
+	tempDir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("Problem creating temp dir at %s: %s\n", tempDir, err)
+	}
+	defer os.RemoveAll(tempDir)
+
+	// Ensure we'll get the prompt when loading
+	c := &Config{
+		EncryptConfig: 0,
+	}
+
+	// Save config
+	testConfigFile := filepath.Join(tempDir, "config.json")
+	err = c.SaveConfig(testConfigFile, false)
+	if err != nil {
+		t.Fatalf("Problem saving config file in %s: %s\n", tempDir, err)
+	}
+
+	// Answers to the prompt
+	responseFile, err := ioutil.TempFile(tempDir, "*.in")
+	if err != nil {
+		t.Fatalf("Problem creating temp file at %s: %s\n", tempDir, err)
+	}
+	responseFile.WriteString("y\npass\npass\n")
+	responseFile.Close()
+
+	// Temporarily replace Stdin with a custom input
+	cleanup := setAnswersFile(t, responseFile.Name())
+	defer cleanup()
+
+	// Run the test
+	c = &Config{}
+	c.ReadConfig(testConfigFile, false)
+
+	// Verify results
+	data, err := ioutil.ReadFile(testConfigFile)
+	if err != nil {
+		t.Fatalf("Problem reading saved file at %s: %s\n", testConfigFile, err)
+	}
+	if c.EncryptConfig != fileEncryptionEnabled {
+		t.Error("Config encryption flag should be set after prompts")
+	}
+	if !ConfirmECS(data) {
+		t.Error("Config file should be encrypted after prompts")
+	}
+}

config/config_types.go

@@ -67,9 +67,8 @@ const (
 
 // Variables here are used for configuration
 var (
-	Cfg            Config
-	IsInitialSetup bool
-	m              sync.Mutex
+	Cfg Config
+	m   sync.Mutex
 )
 
 // Config is the overarching object that holds all the information for
@@ -99,6 +98,9 @@ type Config struct {
 	FiatDisplayCurrency *currency.Code            `json:"fiatDispayCurrency,omitempty"`
 	Cryptocurrencies    *currency.Currencies      `json:"cryptocurrencies,omitempty"`
 	SMS                 *SMSGlobalConfig          `json:"smsGlobal,omitempty"`
+	// encryption session values
+	storedSalt []byte
+	sessionDK  []byte
 }
 
 // ConnectionMonitorConfig defines the connection monitor variables to ensure