Add support for using credstash as a secret store #8494

justin8 · 2017-07-16T09:30:30Z

Description:

Add support for using credstash to store secrets.

It is used by using the same !secret foo syntax as the existing secrets.yaml and keyring methods of storing secrets. The CLI script for managing secrets is very similar to the one for using a keyring. Examples can be seen in the documentation PR below

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

Checklist:

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

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

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.

mention-bot · 2017-07-16T09:30:34Z

@justin8, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @balloob and @kellerza to be potential reviewers.

houndci-bot · 2017-07-16T09:30:40Z

homeassistant/scripts/credstash.py

+        the_secret = getpass.getpass('Please enter the secret for {}: '
+                                     .format(args.name))
+        current_version = int(credstash.getHighestVersion(args.name, table=_SECRET_NAMESPACE))
+        credstash.putSecret(args.name, the_secret, version=current_version+1, table=_SECRET_NAMESPACE)


line too long (102 > 79 characters)

houndci-bot · 2017-07-16T09:30:40Z

homeassistant/scripts/credstash.py

+    if args.action == 'put':
+        the_secret = getpass.getpass('Please enter the secret for {}: '
+                                     .format(args.name))
+        current_version = int(credstash.getHighestVersion(args.name, table=_SECRET_NAMESPACE))


line too long (94 > 79 characters)

houndci-bot · 2017-07-16T09:30:40Z

homeassistant/scripts/credstash.py

