-
Notifications
You must be signed in to change notification settings - Fork 434
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
GET MAY return creds unable to be used #517
GET MAY return creds unable to be used #517
Conversation
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. |
spec.md
Outdated
@@ -1374,7 +1374,7 @@ For success responses, the following fields are defined: | |||
|
|||
| Response Field | Type | Description | | |||
| --- | --- | --- | | |||
| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. | | |||
| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. If credentials have been stored in a format that does not allow retrieval of a usable result (e.g. a salted and hashed password), the broker MAY omit the field from `credentials`. This can result in a return value that is not usable to access the service. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/from credentials
//
I assume they should just skip the entire "credentials" field, and not just certain items in it. Returning a partial list of creds could be very misleading so I think it should be all or nothing.
spec.md
Outdated
@@ -1374,7 +1374,7 @@ For success responses, the following fields are defined: | |||
|
|||
| Response Field | Type | Description | | |||
| --- | --- | --- | | |||
| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. | | |||
| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. If credentials have been stored in a format that does not allow retrieval of a usable result (e.g. a salted and hashed password), the broker MAY omit this field. This can result in a return value that is not usable to access the service. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can result in a return value that is not usable to access the service.
That statement seems ambiguous. Are you saying the response that lacks a credentials field will result in a value that is unable to access the service or the fact that returning a salted+hashed password results in not being able to use it.
This addition reads to me as a way to allow a broker to never allow access to a service. How would such a service perform an async bind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we just remove that sentence.
re: async-binding... perhaps we need to add something else here to make it clear that each GET can decide when to return the credentials. Meaning, the first GET might do so, but then subsequent GETs may not because they're stored in a hash. This allows for a one-time-only retrieval so async bindings work, but after that things remain secure. But, in the case of async it MUST support at least one GET of the creds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is removing that sentence acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a password based service like that, it looks like it would be best to not support async bind, or a broker specific implementation that returns the credentials only for a certain limit (time, number of attempts, a combination, new credentials each time, etc).
I am not sure how this would interact with other token/cert/other based access. It is probably similar privileged key-like material that needs to be guarded in a similar manner.
Not sure how to encode this intent beyond the given example.
Is removing that sentence acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this today's agenda - I think some real-time discussions might help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If credentials have been stored in a format that does not allow retrieval of a usable result the broker MAY omit this field.
@n3wscott @mattmcneeney Is only that sentence being added acceptable? I feel like it makes things murkier rather than clearer without the example.
@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. |
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:
|
How about a minor addition:
|
Does it all just go into one giant line? Seems to render fine, but looks terrible in plaintext. |
spec.md
Outdated
@@ -1374,7 +1374,7 @@ For success responses, the following fields are defined: | |||
|
|||
| Response Field | Type | Description | | |||
| --- | --- | --- | | |||
| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. | | |||
| credentials | object | 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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@n3wscott does this address the async bind case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little weird to have async bind information in this PR since it may be submitted before #334
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattmcneeney does that mean you want it back out and a reminder in #334 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addition introduces an inconstancy. If we allow the omission of the credentials here, we also have to allow the omission for the same reasons when a binding is (re-)created with the identical parameters (status code 200 OK).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MHBauer please make similar edits to the create per Florian's suggestion - then we'll discuss at the f2f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. The door shuts eventually, but that doesn't mean it's shut from the start.
For synchronous binding the credentials can be returned the once before becoming irretrievable.
For async binding we have to preserve the response for only so long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MHBauer this is about the case where a replayed create comes in, at that point its like a GET when the params are the same so the broker can then be free to not include the creds. I agree that a non-replayed create should return the creds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is very much in conflict with stateless-ness. It seems like it would now be saying "must be preserved forever". Being able to do this now implies that the credential generation is deterministic. I'm not sure this is clarifying anything anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the equivalent of MUST include this field in at least the first response
? We now appear to be saying, "MUST include forever".
1 similar comment
Decision in 2018-05-29 meeting is hold on merge until merge of #334. |
Please review but adding DO NOT MERGE since we need the async PR to go in first. |
spec.md
Outdated
@@ -1280,7 +1280,7 @@ For success responses, the following fields are defined: | |||
|
|||
| Response Field | Type | Description | | |||
| --- | --- | --- | | |||
| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. | | |||
| credentials | object | 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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "This field MAY be omitted..." sentence needs to be prefixed with "If a 200 is returned..." or something like that.
spec.md
Outdated
@@ -1360,7 +1360,7 @@ For `200 OK` and `201 Created` response codes, the following fields are defined: | |||
|
|||
| Response Field | Type | Description | | |||
| --- | --- | --- | | |||
| 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. | | |||
| credentials | object | 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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/application/Application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And who is the 'user'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the sentence starting with Note that during an asynchronous bind operation
should be in this table.
spec.md
Outdated
@@ -1459,7 +1459,7 @@ For success responses, the following fields are defined: | |||
|
|||
| Response Field | Type | Description | | |||
| --- | --- | --- | | |||
| 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. | | |||
| credentials | object | 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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/application/Application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And who is the 'user'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a GET endpoint, so I'm not sure it makes sense to talk about 'during an asyhnchronous bind operation'.
spec.md
Outdated
@@ -1195,6 +1195,12 @@ utilize the Service Instance. Credentials SHOULD be unique whenever possible, so | |||
access can be revoked for each binding without affecting consumers of other | |||
bindings for 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. During an asynchronous bind | |||
operation the Service Broker MUST include this field in at least the first response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first GET response (with link)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to what?
spec.md
Outdated
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. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dug suggested
Note: some brokers (due to security concerns... blah blah) MAY choose to only return the credentials for a binding once even though there could be replayed binding requests or GETs of the binding resource.
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? |
In my mind this conflicts with the MUST on the binding response for credentials
Should we change the response description for fetch binding to reflect this optionality? |
resolves #516