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.

Sign up for GitHub

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

Jump to bottom

[bldr] encryption/decryption #171

Merged
merged 1 commit into from
Jan 28, 2016
Merged

[bldr] encryption/decryption #171

merged 1 commit into from
Jan 28, 2016

Conversation

bookshelfdave
Copy link
Contributor

This PR implements or enhances all of these commands*:

bldr generate-user-key <key> <password> <email> [--expire-days=<expire_days>] [-vn]
bldr generate-service-key <key> [--expire-days=<expire_days>] [--group=<group>] [-vn]
bldr encrypt --user <userkey> --service <servicekey> --infile <infile> --outfile <outfile> [--password <password>] [--group=<group>] [-vn]
bldr decrypt --infile <infile> --outfile <outfile> [-vn]
bldr import-key (--infile <infile> | <key> --u <url>) [-vn]
bldr export-key (--user <userkey> | --service <servicekey>) --outfile <outfile> [--group=<group>] [-vn]
bldr download-repo-key <key> [-vn]
bldr upload-repo-key <key> -u <url> [-vn]
bldr list-keys [-vn]
  • with the exception of download/upload, which have been renamed for clarity.

Notes:

  • Encrypt uses a service public key and a user public key as recipients, message signing uses a user private key.
    • Key trust is not implemented, and encryption uses the ENCRYPT_ALWAYS_TRUST flag.
  • Decrypt uses a service private key and a user public key to verify an encrypted message.
    • a service OR a user should be able to decrypt a message.
  • If --password isn't specified to encrypt, then an echoless password prompt appears. We can talk about other ways to obtain a password in another PR (~/.bldr/config?)
  • import-key and export-key only do public key IO, we can't export private keys.
  • Key input/output functions have been refactored to take --infile and --outfile.
  • rngd is started/stopped on demand to generate entropy for key generation
  • download-repo-key / upload-repo-key need to be readdressed in a separate PR.
  • Keys are generated with a bldr_ prefix to distinguish them from any other key that is imported/exported into the GPG cache. Additionally, GPG has a 4 character minimum on key names, so having a common bldr prefix allows keys with >= 1 character. (Really, I just wanted to have a key named foo). TODO: The only wrinkle here is that specifying a name on the CLI for a key should work with both normalized + non-normalized names (unless there are keys with normalized + non-normalized names that are the same).
  • bldr will honor the BLDR_GPG_CACHE environment variable to point at a different GPG cache. This is primarily used for testing encrypt/decrypt/import/export.
  • New dependency: rpassword (MIT license) for echoless password entry

TODO:

  • implement colorized output
  • gpg module shouldn't display any output
  • test Decrypt as user Not possible, I need to be able to export private keys
  • list keys test
  • test service group params
  • test user export
  • find_key should be an exact match
  • find_key should do an exact match using the first name on a key
  • key tests need to be multi-GPG_CACHE aware so they don't litter the main gpg cache
  • normalize key names (did the user enter bldr_foo or foo?)
  • clone wars

skitch

skitch-1

@bookshelfdave bookshelfdave force-pushed the dp_tales_from_the_encrypt branch 3 times, most recently from 8d814af to aa159a1 Compare January 26, 2016 22:12
@bookshelfdave
Copy link
Contributor Author

gif-keyboard-11854558452584748699

@bookshelfdave bookshelfdave changed the title [bldr] WIP - encryption/decryption [bldr] encryption/decryption Jan 26, 2016
@bookshelfdave bookshelfdave force-pushed the dp_tales_from_the_encrypt branch from 728ba9b to 24c5d60 Compare January 27, 2016 00:49
@bookshelfdave
Copy link
Contributor Author

gpg::find_key always uses a search expression, so searching for the exact key with name foo will also find similar keys such as foobar. Looking at the gpgme docs, it looks like the only way to pull back a single exact key is by fingerprint, which I don't have (I would need to find the key first to get the fingerprint).

I have a test and a fix that I'll commit tomorrow AM.

reset
reset reviewed Jan 27, 2016
View reviewed changes
src/bldr/command/key.rs
if url.is_empty() {
try!(gpg::import(&config.key()));
}
pub fn import(config: &Config) -> BldrResult<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If somebody specifies both an --infile and -u flag this function will silently prefer the --infile option. I'd recommend that we make the flags exclusive and notify the user or condense them into a single flag representing "location" where a value starting with http:// or https:// is a remote location and anything else is considered a filesystem location

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind! It looks like that you've covered this in docopt by specifying these two flags with an | operator :)

@reset
Copy link
Collaborator

reset commented Jan 27, 2016

@metadave overall things are looking great!

I left some comments about the heavy usage of clone(). This is a "code smell" that you should be on the lookout for. You can easily overcome the borrower checker by using clone(), but it's not the right way to go about solving a lot of problems. If you want to pair program to see how to knock out the unnecessary calls to clone() I can absolutely help you out. I might have provided enough information in this PR already for you to get it under control, though ;)

@bookshelfdave bookshelfdave force-pushed the dp_tales_from_the_encrypt branch from 4a58a74 to dd10510 Compare January 27, 2016 22:10
bookshelfdave
bookshelfdave reviewed Jan 27, 2016
View reviewed changes
src/bldr/command/key.rs
// prepend BLDR_KEY_PREFIX to the key name, these are the bldr keys
let full_userkey = gen_user_key_name(&userkey);
let full_servicekey = gen_service_key_name(&servicekey, config.group());
let password = match config.password().clone() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reset This is the only clone I'm stuck on

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is because when you're peforming your match the binding p is attempting to take ownership of the value wrapped in the option returned by password(). You need to use the ref keyword in the match to step around this:

match config.password() {
    Some(ref p) => p,
    None => { ... },
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but now p is a &str and since I want to return a string from this block, don't I need a String?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you want a String back from that block. It looks like you're just passing password to gpg::encrypt_and_sign() which takes an &str value for password

@bookshelfdave bookshelfdave force-pushed the dp_tales_from_the_encrypt branch from dd10510 to 6d32f35 Compare January 27, 2016 22:23
@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

@bookshelfdave
Copy link
Contributor Author

updated from review.

gif-keyboard-12993223462365332485

@reset
Copy link
Collaborator

reset commented Jan 28, 2016

@delivery approve

chef-delivery added a commit that referenced this pull request Jan 28, 2016
@chef-delivery
Merge pull request #171 from chef/dp_tales_from_the_encrypt
4e829b3 
Merged change 35832acd-e8fe-4656-ba15-7b8398183ae4

From review branch dp_tales_from_the_encrypt into master

Signed-off-by: reset <reset@chef.io>
@chef-delivery chef-delivery merged commit 4e829b3 into master Jan 28, 2016
@chef-delivery
Copy link
Contributor

Change: 35832acd-e8fe-4656-ba15-7b8398183ae4 approved by: @reset

@chef-delivery chef-delivery deleted the dp_tales_from_the_encrypt branch January 28, 2016 19:37
@reset
Copy link
Collaborator

reset commented Jan 28, 2016

gif-keyboard-7674191353213823093

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bookshelfdave @reset @chef-delivery