show perms error only if config contains secrets v1 PROPOSED

Bruno BELANYI: 1
 show perms error only if config contains secrets

 1 files changed, 57 insertions(+), 7 deletions(-)

Hello,

On Mon, 12 Apr 2021 19:08:32 +0200
Reto <reto@labrat.space> wrote:

[PATCH] show perms error only if config contains secrets Export this patch

---
This patch allows the user to store their configuration file with laxer
permissions if it does not contain any actual secrets (i.e: paswords or
OAuth tokens).

Reto <reto@labrat.space>

4 years ago

Why do you need this in the first place?
Your user has read access, why do other users need to read the config?

Next

I'm not really familiar with Go or this codebase, so please do give me
feedback about anything that I might have missed.

 config/config.go | 64 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 7 deletions(-)

diff --git a/config/config.go b/config/config.go
index 8b409fe..462b93f 100644
--- a/config/config.go
+++ b/config/config.go
@@ -162,7 +162,9 @@ func mapName(raw string) string {
 	return string(newstr)
 }
 
-func loadAccountConfig(path string) ([]AccountConfig, error) {
+// Load account configuration from the path, but do not extract pasword using
+// provided commands
+func loadAccountConfigPure(path string) ([]AccountConfig, error) {
 	file, err := ini.Load(path)
 	if err != nil {
 		// No config triggers account configuration wizard
@@ -220,21 +222,34 @@ func loadAccountConfig(path string) ([]AccountConfig, error) {
 			return nil, fmt.Errorf("Expected from for account %s", _sec)
 		}
 
+		accounts = append(accounts, account)
+	}
+	return accounts, nil
+}
+
+func loadAccountConfig(path string) ([]AccountConfig, error) {
+	accounts, err := loadAccountConfigPure(path)
+	if err != nil {
+		return nil, err
+	}
+
+	var parsedAccounts []AccountConfig
+	for _, account := range accounts {
 		source, err := parseCredential(account.Source, account.SourceCredCmd)
 		if err != nil {
-			return nil, fmt.Errorf("Invalid source credentials for %s: %s", _sec, err)
+			return nil, fmt.Errorf("Invalid source credentials for %s: %s", account.Name, err)
 		}
 		account.Source = source
 
 		outgoing, err := parseCredential(account.Outgoing, account.OutgoingCredCmd)
 		if err != nil {
-			return nil, fmt.Errorf("Invalid outgoing credentials for %s: %s", _sec, err)
+			return nil, fmt.Errorf("Invalid outgoing credentials for %s: %s", account.Name, err)
 		}
 		account.Outgoing = outgoing
-
-		accounts = append(accounts, account)
+		parsedAccounts = append(parsedAccounts, account)
 	}
-	return accounts, nil
+
+	return parsedAccounts, nil
 }
 
 func parseCredential(cred, command string) (string, error) {
@@ -628,6 +643,41 @@ func LoadConfigFromFile(root *string, sharedir string) (*AercConfig, error) {
 	return config, nil
 }
 
+func hasSecrets(filename string) bool {
+	accounts, err := loadAccountConfigPure(filename)
+	if err != nil {
+		// Conservatively assume that there is a secret on errors
+		return true
+	}
+
+	for _, account := range accounts {
+		sourceWithPass, err := parseCredential(account.Source, account.SourceCredCmd)
+		if err != nil {
+			return true
+		}
+		if account.Source == sourceWithPass {
+			return true
+		}
+		if strings.Contains(account.Source, "+oauthbearer://") {
+			return true
+		}

Reto <reto@labrat.space>

4 years ago

I don't think this is the right approach, this is way to brittle.
The code that actually adds new auth schemes doesn't life in this function and
chances are this gets missed once a new scheme is introduced.
Meaning we now have a security problem waiting to happen.

Next

+
+		outgoingWithPass, err := parseCredential(account.Outgoing, account.OutgoingCredCmd)
+		if err != nil {
+			return true
+		}
+		if account.Outgoing == outgoingWithPass {
+			return true
+		}
+		if strings.Contains(account.Outgoing, "+oauthbearer://") {
+			return true
+		}
+	}
+
+	// No secrets have been identified
+	return false
+}
+
 // checkConfigPerms checks for too open permissions
 // printing the fix on stdout and returning an error
 func checkConfigPerms(filename string) error {
@@ -637,7 +687,7 @@ func checkConfigPerms(filename string) error {
 	}
 	perms := info.Mode().Perm()
 	// group or others have read access
-	if perms&044 != 0 {
+	if perms&044 != 0 && hasSecrets(filename) {
 		fmt.Fprintf(os.Stderr, "The file %v has too open permissions.\n", filename)
 		fmt.Fprintln(os.Stderr, "This is a security issue (it contains passwords).")
 		fmt.Fprintf(os.Stderr, "To fix it, run `chmod 600 %v`\n", filename)
-- 
2.31.1

Reto <reto@labrat.space>

4 years ago

Kind regards,
Reto

Reto <reto@labrat.space>

4 years ago

Hi,
Thanks for the patch.