`hab cli` api/depot authentication #708

reset · 2016-06-10T01:13:30Z

We now honor the HAB_AUTH_TOKEN env variable and --auth/-z flag when calling CLI functions that require authentication. We currently only protect origin key upload and package upload.

This also contains an improvement to SessionSrv which will perform a session create if a session get is a cache miss. To accomplish this SessionSrv needed to be able to speak to GitHub's API so the oauth module has moved from the builder-api crate to the net crate

thesentinels · 2016-06-10T01:13:32Z

By analyzing the blame information on this pull request, we identified @metadave, @fnichol, @adamhjk, @smith and @seth to be potential reviewers

adamhjk · 2016-06-10T01:15:14Z

components/builder-api/Cargo.lock

@@ -54,7 +54,7 @@ dependencies = [
 "iron 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
 "persistent 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
 "plugin 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)",
- "serde 0.7.7 (registry+https://github.com/rust-lang/crates.io-index)",
+ "serde 0.7.8 (registry+https://github.com/rust-lang/crates.io-index)",


We're not using the parts of serde that require nightly rust, right?

Nope! I don't think we actually even use serde, whatever crate is pulling this in just probably doesn't have/we didn't use the feature flag to disable serde

adamhjk · 2016-06-10T01:15:53Z

components/builder-api/Cargo.lock

 "libc 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)",
 "log 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)",
 "protobuf 1.0.21 (registry+https://github.com/rust-lang/crates.io-index)",
+ "rustc-serialize 0.3.19 (registry+https://github.com/rust-lang/crates.io-index)",


Both rustc-serialize and serde is probably overkill.

I just checked - Unfortunately I can't get rid of the Serde dependency here because the bodyparser doesn't expose a feature flag to disable Serde and it introduces it as a dependency

adamhjk · 2016-06-10T01:19:26Z

I haven't run or tested this code at all; but it all looks sane. :)

No nobody will need to ask:

Signed-off-by: Jamie Winsor <[email protected]>

bookshelfdave · 2016-06-10T01:37:55Z

components/depot/src/server.rs

+        }
+        Err(e @ hab_net::Error::JsonDecode(_)) => {
+            debug!("github user get, err={:?}", e);
+            let err = net::err(ErrCode::BAD_REMOTE_REPLY, "rg:auth:1");


is rg: the correct prefix here?

On SessionGet if the session is expired a new session will now be created Signed-off-by: Jamie Winsor <[email protected]>

bookshelfdave · 2016-06-10T01:43:00Z

so good.

fnichol · 2016-06-10T02:18:44Z

components/builder-protocol/src/message/net.rs

+    0x00, 0x02, 0x00, 0x04, 0x12, 0x03, 0x0b, 0x02, 0x0a, 0x0a, 0x0c, 0x0a, 0x05, 0x04, 0x00, 0x02,
+    0x00, 0x06, 0x12, 0x03, 0x0b, 0x0b, 0x13, 0x0a, 0x0c, 0x0a, 0x05, 0x04, 0x00, 0x02, 0x00, 0x01,
+    0x12, 0x03, 0x0b, 0x14, 0x1c, 0x0a, 0x0c, 0x0a, 0x05, 0x04, 0x00, 0x02, 0x00, 0x03, 0x12, 0x03,
+    0x0b, 0x1f, 0x20, 0x0a, 0x0b, 0x0a, 0x04, 0x04, 0x00, 0x02, 0x01, 0x12, 0x03, 0x0c, 0x02, 0x1b,


Typo here, should read "0x0a, 0x0b, L0LH3XC0DE"

fnichol · 2016-06-10T02:25:37Z

@reset Wow, I'm impressed, sliding that into the CLI was far less effort than I thought. Glad to see that depot-client is doing all the heavy lifting (which frankly isn't tons).

One question came to mind: is it ever appropriate to serialize the bearer token to disk for repeated use over a time period? What I'm wondering is: could/should this be read from disk if we were to ask for a token in hab cli setup?

fnichol · 2016-06-10T02:26:30Z

I am merge 👍 on this stuff!

reset · 2016-06-10T02:37:43Z

@fnichol totally! If you'd like to have a part of setup where it displays:

## Generate auth token
We need you to generate an authentication token from GitHub to validate that you, are you.

1. Login to GitHub
1. Go to your personal access tokens: https://github.com/settings/tokens
1. Generate a new token
  a. Check the `user:email` box
1. Copy the newly generated personal access token

[press enter to continue]

GitHub personal access token: <enter token>
[press enter to continue]

We could then write this value into a settings file on disk at ~./hab/config.toml. After that you've just got to update the CLI to read from that file.

fnichol · 2016-06-10T02:59:53Z

@reset I could probably turn something around here. Wouldn't impact moving this through though.

fnichol · 2016-06-10T03:01:02Z

@reset Do we need to wait on merging (pending some other PR), or can I do the merge dance?

reset · 2016-06-10T03:02:20Z

@fnichol if you're going to build on top of this why don't you go ahead and merge it. I'll accept a review from Dave tomorrow on the merged PR and fix/add whatever he wants. Sound good?

