Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
vorbidan opened this issue Jun 14, 2019 · 19 comments · May be fixed by #686
Open

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

vorbidan opened this issue Jun 14, 2019 · 19 comments · May be fixed by #686

Comments

@vorbidan
Copy link

vorbidan commented Jun 14, 2019

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?

  • developers,
  • platform authors
  • broker authors

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
Copy link
Contributor

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
Copy link
Author

vorbidan commented Jun 17, 2019

@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
Copy link
Contributor

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

@vorbidan
Copy link
Author

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

@mattmcneeney
Copy link
Contributor

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
Copy link
Author

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

@waterlink
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

waterlink commented Jun 17, 2019

@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
Copy link
Contributor

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
Copy link
Author

vorbidan commented Jun 18, 2019

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

mattmcneeney commented Jun 24, 2019

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
Copy link
Member

fmui commented Jun 25, 2019

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
Copy link
Contributor

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

@mattmcneeney mattmcneeney linked a pull request Jun 27, 2019 that will close this issue
2 tasks
@mattmcneeney mattmcneeney self-assigned this Jun 27, 2019
@mattmcneeney
Copy link
Contributor

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

@jberkhahn
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Inbox
Development

Successfully merging a pull request may close this issue.

5 participants