---
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).
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
+ }
+
+ 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
Hi,
Thanks for the patch.
On Thu, Apr 08, 2021 at 07:59:47PM +0200, Bruno BELANYI wrote:
> 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).
Why do you need this in the first place?
Your user has read access, why do other users need to read the config?
> I'm not really familiar with Go or this codebase, so please do give me
> feedback about anything that I might have missed.
> +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
> + }
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.
> + 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
>
Kind regards,
Reto