-
Notifications
You must be signed in to change notification settings - Fork 433
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
add opaque Bearer Token to approved authentication methods #398
Conversation
Hey jboyd01! Thanks for submitting this pull request! All pull request submitters and commit authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate). When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization. If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions. Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look. |
I'm not sure we can do it this way w/o breaking backwards compat. Rather I think we can get the same net results by adding a subsection that basically talks about how "if" bearer tokens are used then this is how you do it. |
+1 to @duglin. It would be nice to keep the |
Hey jboyd01! 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. |
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 this looks ok, thanks @jboyd01. LGTM
This looks good to me too. As a side note @jboyd01, do you have any documentation outlining how you see this working generally? |
spec.md
Outdated
An alternative to carrying basic authentication on every request is to utilize a | ||
bearer token via the `Authorization: Bearer` header. When a bearer token is | ||
used additional processes are often required to deal with token expiration and | ||
renewal. These are details not covered by this specification and must be worked |
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.
Travis will complain about lowercase must
spec.md
Outdated
@@ -165,6 +165,13 @@ Service Broker using HTTP basic authentication (the `Authorization:` header) | |||
on every request. This specification does not specify how Platform and Service | |||
Brokers agree on other methods of authentication. | |||
|
|||
An alternative to carrying basic authentication on every request is to utilize a | |||
bearer token via the `Authorization: Bearer` header. When a bearer token is | |||
used additional processes are often required to deal with token expiration and |
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.
Travis will complain about lowercase required
@mattmcneeney, thanks for the review. regarding your question above about "any documentation outlining how you see this working generally" - I don't think we have any doc, but as a concrete example: Service Catalog uses opaque tokens on Open Shift for authenticating with our service brokers such as the Ansible Service Broker. When a new Broker is created, the broker authentication information specifies to use a specific secret whose content contains the necessary bearer token. The bearer token secret is created external to Catalog by the Broker installation and the token does not expire. If you desire token expiration, you would need a mechanism to update the K8s secret. The updated secret would automatically be picked up by Service Catalog the next time it is referenced. |
The text itself looks good. Just one higher-level question... why are we calling out this one specific security mechanism? Will this open the door to people wanted to add guidance for others as well? For now I guess its ok, but I'm not keen on duplicating what should really be part of the "out of bands agreement(spec)" that we say the platform and broker decide on. Its kind of like we're saying "its none of our business, but we're gonna tell you want to do anyway" :-) |
spec.md
Outdated
used additional processes are often needed to deal with token expiration and | ||
renewal. These are details not covered by this specification and need to be | ||
worked out by the Platform. If bearer tokens are used communication with the | ||
Broker MUST be secured over TLS. |
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.
Broker
-> Service Broker
Per our last call, I believe Jay, Aaron and Alex are going to talk off-line about rewording this. I think to mainly focus on how using TLS is good when any auth mechanism is used. |
Alex, Aaron and I are doing some additional rework on this removing the specifics about opaque token and clarifying some aspects around authentication. Marked as Work In Progress for now, not ready for review. |
Hey @jboyd01 You may want to rebase this PR on top of master as I believe it's showing many changes that aren't part of this change! |
@mattmcneeney Yes, it was a rebase gone bad, got it straightened out now. Thanks |
spec.md
Outdated
@@ -163,9 +163,15 @@ Service Broker using HTTP basic authentication (the `Authorization:` header) | |||
on every request. This specification does not specify how Platform and Service | |||
Brokers agree on other methods of authentication. | |||
|
|||
Platforms and brokers may agree on an authentication mechanism other than |
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/brokers/Service Brokers/
s/may/MAY/
profile.md
Outdated
|
||
The [specification](spec.md) requires that brokers implement HTTP Basic authentication, but also allows for other mechanisms if the platform and the broker agree upon them outside of the specification. This section contains a list of supported mechanisms that platforms and brokers support. The list will be growing as time goes on. | ||
|
||
- The [Kubernetes Service Catalog](https://github.com/kubernetes-incubator/service-catalog) supports arbitrary HTTP Bearer tokens over TLS as an authentication mechanism |
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'm not sure we want to add this to the profile - it feels kind of awkward since each time an impl changes what it supports we'd need to publish a new version of the profile. Putting it in our wiki might be ok though. Or just pointing people to the docs for each impl.
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 we wanted to add something to the Profile about auth, then I think having auth-specific rules or guidance would be appropriate - stuff that all impls should adhere to.
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.
As a service author, where can I go to learn how to build a broker against this? The service-catalog readme doesn't direct me anywhere to learn this
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 why the info here shouldn't really be platform specific, rather, if anything, it would be auth specific. E.g. I don't think svc-cat's use of HTTP bearer tokens is unique, or at least I hope not, otherwise we wouldn't have interop.
I kind of wonder what the purpose of this text is. I don't think we should just reiterate what each platform's docs say w.r.t. which auth mechanisms it supports. That's not only duplicate info but requires us to keep things in sync. I'd prefer for this section to be more about any OSB API specific aspects of the various auth mechanisms Platforms support that people will need to know about if they want to write code - that way the odds of having interop go up.
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.
As a service author, where can I go to learn how to build a broker against this? The service-catalog readme doesn't direct me anywhere to learn this
We've got a general documentation issue we hope to address for the next major release which will help, but you are right, presently there isn't much doc on this
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.
+1 to ^^ - we don't have much to put here yet, but it would be nice to set the precedent. if we don't want to put specific systems in here, I would be ok with something like a "coming soon" in this section
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.
Per offline conversation with @duglin, @jboyd01 and myself, we will:
- move this section into the spec.md, replacing it with the link to profile.md
- link to a wiki page which @duglin will create
- the wiki page will be non-normative and contain at least a non-exhaustive list of known platforms (and possibly brokers in the future?) that implement other auth. mechanisms
profile.md
Outdated
@@ -232,6 +233,12 @@ part of a Kubernetes API call: | |||
} | |||
``` | |||
|
|||
## Platform to Service Broker Authentication | |||
|
|||
The [specification](spec.md) requires that brokers implement HTTP Basic authentication, but also allows for other mechanisms if the platform and the broker agree upon them outside of the specification. This section contains a list of supported mechanisms that platforms and brokers support. The list will be growing as time goes on. |
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/brokers/Service Brokers
profile.md
Outdated
|
||
The [specification](spec.md) requires that brokers implement HTTP Basic authentication, but also allows for other mechanisms if the platform and the broker agree upon them outside of the specification. This section contains a list of supported mechanisms that platforms and brokers support. The list will be growing as time goes on. | ||
|
||
- The [Kubernetes Service Catalog](https://github.com/kubernetes-incubator/service-catalog) supports arbitrary HTTP Bearer tokens over TLS as an authentication mechanism |
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.
As a service author, where can I go to learn how to build a broker against this? The service-catalog readme doesn't direct me anywhere to learn this
spec.md
Outdated
using the predetermined authentication mechanism and MUST return a `401 | ||
Unauthorized` response if the authentication fails. | ||
using the predetermined authentication mechanism, securing communications | ||
via TLS, and MUST return a `401 Unauthorized` response if the authentication |
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.
A dependency on TLS makes service broker development more difficult. For production TLS is a must, for development not necessarily. If we insist on TLS, do we allow self-signed certificates?
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'd be ok with self-signed certs for non-production deployments, or honestly even not requiring TLS in those cases
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.
rebase needed |
there are a ton of other changes in here, did you mean to make those? |
d9e00f2
to
2619553
Compare
Platforms and Service Brokers MAY agree on an authentication mechanism other | ||
than basic authentication, but the specific agreements are not covered by this | ||
specification. Please see the | ||
[Platform Features authentication mechanisms wiki document](https://github.com/openservicebrokerapi/servicebroker/wiki/Platform-Features) |
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 understand why this isn't in the spec, but I'm happy with this for now.
@duglin this should be all set, PTAL. |
spec.md
Outdated
using the predetermined authentication mechanism, and MUST return a `401 Unauthorized` | ||
response if the authentication fails. | ||
|
||
Additionally, the Service Broker MUST secure communucations with TLS. The Platform |
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.
communucations -> communications
fixes #381
per https://tools.ietf.org/html/rfc6750 I explicitly called out that when using bearer tokens you must be using TLS.