Add an Originating Identity http header #222

duglin · 2017-06-12T12:39:15Z

Per my AI from last week's call, I've created a PR for the proposal so its not hidden within the conversation in #110.

Closes #110

Signed-off-by: Doug Davis dug@us.ibm.com

cfdreddbot · 2017-06-12T12:39:16Z

Hey duglin!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

jim-minter · 2017-06-12T14:50:28Z

(copy from #110) I think there's value in copying the Authorization header more closely as follows:
a) use base64 encoding - less chance of implementers tripping up compared to ;charset; spec makes no comment about what's contained inside
b) prepend an identity scheme in us-ascii followed by space (similar to the 'Basic' or 'Bearer' in the Authorization header); allows some extensibility and allows implementations to recognise a scheme without diving into the base64 blob.

example: X-Broker-API-Originating-Identity: MyJSONUserAndGroupScheme eyJ1c2VyIjoiZm9vIiwiZ3JvdXBzIjpbImJhciIsImJheiJdfQ==

pmorie · 2017-06-12T17:46:12Z

spec.md

+for auditing or authorization purposes. In order to facilitate this, the
+platform will need to provide this identification information to the broker
+on each request. If the platform wishes to provide this information then
+it MUST include an additional HTTP header on all requests it sends to the


Is this really all requests? Or just creational requests (ie, provision, bind)

Also, 'MUST' seems like a backward-incompatible change.

Its for all requests.

The "MUST" is predicated by the "if". If you want to send this info then it MUST be done this way.

correction - all except GET /v2/catalog

I gotcha, missed the 'if' qualification

pmorie · 2017-06-12T17:47:03Z

spec.md

+
+```
+X-Broker-API-Originating-Identity: value[;charset=...]
+```


Do we need to send a scheme as well? Ie, 'I am sending a kubernetes identity'

I don't think so. Right now the assumption is that there's some out-of-band understanding between the platform and broker on how to interpret this data.

pmorie · 2017-06-12T17:47:51Z

We should note specifically in the spec body when the platform should send this header on a request by request basis.

duglin · 2017-06-12T17:58:12Z

@pmorie I like the idea of being more explicit - I think it makes sense to do it for the other http header(s) too - will give it a shot and see how it looks.

vaikas · 2017-06-13T08:37:21Z

Can we discuss the base64 encoding brought up by @jim-minter ?

pmorie · 2017-06-13T11:57:25Z

I also support @jim-minter's suggestion of base64 encoding the identity payload (comment: #222 (comment))

pmorie · 2017-06-13T11:58:23Z

spec.md

