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

bearer token proposal #373

Merged
merged 3 commits into from Aug 17, 2017
Merged

bearer token proposal #373

merged 3 commits into from Aug 17, 2017

Conversation

jmrodri
Copy link
Contributor

@jmrodri jmrodri commented Aug 15, 2017

Add bearer token auth to the service broker.

Copy link
Member

@jwmatthews jwmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
I think going with the approach of FileTokenService to start makes sense.

ACK

Other items to consider for consistency sake, but not directly required for this
proposal:

- Rename `UserServiceAdapter` to `UserService`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

- Will this change APBs?

The bearer token will have no affect on the APBs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be any developer impact? Will bearer tokens be the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the proposal to answer your question, but both basic and bearer will be enabled in catasb but for make run all auth is currently disabled.


- How will the broker's behavior change?

The broker will now have 2 ways for authentication: basic auth and bearer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two different ways to authenticate. Are these two independent code paths? In other words can you enable basic auth and use a bearer token?

Can you expand this question into an implementation section. A section## Broker Behavior is a good place to answer this question in detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rthallisey yep, I can expand on it. You can have all the auths enabled if you want. Basically, the middleware handler will pick the first one that accepts the right authentication header. So if you have basic auth and bearer token enabled. And send in a Bearer token header, the basic auth provider will ignore it since there is no basic auth header. The bearer token provider will pick it up. If we had an SSL auth provider as well, it will have gotten skipped because the bearer token provider already processed the authentication.

For example, I want to have a `BasicAuth` that uses the `FileUserService` and a
`BasicAuth` that uses `DBUserService`, a fictitious service that loads users from a
database. Today's configuration does not support specifying a service backend
to a particular `AuthProvider`. Thoughts?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an expansion of existing auth. Does this belong here?

I'm trying to understand what's included as an AuthProvider. Is the existing auth an AuthProvider? Then I think the problem you're talking about here is the broker trying to figure out if a service backend is associated with either basic auth or a bearer token? Do I have that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rthallisey Yes, BasicAuth is an AuthProvider. And AuthProvider can be associated with some backend service adapter. Today BasicAuth is configured to use FileUserServiceAdapter. But conceivably I'd like to configure BasicAuth to use a database of users instead something called DBUserService. I'm not sure how to represent that in our configuration.

func createProvider(providerType string, log *logging.Logger) (Provider, error) {
	switch strings.ToLower(providerType) {
	case "basic":
		log.Info("Configured for basic auth")
		usa, err := GetUserServiceAdapter(log)
		if err != nil {
			return nil, err
		}
		return NewBasicAuth(usa, log), nil
	// add case "oauth":
	default:
		panic("Unknown auth provider")
	}
}

// GetUserServiceAdapter returns the configured UserServiceAdapter
func GetUserServiceAdapter(log *logging.Logger) (UserServiceAdapter, error) {
	// TODO: really need to figure out a better way to define what
	// should be returned.
	return NewFileUserServiceAdapter("/var/run/asb-auth", log)
}

I could extend the configuration to also include a key for the service backend to use:

  auth:
    - type: basic
      backend: file
      enabled: false
    - type: basic
      backend: db
      enabled: true

Or if we want to support multiple basic auths something like this:

  auth:
    - type: basicfile
      enabled: true
    - type: basicdb
      enabled: true

each would be distinct keys in the list of supported values.

@jmrodri
Copy link
Contributor Author

jmrodri commented Aug 16, 2017

@rthallisey I updated the docs to clarify the questions section, and I tried to answer your question about AuthProvider.

Copy link
Contributor

@rthallisey rthallisey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

`BasicAuth` that uses `DBUserService`, a fictitious service that loads users from a
database. Today's configuration does not support specifying a service backend
to a particular `AuthProvider`. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a solution for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rthallisey not yet, right now basicauth is tied to FileUserService and bearer auth would be tied to FileTokenService. Just need to decide how we want this in the config file.

@jmrodri jmrodri merged commit e74ec8c into master Aug 17, 2017
@jmrodri jmrodri deleted the bearer-auth-proposal branch August 18, 2017 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants