openservicebrokerapi / servicebroker

Open Service Broker API Specification
https://openservicebrokerapi.org/
Apache License 2.0
1.19k stars 435 forks source link

broker requires user identity to perform operations on users behalf #110

Closed shalako closed 6 years ago

shalako commented 7 years ago

Proposal from @gberche-orange: https://docs.google.com/document/d/1DoAbJa_YiGIJbOZ_zPzakh7sc4TB9Tmadq41cfSX0dw/edit?usp=sharing. Discussion of that proposal: http://cf-dev.70369.x6.nabble.com/cf-dev-service-broker-user-delegation-beyond-service-dashboard-tt2658.html

Proposal from @fraenkel: https://docs.google.com/document/d/1w4swx_5erf_NmPtfcCW2fn2jbEBfRHf2xLVhWpc1PLM/edit

fraenkel commented 7 years ago

@gberche-orange We need to reconcile the 2 proposals into one.

gberche-orange commented 7 years ago

We have synced up today @fraenkel and myself. Our notes are at https://docs.google.com/document/d/1O3ltbvMbkM7F4xzJ-Qt1xTULpknaIlJFGRZFmXQOO7c/edit likely to also be the document that will contain the upcoming merged proposal.

shalako commented 7 years ago

Thank you both for collaborating. Please let use know here when the merged proposal is ready for review.

duglin commented 7 years ago

Guillaume, thanks. As soon as the current spec “extraction” process is completed, it would be great to see a marked-up version of the spec with the proposed changes. Or, if people are anxious, one based on today’s spec, since hopefully moving it to the new stuff should just be a copy-n-paste. -Doug

On Jan 6, 2017, at 1:03 PM, Guillaume Berche notifications@github.com wrote:

We have synced up today, with notes at https://docs.google.com/document/d/1O3ltbvMbkM7F4xzJ-Qt1xTULpknaIlJFGRZFmXQOO7c/edit https://docs.google.com/document/d/1O3ltbvMbkM7F4xzJ-Qt1xTULpknaIlJFGRZFmXQOO7c/edit likely to also contain the next proposal.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openservicebrokerapi/servicebroker/issues/110#issuecomment-270964250, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2sXzvECcWu9fHqelS_XXOgZvncxlaoks5rPoH6gaJpZM4LF2R-.

duglin commented 7 years ago

See: https://docs.google.com/document/d/1W1mkkhlf-Wxj0c3WmPewaa7Yi0q0RXtrtJbzyrxm-Vs/edit?ts=58bef7b9

duglin commented 7 years ago

What do people this of this proposal? https://github.com/openservicebrokerapi/servicebroker/compare/master...duglin:issue110

liggitt commented 7 years ago

If the intent of the header is to provide an authentication token:

If the intent of the header is to plumb user identity info:

duglin commented 7 years ago

I would envision it being an auth token most of the time but its really up to the platform to decide - to the broker its just an opaque string.

For timeout of the token, that's outside the scope of the spec. In general though I would expect the token to be valid long enough for the broker to do its job. Whether its a one-time thing at the time of provisioning/binding or a long-lived thing (e.g. for the life the app) is out of scope. There is no mechanism to have the broker regen/rotate its token. If we want to support that then I think we should lump that discussion into the one we have around rotating keys for the bindings - I would think a similar solution could be used for both.

bparees commented 7 years ago
 For example, in the case of an auto-scaling service, it will need to be able to call into the platform to  scale the desired application based on the current load on the application.

I'm puzzled by the use of "platform" and how this scenario plays out.

Assuming the most loosely coupled system consisting of:

1) a service catalog (that seems to be "the platform" in the above paragraph, based on the later usage that has the platform making provision calls to the broker) 2) a broker that provisions a database in a cloud DB service running somewhere else 3) the cloud DB service itself

The broker would not be calling back to the service catalog to scale things (not even clear to me why the broker is responsible for scaling anything, or under what circumstances it would be).

So overall I guess i'm struggling with why the broker is calling the service catalog (Again, assuming that is "the platform" in this context). If "the platform" is intended to be a more generic stand in (ie it could be the cloud DB service in my example) then i don't see how the service catalog is going to have the credentials(for the db cloud service) to provide to the broker in the first place (nor should it).

For our specific broker (the openshift template broker) there are two pieces of auth we are concerned with:

1) our broker apis are protected by openshift auth flows. That means a call to our broker api must provide an openshift authentication token. Currently the service catalog->broker api call flow only supports basic auth and that's not sufficient for us.

2) the broker itself needs to perform actions on the openshift cluster, on behalf of the user. So we need to know the user identity that we need to use when performing actions in the openshift cluster. For now we've been assuming that will be provided to us as a standard parameter on the provision call. (and we will assume the service catalog has already confirmed the user's identity before setting it in the parameter, and we will only trust API invocations from the service catalog, via the mechanism described in (1)).

duglin commented 7 years ago

@bparees in the case of this type of scaling service its best to not think of a normal DB service. Think of it as just a piece of code that is told what application it is supposed to manage. So the createInstance() request has the org, space and appID. With that knowledge this code can call back into the platform (think CF) and monitor the app and then tell CF when it needs to scale it, based on the load. How the scaling service gets its job done is out of scope for the spec beyond providing the hooks for the platform to give the service the info it needs.