+
+| Header | Type | Description |
+| --- | --- | --- |
+| X-Broker-API-Version* | string | See [API Version Header](#api-version-header) |


I think we should leave the other headers out of this proposal

I'm not comfortable leaving the spec in a 1/2 completed state. Either we add a table for http headers to each API and fill it out completely, or we don't (perhaps leaving it for a new PR). But I'm not keen on a PR that does 1/2 the job.

A swagger spec would resolve the need for this duplication

I'm in favor of having a headers table for each endpoint.

shalako · 2017-06-20T22:22:46Z

Thoughts on base64 encoding the header?

pmorie

Two questions, close to LGTM

pmorie · 2017-06-21T03:31:59Z

spec.md

@@ -92,7 +93,7 @@ that do not understand them.

 Requests from the platform to the service broker MUST contain a header that declares the version number of the Service Broker API that the marketplace will use:

-`X-Broker-Api-Version: 2.11`
+`X-Broker-API-Version: 2.11`


Opened #246 for this change

Any reason you created a dup PR?

pmorie · 2017-06-21T03:32:46Z

spec.md

+Where:
+- its REQUIRED `value` is a non-empty string.
+- it MAY include a `charset` parameter specifying the character encoding of
+  the `value`. If not specified, the default value MUST be `us-ascii`.


Did we want to give an example for CF and Kubernetes here?

Also, do we want to discuss that the value may be any scheme? For example, in Kubernetes, we'll probably want to send the base64 encoded userinfo. See kubernetes-retired/service-catalog#939

I'd kind of like to leave that up to the platform instead of being prescriptive since the needs of the platform may vary.

duglin · 2017-06-28T02:25:14Z

From today's call I have a few questions about some of the things people were proposing:

base64 encoding of the value. I'd like to understand why we should do this. I'm not totally against it but I'm wondering if its because people think it'll be used to send sensitive data? If so we should discuss that because I don't think that was the intent - its meant to just convey the user initiating the request, nothing more. As such, needing something like a scheme seems like an unnecessary thing to me.
@mattmcneeney you said you wanted more structure/definition around the value itself. Can you provide a concrete proposal for what you're looking for and why if we limit it to just "the user"?
if we extend this beyond a simple string and allow it to be something like a json object, I'm worried that people will use it to store tons of other data that's not really the intent of this header. People should probably be using the Body/Context to send this other info but I'd like to understand the usecases people have in mind so we can make the right decision.
@pmorie you were mentioned using it to store data about where in the platform to provision stuff. W/o knowing more about the usecase that would seem to be better placed into the Context object, or someplace else in the body like the parameters, but we can get more info about this use?

At the last f2f we discussed moving the Context into and http header and realized that it would be problematic - not least of which because of the potential to hit a length limit. Encouraging arbitrary data in this PR's header, and not limiting it to something as simple as an email/userID/uuid/etc.. will lead us back to that same state, so it might be best if we only focus on our immediate needs.

jim-minter · 2017-06-28T07:36:35Z

base64 encoding of the value

My intention when suggesting this was to reuse a standard, recognisable, simple to implement mechanism to allow small amounts of arbitrary data to be reliably exchanged. Not to obfuscate sensitive data.

json object

A relevant example here is sending both the user identifier and their group memberships, for example. Not reams of information, but enough to make a compound object useful (and if it's base64 encoded by default, then there's protection against usernames with non-ascii characters, etc. I'd hope people wouldn't intend to send more than what you might find in a HTTP Authorization header.

People should probably be using the Body/Context to send this other info

Yes, this would probably better, but one issue is the body on DELETE problem, and it would be really nice to have access to user identification on delete to be able to check authorization.

mattmcneeney · 2017-06-28T09:50:04Z

@duglin As far as I know, we don't have a use case in CF for this today. I can imagine us wanting to use this for either auditing purposes or possibly for asking brokers to perform actions on a users' behalf. The latter would probably require more than one piece of information (ie. an access token and a refresh token?), but without any concrete examples of how we want to use this, it's hard to plan for the future. (cc @avade @shalako in case I've missed something)

I totally agree with you though that I think storing complex objects in the header would be a bad idea as it puts onus on brokers to deal with errors when the value is not in the format it expects. We purposely avoided doing this when discussing how to send context to brokers.

duglin · 2017-06-28T12:21:27Z

@jim-minter yea, DELETE is always the problem child :-) but I'm hoping that the semantics of that operation shouldn't need additional info that would require it to be passed in the body. Besides, there's always query parameters or other HTTP headers if people really need it. Mashing non-user-identifying info into this header should seem non-kosher to me.

I can see why we may need more than a single string, and thus a struct would be nice - if we add words to make it clear that this isn't a grab-bag to stick any random stuff (even though we can't technically stop people). Do we really need a scheme tag or can we just say its always a base64 json object?

jim-minter · 2017-06-28T13:05:32Z

@duglin it's definitely valuable to have the user info on delete - as much as we're discussing at the moment - and the header is good enough to provide that. I'm not personally aware of other information that's needed on delete beyond that (the fact that there's no context object on delete isn't blocking me, at least).

I think a scheme tag is valuable for at least the following reasons:
1- hopefully prevents the need to manually configure/match-up the identity scheme that a catalog sends and that a broker receives at broker registration time
2- makes it easy for a broker to tell what scheme it's dealing with and discard early if it doesn't understand it
3- allows implementation of brokers which can understand multiple schemes if needs be (i.e. understands what kubernetes uses, and what CF uses and can be a broker to either catalog).

duglin · 2017-06-28T13:06:46Z

Sorry if i wasn't clear - I do want this header on DELETE

duglin · 2017-06-28T13:15:15Z

