openservicebrokerapi / servicebroker

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

GET MAY return creds unable to be used #517

Closed MHBauer closed 6 years ago

MHBauer commented 6 years ago

resolves #516

cfdreddbot commented 6 years ago

Hey MHBauer!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

duglin commented 6 years ago

LGTM

Approved with PullApprove

duglin commented 6 years ago

@MHBauer did you want to address @n3wscott's comment?

MHBauer commented 6 years ago

@mattmcneeney During the call today, you took specific issue with the given example. I do not recall the exact context, but I feel like it adds one specific intent without restricting it to solely that case. I'll work with @duglin on alternative phrases and positions.

mattmcneeney commented 6 years ago

Sounds good, thanks @MHBauer. My only concern with the current text is that it suggests the fact a credential is hashed/salted is a reason not to return it, but from a broker author's perspective, their concern really is security.

Maybe something like this:

A free-form hash of credentials that can be used by applications or users to access the Service Instance. This field MAY be omitted if the Service Broker either cannot return the credentials or does not want to for security reasons.
duglin commented 6 years ago

How about a minor addition:

A free-form hash of credentials that can be used by applications or users to access
the Service Instance. This field MAY be omitted if the Service Broker either cannot
return the credentials or does not want to for security reasons. Note that during
an asynchronous bind operation the Service Broker MUST include this field in at least
the first response after the Service Binding has been successfully created, otherwise
the Platform will not have the credentials to pass along to the application/user.
MHBauer commented 6 years ago

Does it all just go into one giant line? Seems to render fine, but looks terrible in plaintext.

duglin commented 6 years ago

LGTM

Approved with PullApprove

fmui commented 6 years ago

LGTM

Approved with PullApprove

MHBauer commented 6 years ago

Decision in 2018-05-29 meeting is hold on merge until merge of #334.

duglin commented 6 years ago

Please review but adding DO NOT MERGE since we need the async PR to go in first.

MHBauer commented 6 years ago

What's next to move this forward? Going back to some comment in each table? Or keep the guidance on the description of a credentials binding?

fmui commented 6 years ago

LGTM

Approved with PullApprove

duglin commented 6 years ago

LGTM

Approved with PullApprove

zrob commented 6 years ago

lgtm

Approved with PullApprove

tinygrasshopper commented 6 years ago

In my mind this conflicts with the MUST on the binding response for credentials

| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. MUST be returned if the Service Broker supports generation of credentials and the Service Binding was provisioned asynchronously. |

Should we change the response description for fetch binding to reflect this optionality?