+def run(args):
+    """Handle credstash script."""
+    parser = argparse.ArgumentParser(
+        description=("Modify Home-Assistant secrets in the default credstash store. "


line too long (85 > 79 characters)

houndci-bot · 2017-07-16T09:30:40Z

homeassistant/scripts/credstash.py

@@ -0,0 +1,60 @@
+"""Script to get, put and delete secrets stored in credstash."""
+import os


'os' imported but unused

MartinHjelmare · 2017-07-18T11:30:50Z

So this uses the same syntax as for the yaml !secret? Maybe add a short explanation here in the description and a config example? The documentation PR code explains it though.

justin8 · 2017-07-18T11:50:46Z

Correct, it uses the same syntax. I did forget to mention that here, I updated the PR comment to reflect that.

As for the documentation, I kept it similar to the keyring one that already exists as it functions almost identically

balloob · 2017-07-24T07:05:36Z

homeassistant/scripts/credstash.py

+
+    if args.action == 'list':
+        secrets = [i['name'] for i in credstash.listSecrets(table=table)]
+        deduped_secrets = list(dict.fromkeys(secrets))


sorted(set(i['name'] for i in credstash.listSecrets(table=table)))

Done (mostly). I kept it as 2 variables to keep the line length under 80 and also to make it a little bit more readable.

balloob · 2017-07-24T07:07:26Z

tests/util/test_yaml.py

@@ -372,6 +386,17 @@ def test_secrets_keyring(self):
        _yaml = load_yaml(self._yaml_path, yaml_str)
        self.assertEqual({'http': {'api_password': 'yeah'}}, _yaml)

+    def test_secrets_credstash(self):
+        """Test credstash fallback & get_password."""
+        yaml.credstash = None


Tests should not have any side effects, please use

from unittest.mock import patch with patch.object(yaml, 'credstash', None): …

Good point, I should've known better. I was just copying the test case directly above to keep in line with existing tests/formatting

balloob · 2017-07-24T07:09:19Z

tests/util/test_yaml.py

@@ -372,6 +386,17 @@ def test_secrets_keyring(self):
        _yaml = load_yaml(self._yaml_path, yaml_str)
        self.assertEqual({'http': {'api_password': 'yeah'}}, _yaml)

+    def test_secrets_credstash(self):


It doesn't look like your test uses any of the setup in this class. Please move it to be a standalone function in this file.

It uses the self._yaml_path from setup, which itself uses some other parts of the setup.

The path is needed to ensure fall-through when a secret isn't in the local file, that credstash.getSecret is called

balloob · 2017-07-24T15:52:19Z

Thanks 🐬

Ok to merge when tests pass.

balloob · 2017-08-05T06:29:51Z

This PR is not mocking out credstash correctly during other tests and now we're doing I/O during unit tests + can no longer run tests offline.

justin8 · 2017-08-05T07:25:26Z

Hmm, how can I mock it out for all the other tests? should I just be adding it to the other test functions for the yaml parser?

balloob · 2017-08-05T21:05:04Z

I'm fixing it like this right now: https://github.com/home-assistant/home-assistant/pull/8834/files#diff-0897a49ea1455f0a57774d651fec8d47R16

The credstash test still passes, so I guess that's good?

homeassistant added the cla-signed label Jul 16, 2017

balloobbot added core requirement labels Jul 16, 2017

houndci-bot reviewed Jul 16, 2017

View reviewed changes

justin8 mentioned this pull request Jul 16, 2017

Add documentation on using credstash as a secret store home-assistant/home-assistant.io#2988

Merged

justin8 force-pushed the credstash branch from 683060c to b79809b Compare July 16, 2017 09:53

homeassistant added the cla-signed label Jul 16, 2017

justin8 force-pushed the credstash branch from f25f851 to c34d33c Compare July 16, 2017 14:20

homeassistant added the cla-signed label Jul 16, 2017

justin8 force-pushed the credstash branch from c34d33c to d784002 Compare July 16, 2017 14:21

homeassistant added the cla-signed label Jul 16, 2017

pvizeli requested a review from balloob July 20, 2017 13:50

pvizeli assigned balloob Jul 20, 2017

balloob requested changes Jul 24, 2017

View reviewed changes

Add support for using credstash as a secret store

28cb314

justin8 force-pushed the credstash branch from d784002 to 28cb314 Compare July 24, 2017 15:18

homeassistant added the cla-signed label Jul 24, 2017

balloob approved these changes Jul 24, 2017

View reviewed changes

balloob merged commit 98568b5 into home-assistant:dev Jul 24, 2017

justin8 deleted the credstash branch July 25, 2017 01:34

balloob mentioned this pull request Jul 29, 2017

0.50 #8685

Merged

dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017

Add support for using credstash as a secret store (home-assistant#8494)

5fc589a

home-assistant locked and limited conversation to collaborators Dec 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for using credstash as a secret store #8494

Add support for using credstash as a secret store #8494

justin8 commented Jul 16, 2017 •

edited

Loading

mention-bot commented Jul 16, 2017

houndci-bot Jul 16, 2017

houndci-bot Jul 16, 2017

houndci-bot Jul 16, 2017

houndci-bot Jul 16, 2017

MartinHjelmare commented Jul 18, 2017

justin8 commented Jul 18, 2017

balloob Jul 24, 2017

justin8 Jul 24, 2017 •

edited

Loading

balloob Jul 24, 2017

justin8 Jul 24, 2017

balloob Jul 24, 2017

justin8 Jul 24, 2017

balloob Jul 24, 2017

balloob commented Jul 24, 2017

balloob commented Aug 5, 2017

justin8 commented Aug 5, 2017

balloob commented Aug 5, 2017

		@@ -0,0 +1,60 @@
		"""Script to get, put and delete secrets stored in credstash."""
		import os

Add support for using credstash as a secret store #8494

Add support for using credstash as a secret store #8494

Conversation

justin8 commented Jul 16, 2017 • edited Loading

Description:

Checklist:

mention-bot commented Jul 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare commented Jul 18, 2017

justin8 commented Jul 18, 2017

Choose a reason for hiding this comment

justin8 Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Jul 24, 2017

balloob commented Aug 5, 2017

justin8 commented Aug 5, 2017

balloob commented Aug 5, 2017

justin8 commented Jul 16, 2017 •

edited

Loading

justin8 Jul 24, 2017 •

edited

Loading