Introduce authentication to the broker #308

jmrodri · 2017-07-27T13:56:29Z

Currently supports just basic auth but it's structured to allow extension for additional auth backends.

shawn-hurley

I really like the structure mostly style nits but one or two functional things that I would like to discuss

shawn-hurley · 2017-07-27T14:23:46Z

pkg/app/config_test.go

+	ft "github.com/openshift/ansible-service-broker/pkg/fusortest"
+)
+
+func TestCreateConfig(t *testing.T) {


shawn-hurley · 2017-07-27T14:29:26Z

pkg/auth/auth.go

+	return &fusa, nil
+}
+
+func (d *FileUserServiceAdapter) buildDb() error {


buildDB() - because Db is an acronym, it should be all uppercase or lowercase.

shawn-hurley · 2017-07-27T14:31:05Z

pkg/auth/auth.go

+	// userdb is probably overkill, but if we ever want to allow multiple users,
+	// it'll come in handy.
+	d.userdb = make(map[string]User)
+	unamestr := string(username)


can you just cast to string in struct literal? you are for password so would just like to match style IMO.

shawn-hurley · 2017-07-27T14:33:08Z

pkg/auth/auth.go

+
+// FindByLogin - given a login name, this will return the associated User or an
+// error
+func (d FileUserServiceAdapter) FindByLogin(login string) (User, error) {


Should this be unexported?

shawn-hurley · 2017-07-27T14:34:10Z

pkg/auth/auth.go

+// our backend storage. Returns fals otherwise.
+func (d FileUserServiceAdapter) ValidateUser(username string, password string) bool {
+	user, err := d.FindByLogin(username)
+	if err != nil {


Can we log the error here, the user not found error above will be lost and not printed out. Could make it hard to debug.

Probably, I don't have the log in these structs so I removed the fmt I had. Definitely will log though. Thanks.

shawn-hurley · 2017-07-27T14:49:45Z

pkg/auth/auth.go

+// Handler - does the authentication for the routes
+func Handler(h http.Handler, providers []Provider) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// TODO: loop through the providers


Remove todos?

shawn-hurley · 2017-07-27T14:53:39Z

pkg/auth/basic_auth.go

+			fmt.Println(err.Error())
+		}
+		userpass := strings.Split(string(decodedheader), ":")
+		username = userpass[0]


Should we check this if I pass hearder Authorization Basic I think this will panic. just thinking out loud

So I tried quite a few things in tests, all are Authorization Basic with different encoded values:

just the username "admin"

just the password ":password"

empty string

all are handled correctly no panic on this line. strings.Split always returns a slice of at least one item even if that item is empty string.

shawn-hurley · 2017-07-27T14:55:22Z

pkg/handler/handler.go

@@ -84,6 +85,7 @@ func (h handler) bootstrap(w http.ResponseWriter, r *http.Request, params map[st
 }

 func (h handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	fmt.Println("entered handler.ServeHTTP")


debug statement

rthallisey · 2017-07-27T20:33:04Z

@jmrodri Can you try rebasing to see if it makes travis happier?

jmrodri · 2017-08-03T03:10:14Z

@shawn-hurley authHandler now returns json.

$ curl -k -X GET -u admin:admin https://asb-1338-ansible-service-broker.172.17.0.1.nip.io/v2/catalog
{
  "description": "invalid credentials"
}

jmrodri · 2017-08-03T04:51:30Z

TEST

First build a new broker image for testing. NOTE: make run won't work. There's a trello card and an issue for it.

checkout branch

git checkout authimpl

build broker

make build

build image

make build-image ORG=jmrodri TAG=authimpl

push image

docker push docker.io/jmrodri/ansible-service-broker:authimpl

Next, using the authimpl branch of catasb: fusor/catasb#105 this PR contains the broker configuration secret when creating the broker instance on the service-catalog.

Configure catasb to read the deployment template FROM YOUR LOCAL BROKER REPO
change asb_template_url in ansible/group_vars/all.yml b/ansible/group_vars/all.yml

-asb_template_url: https://raw.githubusercontent.com/openshift/ansible-service-broker/master/templates/deploy-ansible-service-broker.template.yaml
+asb_template_url: file:///home/jesusr/dev/src/github.com/openshift/ansible-service-broker/templates/deploy-ansible-service-broker.template.yaml

Configure catasb to use YOUR newly built docker images from step 4 above. Set broker_tag to authimpl and set broker_image_name to docker.io/jmrodri/ansible-service-broker. Be sure to use the appropriate values you used when building your own images.

-broker_tag: "latest"
+broker_tag: "authimpl"
 
-broker_image_name: docker.io/ansibleplaybookbundle/ansible-service-broker
+broker_image_name: docker.io/jmrodri/ansible-service-broker

Run catasb stock or with coalmine enabled.
Things should startup normally. To verify auth is being used look at the asb logs for some debug logs:

^[[36m[2017-08-02T18:11:29.42Z] [DEBUG] user found, returning true^[[0m
^[[36m[2017-08-02T18:11:29.42Z] [DEBUG] We found one. HOORAY!^[[0m

jmrodri · 2017-08-03T04:52:06Z

TROUBLESHOOTING

If you see no route to host in the controller-manager logs when talking to the broker, check your firewall!

jwmatthews · 2017-08-03T09:49:10Z

templates/deploy-ansible-service-broker.template.yaml

+    name: asb-auth-secret
+    namespace: ansible-service-broker
+  data:
+    username: YnJva2VyLWFkbWlu


Let's introduce 2 params for the auth username/password in the parameters section

BASIC_AUTH_USER_NAME=YnJva2VyLWFkbWlu
BASIC_AUTH_USER_PASSWORD=TFl0SU5lc0hBZw==

Then refer to them here so it's easier to adjust the username/password on deploying.

jwmatthews

Let's add params for the basic auth username/password to the template.

rthallisey

@jmrodri Just like in fusor/catasb#105 , we need to add the auth info to the CI broker template: https://github.com/openshift/ansible-service-broker/blob/master/scripts/broker-ci/broker-resource.yaml .

rthallisey · 2017-08-03T12:17:01Z

pkg/auth/auth.go

+
+// GetType - returns the type of Principal. This is a user principal.
+func (u User) GetType() string {
+	return "user"


Should this be const variable instead?

rthallisey · 2017-08-03T12:20:09Z

pkg/auth/auth.go

+
+// FileUserServiceAdapter - a file based UserServiceAdapter which seeds its
+// users from a file.
+type FileUserServiceAdapter struct {


NIT: Can you organize the structs at the top of the file or add them all to a type.go?

I prefer to keep the methods of the struct near the struct definition. What I did do was move the NewFileUser ctor method further down so that the structs methods are below the struct.

shawn-hurley · 2017-08-03T12:49:37Z

ACK

* make auth look at /var/run/asb-auth * add /var/run/asb-auth, might be able to delete * use proper secret name

eriknelson

Looks good! I like the interfaces and tests. One update would be the template params @jwmatthews mentioned.

eriknelson · 2017-08-03T15:25:35Z

etc/example-config.yaml

+  auth:
+    - type: basic
+      enabled: true
+    - type: oauth


Spoke with @jmrodri, comment out or delete, up to you.

deleted the oauth lines.

eriknelson · 2017-08-03T17:04:44Z

pkg/auth/auth.go

+)
+
+// ConfigEntry - Configuration for authentication
+type ConfigEntry struct {


Think the other packages take the pattern of naming this Config for each section, so it's referenced as Config within the package, and auth.Config outside.

I was conflicted about this because it is a list of auth definitions.

auth: - type: basic enabled: true - type: oauth enabled: true - type: cert enabled: true

So each type is a config entry. I guess we could say it's an array of auth.Config.

* remove fmt.println * warn potential users of echo

jmrodri · 2017-08-04T02:57:07Z

@jwmatthews variables added to template.

jmrodri · 2017-08-04T03:10:48Z

@rthallisey CI build fixed. I was missing the secret but while I copied that in I put in the wrong broker url I had asb-1337... instead of asb-1338... We're green now!

jmrodri · 2017-08-04T03:11:03Z

@eriknelson retest please.

shawn-hurley · 2017-08-04T11:50:47Z

pkg/auth/auth.go

+// an error
+func (d FileUserServiceAdapter) FindByLogin(login string) (User, error) {
+
+	// TODO: add some error checking


Did we make a card for this?

eriknelson · 2017-08-04T12:45:05Z

@jmrodri All my tests passed. Started up catasb, saw authenticated catalog requests coming from the service-catalog. Checked curl without auth fails, checked curl with bad credentials fails, checked curl with correct credentials succeeds, and verified I could disable auth editing the config value.

Think this is ready to merge unless anyone objects.

eriknelson · 2017-08-04T12:48:10Z

Merging since complaints have been addressed, CI is working, and a bunch of people have tested this back to front.

jmrodri requested review from eriknelson and shawn-hurley July 27, 2017 14:48

shawn-hurley requested changes Jul 27, 2017

View reviewed changes

jmrodri force-pushed the authimpl branch from 8811463 to d051c3f Compare July 29, 2017 02:01

shawn-hurley approved these changes Aug 1, 2017

View reviewed changes

jmrodri force-pushed the authimpl branch 2 times, most recently from 9522f0c to 16484df Compare August 2, 2017 21:17

jmrodri mentioned this pull request Aug 3, 2017

configure broker with auth secret fusor/catasb#105

Merged

jmrodri changed the title ~~[WIP] Introduce authentication to the broker~~ Introduce authentication to the broker Aug 3, 2017

jmrodri requested a review from rthallisey August 3, 2017 04:55

jwmatthews reviewed Aug 3, 2017

View reviewed changes

jwmatthews requested changes Aug 3, 2017

View reviewed changes

rthallisey suggested changes Aug 3, 2017

View reviewed changes

jmrodri force-pushed the authimpl branch from 323b388 to 27ce92b Compare August 3, 2017 14:21

jmrodri added 11 commits August 3, 2017 10:24

initial auth implementation

3f242fe

basic auth

cf4a528

add auth filter

05dc700

hooking things up to the handler

f5cbd53

actually validate the user with the service adapter

7fdedae

add comments, rename AuthProvider -> Provider.

1509b8a

return 401 if unauthenticated

68b6005

put AuthHandler in auth pkg. Add tests

18c1a8f

add config test

77caa2d

link up providers & loop through the providers

6fa5bef

lint: rename AuthEntry to avoid stutter and other errors

02a53b2

jmrodri added 7 commits August 3, 2017 10:25

add secret and volume to template

ae998e2

* make auth look at /var/run/asb-auth * add /var/run/asb-auth, might be able to delete * use proper secret name

configure basic auth on deployed brokers

1b231dc

add logging and address review comments

1f69627

add more tests to protect against invalid headers

6c86956

use BasicAuth method from request, thx Shawn

bd21311

move auth.Handler to handler pkg

c642139

log message stating we're using auth

d8d0d21

jmrodri force-pushed the authimpl branch from 27ce92b to d8d0d21 Compare August 3, 2017 14:27

add secret to broker instance in service catalog

08d45e5

eriknelson suggested changes Aug 3, 2017

View reviewed changes

jmrodri mentioned this pull request Aug 3, 2017

configure broker with auth secret fusor/catasb#107

Merged

jmrodri added 6 commits August 3, 2017 16:35

parameterize broker username & password

3d52afc

readd image pull policy

52ba4fd

fix default auth value, newlines matter

6db0c7e

* remove fmt.println * warn potential users of echo

address review comments

a09de4f

change ConfigEntry -> Config

b82e2ed

helps to use the proper url

d8a329e

shawn-hurley reviewed Aug 4, 2017

View reviewed changes

eriknelson merged commit fdbf722 into master Aug 4, 2017

eriknelson deleted the authimpl branch August 4, 2017 12:48

Introduce authentication to the broker #308

Introduce authentication to the broker #308

jmrodri commented Jul 27, 2017

shawn-hurley left a comment

shawn-hurley Jul 27, 2017

shawn-hurley Jul 27, 2017 •

edited

shawn-hurley Jul 27, 2017

shawn-hurley Jul 27, 2017

shawn-hurley Jul 27, 2017

jmrodri Jul 27, 2017

shawn-hurley Jul 27, 2017

shawn-hurley Jul 27, 2017

jmrodri Jul 31, 2017

shawn-hurley Jul 27, 2017

rthallisey commented Jul 27, 2017

jmrodri commented Aug 3, 2017

jmrodri commented Aug 3, 2017 •

edited by rthallisey

jmrodri commented Aug 3, 2017

jwmatthews Aug 3, 2017

jwmatthews left a comment

rthallisey left a comment

rthallisey Aug 3, 2017

rthallisey Aug 3, 2017

jmrodri Aug 4, 2017

shawn-hurley commented Aug 3, 2017

eriknelson left a comment

eriknelson Aug 3, 2017

jmrodri Aug 4, 2017

eriknelson Aug 3, 2017

jmrodri Aug 4, 2017

jmrodri commented Aug 4, 2017

jmrodri commented Aug 4, 2017

jmrodri commented Aug 4, 2017

shawn-hurley Aug 4, 2017

eriknelson commented Aug 4, 2017

eriknelson commented Aug 4, 2017

Introduce authentication to the broker #308

Introduce authentication to the broker #308

Conversation

jmrodri commented Jul 27, 2017

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley Jul 27, 2017 • edited

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

rthallisey commented Jul 27, 2017

jmrodri commented Aug 3, 2017

jmrodri commented Aug 3, 2017 • edited by rthallisey

TEST

jmrodri commented Aug 3, 2017

TROUBLESHOOTING

Choose a reason for hiding this comment

jwmatthews left a comment

Choose a reason for hiding this comment

rthallisey 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

shawn-hurley commented Aug 3, 2017

eriknelson 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

jmrodri commented Aug 4, 2017

jmrodri commented Aug 4, 2017

jmrodri commented Aug 4, 2017

Choose a reason for hiding this comment

eriknelson commented Aug 4, 2017

eriknelson commented Aug 4, 2017

shawn-hurley Jul 27, 2017 •

edited

jmrodri commented Aug 3, 2017 •

edited by rthallisey