openservicebrokerapi / servicebroker

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

Should the X-Api-Info-Location header be a part of the spec? #676

Open vorbidan opened 5 years ago

vorbidan commented 5 years ago

What is the problem? When the service broker is used across multiple platform instances, it would be nice to understand which instance is making a call.

It seems that Spring Cloud Open Service Broker implementation has such a header defined, however, I could not find traces of it in the spec..

Who does this affect? Does this affect service broker authors, platform authors or developers?

Do you have any proposed solutions? Include an X-Api-Info-Location header as a part of the specification and allow for platforms to set this header during all requests (catalog, provision, bind, unbind, update, deprovision)

The format of the header MUST be:

X-Api-Info-Location: Platform value

Example:

X-Api-Info-Location: cloudfoundry ewogICJpbmZvX2xvY2F0aW9uX3VybCI6ICJodHRwczovL2FwaS5wbGF0Zm9ybS5leGFtcGxlLmNvbS92Mi9pbmZvIgp9

Where the value, when decoded, is:

{
  "info_location_url": "https://api.platform.example.com/v2/info"
}

Additional context Here is how Pivotal describes the usage:

X-Api-Info-Location Header

All calls to the broker from Cloud Foundry include an X-Api-Info-Location header containing >the /v2/info url for that instance. The /v2/info endpoint will return further information, >including the location of that Cloud Foundry instance’s UAA.

Support for this header was introduced in cf-release v212. Pivotal Docs

mattmcneeney commented 5 years ago

Hey @vorbidan

Thanks for opening this issue.

There are two headers sent on those requests today which may help you understand where the request originated from:

  1. The Originating Identify hedaer (X-Broker-API-Originating-Identity)
  2. The Request Identity

Did you look at either of those fields and see if it is possible to understand which platform the call originated from?

vorbidan commented 5 years ago

@mattmcneeney - thank you for following up with the response. Somehow I missed that the X-Broker-API-Request-Identity was mentioned int the spec... Still, I believe, it does not address the question of identifying the platform for the purposes of issuing a ping-back. Having just the platform ID in the header, may require keeping the state of the registered platform in some sort of the data store, if we want to further de-reference and issue a request back to platform... I was also thinking about the context - it is platform specific, i.e. cloudfoundry, it also now holds values like org_id and space_id, would it be inappropriate to add a field representing a platform API url, and/or the platform info location?

mattmcneeney commented 5 years ago

That makes sense. What about having a platform_guid in the context object alongside org_guid etc?

vorbidan commented 5 years ago

What would the platform_guid be? a URI, or UUID?

mattmcneeney commented 5 years ago

Good question, maybe a GUID doesn't make sense. Maybe a URL is better, but do we have to worry about passing CC API URLs to service providers for security reasons?

@waterlink @williammartin any thoughts on this?

vorbidan commented 5 years ago

What does Pivotal use in X-Api-Info-Location header in requests from the platform?

waterlink commented 5 years ago

@vorbidan @mattmcneeney

Right now, CF sends X-Api-Info-Location header to service brokers with the URL value pointing to external domain + /v2/info. For example:

X-Api-Info-Location: api.example.org/v2/info
vorbidan commented 5 years ago

Great! So it looks like we may want to discuss 2 options:

  1. Make the X-Api-Info-Location header a part of the spec (this header is already being sent by CF, and is "understood" by Spring Cloud Services Broker implementation, and if my PR is accepted, will be recognized by Go implementation)
  2. Make this info to be a part of the context

In either case @mattmcneeney 's security concern needs to be addressed

waterlink commented 5 years ago

@mattmcneeney

About the security concern.

I’m not sure what you’re hinting at? What does the CC URL give to the potential attacker?


And even if it does have some concern, I can see how an attacker can get this URL by a variety of other means cheaply and quickly (including social engineering, or just signing up for the account if the platform is hosted; it’s also not hard to guess given enough information about the target).

mattmcneeney commented 5 years ago

Well if we are already sending this information today, then I think my concern is void. If this is a header we send today then it would make sense for it to be an optional part of the spec. Any concerns about this from the svc-cat side?

vorbidan commented 5 years ago

It looks like it makes a lot of sense to just add the header as an optional part of the spec, to bring it on par with implementation. The only caveat I see is that, at the moment, this header is implemented in CF world only. What about Kubernetes? Or because it is optional, it's not a big deal?

mattmcneeney commented 5 years ago

@jberkhahn would you mind this header being part of the core spec? Would you use X-Api-Info-Location for specifying the location of the k8s cluster for example?

jberkhahn commented 5 years ago

That would be difficult as service catalog doesn't inherently know where the kube cluster is running, and also the part issuing the requests (the service catalog controller) is an entirely different piece from the part you normally talk to (the main kube apiserver). On top of that, the actual api of the apiserver is obfuscated behind a go client lib, so I don't even know what endpoint on the apiserver I would put in there even if I did know the apiserver's address.

So yeah, I don't really know what we'd put in there. I'll try to bring this up at a SIG meeting and get back to you

mattmcneeney commented 5 years ago

Great, thanks @jberkhahn

So depending on the outcome from that discussion, we have two options:

  1. Add the header to the spec, as an optional header.
  2. Add the header to the profile, probably as a new section, though I do worry about adding a new first-level thing in profile as it's less visible than the main spec file (and all of the other info in profile to date is linked from the main spec - we could add a link though).

Let me know which of these seems most suitable for you @jberkhahn and we can look to open a PR.

fmui commented 5 years ago

If a broker hands out different credentials to the platform instances, it can use these to identify the calling platform. There is no need for an additional header.

mattmcneeney commented 5 years ago

@fmui That's true, but I think there are many cases where that doesn't happen today

mattmcneeney commented 5 years ago

Hey @vorbidan - I opened up a PR for this. Let me know what you think! #686

jberkhahn commented 5 years ago

Dug a bit more into this. The k8s equivalent is a kubectl command called cluster-info, but the way it actually works is it queries several endpoints, all of which are authenticated. There isn't really a k8s equivalent for this.

mattmcneeney commented 5 years ago

Cool - will update the PR to move this to profile only since it sounds like only CF will use this.