Skip to content
This repository has been archived by the owner on Feb 2, 2018. It is now read-only.

DO NOT MERGE: Encryption example for review. #111

Closed
wants to merge 3 commits into from

Conversation

ashcrow
Copy link
Collaborator

@ashcrow ashcrow commented Mar 13, 2017

Encryption code example similar to CPD-101.

Copy link
Contributor

@mbarnes mbarnes left a comment

Choose a reason for hiding this comment

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

Couple nits, but so far so good.

Where is the passphrase for GNUPGEncryption coming from?

for uid in keydata['uids']:
if uid.split(' ')[0] == name:
self.fingerprint = keydata['fingerprint']
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Looping over keydata['uids'] implies you're allowing for multiple iterations, so should this break be indented further?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch.

self.fingerprint = None
for keydata in self._encryptor.list_keys():
for uid in keydata['uids']:
if uid.split(' ')[0] == name:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a case-insensitive comparison? Exact match seems overly strict to me.

@ashcrow
Copy link
Collaborator Author

ashcrow commented Mar 13, 2017

Likely the passphrase would be coming from a configuration file such as the storage-services.

@mbarnes
Copy link
Contributor

mbarnes commented Mar 14, 2017

Hmm, storing secrets in plain text files worries me but I guess that would be okay if file permissions are restrictive enough.

I was wondering, maybe as a future feature, if we could delegate key storage to Custodia? It can also use etcd as a backing store but handles the crypto, if I understand correctly. (I don't know how mature that project is though; maybe still too early?)

@ashcrow
Copy link
Collaborator Author

ashcrow commented Mar 15, 2017

@mbarnes I like the idea, but I'm not sure if we want to require Custodia to be set up to use Commissaire.

@ashcrow
Copy link
Collaborator Author

ashcrow commented Mar 15, 2017

@mbarnes though we could make it part of commissaire proper install as it does seem like a good fit.

@ashcrow
Copy link
Collaborator Author

ashcrow commented Mar 15, 2017

The more I think about this the more it seems to make sense install and use Custodia as part of Commissaire standard install. We can then front it's API as well which would continue the idea of providing one api location to look at when one has Commissaire.

@tiran Any thoughts around this?

@ashcrow
Copy link
Collaborator Author

ashcrow commented Mar 15, 2017

If we do use Custodia it will significantly change the 101 proposal. We could take advantage of Custodia for storage and retrieval of sensitive data. The need to provide our on encryption backend system wouldn't be required any longer.

@tiran
Copy link

tiran commented Mar 15, 2017

@ashcrow In general I'm +1 on any new use of Custodia. However I'm not familiar with commissaire yet. Give me a chance to learn more about the project first and understand its role in Atomic. I'll get back to you tomorrow.

@ashcrow
Copy link
Collaborator Author

ashcrow commented Mar 16, 2017

Closing this example PR and will update CPD-101 with Custodia wording.

@ashcrow ashcrow closed this Mar 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants