Issue #121: BasicAuth REST API #147

dgrisham · 2017-09-07T21:58:17Z

@hsanjuan Here's a start to the BasicAuth implementation. Todos:

Server-side
- Default to HTTPS when BasicAuth active
- Better response to bad credentials.
CLI flags for server
- (De?)activate BasicAuth
- Force BasicAuth w/ HTTP (HTTPS by default)
Client-side
- BasicAuth impl
- -c <username>:<password> flag
- Env var input for username/password
  - CLUSTER_CREDENTIALS="<username>:<password>"
- Default to HTTPS with BasicAuth (flag to override) -- this mirrors the server behavior
Tests
- Sharness

coveralls · 2017-09-07T23:15:18Z

Coverage decreased (-0.1%) to 73.707% when pulling b010c40 on feat/api-basic-auth into af8a385 on master.

hsanjuan

Hey, looks very good so far!

hsanjuan · 2017-09-11T15:01:43Z

api/restapi/restapi.go

-func NewTLSRESTAPI(apiMAddr ma.Multiaddr, tlsCfg *tls.Config) (*RESTAPI, error) {
+// NewRESTAPI creates a new REST API component. It receives
+// the multiaddress on which the API listens.
+func NewRESTAPIWithConfig(apiMAddr ma.Multiaddr, cfg *Config) (*RESTAPI, error) {


Should include the apiMaddr as part of the configuration I guess

Sounds good -- the only difference that I see so far is that the apiMAddr is required, while everything in the cfg is optional. Not sure if that difference matters much, but for now I'm combining them into a single config (so we'll have NewRESTAPI(cfg *Config)).

hsanjuan · 2017-09-11T15:02:05Z

api/restapi/restapi.go

@@ -56,6 +56,11 @@ type RESTAPI struct {
 	wg           sync.WaitGroup
 }

+type Config struct {


Don't forget to document public types

dgrisham · 2017-10-04T21:04:22Z

@hsanjuan @ZenGround0 Just made a push, the current state should be consistent with the Todo list in the OP.

dgrisham · 2017-10-07T21:24:50Z

sharness/t0042-basic-auth.sh

+
+test_expect_success "BasicAuth fails without credentials" '
+    id=`cluster_id`
+    ipfs-cluster-ctl id | grep -q Unauthorized


I'm not convinced checking for 'Unauthorized' in the response is the best approach to this -- e.g. we could accidentally be leaking info in some case but the presence of the word 'Unauthorized' would allow this test to pass anyway.

@dgrisham check that the command did not return 0 for exit code and then check for Unauthorized (should be ok for the moment).

dgrisham · 2017-10-09T15:18:58Z

@hsanjuan Two questions on the remaining Todos I have listed:

Config file to hold cluster-ctl config -- Should I leave that to Configuration file with sections per component #162 (or some other PR) and just support CLI/env var input for now?
Go tests -- sharness seems sufficient/the best way to test this (think we did the same for TLS), but lmk if you think we need Go tests here too

coveralls · 2017-10-09T15:43:52Z

Coverage decreased (-0.4%) to 73.409% when pulling ca6cf9a on feat/api-basic-auth into 72d0d22 on master.

hsanjuan · 2017-10-10T08:04:36Z

ipfs-cluster-ctl/main.go

@@ -105,11 +107,30 @@ func main() {
 			Name:  "debug, d",
 			Usage: "set debug log level",
 		},
+		cli.StringFlag{
+			Name: "credentials, c",
+			Usage: "specify BasicAuth credentials for server that requires " +


Would need a hint that they are passed as "user:password"

hsanjuan · 2017-10-10T08:06:50Z

ipfs-cluster-ctl/main.go

@@ -105,11 +107,30 @@ func main() {
 			Name:  "debug, d",
 			Usage: "set debug log level",
 		},
+		cli.StringFlag{
+			Name: "credentials, c",


perhaps rename to --basic-auth (or even -u, --user USER[:PASSWORD] Server user and password like curl does) . Not sure if it deserves a short-hand alias. I'd like to keep -c for config (in line with server).

Hm, so right now (I think) an empty password would work by passing --basic-auth <username>: (note the : at the end) but you'd get an error if you just passed --basic-auth <username> (no :). Might be nice to support the latter, I'll make that change.

sounds good

hsanjuan · 2017-10-10T08:11:20Z

Seems sharness is complaining too. But other than that, should be good to go

hsanjuan · 2017-10-10T08:12:19Z

Config file to hold cluster-ctl config -- Should I leave that to #162 (or some other PR) and just support CLI/env var input for now?

Yes, let's leave it like that for the moment

Go tests -- sharness seems sufficient/the best way to test this (think we did the same for TLS), but lmk if you think we need Go tests here too

Sharness is ok

hsanjuan · 2017-10-11T12:46:24Z

Implements #121

ZenGround0 · 2017-10-12T13:42:41Z

@dgrisham I had the same Travis problem yesterday. Rebasing on top of master should do the trick.

hsanjuan · 2017-10-12T14:46:24Z

ipfs-cluster-ctl/main.go

 	client := &http.Client{Transport: defaultTransport}
 	resp, err := client.Do(r)
+	if err == nil && resp.StatusCode >= 400 {


Now that this has been brought to my attention :)

I think the best place to detect error and set exit code in such case is the end of formatResponse. This ensures the received data/error is shown to the user in the desired format. Only then you can check resp.StatusCode and exit with 2 when it was not a successful request.

Gotcha, yeah I was definitely planning on discussing/correcting this approach. And out of curiosity, why exit code 2 specifically? Is there a good rule of thumb for the first few exit codes that I can keep in mind?

Nah, I said 2 because 1 is used for the rest of errors, so just to use a different one for errors which are from responses. It's completely arbitrary...

Gotcha, good to know. We should document that somewhere for any users who might want to distinguish between to the two.

dgrisham · 2017-10-14T19:31:08Z

@ZenGround0 Ah, dope, thanks for the tip :)

coveralls · 2017-10-14T19:52:22Z

Coverage decreased (-0.4%) to 73.494% when pulling 2ac98dd on feat/api-basic-auth into da0cb5e on master.

coveralls · 2017-10-14T22:14:29Z

Coverage decreased (-0.4%) to 73.518% when pulling 25a910f on feat/api-basic-auth into da0cb5e on master.

dgrisham · 2017-10-14T22:30:00Z

@hsanjuan Tests passing! Let me know if there are any other changes you think I should make.

dgrisham · 2017-10-15T20:35:17Z

@hsanjuan Don't merge this quite yet, there are a couple of sharness tests I want to add first.

hsanjuan

awesome thanks!

hsanjuan reviewed Sep 11, 2017

View reviewed changes

dgrisham force-pushed the feat/api-basic-auth branch 2 times, most recently from 2bf672a to 9d000a1 Compare October 7, 2017 21:23

dgrisham commented Oct 7, 2017

View reviewed changes

dgrisham force-pushed the feat/api-basic-auth branch from 9d000a1 to ca6cf9a Compare October 9, 2017 15:24

hsanjuan requested changes Oct 10, 2017

View reviewed changes

hsanjuan mentioned this pull request Oct 10, 2017

Support basic auth in REST API endpoints #121

Closed

hsanjuan added the status/in-progress In progress label Oct 11, 2017

hsanjuan assigned dgrisham Oct 11, 2017

hsanjuan changed the title ~~BasicAuth REST API~~ Issue #121: BasicAuth REST API Oct 11, 2017

dgrisham force-pushed the feat/api-basic-auth branch 2 times, most recently from b38b25a to b36b3db Compare October 12, 2017 13:22

dgrisham mentioned this pull request Oct 12, 2017

Missing exit status when command errors #174

Closed

hsanjuan reviewed Oct 12, 2017

View reviewed changes

dgrisham force-pushed the feat/api-basic-auth branch 2 times, most recently from 6f320de to 2ac98dd Compare October 14, 2017 19:35

BasicAuth implementation -- CLI, server, and tests.

25a910f

dgrisham force-pushed the feat/api-basic-auth branch from 2ac98dd to 25a910f Compare October 14, 2017 21:55

hsanjuan approved these changes Oct 16, 2017

View reviewed changes

hsanjuan merged commit 6e40610 into master Oct 16, 2017

ghost removed the status/in-progress In progress label Oct 16, 2017

hsanjuan deleted the feat/api-basic-auth branch October 16, 2017 11:22

hsanjuan mentioned this pull request Oct 17, 2017

Release v0.2.0 #180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #121: BasicAuth REST API #147

Issue #121: BasicAuth REST API #147

dgrisham commented Sep 7, 2017 •

edited

coveralls commented Sep 7, 2017

hsanjuan left a comment

hsanjuan Sep 11, 2017

dgrisham Sep 20, 2017 •

edited

hsanjuan Sep 11, 2017

dgrisham commented Oct 4, 2017

dgrisham Oct 7, 2017

dgrisham Oct 7, 2017

hsanjuan Oct 10, 2017

dgrisham commented Oct 9, 2017 •

edited

coveralls commented Oct 9, 2017

hsanjuan Oct 10, 2017

hsanjuan Oct 10, 2017

dgrisham Oct 11, 2017 •

edited

hsanjuan Oct 11, 2017

hsanjuan commented Oct 10, 2017

hsanjuan commented Oct 10, 2017

hsanjuan commented Oct 11, 2017 •

edited

ZenGround0 commented Oct 12, 2017

hsanjuan Oct 12, 2017

dgrisham Oct 14, 2017 •

edited

hsanjuan Oct 15, 2017

dgrisham Oct 15, 2017

dgrisham commented Oct 14, 2017

coveralls commented Oct 14, 2017

coveralls commented Oct 14, 2017

dgrisham commented Oct 14, 2017

dgrisham commented Oct 15, 2017

hsanjuan left a comment

Issue #121: BasicAuth REST API #147

Issue #121: BasicAuth REST API #147

Conversation

dgrisham commented Sep 7, 2017 • edited

coveralls commented Sep 7, 2017

hsanjuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrisham Sep 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrisham commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrisham commented Oct 9, 2017 • edited

coveralls commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrisham Oct 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsanjuan commented Oct 10, 2017

hsanjuan commented Oct 10, 2017

hsanjuan commented Oct 11, 2017 • edited

ZenGround0 commented Oct 12, 2017

Choose a reason for hiding this comment

dgrisham Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrisham commented Oct 14, 2017

coveralls commented Oct 14, 2017

coveralls commented Oct 14, 2017

dgrisham commented Oct 14, 2017

dgrisham commented Oct 15, 2017

hsanjuan left a comment

Choose a reason for hiding this comment

dgrisham commented Sep 7, 2017 •

edited

dgrisham Sep 20, 2017 •

edited

dgrisham commented Oct 9, 2017 •

edited

dgrisham Oct 11, 2017 •

edited

hsanjuan commented Oct 11, 2017 •

edited

dgrisham Oct 14, 2017 •

edited