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

Split secrets from configuration files #2262

Closed
wants to merge 4 commits into from

Conversation

kellerza
Copy link
Member

@kellerza kellerza commented Jun 9, 2016

Description:
Option to split secrets from configuration files. Secrets can be stored in (one or both):

  • secrets.yaml (in same folder as configuration.yaml)
  • keyring (optional, only if dep installed)**

This uses the loaded configuration.yaml and inserts secrets where applicable:

  • It is completely dependent on configuration.yaml. So the user can choose to remove secrets (if at all).
  • Placing a (-) in the value will resolve the secret.
  • If a username is present, and no password the password will be resolved. The username can also contain a (-)
  • If password present it will be left as is. Or resolved if it contains (-)
  • If a secret can't be resolved a WARNING will be logged.
  • Stale secrets in secrets.yaml raises a warning
  • Using a list of the resolved secrets, a UI interface can be created to update secrets in secrets.yaml / keyring
    • Until a UI exists editing secrets.yaml would be the best way to store secrets
    • Easiest way to populate keyring would be a command line option like --passwords?

Related issue (if applicable): fixes #

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#

Example entry for configuration.yaml:

sensor:
   api_key: (-)
http:
  api_password: (-)
mycomponent:
  username: (-)
mycomponent2:
  username: un02
  password: (-)

resolves to:

sensor:
  api_key: ABCDEF
http:
  api_password: JKLMNOP
mycomponent:
  username: un01
  password: pw01
mycomponent 2:
  username: un02
  password: pw02

using secrets.yaml file:

/http/api_password: JKLMNOP
/sensor/api_key: ABCDEF  
/mycomponent/username: un01
/mycomponent/password: pw01
/mycomponent_2/password: pw02

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@balloob
Copy link
Member

balloob commented Jun 9, 2016

How does it work with lists?

@kellerza
Copy link
Member Author

kellerza commented Jun 9, 2016

It will iterate over all items in lists. and handle all OrderedDicts in the lists

@kellerza
Copy link
Member Author

Added some tests, but will have to add the array index to new_path to truly separate items in lists

from homeassistant.exceptions import HomeAssistantError
from homeassistant.const import EVENT_HOMEASSISTANT_START
try:
import keyring # pylint: disable=wrong-import-order,wrong-import-position
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's with the pylint disables ? The imports are ordered built-in, 3rd party, home assistant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pylint complained about the try catch around the import if it is before the home assistant imports. Not sure if there's a better way to do it?

And keyring should probably be optional

@balloob
Copy link
Member

balloob commented Jun 15, 2016

I see an issue with lists. It is very common for people to have a list of platforms that contain duplicate platform types and that could totally be unrelated usernames/passwords. That would not work right now as the path to it would be the same.

And that brings me to the next issue: I think that this is getting way too complex. The goal is to be able to extract sensitive information from the main config and optionally offer an extra way of encrypting this in a keyring.

I think that instead of doing like this, we should go way simpler. Instead of auto-generating namespaced names, let's just allow the user to do it. Base it on the existing !env_var constructor for YAML but instead of looking it up in the environment variables, look it up in a dict. Simple, one to one mapping.

nest:
  username: !yaml_var nest_username
  password: !yaml_var nest_password

I want to try to avoid as much magic as possible when it comes to the config because it is already confusing as is. If we start doing things like auto-resolving password fields if we find username the user will not know how to even start debugging if a problem occurs.

@kellerza
Copy link
Member Author

Ok, that is a good idea, will simplify things a bit

Do you prefer !yaml_var or !secret? Or is the idea to hook into existing envvar code for secrets?

Or !yaml_secret AND !keyring for different storage locations? At the moment it starts with secrets.yaml and then keyring

@balloob
Copy link
Member

balloob commented Jun 17, 2016

For secret, let's use !yaml_var. We can always do keyring later.

@kellerza
Copy link
Member Author

!secret currently supports both in keyring2 branch.

!yaml_var might be a bit confusing, since these vars can only come from secret.yaml at the moment and that is not configurable. If you have a suggestion to make it configurable I can look into it, but since the !secrets are substituted during loading of configuration.yaml, there is no guarantee these vars have been loaded by configuration.yaml before being used for substitution.

@kellerza kellerza deleted the keyring branch July 3, 2016 04:45
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants