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

Document existing CF credential_client_id bind ressource #704

Merged
merged 5 commits into from
May 15, 2020
Merged

Document existing CF credential_client_id bind ressource #704

merged 5 commits into from
May 15, 2020

Conversation

gberche-orange
Copy link
Contributor

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:

credential_client_id is provided by cloudfoundry cloudcontroller in the binding resource since late 2017. See https://www.pivotaltracker.com/n/projects/966314/stories/151111701
mattmcneeney
mattmcneeney previously approved these changes Mar 17, 2020
@mattmcneeney
Copy link
Contributor

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

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:

@mattmcneeney
Copy link
Contributor

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

@tinygrasshopper
Copy link
Member

@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

…cepts

This is necessary to explain present of `credential_client_id` key
@gberche-orange
Copy link
Contributor Author

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

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

gberche-orange commented Apr 1, 2020

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

gberche-orange commented Apr 1, 2020

@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 #704 (comment). I'll double check the wording

@gberche-orange
Copy link
Contributor Author

gberche-orange commented Apr 1, 2020

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

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 mattmcneeney added this to the 2.16 milestone Apr 28, 2020
@mattmcneeney
Copy link
Contributor

Trying to retrigger Travis

@mattmcneeney
Copy link
Contributor

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

@mattmcneeney mattmcneeney merged commit 6932c32 into openservicebrokerapi:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants