Auth0 SSO/SAML Integration with CLI by DABH · Pull Request #245 · confluentinc/cli

David Hyde (DABH) · 2019-08-02T07:51:35Z

No description provided.

codecov · 2019-08-04T20:39:45Z

Codecov Report

❗ No coverage uploaded for pull request base (master@07450f3). Click here to learn what that means.
The diff coverage is 28%.

@@            Coverage Diff            @@
##             master     #245   +/-   ##
=========================================
  Coverage          ?   40.28%           
=========================================
  Files             ?       61           
  Lines             ?     4629           
  Branches          ?        0           
=========================================
  Hits              ?     1865           
  Misses            ?     2523           
  Partials          ?      241

Impacted Files	Coverage Δ
internal/pkg/sdk/user/user.go	`0% <0%> (ø)`
internal/cmd/local/bindata.go	`0% <0%> (ø)`
internal/cmd/auth/auth.go	`67.42% <45.16%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07450f3...d52dafd. Read the comment docs.

Cody A. Ray (codyaray)

auth code with pkce looks like the right flow for me, for now. i bet we could get a nicer "success" page, but this works for v1 anyway.

my main concerns

we're opening a small security hole here that i think our auditors will dislike, in that the new check_email endpoint allows users to query whether emails exist directly. we were already flagged for giving too much info away like this at authentication time by them once or twice. Instead, can we have the user indicate whether they want to "login via SSO" with a new command or flag?
we're exposing auth0 externally as part of our API and hardcoding a lot of API details into our CLI that shouldn't be there. We should really setup a proxy for a lot of this stuff (so instead of https://confluent.auth0.com/api/v2/authorize it looks like https://confluent.cloud/oauth/authorize). I think both Apache webserver and Nginx support this sort of proxying, or something like https://github.com/trondhindenes/Auth0Proxy (which configures apache in a docker container)

Cody A. Ray (codyaray) · 2019-08-12T13:24:56Z

+	// Check if user has an enterprise SSO connection enabled.  If so we need to start
+	// a background HTTP server to support the authorization code flow with PKCE
+	// described at https://auth0.com/docs/flows/guides/auth-code-pkce/call-api-auth-code-pkce
+	userSSO, err := client.User.CheckEmail(context.Background(), &orgv1.User{Email: email})


CheckEmail is like GetUserByEmail? And userSSO is just our normal ccloudapis User object?

This is a security hole, since it exposes an easy method for attackers to figure out which emails exist in our system without authenticating.

Yes, except all the returned User object contains (if non-empty) is the email and sso field (everything else is blanked out).

The GUI also has always had the same "vulnerability" according to when I talked to FE team. cc laura-brouckman . So this is not an additional security hole, if you'd even consider it one.

And unfortunately there is no better way to do this without compromising the UX :(

We could consider not returning the email in the returned User object from the route, that might make it a bit more secure? So it's just sso or (no sso) or (no sso because invalid)? Wdyt?

This should not make it obvious when a user exists or not.

Suggested output

(Nesting it inside "user" means it should be backwards compatible with the current GetUserReply-based output)

Exists with SSO:

200 OK HTTP/1.1 { "user": { "sso": { "enabled": true, "auth0_connection_name": "noah-okta-test" } } }

Exists without SSO:

200 OK HTTP/1.1 { "user": { "sso": { "enabled": false, "auth0_connection_name": "" } } }

Doesn't exist (identical to exists without SSO):

200 OK HTTP/1.1 { "user": { "sso": { "enabled": false, "auth0_connection_name": "" } } }

Current Behavior

Exists with SSO

{ "user": { "id": 0, "email": "david.hyde@confluent.io", "first_name": "", "last_name": "", "organization_id": 0, "deactivated": false, "verified": null, "created": null, "modified": null, "password_changed": null, "service_name": "", "service_description": "", "service_account": false, "sso": { "enabled": true, "auth0_connection_name": "noah-okta-test" } }, "account": null, "organization": null, "error": null, "accounts": [] }

Exists without SSO:

{ "user": { "id": 0, "email": "cody@confluent.io", "first_name": "", "last_name": "", "organization_id": 0, "deactivated": false, "verified": null, "created": null, "modified": null, "password_changed": null, "service_name": "", "service_description": "", "service_account": false, "sso": { "enabled": false, "auth0_connection_name": "" } }, "account": null, "organization": null, "error": null, "accounts": [] }

Not exists

{ "user": null, "account": null, "organization": null, "error": { "code": 404, "message": "User not found", "nested_errors": {}, "details": [], "stack": null }, "accounts": [] }

Yes, we will do this as a follow up PR in cc-gateway-service (should require no changes in CLI).

David Hyde (DABH) · 2019-08-13T00:42:39Z

Cody A. Ray (@codyaray) yeah I'm going to spruce up the success page a bit before merging. css!

David Hyde (DABH) · 2019-08-13T06:41:20Z

To your point #2, that URL is actually an ID, not the hostname we connect to. And I think it's provided by Auth0, I don't think we can change that. cc Eric Sirianni (@sirianni)

Cody A. Ray (codyaray) · 2019-08-13T22:55:56Z

+	// Check if user has an enterprise SSO connection enabled.  If so we need to start
+	// a background HTTP server to support the authorization code flow with PKCE
+	// described at https://auth0.com/docs/flows/guides/auth-code-pkce/call-api-auth-code-pkce
+	userSSO, err := client.User.CheckEmail(context.Background(), &orgv1.User{Email: email})


This should not make it obvious when a user exists or not.

Suggested output

(Nesting it inside "user" means it should be backwards compatible with the current GetUserReply-based output)

Exists with SSO:

200 OK HTTP/1.1 { "user": { "sso": { "enabled": true, "auth0_connection_name": "noah-okta-test" } } }

Exists without SSO:

200 OK HTTP/1.1 { "user": { "sso": { "enabled": false, "auth0_connection_name": "" } } }

Doesn't exist (identical to exists without SSO):

200 OK HTTP/1.1 { "user": { "sso": { "enabled": false, "auth0_connection_name": "" } } }

Current Behavior

Exists with SSO

{ "user": { "id": 0, "email": "david.hyde@confluent.io", "first_name": "", "last_name": "", "organization_id": 0, "deactivated": false, "verified": null, "created": null, "modified": null, "password_changed": null, "service_name": "", "service_description": "", "service_account": false, "sso": { "enabled": true, "auth0_connection_name": "noah-okta-test" } }, "account": null, "organization": null, "error": null, "accounts": [] }

Exists without SSO:

{ "user": { "id": 0, "email": "cody@confluent.io", "first_name": "", "last_name": "", "organization_id": 0, "deactivated": false, "verified": null, "created": null, "modified": null, "password_changed": null, "service_name": "", "service_description": "", "service_account": false, "sso": { "enabled": false, "auth0_connection_name": "" } }, "account": null, "organization": null, "error": null, "accounts": [] }

Not exists

{ "user": null, "account": null, "organization": null, "error": { "code": 404, "message": "User not found", "nested_errors": {}, "details": [], "stack": null }, "accounts": [] }

Cody A. Ray (codyaray)

aside from my earlier comments lgtm

you're going to update the backend check_email so it doesn't show if user/email exists or not
you're adding a prettier/auto-closing page in the CLI's http server
we're stuck with the auth0 identifier
you'll update the Auth0ConnectionName in cc-structs and json field, but will have to deprecate it at this point and wait until we know no one would be using it to remove it.

Eric Sirianni (sirianni) · 2019-08-14T14:08:57Z

4. you'll update the Auth0ConnectionName in cc-structs and json field, but will have to deprecate it at this point and wait until we know no one would be using it to remove it.

I'm not following this one ☝️

David Hyde (DABH) · 2019-08-16T17:41:15Z

Here is what the browser callback screen looks like now. Shout out to @rob.ashe for the logo.

Eric Sirianni (sirianni) · 2019-08-16T18:44:05Z

Here is what the browser callback screen looks like now. Shout out to @rob.ashe for the logo.

💥

You may have a future on the frontend team! 😛

Auth0 Authorization Code flow working so far...

2b6177d

David Hyde (DABH) requested a review from a team as a code owner August 2, 2019 07:51

use connection name

2b4ff36

Eric Sirianni (sirianni) reviewed Aug 2, 2019

View reviewed changes

Comment thread internal/pkg/auth-server/auth-server.go Outdated

Probably works now, if backend fixes get merged in

9f2b0cc

David Hyde (DABH) changed the title ~~[WIP] Auth0 SSO/SAML Integration with CLI~~ Auth0 SSO/SAML Integration with CLI Aug 4, 2019

David Hyde (DABH) requested review from laura-brouckman and Eric Sirianni (sirianni) August 4, 2019 07:00

David Hyde (DABH) added 3 commits August 4, 2019 00:20

I broke the tests, need to mock CheckEmail...

215661c

fix tests

a7404da

bump sdk

a517e84

David Hyde (DABH) added 2 commits August 4, 2019 19:51

sdk pr merged

8913402

Merge branch 'master' into auth0-sso

ac72d53

Cody A. Ray (codyaray) reviewed Aug 12, 2019

View reviewed changes

David Hyde (DABH) added 8 commits August 12, 2019 17:47

address pr comment; use 1.12.7 in ci, or try to

6da1559

Merge branch 'auth0-sso' of github.com:confluentinc/cli into auth0-sso

dbcd542

merge

7537597

bug fix...

d4883e2

sad semaphore

e53158a

clean up / pr feedback

4113aaf

err check

c27c048

refactor internal env detection code

031c30d

David Hyde (DABH) added 2 commits August 13, 2019 15:32

Address pr feedback

4a04c77

fix more names

f9b9d82

Cody A. Ray (codyaray) reviewed Aug 13, 2019

View reviewed changes

add comment

39e248e

Cody A. Ray (codyaray) approved these changes Aug 13, 2019

View reviewed changes

Cody A. Ray (codyaray) reviewed Aug 14, 2019

View reviewed changes

Comment thread internal/pkg/auth-server/auth-server.go Outdated

David Hyde (DABH) added 5 commits August 15, 2019 16:04

WIP Spicing up the callback page, do proper CSRF validation

c21ae5b

merge

5af6558

Clean up HTML, go mod tidy, etc.

e7def3d

cover more cases

076afee

clean up html

2112f6e

update prod client id

8e1f218

dev and stag changed too...

d52dafd

David Hyde (DABH) merged commit bd51433 into master Aug 16, 2019

David Hyde (DABH) deleted the auth0-sso branch August 16, 2019 19:36

Uh oh!

Conversation

David Hyde (DABH) commented Aug 2, 2019

Uh oh!

Uh oh!

codecov Bot commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Cody A. Ray (codyaray) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Cody A. Ray (codyaray) Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

David Hyde (DABH) Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

Cody A. Ray (codyaray) Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Suggested output

Current Behavior

Uh oh!

David Hyde (DABH) Aug 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

David Hyde (DABH) commented Aug 13, 2019

Uh oh!

David Hyde (DABH) commented Aug 13, 2019

Uh oh!

Uh oh!

Cody A. Ray (codyaray) Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Suggested output

Current Behavior

Uh oh!

Uh oh!

Uh oh!

Cody A. Ray (codyaray) left a comment

Choose a reason for hiding this comment

Uh oh!

Eric Sirianni (sirianni) commented Aug 14, 2019

Uh oh!

Uh oh!

David Hyde (DABH) commented Aug 16, 2019

Uh oh!

Eric Sirianni (sirianni) commented Aug 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Aug 4, 2019 •

edited

Loading

Cody A. Ray (codyaray) Aug 13, 2019 •

edited

Loading

Cody A. Ray (codyaray) Aug 13, 2019 •

edited

Loading