SPEC: broker authentication spec #260

jmrodri · 2017-07-07T19:59:50Z

No description provided.

jwmatthews · 2017-07-07T20:02:45Z

docs/specifications/broker-authentication.md

+* shared secrets are mysterious. [OpenShift docs](https://github.com/openshift/openshift-docs/blob/master/dev_guide/secrets.adoc#creating-secrets) state:
+> Secret data can be shared within a namespace
+Except that the broker and the service-catalog run in different namespaces. How
+do we *share* a secret in this case?


I have same question on how to share the secret between service-catalog/broker
Probably good to ask @pmorie for some advice here.

Brief conversation with @shawn-hurley we kind of thought the sharing is that we create two secrets with the same value. That is when we create the secret in the service-catalog namespace. When we create the broker resource in service-catalog we give it that secret. When we deploy the broker, we create a new secret with the same values in the broker namespace. Then then tell the broker to use that secret.

They are shared in a sense that we gave both of them the same values to use for each other. Similar to when we used to configure a webapp by giving it the username password for the database that we entered during the setup of the database.

I'm a little confused here. Correct me where I'm wrong.

We create a secret in the broker when initially creating the broker resources. This is the golden source. Where do the credentials come from?

After the secret exists and all the broker resources, the broker credentials secret is created in the service-catalog first, presumably because it will need it before creating the broker resource. Broker resource is created, catalog finds the broker secret and is able to auth it's catalog request against our broker (and all subsequent requests)?

--

I have a couple more q's:

Do we need to handle revocation of credentials? Any way to issue new credentials? (Sounds like this may be related to secret rotation).

The proposal effectively uses secrets as a stateful store for credentials. What happens when the broker is not running inside a cluster and secrets are not available? Is there any notion of sessions or requirement for any other session state?

jmrodri · 2017-07-07T20:12:57Z

I'm doing things a little different for this spec. We tried doing things a few different ways, I'm hoping this might be the best of all worlds.

spec as a github issue
- public making it accessible
- difficult to comment on specific line
- difficult to keep track of comments
- no history of revisions
google docs
- not able to make public outside Red Hat
- ability to comment on specific lines
- lengthy discussions were hard to keep track of
- no history of revisions

Here is the hope for the PR process:

public
easy to comment on specific lines
familiar process to other code reviews
becomes part of the repo, can reference later
lengthy discussions seem easier to deal with

jmrodri · 2017-07-07T21:35:10Z

(13) Add an Authentication mechanism to Ansible Service Broker API

jmrodri · 2017-07-10T04:28:50Z

docs/specifications/broker-authentication.md

+```
+auth:
+   - type: basic | INSERT OTHER TYPE HERE
+   - enable: true | false


wondering if this should be part of the broker section or its own section?

Its own section.

broker: dev_broker: true launch_apb_on_bind: false recovery: true output_request: true auth: type: basic enable: true

Part of broker.

broker: dev_broker: true launch_apb_on_bind: false recovery: true output_request: true auth: type: basic enable: true

Opinions?

I don't have a strong opinion. While I think that auth is only a configuration for the broker. What about adding a key for specfic web facing "handler" configuration? It seems to be that the broker configuration is getting overloaded.

example:

broker: launch_apb_on_bind: false recovery: true output_request: true handler: dev_broker: true output_request: true auth: type: basic enable: true

I don't really see a reason to restrict the broker section after N fields (or any section for that matter). What I would want to see is nice comments for each explaining what they do inside the example config. Personally, I think this makes sense in the broker section.

The config file is mainly for broker operators, they probably don't even know what a handler is. broker.auth makes sense to that audience though.

I will go with making auth part of broker:

broker: dev_broker: true launch_apb_on_bind: false recovery: true output_request: true auth: type: basic enable: true

shawn-hurley · 2017-07-10T12:01:40Z

docs/specifications/broker-authentication.md

+```
+auth:
+   - type: basic | INSERT OTHER TYPE HERE
+   - enable: true | false


I don't have a strong opinion. While I think that auth is only a configuration for the broker. What about adding a key for specfic web facing "handler" configuration? It seems to be that the broker configuration is getting overloaded.

example:

broker: launch_apb_on_bind: false recovery: true output_request: true handler: dev_broker: true output_request: true auth: type: basic enable: true

shawn-hurley · 2017-07-10T12:07:19Z

docs/specifications/broker-authentication.md

+
+* secret rotation is desired but needs more research, see [#open-issues](#open-issues)
+
+* use [`http.BasicAuth`](https://golang.org/pkg/net/http/#Request.BasicAuth) to handle the basic auth case


Are you going to create a middleware for the auth?

We should make sure that if adding auth affects dev routes we should change the APB tool or allow the dev routes to bypass the auth.

+1, I was expecting this to be req middleware. Think it makes sense to make dev routes unauthenticated. They shouldn't even be active in a production setting.

@eriknelson @shawn-hurley what do you mean by "middleware"? +1 to keeping dev routes unauthenticated. If devbroker is enabled and auth is enabled, I will log a warning that dev routes will not be authenticated. Just to make it explicit that it won't block people from using those endpoints

Yeah it's a way overloaded term. In the context of requests and auth, basically a hook in the incoming request stack that runs before the request hits the handler. Allows you to inspect the incoming req, authenticate (check auth params against known user, create login record, possibly enrich request with session data) and call some kind of next() that allows the request to pass through the rest of the stack, or reject() that will error out the request with a 401.

http://www.gorillatoolkit.org/pkg/handlers
https://github.com/urfave/negroni

Those are examples of middleware, it is the pattern of wrapping the http.Handler and returning a http.Handler to do something before/after calling the wrapped handler

We should make sure that if adding auth affects dev routes we should change the APB tool or allow the
dev routes to bypass the auth.

How will this be implemented? I don't want us to create a code split on a config setting. In other words, I think setting the dev route to bypass auth should be the dev turning off auth in the broker config.

@rthallisey so you're suggesting that if auth:enable = true auth is on for EVERYTHING including dev. If they want to disable auth for dev, then they need to disable auth completely?

Ya I think this can be solved by a config setting instead of logic in the broker. Just so there isn't confusion about configs, we can document the developer vs production setup and carry the developer config file in master.

handler: dev_broker: true output_request: true auth: type: basic enable: false

Then when we branch, default the config to production defaults.

handler: dev_broker: false output_request: false auth: type: basic enable: true

If we feel like our developer config options are growing, we could have a single dev=true/false option that defaults all the developer options unless they are specifically mentioned.

broker: development: true #hander: # dev_broker: true # output_request: true # auth: # type: basic # enable: false

@rthallisey +1, I share your concern. I'm fine with just disabling auth.

eriknelson · 2017-07-10T13:58:41Z

docs/specifications/broker-authentication.md

+          * [config.md](https://github.com/openshift/ansible-service-broker/blob/master/docs/config.md)
+          * [local_deployment.md](https://github.com/openshift/ansible-service-broker/blob/master/docs/local_deployment.md)l
+      * [`make deploy`](https://github.com/openshift/ansible-service-broker/blob/master/Makefile#L57-L58) does not need updating because it uses the above template.
+  * [catasb](https://github.com/fusor/catasb/tree/dev)


There are a bunch of places in catasb that require update when adding configuration values if we want to keep group_vars/all.yaml as a single source of config. IIRC, wherever the oc process takes place, it needs to be updated with additional -p vars to pass them through the ansible env to the template as an override.

eriknelson · 2017-07-10T14:01:38Z

docs/specifications/broker-authentication.md

+```
+auth:
+   - type: basic | INSERT OTHER TYPE HERE
+   - enable: true | false


I don't really see a reason to restrict the broker section after N fields (or any section for that matter). What I would want to see is nice comments for each explaining what they do inside the example config. Personally, I think this makes sense in the broker section.

The config file is mainly for broker operators, they probably don't even know what a handler is. broker.auth makes sense to that audience though.

eriknelson · 2017-07-10T14:12:13Z

docs/specifications/broker-authentication.md

+* shared secrets are mysterious. [OpenShift docs](https://github.com/openshift/openshift-docs/blob/master/dev_guide/secrets.adoc#creating-secrets) state:
+> Secret data can be shared within a namespace
+Except that the broker and the service-catalog run in different namespaces. How
+do we *share* a secret in this case?


I'm a little confused here. Correct me where I'm wrong.

We create a secret in the broker when initially creating the broker resources. This is the golden source. Where do the credentials come from?

After the secret exists and all the broker resources, the broker credentials secret is created in the service-catalog first, presumably because it will need it before creating the broker resource. Broker resource is created, catalog finds the broker secret and is able to auth it's catalog request against our broker (and all subsequent requests)?

--

I have a couple more q's:

Do we need to handle revocation of credentials? Any way to issue new credentials? (Sounds like this may be related to secret rotation).

The proposal effectively uses secrets as a stateful store for credentials. What happens when the broker is not running inside a cluster and secrets are not available? Is there any notion of sessions or requirement for any other session state?

eriknelson · 2017-07-10T14:17:53Z

docs/specifications/broker-authentication.md

+
+The Service Catalog calls [req.SetBasicAuth(c.username, c.password)](https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/brokerapi/openservicebroker/open_service_broker_client.go#L121) when calling teh broker.
+
+This is done by creating a secret with the username and password in it. Then


Is it our responsibility to create this secret, or does the service catalog create it? I was under the impression we need to create the secret and tell the catalog where to look. The text here suggested to me the catalog might do it, just wanted to clarify.

I will reword since the catalog does NOT create the secret. The catalog expects to be given the secret to use.

I assume we need to create the secret ourselves.

@eriknelson my impression was WE create it via the template.

@eriknelson

Do we need to handle revocation of credentials? Any way to issue new credentials? (Sounds like this may be related to secret rotation).

The revocation would happen during the rotation. At least that is how I interpreted it.

rthallisey · 2017-07-13T13:27:33Z

docs/specifications/broker-authentication.md

@@ -0,0 +1,109 @@
+# Broker Authentication


One last comment the LGTM. Can you rename the directory to specs instead of specifications? A lot of projects I've seen used the shortened name.

+1 specs sounds good.

jmrodri added feature proposal labels Jul 7, 2017

jmrodri requested review from jwmatthews, eriknelson and rthallisey July 7, 2017 20:00

broker authentication proposal

383ba96

jmrodri force-pushed the authspec branch from 7e5b0bd to 383ba96 Compare July 7, 2017 20:01

jwmatthews reviewed Jul 7, 2017

View reviewed changes

jmrodri commented Jul 10, 2017

View reviewed changes

shawn-hurley requested changes Jul 10, 2017

View reviewed changes

eriknelson reviewed Jul 10, 2017

View reviewed changes

shawn-hurley approved these changes Jul 11, 2017

View reviewed changes

jmrodri added 2 commits July 11, 2017 15:15

explicitly state that the config will be in broker

c4fe7e1

clarify who creates secret

62fb38d

rthallisey reviewed Jul 13, 2017

View reviewed changes

eriknelson approved these changes Jul 13, 2017

View reviewed changes

renaming directory to specs

3c4874e

jmrodri merged commit d464501 into master Jul 14, 2017

jmrodri deleted the authspec branch July 14, 2017 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPEC: broker authentication spec #260

SPEC: broker authentication spec #260

jmrodri commented Jul 7, 2017

jwmatthews Jul 7, 2017

jmrodri Jul 7, 2017

jmrodri Jul 7, 2017

eriknelson Jul 10, 2017

jmrodri commented Jul 7, 2017

jmrodri commented Jul 7, 2017

jmrodri Jul 10, 2017

shawn-hurley Jul 10, 2017

eriknelson Jul 10, 2017

jmrodri Jul 10, 2017

shawn-hurley Jul 10, 2017

shawn-hurley Jul 10, 2017

eriknelson Jul 10, 2017

jmrodri Jul 10, 2017

eriknelson Jul 10, 2017

shawn-hurley Jul 10, 2017

rthallisey Jul 11, 2017

jmrodri Jul 12, 2017

rthallisey Jul 12, 2017

eriknelson Jul 12, 2017

eriknelson Jul 10, 2017

eriknelson Jul 10, 2017

eriknelson Jul 10, 2017

eriknelson Jul 10, 2017

jmrodri Jul 10, 2017

jwmatthews Jul 10, 2017

jmrodri Jul 11, 2017

jmrodri Jul 11, 2017 •

edited

rthallisey Jul 13, 2017

jmrodri Jul 14, 2017


		* secret rotation is desired but needs more research, see [#open-issues](#open-issues)

		* use [`http.BasicAuth`](https://golang.org/pkg/net/http/#Request.BasicAuth) to handle the basic auth case


		The Service Catalog calls [req.SetBasicAuth(c.username, c.password)](https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/brokerapi/openservicebroker/open_service_broker_client.go#L121) when calling teh broker.

		This is done by creating a secret with the username and password in it. Then

SPEC: broker authentication spec #260

SPEC: broker authentication spec #260

Conversation

jmrodri commented Jul 7, 2017

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 Jul 7, 2017

jmrodri commented Jul 7, 2017

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

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

Choose a reason for hiding this comment

jmrodri Jul 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri Jul 11, 2017 •

edited