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

Use Client defined in dex instead of go-oidc for storing clients #411

Merged
merged 9 commits into from
Apr 20, 2016

Conversation

bobbyrullo
Copy link
Contributor

We still use oidc.ClientMetadata and oidc.ClientCredentials but now we can easily add dex-specific stuff to the Client object. Will be very useful for the cross client stuff I'm about to do.

@bobbyrullo bobbyrullo closed this Apr 15, 2016
@bobbyrullo bobbyrullo reopened this Apr 15, 2016
func ClientsFromReader(r io.Reader) ([]Client, error) {
var c []struct {
ID string `json:"id"`
Secret []byte `json:"secret"`
Copy link
Contributor

@ericchiang ericchiang Apr 20, 2016

Choose a reason for hiding this comment

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

For --no-db mode, I think this now forces the secrets to be doubly base64 encoded. Once because of the []byte for encoding/json, then again for the db code.

  • Non base64 encoded secret. Doesn't work on master, still doesn't work on this branch.
[
  {
    "id": "example-app",
    "secret": "example-app-secret",
    "redirectURLs": ["http://127.0.0.1:5555/callback"]
  }
]
$ ./bin/dex-worker --no-db --clients clients.json
WARN: Running in-process without external database or key rotation
FATAL: Unable to build Server: unable to read clients from file clients.json: illegal base64 data at input byte 7
  • base64ed secret. Currently works on master, doesn't work on this branch.
[
  {
    "id": "example-app",
    "secret": "ZXhhbXBsZS1hcHAtc2VjcmV0",
    "redirectURLs": ["http://127.0.0.1:5555/callback"]
  }
]
$ ./bin/dex-worker --no-db --clients clients.json
WARN: Running in-process without external database or key rotation
FATAL: Unable to build Server: failed to create client identity repo: illegal base64 data at input byte 16
  • base64ed version of the base64ed secret. Currently "works" on master, only thing that works on this branch.
[
  {
    "id": "example-app",
    "secret": "WlhoaGJYQnNaUzFoY0hBdGMyVmpjbVYw",
    "redirectURLs": ["http://127.0.0.1:5555/callback"]
  }
]
$ ./bin/dex-worker --no-db --clients clients.json
WARN: Running in-process without external database or key rotation
INFO: Loaded IdP connector: id=local type=local
INFO: Loaded IdP connector: id=google type=oidc
INFO: Loaded IdP connector: id=github type=github
INFO: Loaded IdP connector: id=bitbucket type=bitbucket
WARN: bindTemplate not used when searchBeforeAuth specified.
INFO: Loaded IdP connector: id=ldap type=ldap
INFO: Binding to 127.0.0.1:5556...

@@ -78,7 +78,7 @@ func ValidRedirectURL(rURL *url.URL, redirectURLs []url.URL) (url.URL, error) {
func ClientsFromReader(r io.Reader) ([]Client, error) {
var c []struct {
ID string `json:"id"`
Secret []byte `json:"secret"`
Secret string `json:"secret"`
RedirectURLs []string `json:"redirectURLs"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericchiang This fixes the doubly encoded issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the type cast to a string later on in the function is no longer necessary.

Secret: string(client.Secret)

@ericchiang
Copy link
Contributor

small nit #411 (comment)

other than that lgtm

@bobbyrullo
Copy link
Contributor Author

fixed your nit - gonna squash now

Bobby Rullo added 7 commits April 20, 2016 14:31
This is instead of oidc.ClientIdentity. This makes it easier to add new
fields custom to dex to the client.
Get rid of all outdated "ClientIdentity" terminology.
Also require client ID and secret.
It's never used by downstream code, and besides, it's not really the
secret but a Hash of the secret.
* TestCreateClient was missing test coverage on error cases
* Fixed bug where 500s were being reported for bad requests
* changed function signature of NewAdminAPI back to old way of passing
  in lots of repos: passing in a DbMap made it difficult to test
* added swappable ID and Secret generators when creating Clients
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.

3 participants