Update readable user conf format (#1456)

* use json as readable fmt for user config

apipb/types.proto

@@ -58,6 +58,7 @@ message ProxyConnectConfig {
   ProxyLocation location = 3;
   string name = 4; // Used for logging.
   int32 port = 5;
+  string protocol = 6;
 
   // General config.
   bytes cert_pem = 10;

config/user/config.go

@@ -5,15 +5,14 @@ package userconfig
 import (
 	"context"
 	"fmt"
-	"io"
 	"os"
 	"path/filepath"
 	"sync"
 	"sync/atomic"
 
 	"github.com/getlantern/eventual/v2"
 	"github.com/getlantern/golog"
-	"github.com/getlantern/rot13"
+	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/proto"
 
 	"github.com/getlantern/flashlight/v7/apipb"
@@ -43,15 +42,15 @@ type Config struct {
 
 	// filePath is where we should save new configs and look for existing saved configs.
 	filePath string
-	// obfuscate specifies whether or not to obfuscate the config on disk.
-	obfuscate bool
+	// readable specifies whether the config file should be saved in a human-readable format.
+	readable bool
 
 	// listeners is a list of functions to call when the config changes.
 	listeners []func(old, new *UserConfig)
 	mu        sync.Mutex
 }
 
-func Init(saveDir string, obfuscate bool) (*Config, error) {
+func Init(saveDir string, readable bool) (*Config, error) {
 	if !initCalled.CompareAndSwap(false, true) {
 		return _config, nil
 	}
@@ -60,12 +59,16 @@ func Init(saveDir string, obfuscate bool) (*Config, error) {
 		saveDir = DefaultConfigSaveDir
 	}
 
+	return initialize(saveDir, DefaultConfigFilename, readable)
+}
+
+func initialize(saveDir, filename string, readable bool) (*Config, error) {
 	_config.mu.Lock()
-	_config.filePath = filepath.Join(saveDir, DefaultConfigFilename)
-	_config.obfuscate = obfuscate
+	_config.filePath = filepath.Join(saveDir, filename)
+	_config.readable = readable
 	_config.mu.Unlock()
 
-	saved, err := readExistingConfig(_config.filePath, obfuscate)
+	saved, err := readExistingConfig(_config.filePath, readable)
 	if err != nil {
 		log.Error(err)
 		return nil, err
@@ -109,7 +112,7 @@ func (c *Config) SetConfig(new *UserConfig) {
 	log.Tracef("Config changed:\nold:\n%+v\nnew:\n%+v\nmerged:\n%v", old, new, updated)
 
 	c.config.Set(updated)
-	if err := saveConfig(c.filePath, updated, c.obfuscate); err != nil {
+	if err := saveConfig(c.filePath, updated, c.readable); err != nil {
 		log.Errorf("Failed to save client config: %v", err)
 	}
 
@@ -132,67 +135,56 @@ func GetConfig(ctx context.Context) (*UserConfig, error) {
 	return conf.(*UserConfig), nil
 }
 
-// readExistingConfig reads a config from a file at the specified path, filePath,
-// deobfuscating it if obfuscate is true.
-func readExistingConfig(filePath string, obfuscate bool) (*UserConfig, error) {
-	infile, err := os.Open(filePath)
+// readExistingConfig reads a config from a file at filePath. readable specifies whether the file
+// is in JSON format. A nil error is returned even if the file does not exist or is empty as these
+// are not considered errors.
+func readExistingConfig(filePath string, readable bool) (*UserConfig, error) {
+	bytes, err := os.ReadFile(filePath)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil, nil // file does not exist
 		}
 
 		return nil, fmt.Errorf("unable to open config file %v for reading: %w", filePath, err)
 	}
-	defer infile.Close()
-
-	log.Debugf("Reading existing config from %v", filePath)
-	var in io.Reader = infile
-	if obfuscate {
-		in = rot13.NewReader(infile)
-	}
-
-	bytes, err := io.ReadAll(in)
-	if err != nil {
-		return nil, fmt.Errorf("failed to read config from %v: %w", filePath, err)
-	}
 
 	if len(bytes) == 0 { // file is empty
-		// we treat an empty file as if it doesn't
+		// we treat an empty file as if it doesn't exist
 		return nil, nil
 	}
 
 	conf := &UserConfig{}
-	if err = proto.Unmarshal(bytes, conf); err != nil {
-		return nil, err
+	if readable {
+		err = protojson.Unmarshal(bytes, conf)
+	} else {
+		err = proto.Unmarshal(bytes, conf)
+	}
+	if err != nil {
+		return nil, fmt.Errorf("unable to unmarshal config: %w", err)
 	}
 
 	return conf, nil
 }
 
-// saveConfig writes conf to a file at the specified path, filePath, obfuscating it if
-// obfuscate is true. If the file already exists, it will be overwritten.
-func saveConfig(filePath string, conf *UserConfig, obfuscate bool) error {
-	in, err := proto.Marshal(conf)
-	if err != nil {
-		return fmt.Errorf("unable to marshal config: %w", err)
+// saveConfig writes conf to a file at filePath. If readable is true, the file will be written in
+// JSON format. Otherwise, it will be written in protobuf format. If the file already exists, it
+// will be overwritten.
+func saveConfig(filePath string, conf *UserConfig, readable bool) error {
+	var (
+		buf []byte
+		err error
+	)
+	if readable {
+		buf, err = protojson.Marshal(conf)
+	} else {
+		buf, err = proto.Marshal(conf)
 	}
 
-	outfile, err := os.OpenFile(filePath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0644)
 	if err != nil {
-		return fmt.Errorf("unable to open file %v for writing: %w", filePath, err)
-	}
-	defer outfile.Close()
-
-	var out io.Writer = outfile
-	if obfuscate {
-		out = rot13.NewWriter(outfile)
-	}
-
-	if _, err = out.Write(in); err != nil {
-		return fmt.Errorf("unable to write config to file %v: %w", filePath, err)
+		return fmt.Errorf("unable to marshal config: %w", err)
 	}
 
-	return nil
+	return os.WriteFile(filePath, buf, 0644)
 }
 
 // OnConfigChange registers a function to be called on config change. This allows callers to

config/user/config_test.go

@@ -2,17 +2,15 @@ package userconfig
 
 import (
 	"fmt"
-	"io"
 	"os"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/getlantern/eventual/v2"
-	"github.com/getlantern/rot13"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/encoding/protojson"
 
 	"github.com/getlantern/flashlight/v7/apipb"
 )
@@ -21,108 +19,52 @@ func TestInitWithSavedConfig(t *testing.T) {
 	conf := newTestConfig()
 	defer resetConfig()
 
-	withTempConfigFile(t, conf, false, func(tmpfile *os.File) {
-		Init("", false)
-		existing, _ := GetConfig(eventual.DontWait)
+	filename, err := newTestConfigFile(conf)
+	require.NoError(t, err, "unable to create test config file")
+	defer os.Remove(filename)
 
-		want := fmt.Sprintf("%+v", conf)
-		got := fmt.Sprintf("%+v", existing)
-		assert.Equal(t, want, got, "failed to read existing config file")
-	})
+	initialize(DefaultConfigSaveDir, filename, true)
+	existing, _ := GetConfig(eventual.DontWait)
+
+	want := fmt.Sprintf("%+v", conf)
+	got := fmt.Sprintf("%+v", existing)
+	assert.Equal(t, want, got, "failed to read existing config file")
 }
 
 func TestNotifyOnConfig(t *testing.T) {
 	conf := newTestConfig()
 	defer resetConfig()
 
-	withTempConfigFile(t, conf, false, func(tmpfile *os.File) {
-		called := make(chan struct{}, 1)
-		OnConfigChange(func(old, new *UserConfig) {
-			called <- struct{}{}
-		})
-
-		Init("", false)
-		_config.SetConfig(newTestConfig())
-
-		select {
-		case <-called:
-			t.Log("recieved config change notification")
-		case <-time.After(time.Second):
-			assert.Fail(t, "timeout waiting for config change notification")
-		}
-	})
-}
-
-func TestInvalidFile(t *testing.T) {
-	withTempConfigFile(t, nil, false, func(tmpfile *os.File) {
-		tmpfile.WriteString("real-list-of-lantern-ips: https://youtu.be/dQw4w9WgXcQ?t=85")
-		tmpfile.Sync()
+	filename, err := newTestConfigFile(conf)
+	require.NoError(t, err, "unable to create test config file")
+	defer os.Remove(filename)
 
-		_, err := readExistingConfig(tmpfile.Name(), false)
-		assert.Error(t, err, "should get error if config file is invalid")
+	called := make(chan struct{}, 1)
+	OnConfigChange(func(old, new *UserConfig) {
+		called <- struct{}{}
 	})
-}
-
-func TestReadObfuscatedConfig(t *testing.T) {
-	conf := newTestConfig()
-	withTempConfigFile(t, conf, true, func(tmpfile *os.File) {
-		fileConf, err := readExistingConfig(tmpfile.Name(), true)
-		assert.NoError(t, err, "unable to read obfuscated config file")
-
-		want := fmt.Sprintf("%+v", conf)
-		got := fmt.Sprintf("%+v", fileConf)
-		assert.Equal(t, want, got, "obfuscated config file doesn't match")
-	})
-}
-
-func TestSaveObfuscatedConfig(t *testing.T) {
-	withTempConfigFile(t, nil, false, func(tmpfile *os.File) {
-		tmpfile.Close()
-
-		conf := newTestConfig()
-		err := saveConfig(tmpfile.Name(), conf, true)
-		require.NoError(t, err, "unable to save obfuscated config file")
-
-		file, err := os.Open(tmpfile.Name())
-		require.NoError(t, err, "unable to open obfuscated config file")
-		defer file.Close()
 
-		reader := rot13.NewReader(file)
-		buf, err := io.ReadAll(reader)
-		require.NoError(t, err, "unable to read obfuscated config file")
+	initialize(DefaultConfigSaveDir, filename, true)
+	_config.SetConfig(newTestConfig())
 
-		fileConf := &UserConfig{}
-		assert.NoError(t, proto.Unmarshal(buf, fileConf), "unable to unmarshal obfuscated config file")
-
-		want := fmt.Sprintf("%+v", conf)
-		got := fmt.Sprintf("%+v", fileConf)
-		assert.Equal(t, want, got, "obfuscated config file doesn't match")
-	})
+	select {
+	case <-called:
+		t.Log("recieved config change notification")
+	case <-time.After(time.Second):
+		assert.Fail(t, "timeout waiting for config change notification")
+	}
 }
 
-func withTempConfigFile(t *testing.T, conf *UserConfig, obfuscate bool, f func(*os.File)) {
-	tmpfile, err := os.OpenFile(DefaultConfigFilename, os.O_CREATE|os.O_TRUNC|os.O_RDWR, 0644)
+func TestInvalidFile(t *testing.T) {
+	tmpfile, err := os.CreateTemp("", DefaultConfigFilename)
 	require.NoError(t, err, "couldn't create temp file")
-	defer func() { // clean up
-		tmpfile.Close()
-		os.Remove(tmpfile.Name())
-	}()
-
-	if conf != nil {
-		buf, _ := proto.Marshal(conf)
-
-		var writer io.Writer = tmpfile
-		if obfuscate {
-			writer = rot13.NewWriter(tmpfile)
-		}
+	tmpfile.WriteString("real-list-of-lantern-ips: https://youtu.be/dQw4w9WgXcQ?t=85")
+	tmpfile.Close()
 
-		_, err := writer.Write(buf)
-		require.NoError(t, err, "unable to write to test config file")
+	_, err = readExistingConfig(tmpfile.Name(), true)
+	assert.Error(t, err, "should get error if config file is invalid")
 
-		tmpfile.Sync()
-	}
-
-	f(tmpfile)
+	os.Remove(tmpfile.Name())
 }
 
 const (
@@ -151,6 +93,26 @@ func newTestConfig() *UserConfig {
 	}
 }
 
+func newTestConfigFile(conf *UserConfig) (string, error) {
+	tmpfile, err := os.CreateTemp("", DefaultConfigFilename)
+	if err != nil {
+		return "", nil
+	}
+	buf, err := protojson.Marshal(conf)
+	if err != nil {
+		return "", err
+	}
+	_, err = tmpfile.Write(buf)
+	if err != nil {
+		tmpfile.Close()
+		os.Remove(tmpfile.Name())
+		return "", err
+	}
+	tmpfile.Sync()
+	tmpfile.Close()
+	return tmpfile.Name(), nil
+}
+
 func buildProxy(proto string) *apipb.ProxyConnectConfig {
 	conf := &apipb.ProxyConnectConfig{
 		Name:      "AshKetchumAll",
@@ -187,7 +149,7 @@ func resetConfig() {
 	_config.mu.Lock()
 	_config.config.Reset()
 	_config.filePath = ""
-	_config.obfuscate = false
+	_config.readable = true
 	_config.listeners = nil
 	_config.mu.Unlock()
 }

flags.go

@@ -33,7 +33,7 @@ type Flags struct {
 	ForceProxyAddr     string        `flag:"force-proxy-addr" help:"if specified, force chained proxying to use this address instead of the configured one, assuming an HTTP proxy"`
 	ForceAuthToken     string        `flag:"force-auth-token" help:"if specified, force chained proxying to use this auth token instead of the configured one"`
 	ForceConfigCountry string        `flag:"force-config-country" help:"if specified, force config fetches to pretend they're coming from this 2 letter country-code"`
-	ReadableConfig     bool          `flag:"readableconfig" help:"if specified, disables obfuscation of the config yaml so that it remains human readable"`
+	ReadableConfig     bool          `flag:"readableconfig" help:"if true, the config file will be saved in a human-readable format"`
 	Help               bool          `flag:"help" help:"Get usage help"`
 	NoUiHttpToken      bool          `flag:"no-ui-http-token" help:"don't require a HTTP token from the UI"`
 	Standalone         bool          `flag:"standalone" help:"run Lantern in its own browser window (doesn't rely on system browser)"`

flashlight.go

@@ -336,7 +336,8 @@ func New(
 	}
 
 	readable, _ := f.flagsAsMap["readableconfig"].(bool)
-	_, err := userconfig.Init(f.configDir, !readable)
+	sticky, _ := f.flagsAsMap["stickyconfig"].(bool)
+	_, err := userconfig.Init(f.configDir, readable || sticky)
 	if err != nil {
 		log.Errorf("user config: %v", err)
 	}
@@ -529,7 +530,8 @@ func (f *Flashlight) StartBackgroundServices() (func(), error) {
 
 func (f *Flashlight) startConfigService() (services.StopFn, error) {
 	readable, _ := f.flagsAsMap["readableconfig"].(bool)
-	handler, err := userconfig.Init(f.configDir, !readable)
+	// we don't need to also check for sticky here because this function is only called if sticky is not set
+	handler, err := userconfig.Init(f.configDir, readable)
 	if err != nil {
 		return nil, err
 	}