Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config: new option -r redacts sensitive fields #1376

Merged
merged 4 commits into from Mar 28, 2015
Merged

Config: new option -r redacts sensitive fields #1376

merged 4 commits into from Mar 28, 2015

Conversation

tomjaspers
Copy link
Collaborator

I think this was brought up in some issues before, as it makes it easier for people to blanket copy/paste their configuration when posting issues, without worrying about spilling their keys.

This PR introduces the config -r ( config --redacted) flag, which does the same as config, with the fields redacted, e.g.,

$ ./beet config -r
lyrics:
    sources: [lyricwiki, lyrics.com, musixmatch]
    google_API_key: REDACTED
    force: no
    auto: yes
    google_engine_ID: REDACTED
    fallback:
[...]
  • Hard-coded a bunch of typical field names that need to be redacted (['password', 'username', 'userid', 'apikey', apisecret', 'email']).
  • Plugins can register their own fields to be redacted, e.g.,:
    self.config.add_redacted_fields(['google_API_key', 'google_engine_ID'])

I was considering working with regex, matching 'secret', 'key', etc, but the above list seems to cover all the fields from the plugins listed in the official beets docs.

- `config -r` will show 'REDACTED' instead of the actual value
- Has a default list of field names that should be redacted (e.g., 'password')
- Plugins can add redacted fields that they introduce
@brunal
Copy link
Collaborator

brunal commented Mar 25, 2015

Nice! About 'key' and 'secret': no need for a regex, you could simply do if any(sensitive in field_name for sensitive in ('key', 'secret'))

- `add_redacted_fields(self, *field_names)` to use argument unpacking
- foo =| bar instead of foo = foo | bar
@tomjaspers
Copy link
Collaborator Author

Thanks for the review, @brunal ! Good suggestions, as always.

About key, secret etc: I like the idea, but it would give false positives (e.g., duplicates plugin has a field 'keys'), so I think it'll be okay as it is.

I'll add some docs a bit later.

@brunal
Copy link
Collaborator

brunal commented Mar 25, 2015

LGTM. Could you add tests in test/test_config_command.py?

@sampsyo
Copy link
Member

sampsyo commented Mar 28, 2015

Thank you, @tomjaspers! This looks great.

I'm going to make a few enhancements to the modularity of the redaction itself in confit.py. This should make the functionality a bit more generic and also more local, so add_redacted_fields does not propagate all the way to the root.

@sampsyo sampsyo merged commit ce78be3 into beetbox:master Mar 28, 2015
sampsyo added a commit that referenced this pull request Mar 28, 2015
Config: new option -r redacts sensitive fields

Conflicts:
	beets/util/confit.py
@sampsyo
Copy link
Member

sampsyo commented Mar 28, 2015

OK, done.

The API is now quite a bit different: rather than a hard-coded set of keys, you now mark individual views as redacted. Like this:

self.config['password'].redact = True

So it's possible I missed some things that were covered by the previous hard-coded list, but which I haven't yet found by searching the code. Please let me know if it seems like I left a gap.

I also have one question of opinion: What do you think about making redaction the default? People would have to pass --clear or something to show passwords. It might be sensible to make the common case the secure case.

@tomjaspers
Copy link
Collaborator Author

I think the main use-case for dumping the config is for sharing it elsewhere, so I'm all for making it the default redacted 👍

sampsyo added a commit that referenced this pull request Mar 29, 2015
@sampsyo
Copy link
Member

sampsyo commented Mar 29, 2015

Great! Redaction is now the default. The flag --clear disables it.

@tomjaspers tomjaspers deleted the config-redacted-fields branch May 20, 2015 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants