Document existing CF credential_client_id bind ressource

gberche-orange commented 4 years ago

What is the problem this PR solves?

credential_client_id is provided by cloudfoundry cloudcontroller in the binding resource since late 2017. See background at https://www.pivotaltracker.com/n/projects/966314/stories/151111701

Checklist:

[ ] The swagger.yaml doc has been updated with any required changes
- Existing profile fields seems not described in the current swagger.yml ServiceBindingResourceObject
[ ] The openapi.yaml doc has been updated with any required changes
- Existing profile fields seems not described in the current openapi.yaml ServiceBindingResource

mattmcneeney commented 4 years ago

Thanks for adding this to the spec @gberche-orange !

There seem to be some legitimate travis failures to investigate when you have the change: https://travis-ci.org/github/openservicebrokerapi/servicebroker/builds/661911797?utm_source=github_status&utm_medium=notification

gberche-orange commented 4 years ago

thanks @mattmcneeney for your review and heads up on failed test.

I've fixed the formatting violations reported by the build. Remains the following error in build 663828758 which I assume is false negative since the PR is not touching schemas

Errors:

Could not find components/schemas/# in contents of http://json-schema.org/draft-04/schema

mattmcneeney commented 4 years ago

Yep you’re right. I’ve opened a PR to fix that so once that’s merged I’ll retrigger this build!

tinygrasshopper commented 4 years ago

@gberche-orange the PR makes sense, a quick question how would the broker discover the url of the credhub its supposed to put credentials in. OR that does not come thru the spec, its configured on the broker

gberche-orange commented 4 years ago

@tinygrasshopper thanks for the heads up. My reverse engineering of the presence of this field was incomplete (I haven't yet used this credhub ref feature but I'm keen to have it documented in the spec as I'm still observing it in OSB payloads my service brokers receive).

Following a deeper read of https://www.pivotaltracker.com/n/projects/966314/stories/151111701 and credhub documentation, I've updated the PR to introduce the CloudFoundry service binding, service key and credhub reference concepts, and make it clear that the provisioning of credhub url and authN/authZ to Service Brokers is performed out of band.

It might make sense to add to the specs the credential format when using credhub reference, likely in #683 or #116

mattmcneeney commented 4 years ago

@gberche-orange The way I read this change now is that Service Brokers may return a credhub reference for a 'service key' request, but not for a normal 'service binding'. Is that the correct reading or am I missing something?

gberche-orange commented 4 years ago

@mattmcneeney

The way I read this change now is that Service Brokers may return a credhub reference for a 'service key' request, but not for a normal 'service binding'. Is that the correct reading or am I missing something?

No, credhub references can be returned in both SB and SK, but the client_id is included in the bind resource only for SK.

For a service binding, service brokers should grant the bound app (through its provided appguid) permission to read the CredHub reference

For a service key, it is instead assumed that the credential will be looked up by the cloudcontroller (or another credhub client), and therefore the specified credhub client id should be granted permission to read the CredHub reference

See https://www.pivotaltracker.com/story/show/151111701/comments/180009415

verified service key request contains client

2017-09-21T16:21:11.11-0700 [APP/PROC/WEB/0] OUT INFO: Request body: {"service_id":"f929f899-1818-4fc2-9454-f8def6c3f0e8","plan_id":"1faef310-2c21-4a1a-88ce-1fe06df77f76","bind_resource":{"credential_client_id":"cc_service_key_client"},"context":{"platform":"cloudfoundry","organization_guid":"d65a9c3f-0507-447c-bd4b-2e63c8088bfd","space_guid":"524df9a7-2236-4793-b0da-606066c6a214"}}

verified bind request does not contain client

2017-09-21T16:22:15.18-0700 [APP/PROC/WEB/0] OUT INFO: Request body: {"service_id":"f929f899-1818-4fc2-9454-f8def6c3f0e8","plan_id":"1faef310-2c21-4a1

Does it make sense ?

Maybe @zrob which contributed this feature back in 2017 can provide more background if necessary

gberche-orange commented 4 years ago

@mattmcneeney

Sorry,credhub references can be returned in both SB and SK, but the client_id is included in the bind resource for SK. I've corrected https://github.com/openservicebrokerapi/servicebroker/pull/704#issuecomment-607114168. I'll double check the wording

gberche-orange commented 4 years ago

@mattmcneeney

Double reading the proposed changed, I did not spot where there could have an ambiguity about service binding responses not supporting credhub references. Can you please use github review feature to point the specific line and if possible suggest a better wording ?

mattmcneeney commented 4 years ago

I mistook the bind_resource object being part of the response rather than the request. I think this actually makes sense to me now! Thanks for the explanation

mattmcneeney commented 4 years ago

Trying to retrigger Travis

mattmcneeney commented 4 years ago

@tinygrasshopper / @rsampaio if one of you can approve this one before Friday we can try to include it in the release!

openservicebrokerapi / servicebroker

Document existing CF credential_client_id bind ressource #704