fnichol · 2016-06-10T03:07:02Z

Works for me

@thesentinels r+

thesentinels · 2016-06-10T03:07:03Z

📌 Commit a3416df has been approved by fnichol

thesentinels · 2016-06-10T03:10:12Z

⌛ Testing commit a3416df with merge 4ddc698...

Signed-off-by: Jamie Winsor <[email protected]> Pull request: #708 Approved by: fnichol

On SessionGet if the session is expired a new session will now be created Signed-off-by: Jamie Winsor <[email protected]> Pull request: #708 Approved by: fnichol

thesentinels · 2016-06-10T03:23:53Z

☀️ Test successful - travis

Signed-off-by: Jamie Winsor <[email protected]> Pull request: #708 Approved by: fnichol

On SessionGet if the session is expired a new session will now be created Signed-off-by: Jamie Winsor <[email protected]> Pull request: #708 Approved by: fnichol

adamhjk reviewed Jun 10, 2016
View reviewed changes

reset force-pushed the cli-auth branch from 4d021ed to 681f2a9 Compare June 10, 2016 01:15

adamhjk reviewed Jun 10, 2016
View reviewed changes

Accept authorization token on package/key upload

dcf2566

Signed-off-by: Jamie Winsor <[email protected]>

reset force-pushed the cli-auth branch from 681f2a9 to 84f71a9 Compare June 10, 2016 01:31

bookshelfdave reviewed Jun 10, 2016
View reviewed changes

Improve SessionGet into SessionGetOrCreate

a3416df

On SessionGet if the session is expired a new session will now be created Signed-off-by: Jamie Winsor <[email protected]>

reset force-pushed the cli-auth branch from 84f71a9 to a3416df Compare June 10, 2016 01:40

fnichol reviewed Jun 10, 2016
View reviewed changes

thesentinels pushed a commit that referenced this pull request Jun 10, 2016

Accept authorization token on package/key upload

09c725d

Signed-off-by: Jamie Winsor <[email protected]> Pull request: #708 Approved by: fnichol

thesentinels pushed a commit that referenced this pull request Jun 10, 2016

Improve SessionGet into SessionGetOrCreate

4ddc698

On SessionGet if the session is expired a new session will now be created Signed-off-by: Jamie Winsor <[email protected]> Pull request: #708 Approved by: fnichol

thesentinels merged commit a3416df into master Jun 10, 2016

fnichol deleted the cli-auth branch June 10, 2016 04:31

jtimberman pushed a commit that referenced this pull request Jun 12, 2016

Accept authorization token on package/key upload

f31c621

Signed-off-by: Jamie Winsor <[email protected]> Pull request: #708 Approved by: fnichol

jtimberman pushed a commit that referenced this pull request Jun 12, 2016

Improve SessionGet into SessionGetOrCreate

4e0a85e

On SessionGet if the session is expired a new session will now be created Signed-off-by: Jamie Winsor <[email protected]> Pull request: #708 Approved by: fnichol

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`hab cli` api/depot authentication #708

`hab cli` api/depot authentication #708

reset commented Jun 10, 2016

thesentinels commented Jun 10, 2016

adamhjk Jun 10, 2016

reset Jun 10, 2016

adamhjk Jun 10, 2016

reset Jun 10, 2016

adamhjk commented Jun 10, 2016

bookshelfdave Jun 10, 2016

bookshelfdave commented Jun 10, 2016

fnichol Jun 10, 2016

fnichol commented Jun 10, 2016

fnichol commented Jun 10, 2016

reset commented Jun 10, 2016

fnichol commented Jun 10, 2016

fnichol commented Jun 10, 2016

reset commented Jun 10, 2016

fnichol commented Jun 10, 2016

thesentinels commented Jun 10, 2016

thesentinels commented Jun 10, 2016

thesentinels commented Jun 10, 2016

hab cli api/depot authentication #708

hab cli api/depot authentication #708

Conversation

reset commented Jun 10, 2016

thesentinels commented Jun 10, 2016

adamhjk Jun 10, 2016

Choose a reason for hiding this comment

reset Jun 10, 2016

Choose a reason for hiding this comment

adamhjk Jun 10, 2016

Choose a reason for hiding this comment

reset Jun 10, 2016

Choose a reason for hiding this comment

adamhjk commented Jun 10, 2016

bookshelfdave Jun 10, 2016

Choose a reason for hiding this comment

bookshelfdave commented Jun 10, 2016

fnichol Jun 10, 2016

Choose a reason for hiding this comment

fnichol commented Jun 10, 2016

fnichol commented Jun 10, 2016

reset commented Jun 10, 2016

fnichol commented Jun 10, 2016

fnichol commented Jun 10, 2016

reset commented Jun 10, 2016

fnichol commented Jun 10, 2016

thesentinels commented Jun 10, 2016

thesentinels commented Jun 10, 2016

thesentinels commented Jun 10, 2016

`hab cli` api/depot authentication #708

`hab cli` api/depot authentication #708