Encrypt and upload secrets. #4706

elliott-davis · 2018-03-05T22:35:57Z

Blocked by: habitat-sh/builder#232

Signed-off-by: Elliott Davis elliott@excellent.io

thesentinels · 2018-03-05T22:35:58Z

Thanks for the pull request! Here is what will happen next:

Your PR will be reviewed by the maintainers
If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

baumanj · 2018-03-06T15:01:45Z

components/builder-depot-client/src/lib.rs

+
+        if res.status != StatusCode::Created {
+            return Err(err_from_response(res));
+        };


Is that ; doing something here?

baumanj · 2018-03-06T15:08:31Z

components/builder-depot-client/src/lib.rs

+    /// # Failures
+    ///
+    /// * Remote Builder is not available
+    pub fn create_origin_secret(&self, origin: &str, token: &str, key: &str, secret: &str) -> Result<()> {


How does one update a secret? Presumably by calling this function again? I'm wondering if there's utility in having there be any distinction in the interface about whether there was already a value for the provided key (such as a Result<bool> return) or requiring something like an overwrite: bool parameter. I don't actually know; just throwing that idea out there.

right now there will be a unique constraint on the key name per origin. Users will have to delete and recreate a key if the want to reuse it. This behavior should be consistent with Travis CI

That sounds good. Where is the the depot/origins/{}/secret implementation? Also, might be worth a note in the help.

habitat-sh/builder#232

baumanj · 2018-03-06T15:24:46Z

components/hab/src/cli.rs

+                         be taken from the HAB_BLDR_URL environment variable if defined. (default: \
+                         https://bldr.habitat.sh)")
+                    (@arg AUTH_TOKEN: -z --auth +takes_value "Authentication token for Builder")
+                    (@arg ORIGIN: +takes_value


I wonder if it might be a bit more usable to make ORIGIN an option (requiring -o, --origin)

hab origin secret delete foo bar

is less obvious to me than

hab origin secret delete --origin bar foo

And generally, I think making the purpose of arguments that will often be omitted explicit reduces cognitive load. This goes for all the hab origin secret subcommands

I agree with the idea of having origin be a flag but in an effort to remain consistent with the existing UX I left it as a positional argument. I think a larger refactor is necessary to make origin a flag everywhere to keep everything consistent

I looked at the rest of the UX when there are multiple positional parameters, we do make origin an option:

hab bldr job promote [FLAGS] [OPTIONS] <GROUP_ID> <CHANNEL> … OPTIONS: … -o, --origin <ORIGIN> Limit the promotable packages to the specified origin

When the origin is the only possible positional argument, it's fine, but in this case I think it's the UX consistent behavior to make it an option requiring a flag.

My .02 is that I also prefer the clarity of using the -o or --origin flag... that also helps with not having to concern yourself with the order of the arguments - its explicit that the thing following the flag is in fact the origin name, in other words.

baumanj · 2018-03-06T15:25:21Z

components/hab/src/cli.rs

+                )
+                (@subcommand delete => 
+                    (about: "Delete a secret for your origin")
+                    (@arg KEY_NAME: +required +takes_value


Should we add aliases to be consistent with the other commands?

I don't think so. Personally I'd like to remove the other aliases as well. I find they often lead to typo bugs and provide an inconsistent user experience.

I'm on-board with removing all the aliases, but I think we should do that as a separate change rather than introducing just one new command that doesn't follow the pattern and leads to an inconsistent user experience. Not a big deal; do what feels right to you.

baumanj · 2018-03-06T15:26:38Z

components/hab/src/cli.rs

+            (@subcommand secret =>
+                (about: "Commands related to secret management")
+                (@setting ArgRequiredElseHelp)
+                (@subcommand generate => 


The name generate implies to me that I don't have to come up with the value. To me add is the more natural counterpart to delete.

I talked with @ryankeairns about this and he wanted the UX to be consistent with origin key generate. Previously I had origin secret create to follow the CRUD pattern.

I 💯 on-board with consistency, but in this case I'd say it's a different operation and so should have a different name to make that clear. Not a hill I'm looking to die on or anything, it just struck me as unexpected. I'm interested what @ryankeairns thinks.

Just so I'm following... generate generates a new key; are we also creating/generating a new secret? or is that not exactly what's going on?

@elliott-davis can correct me if I'm wrong, but what we're doing here is asking builder to store an encrypted key/value pair that we are supplying. Builder isn't generating a value for us.

we're asking builder to encrypt and store a value for us. Technically we're encrypting it locally and asking builder to store it.

I don't want to beat this to death (too late!), but perhaps upload is the verb then.

baumanj · 2018-03-06T18:26:29Z

components/hab/src/command/origin/secret/generate.rs

+
+    ui.status(Status::Encrypting, format!("value for key {}.", key))?;
+    let encrypted_secret_bytes = encryption_key.encrypt(secret.as_bytes(), None)?;
+    let encrypted_secret_string = String::from_utf8_lossy(&encrypted_secret_bytes);


If this is arbitrary bytes, I worry about storing them in a str, which guarantees utf8 validity. Can we switch to either base64 encoding or using a Vec<u8> in create_origin_secret?

I think I can switch from_utf8_lossy to from_utf8 to get a utf8 guarantee? What do you think?

I think that you're going to get errors at the rate which a random series of bytes do not make valid UTF-8, right? I don't know what that frequency is off the top of my head, but if there a reason why you wouldn't?

I'm not sure I follow the random series of bytes bit. We are taking a user supplied string, converting it to bytes, sending it to encrypt which returns a base64 encoded series of bytes, then we convert that back into a string for a json payload. Does that match up with what you were thinking?

For posterity, the confusion was mine. I did not realize that BoxKeyPair::encrypt returned already base64-encoded data. I assumed since it's return type was Result<Vec<u8>> that it was raw bytes. This is pretty misleading, so I filed an issue: habitat-sh/core#15

elliott-davis · 2018-03-08T18:09:42Z

→ Determining secrets for core.
for
elliott@elliott-MacBookPro:/src/habitat$ target/debug/hab origin secret upload -u http://localhost:9636/v1 --origin core foo bar
☛ Encrypting value for key foo.
✓ Encrypted foo=[REDACTED].
↑ Uploading secret for key foo.
✓ Uploaded secret for foo.
elliott@elliott-MacBookPro:/src/habitat$ target/debug/hab origin secret list -u http://localhost:9636/v1 --origin core
→ Determining secrets for core.
foo
for
elliott@elliott-MacBookPro:/src/habitat$ target/debug/hab origin secret delete -u http://localhost:9636/v1 --origin core foo
☒ Deleting secret foo.
✓ Deleted secret foo.

baumanj · 2018-03-08T19:39:26Z

All the changes look look great! I'm still a unclear on #4706 (comment). Your thoughts?

raskchanky · 2018-03-09T18:26:10Z

components/builder-depot-client/src/lib.rs

+            .into_iter()
+            .map(|m| m.into())
+            .map(|s: originsrv::OriginSecret| s.get_name().to_string())
+            .collect();


I love this kind of chaining. So good.

raskchanky · 2018-03-09T18:28:37Z

components/hab/src/command/bldr/channel/destroy.rs

        .map_err(Error::DepotClient)?;

-    ui.status(Status::Deleted, format!("channel {}.", channel))?;
+    ui.status(Status::Deleted, format!("secret key {}.", key))?;


Hmm, I'm a little confused. Why was components/hab/src/command/bldr/channel/destroy.rs changed to destroy origin secrets instead? Wouldn't it be better to leave the channel stuff alone and put the destroying secrets stuff into its own file?

I had the same thought!

:whoa: that's definitely a mistake.

raskchanky · 2018-03-09T18:30:26Z

components/hab/src/command/origin/secret/delete.rs

+    ui.status(Status::Deleted, format!("secret {}.", key))?;
+
+    Ok(())
+}


Right, here's the file I was thinking would be present for deleting secrets. I'm not sure what's happening in the channels file I commented on above.

raskchanky · 2018-03-09T18:31:52Z

components/hab/src/command/origin/secret/upload.rs

+            ))
+        }
+    };
+    ui.status(Status::Encrypted, format!("{}=[REDACTED].", key))?;


The [REDACTED] is awesome.

raskchanky · 2018-03-09T18:33:43Z

I left a few comments, but on the whole this looks terrific!

fnichol

Based on the name of the commit message, I'm assuming that this is approaching completion, but possibly not quite done?

Looking pretty good!

I'm starting to wonder about our timelines for releasing the whole feature (i.e. client through to server). If we're going to have our client code ready but not yet ready to service the calls, it might be worth considering feature toggling these subcommands so we can dark launch the code until we're ready to release every together. It still doesn't prevent anyone from using this new functionality (I'm guessing primarily developers on Builder at the moment), simply setting an environment variable.

fnichol · 2018-03-09T18:38:59Z

components/hab/src/cli.rs

+                (@subcommand upload => 
+                    (about: "Create and upload a secret for your origin")
+                    (@arg KEY_NAME: +required +takes_value
+                        "The name of the variable key to be injected into the studio.")


Would a user provide the key prefix or not here? Maybe an example in the usage like we do in a few other places to clarify?

fnichol · 2018-03-09T18:40:47Z

components/hab/src/cli.rs

+                    (about: "Create and upload a secret for your origin")
+                    (@arg KEY_NAME: +required +takes_value
+                        "The name of the variable key to be injected into the studio.")
+                    (@arg SECRET: +required +takes_value


Potential future idea (which might be terrible): I wonder what this would look like if you optionally allowed for stdin of the secret value so that you could pipe it in like command-that-gets-secret | hab origin secret AWS_SECRET. That's just freethinking though…

fnichol · 2018-03-09T18:41:59Z

components/hab/src/command/bldr/channel/destroy.rs

        .map_err(Error::DepotClient)?;

-    ui.status(Status::Deleted, format!("channel {}.", channel))?;
+    ui.status(Status::Deleted, format!("secret key {}.", key))?;


I had the same thought!

fnichol · 2018-03-09T18:44:13Z

components/hab/src/command/origin/key/download.rs

-        format!("Downloading public encryption origin key for {}", origin),
-    )?;
+    ui.begin(format!(
+        "Downloading public encryption origin key for {}",


I'm guessing this is simply rustfmt reformatting, but now that I read this message, it doesn't really make sense: origin keys aren't about encrypting anything, they're all about signing and verifying things (namely packages). I wonder if taking out the word "encryption" in the origin keys messaging might help to distinguish these 2 concepts in the future.

This function is handling the case specific to origin encryption keys (they were a new concept introduced in a previous PR)

Signed-off-by: Elliott Davis <elliott@excellent.io>

raskchanky · 2018-03-13T21:20:26Z

raskchanky · 2018-03-13T21:21:03Z

@thesentinels approve

thesentinels · 2018-03-13T21:21:05Z

🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests.

thesentinels · 2018-03-13T21:31:11Z

Travis CI has started testing this PR.

thesentinels · 2018-03-13T21:50:59Z

💖 Travis CI reports this PR passed.

It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now.

I just want you and the contributor to answer me one question:

elliott-davis requested review from baumanj, christophermaier, fnichol and reset as code owners March 5, 2018 22:35

baumanj reviewed Mar 6, 2018

View reviewed changes

elliott-davis force-pushed the elliott/encrypt branch 3 times, most recently from a343f33 to 7d08248 Compare March 8, 2018 17:22

elliott-davis force-pushed the elliott/encrypt branch 3 times, most recently from 29d8083 to a74d9d7 Compare March 8, 2018 18:36

elliott-davis added A-cli labels Mar 8, 2018

elliott-davis force-pushed the elliott/encrypt branch from a74d9d7 to b93dab4 Compare March 8, 2018 20:55

elliott-davis changed the title ~~WIP: Encrypt and upload secrets. Todo: Delete, list~~ Encrypt and upload secrets. Todo: Delete, list Mar 8, 2018

elliott-davis changed the title ~~Encrypt and upload secrets. Todo: Delete, list~~ Encrypt and upload secrets. Mar 8, 2018

elliott-davis force-pushed the elliott/encrypt branch 3 times, most recently from a928c0e to 1460bc9 Compare March 9, 2018 15:47

baumanj mentioned this pull request Mar 9, 2018

BoxKeyPair::encrypt functions return type is misleading habitat-sh/core#15

Closed

raskchanky reviewed Mar 9, 2018

View reviewed changes

fnichol reviewed Mar 9, 2018

View reviewed changes

elliott-davis force-pushed the elliott/encrypt branch 2 times, most recently from 1f6280b to 2a48e3f Compare March 9, 2018 19:49

Encrypt and upload secrets. Todo: Delete, list

fabefbe

Signed-off-by: Elliott Davis <elliott@excellent.io>

elliott-davis force-pushed the elliott/encrypt branch from 2a48e3f to fabefbe Compare March 12, 2018 18:20

raskchanky approved these changes Mar 13, 2018

View reviewed changes

thesentinels merged commit 6ce27f6 into master Mar 13, 2018

thesentinels deleted the elliott/encrypt branch March 13, 2018 21:51

fnichol removed the S-awaiting-review label Mar 14, 2018

christophermaier added Focus: CLI Related to the Habitat CLI (core/hab) component Type: Feature Issues that describe a new desired feature and removed A-cli labels Jul 24, 2020

Encrypt and upload secrets. #4706

Encrypt and upload secrets. #4706

Conversation

elliott-davis commented Mar 5, 2018 • edited Loading

thesentinels commented Mar 5, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankeairns Mar 7, 2018 • 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

Choose a reason for hiding this comment

ryankeairns Mar 7, 2018 • 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

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

elliott-davis commented Mar 8, 2018

baumanj commented Mar 8, 2018

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

raskchanky commented Mar 9, 2018

fnichol left a 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

Choose a reason for hiding this comment

raskchanky commented Mar 13, 2018

raskchanky commented Mar 13, 2018

thesentinels commented Mar 13, 2018

thesentinels commented Mar 13, 2018

thesentinels commented Mar 13, 2018

elliott-davis commented Mar 5, 2018 •

edited

Loading

ryankeairns Mar 7, 2018 •

edited

Loading

ryankeairns Mar 7, 2018 •

edited

Loading