bparees commented 7 years ago

So the createInstance() request has the org, space and appID.

why are those not just generic parameters to the provision call, that get stored w/ any other instance state the broker needs to store?

Also i think "platform" is being overloaded. It would be useful to refer to things as: 1) the service catalog 2) the broker 3) the backend service infrastructure/instance

Your scenario description seems to assume 1+3 are the same("platform calls the broker" and "broker calls the platform"), but that doesn't seem like a reasonable assumption/limitation to me. Can't i offer an "autoscale my ec2 instances" broker/service in a service catalog hosted by a k8s cluster?

duglin commented 7 years ago

We're working on the terminology and are slowly heading towards "platform", "broker" and "instance".

We can't stick the identity of the user in the parameters because we sometimes need to track who is making the current call. For example, Joe might have been the one to create the instance but Dave is the one who tried to do the binding, or delete it. The broker may (for auditing or auth purposes) need to track this, either by itself or in conjunction with the platform.

My scenario doesn't assume 1 and 3 are the same, in fact they're not. But they do work together. For example, a scaling service can be independent of CF but it knows how to talk to CF - and it knows how to issue the equivalent of a "cf scale myapp -i 5" (to scale it up to 5 instances) when needed.

bparees commented 7 years ago

For example, a scaling service can be independent of CF but it knows how to talk to CF - and it knows how to issue the equivalent of a "cf scale myapp -i 5" (to scale it up to 5 instances) when needed.

yes but no part of that is "the platform"(service catalog), so why is the service catalog in any way responsible for providing CF credentials to the broker (other than in generic parameter form)?

I think there are still some leftover assumptions from the way the classic cloudfoundry service model works, in which both the service catalog, and the service infrastructure, are part of the same cloudfoundry cluster, which do not apply to the more generic way we are attempting to use the service catalog. How does my ec2 example work in this model? Or is it not a use case intended to be solved here?

Going back to your proposal:

In order to facilitate this, the platform will need to provide authentication
information to the broker than can then be used on subsequent requests from
the broker back to the platform. 

There is an assumption there that the "platform" in the first line is also the "platform" in the last line, and yet they are two different roles. The first "platform" is the service catalog providing some auth information to the broker during the provision/bind operation, the second "platform" is the service infrastructure that's being called by the broker in order to accomplish the provision/bind operation. Since the broker never "calls" the service catalog, clearly the second "platform" can't be the service catalog, unless you are assuming the service catalog and the service providing infrastructure are the same thing.

I now understand the use case but I still think the proposed solution is making assumptions about the relationship between the service catalog and the service infrastructure.

one more point:

When this HTTP header is provided to the broker, the broker MUST
include an additional HTTP header in its requests back to the platform
for operations related to the service instance, or binding, in question.

Again, since the broker never calls the service catalog, the "platform" here would seem to be the service infrastructure that's being provisioned/bound to, and if that's the case, it seems unrealistic to assume every service infrastructure has to support this specific header/auth mechanism. That should be a detail for the broker and the service infrastructure to agree on, it's not a concern of the service catalog/broker api.

gberche-orange commented 7 years ago

Thanks @duglin for this proposal.

I would envision it being an auth token most of the time but its really up to the platform to decide - to the broker its just an opaque string.

Some feedback from a CF user's point of view: CF should document the format of this token and capabilities it provides.

So we might need to refine the proposal to detail the `requires field.

duglin commented 7 years ago

per the f2f, here's an updated proposal for issue #110: https://github.com/openservicebrokerapi/servicebroker/compare/master...duglin:issue110

fmui commented 7 years ago

@duglin, in your proposal you define the header value as UTF-8 encoded by default. But HTTP header values should be US-ASCII encoded (see RFC 7230, section 3.2.4). If we want a different encoding we should either follow RFC 2047 or define it as Base64 encoded, similar to the basic auth header. We could also define it as an opaque string and let the platform decide.

avade commented 7 years ago

We could also define it as an opaque string and let the platform decide.

I would prefer we give some guidance so brokers know how to decode this field

duglin commented 7 years ago

@fmui good catch - I've updated the proposal.

fmui commented 7 years ago

Most HTTP libraries are not able to handle anything else than US-ASCII correctly. I've never seen a HTTP client library that let me specify the character encoding of a header. Although, the proposal is theoretically possible and doesn't violate the HTTP spec, in reality it is difficult to implement. I would rather go for a Base64 encoded UTF-8 string. That's not nice to read but it covers everything.

Can you please remind me why are we not using a token here?

duglin commented 7 years ago

Do we have to say anything at all other than its a string? I'd really prefer to leave it up to the platform/broker to work out the details of how to interpret this value. The spec shouldn't really do anything other than standardize where to look for this string.

jim-minter commented 7 years ago

@duglin @fmui 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==

vaikas commented 7 years ago

+1 to base64 encoding it. Interesting thought about using the same approach, I think it would work nicely.

mattmcneeney commented 7 years ago

Changing this to validation through implementation since the associated PR #222 is currently being validated.