fix: config describe prints secure properties with value redacted

[cveticm]

Proposed changes

Config describe should print secure properties (public_api_key,private_api_key, etc.) with value redacted.

How it works

• Create a new Store (configStore) without reading in env vars
• Get a map of all the insecurely stored profile properties using configStore
• For all secure properties, get the property value using configStore and include it in the map with value redacted
  • No additional logic is needed if the secure property is held insecurely or securely. configStore will call to the appropriate place. And, as configStore does not take into account env vars, these will not cause any trouble either.

Design decisions

1. Checking for all secure properties, despite the set auth_type
   If a user has managed to configure their profile in a way that it has multiple authentication type credentials, this should be reflected in config describe to easier help the user diagnose any auth issues this might cause

2. Redacted text
   To make clear to the user where a secure property is being held, the redacted text includes its source:
   [redacted - source: secure storage]
[redacted - source: config file]

example output:

// before - secure properties not shown at all
bin/atlas config describe sa   
SETTING           VALUE
auth_type         service_account
org_id            670cd17af33cea212ea1ed61
output            plaintext
project_id        670e34d35a4f587387db2102
service           cloud

//after - secure properties show but redacted
bin/atlas config describe sa   
SETTING           VALUE
auth_type         service_account
client_id         [redacted - source: secure storage]
client_secret     [redacted - source: secure storage]
org_id            670cd17af33cea212ea1ed61
output            plaintext
project_id        670e34d35a4f587387db2102
service           cloud

[blva]

[q] is there a way can we expand this on atlas config edit since that one opens the config file? cc @jeroenvervaeke

[cveticm]

Looking at the code, atlas config edit opens the actual config file (with vi or similar). We'd have to entirely refactor how the command works to support this.

[blva]

I think this is the tradeoff of adding it at the config level, having to re-add the redacted values. I found myself confused before when trying to find out if my credentials were correct or not and saw that it was not in the file at atlas config edit.

I'm good with this change anyways since it fixes config describe but I think we should probably discuss if we're happy to keep atlas config edit as is or not in the team channel

[cveticm]

Agreed on discussing if followup work is needed for config edit 👍

I think we could change the command to

1. Open a copy of the config file that we populate with redacted secure properties.
2. When a user is finished, we'd run the migration logic on the file and replace the actual config with the copy.

This is just the first solution that came to mind, happy to discuss more with the team tomorrow during sync

[blva]

yep if we have a list of options that would be awesome to chat w/ the team async, my stamp is 🚢 anyways

[jeroenvervaeke]

I don't think we should alter atlas config edit, since the secrets are not saved in the config I don't think we should re-add them and say [redacted].

I think that approach will make things even more confusing and give customers the impression that our credentials are stored in the config, which is not true.

[coveralls]

coverage: 63.961% (+0.01%) from 63.948%
when pulling 9b360da on fix_describe_secure_properties
into feaebe1 on master.

[jeroenvervaeke]

[q] When we use the insecure store, we still want to redact the secrets. No?

[cveticm]

This is already done by the function config.Map(), see here

[jeroenvervaeke]

Can't this entire function be written as follows?

func (opts *describeOpts) AddSecureProperties(m map[string]string, ctx context.Context) (map[string]string, error) {
	for _, key := range config.SecureProperties {
		if v, found := m[key]; found && v != "" {
			m[key] = redacted
		}
	}

	return m, nil
}

[cveticm]

Map m is only the values in config.toml. Thus we have to populate the map with the additional properties held in secure storage.

[jeroenvervaeke]

Thanks for increasing the test coverage 🙏