connector: add uaa connector

[whitlockjc]

This commit adds support for dex to authenticate users from a
CloudFoundry User Account and Authentication (UAA) Server.

Fixes: #538

[ericchiang]

Thanks! Will spin up a UAA Server after lunch to test this.

[whitlockjc]

I will get you a test recipe if you don't beat me to it.

[whitlockjc]

Below is an example test procedure.

Running UAA

• Clone UAA: git clone https://github.com/cloudfoundry/uaa.git
• Change directories: cd uaa
• Checkout latest release: git checkout 3.6.0
• Run UAA: ./gradlew run

Running Dex

• Build dex with this PR's changes (not documented since you might have your own way of doing this)
• Update static/fixtures/connectors.json.sample to have the following connector configuration in it:

{
  "type": "uaa",
  "id": "local-uaa",
  "clientId": "login",
  "clientSecret": "loginsecret",
  "serverURL": "http://localhost:8080/uaa"
}

• Run dex: ./bin/dex-worker --no-db --issuer=http://127.0.0.1:5556 --enable-registration=true
• Run the example application: ./bin/example-app --client-id=example-app --client-secret=example-app-secret --discovery=http://127.0.0.1:5556

Testing

• Access example application at http://localhost:5555
• Click the Register button
• Click the Use local-uaa button
• Fill in the form with marissa and koala as the username and password respectively
• Click the Create account button

Caveats

At this point, you'll see the following error: {"error":"unable to verify auth code with issuer: invalid_client"} When you look at the logs of the example application, you'll see that it failed to parse some response and you should see the following in the logs:

ERROR: error Decoding client creds: illegal base64 data at input byte 16
ERROR: Failed to Authenticate client example-app
ERROR: couldn't exchange code for token: invalid_client

When debugging this with my ops people, they said the token was successfully provided and so I'm thinking the example application might be busted. If that is not the case of course, I will help if the issue is in the UAA connector.

[whitlockjc]

It looks like the error is because, creds.Secret for the dex application are not Base64 encoded. So whenever client/manager/manager.go#L181 attempts to decode creds.Secret, it's not encoded and there is a failure. I'm not sure if OIDC is the issue (doubtful) or if dex should be updated to not decode an unencoding creds.Secret. I'll let you decide but I don't mind fixing it if there is a bug.

[ericchiang]

@whitlockjc why is the connector code interacting with the client manager?

[whitlockjc]

It's not. :) The error I ran into with the example-app during testing (mentioned in the Caveats section above) is produced in the client manager code. Here is the exact piece of code: https://github.com/coreos/dex/blob/a7b860b9c2dba69b7154feacbd0adbe0c52e2591/client/manager/manager.go#L181 Basically, creds.Secret here is being treated as a Base64 encoded string but it is not. To test this, I made the following change and things worked:

diff --git a/client/manager/manager.go b/client/manager/manager.go
index 75ad7f3..6d25023 100644
--- a/client/manager/manager.go
+++ b/client/manager/manager.go
@@ -180,8 +180,7 @@ func (m *ClientManager) Authenticate(creds oidc.ClientCredentials) (bool, error)

    dec, err := base64.URLEncoding.DecodeString(creds.Secret)
    if err != nil {
-       log.Errorf("error Decoding client creds: %v", err)
-       return false, nil
+       dec = []byte(creds.Secret)
    }

    ok := CompareHashAndPassword(clientSecret, dec) == nil

[ericchiang]

Ah yeah I see. #337 tracks that problem. This is because dex's db code always assumed that secrets would be base64 encoded, and when we switched to SQLite for our in-memory storage we ran up against this assumption. It's a pain in the ass, but I don't thing there's a good solution that'll be backward compatible with existing databases.

[whitlockjc]

Why not use the approach I showed? You make the assumption it is base64 encoded and if it fails to decode, you treat it as if it's not? Then it's backward compatible. I'm unfamiliar with the bigger issue so this might not be ideal but it seems pretty safe based on what little I know. Good news is I can completely test the UAA connector, successfully might I add. I'm super excited.

[ericchiang]

Why not use the approach I showed? You make the assumption it is base64 encoded and if it fails to decode, you treat it as if it's not? Then it's backward compatible.

Because of data already in the database. You can't do the same logic when you're pulling values out of the database. How would you know if those should be base64 encoded or not?

[whitlockjc]

Well, you can't. So you attempt to base64 decode and if that fails, you just treat the value as an unencoded string. I'm not sure of any other alternative. The diff above shows how you could do this, and it does work but I'm not sure of the ramifications of doing this in dex officially.

[ericchiang]

github?

[whitlockjc]

My bad, I thought I fixed that. I already told you I used github as the basis for this work. ;)

[whitlockjc]

Fixed both occurrences.

[ericchiang]

Couple notes. Run the dex-work with the following flags

./bin/dex-worker --no-db --issuer=http://127.0.0.1:5556 --enable-automatic-registration=true

--enable-automatic-registration will skip registration if you have an upstream account in uaa.

Also the base64 errors are because the example-app is using a bad client secret. It doesn't have anything to do with the connector. Use the one registered in the static fixtures instead ("ZXhhbXBsZS1hcHAtc2VjcmV0").

./bin/example-app --client-id=example-app --client-secret=ZXhhbXBsZS1hcHAtc2VjcmV0 --discovery=http://127.0.0.1:5556

[ericchiang]

For some reason this is always the empty string. Any idea why this is?

[whitlockjc]

I think it's just the way the test data for UAA is created. When I test against a live server, name is populated as expected. You can hit the /uaa/userinfo API directly to see if name is set or not but I don't think it is based on the UAA docs.

[whitlockjc]

I'm wrong. Below is the response from /uaa/userinfo for marissa:

{  
  "user_id":"d9b8dfd8-c420-4243-8b86-d957ef25f97c",
  "user_name":"marissa",
  "given_name":"Marissa",
  "family_name":"Bloggs",
  "email":"marissa@test.org",
  "phone_number":null,
  "name":"Marissa Bloggs"
}

I'll see what is up.

[whitlockjc]

I can verify that the Name is being parsed properly and given to oidc.Identity. Still digging.

[whitlockjc]

I think the example-app UI is suggesting that connector/connector_uaa.go#L118 is "" but it's not. When we create the oidc.Identity, the name is properly set to Marissa Bloggs. It seems that when the example-application creates the claims object which gets rendered, it isn't finding this. I'll keep digging but the UAA Connector itself is working properly at the line you referenced.

[whitlockjc]

One thing to note is that the example-app isn't printing out the details of the oidc.Identity created by the connector but is instead printing out the details of the OAuth2 token provided by the provider.

[whitlockjc]

I asked the team working on UAA and they said name is not a standard field: https://self-issued.info/docs/draft-ietf-oauth-json-web-token.html#rfc.section.4.1

[ericchiang]

It's an openid connect thing https://openid.net/specs/openid-connect-basic-1_0.html#StandardClaims

Not a blocker, just wondering.

[whitlockjc]

You are right but that is for the /uaa/userinfo response and the name is set there. The oidc.Identity object that we craft as a result of calling the UserInfo endpoint does have Name set properly. If you debug the line you pointed to, you'll see this is the case.

What I'm saying is that the JWT token you're inspecting in the example-app is an OAuth2 token and name is not a standard field for that response.

[ericchiang]

This was fixed by #537

[whitlockjc]

Gotcha. So the base64 issue was because of busted docs.

[ericchiang]

@whitlockjc yes. I'll update those docs.

[ericchiang]

lgtm. going to merge unless there are any objections.

[whitlockjc]

WOOHOO!!!