re: scheme - I think the assumption here is that the broker knows the platform (and therefore scheme or data it should expect) based on the identity of the auth negotiation of the request itself - and therefore will know the scheme of this header. It would need to know this, along with probably other info, in order to use it to do things like call back into the platform anyway. Not for simple logging purposes though, obviously, but in that case I doubt it needs to understand the json, it would just need to log it.

To be clear, I'm not 100% against adding a scheme, but I want to make sure we're not adding it unnecessarily because the info is already available thru some other means - and it seems to me that in order for someone to really use the data in the json object they'll need to know a lot more than just the scheme anyway. We'd then need to agree on (standardize) the schemes too, no?

jim-minter · 2017-06-28T13:47:09Z

@duglin maybe I'm beginning to understand. Perhaps the scheme isn't utterly essential, but I think it does provide some protection, and maybe mandating it in the spec also provides a small foundation in case in the future a platform could handle multiple identities on behalf of a user and send the right one to a broker.

duglin · 2017-06-28T13:48:49Z

Do you have some sample schemes in mind so I can get a sense of how many (and their shape/values) you're thinking of?

jim-minter · 2017-06-28T14:10:02Z

Nothing more than "the scheme Kubernetes uses" (user,groups,extra); "the scheme CloudFoundry uses" (sorry, don't know what that is). Perhaps "username on its own" for good measure.

duglin · 2017-06-28T14:37:21Z

hmmm would it make sense to push for it to be the same string that used as the "platform" field in the "context" object? To get some consistency.

jim-minter · 2017-06-28T15:09:37Z

Works for me.

* Send the user's guid if available else omit the header openservicebrokerapi/servicebroker#222 [#149216559] Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

pmorie

Not sure about including this on last_operation requests.

pmorie · 2017-08-20T03:50:08Z

spec.md

+| Header | Type | Description |
+| --- | --- | --- |
+| X-Broker-API-Version* | string | See [API Version Header](#api-version-header). |
+| X-Broker-API-Originating-Identity | string | See [Originating Identity](#originating-identity). |


Is this the identity of the user that initiated the original action or the service account the platform component is running under?

I'm not sure that this makes sense to send on last operation, actually, because it seems like it's just checking the status of an action that's already been triggered.

In the Cloud Foundry implementation we omit the header during last_operation and certain orphan mitigation scenarios as it is the platform performing the action.

mattmcneeney · 2017-08-22T16:17:24Z

A PR for this change has now been submitted to the Cloud Controller in CF as part of the validation through implementation: cloudfoundry/cloud_controller_ng#877

* Send the user's guid if available else omit the header openservicebrokerapi/servicebroker#222 [#149216559] Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

mattmcneeney · 2017-09-08T13:59:13Z

This has been merged into the Cloud Controller and the implementation has been validated.
cloudfoundry/cloud_controller_ng#877

Calling for reviews!
@angarg12 @duglin @pmorie @skaegi @shalako @vaikas-google @fmui @avade

angarg12 · 2017-09-12T09:19:42Z

spec.md

+platform will need to provide this identification information to the broker
+on each request. Platforms MAY support this feature, and if they do, they
+MUST adhere to the following:
+- for any OSB API request that is the result of an action taken by


@angarg12 "Caps?" on which word(s) ?

These 3 bullet points begin with a lowercase letter, - for, - any, - if

ah ok, done.

mattmcneeney · 2017-09-15T09:08:56Z

Let's get this one merged folks. @duglin do you need to change anything following @angarg12's comment or are we good to go?

angarg12 · 2017-09-15T09:11:36Z

Yup, it also needs rebase.

Closes openservicebrokerapi#110 Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2017-09-18T13:13:46Z

addressed comments and rebased.

mattmcneeney · 2017-09-18T13:15:41Z

LGTM 👏 👏 👏

angarg12 · 2017-09-18T13:36:36Z

LGTM

fmui · 2017-09-26T10:31:41Z

LGTM

avade · 2017-09-26T15:14:37Z

lgtm

duglin changed the title ~~Add a delegation http header~~ Add an Originating Identity http header Jun 12, 2017

duglin force-pushed the issue110 branch from 7790089 to e77b580 Compare June 12, 2017 12:40

jim-minter mentioned this pull request Jun 12, 2017

service catalog should send user identity to broker kubernetes-retired/service-catalog#934

Closed

pmorie reviewed Jun 12, 2017

View reviewed changes

duglin force-pushed the issue110 branch from 6e1f3cd to c048cf9 Compare June 12, 2017 18:16

pmorie reviewed Jun 13, 2017

View reviewed changes

pmorie reviewed Jun 21, 2017

View reviewed changes

duglin force-pushed the issue110 branch from c048cf9 to 6f3c53a Compare June 26, 2017 13:27

shalako added the 1 - reviewing proposal label Jun 26, 2017

duglin force-pushed the issue110 branch from 6f3c53a to 96270df Compare June 27, 2017 14:52

duglin force-pushed the issue110 branch from 96270df to d4b7317 Compare June 29, 2017 16:25

Samze mentioned this pull request Aug 18, 2017

Add Originating Identity header to requests to Service Brokers cloudfoundry/cloud_controller_ng#877

Merged

4 tasks

pmorie reviewed Aug 20, 2017

View reviewed changes

duglin force-pushed the issue110 branch 3 times, most recently from 23393a4 to b8570de Compare August 29, 2017 16:15

mattmcneeney self-assigned this Sep 1, 2017

mattmcneeney added 3 - reviewing PR and removed 2 - validating through implementation labels Sep 8, 2017

angarg12 reviewed Sep 12, 2017

View reviewed changes

Add an Originating Identity http header

fe0c1cf

Closes openservicebrokerapi#110 Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the issue110 branch from b8570de to fe0c1cf Compare September 18, 2017 13:13

avade merged commit ade39a2 into openservicebrokerapi:master Sep 26, 2017

mattmcneeney removed the 3 - reviewing PR label Sep 26, 2017

mattmcneeney mentioned this pull request Sep 26, 2017

Release Proposal: v2.13 #318

Merged

duglin deleted the issue110 branch October 26, 2017 12:34

Add an Originating Identity http header #222

Add an Originating Identity http header #222

Conversation

duglin commented Jun 12, 2017

cfdreddbot commented Jun 12, 2017

jim-minter commented Jun 12, 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

pmorie commented Jun 12, 2017

duglin commented Jun 12, 2017

vaikas commented Jun 13, 2017

pmorie commented Jun 13, 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

shalako commented Jun 20, 2017 • edited Loading

pmorie left a comment • edited Loading

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

duglin commented Jun 28, 2017

jim-minter commented Jun 28, 2017 • edited Loading

mattmcneeney commented Jun 28, 2017

duglin commented Jun 28, 2017

jim-minter commented Jun 28, 2017

duglin commented Jun 28, 2017 • edited Loading

duglin commented Jun 28, 2017

jim-minter commented Jun 28, 2017

duglin commented Jun 28, 2017

jim-minter commented Jun 28, 2017

duglin commented Jun 28, 2017

jim-minter commented Jun 28, 2017

pmorie 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

mattmcneeney commented Aug 22, 2017

mattmcneeney commented Sep 8, 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

mattmcneeney commented Sep 15, 2017

angarg12 commented Sep 15, 2017

duglin commented Sep 18, 2017

mattmcneeney commented Sep 18, 2017 • edited by chipchilders Loading

angarg12 commented Sep 18, 2017 • edited by chipchilders Loading

fmui commented Sep 26, 2017 • edited by chipchilders Loading

avade commented Sep 26, 2017 • edited by chipchilders Loading

shalako commented Jun 20, 2017 •

edited

Loading

pmorie left a comment •

edited

Loading

jim-minter commented Jun 28, 2017 •

edited

Loading

duglin commented Jun 28, 2017 •

edited

Loading

mattmcneeney commented Sep 18, 2017 •

edited by chipchilders

Loading

angarg12 commented Sep 18, 2017 •

edited by chipchilders

Loading

fmui commented Sep 26, 2017 •

edited by chipchilders

Loading

avade commented Sep 26, 2017 •

edited by chipchilders

Loading