From e71c5086ba749cc1a5b932d191ed344f892a41ad Mon Sep 17 00:00:00 2001 From: Bobby Rullo Date: Thu, 2 Jun 2016 13:05:18 -0700 Subject: [PATCH] server: CodeToken now does Cross-Client auth --- scope/scope.go | 34 ++++++++ server/cross_client_test.go | 160 ++++++++++++++++++++++++++++++++---- server/http.go | 24 +++--- server/http_test.go | 23 +++--- server/server.go | 36 ++++++-- server/server_test.go | 36 ++++---- server/testutil.go | 13 +++ session/session.go | 10 ++- 8 files changed, 269 insertions(+), 67 deletions(-) create mode 100644 scope/scope.go diff --git a/scope/scope.go b/scope/scope.go new file mode 100644 index 00000000..50fd266f --- /dev/null +++ b/scope/scope.go @@ -0,0 +1,34 @@ +package scope + +import "strings" + +const ( + // Scope prefix which indicates initiation of a cross-client authentication flow. + // See https://developers.google.com/identity/protocols/CrossClientAuth + ScopeGoogleCrossClient = "audience:server:client_id:" +) + +type Scopes []string + +func (s Scopes) OfflineAccess() bool { + return s.HasScope("offline_access") +} + +func (s Scopes) HasScope(scope string) bool { + for _, curScope := range s { + if curScope == scope { + return true + } + } + return false +} + +func (s Scopes) CrossClientIDs() []string { + clients := []string{} + for _, scope := range s { + if strings.HasPrefix(scope, ScopeGoogleCrossClient) { + clients = append(clients, scope[len(ScopeGoogleCrossClient):]) + } + } + return clients +} diff --git a/server/cross_client_test.go b/server/cross_client_test.go index bb1a9771..4d1d4120 100644 --- a/server/cross_client_test.go +++ b/server/cross_client_test.go @@ -1,17 +1,22 @@ package server import ( + "encoding/base64" "fmt" "net/http" "net/http/httptest" "net/url" + "sort" "strings" "testing" "github.com/coreos/go-oidc/oidc" + "github.com/kylelemons/godebug/pretty" "github.com/coreos/dex/client" + clientmanager "github.com/coreos/dex/client/manager" "github.com/coreos/dex/connector" + "github.com/coreos/dex/scope" ) func makeCrossClientTestFixtures() (*testFixtures, error) { @@ -20,7 +25,6 @@ func makeCrossClientTestFixtures() (*testFixtures, error) { return nil, fmt.Errorf("couldn't make test fixtures: %v", err) } - creds := map[string]oidc.ClientCredentials{} for _, cliData := range []struct { id string authorized []string @@ -38,24 +42,22 @@ func makeCrossClientTestFixtures() (*testFixtures, error) { u := url.URL{ Scheme: "https://", Path: cliData.id, - Host: "auth.example.com", + Host: cliData.id, } - cliCreds, err := f.clientRepo.New(client.Client{ + cliCreds, err := f.clientManager.New(client.Client{ Credentials: oidc.ClientCredentials{ ID: cliData.id, }, Metadata: oidc.ClientMetadata{ RedirectURIs: []url.URL{u}, }, + }, &clientmanager.ClientOptions{ + TrustedPeers: cliData.authorized, }) if err != nil { return nil, fmt.Errorf("Unexpected error creating clients: %v", err) } - creds[cliData.id] = *cliCreds - err = f.clientRepo.SetTrustedPeers(cliData.id, cliData.authorized) - if err != nil { - return nil, fmt.Errorf("Unexpected error setting cross-client authorizers: %v", err) - } + f.clientCreds[cliData.id] = *cliCreds } return f, nil } @@ -132,30 +134,30 @@ func TestHandleAuthCrossClient(t *testing.T) { wantCode int }{ { - scopes: []string{ScopeGoogleCrossClient + "client_a"}, + scopes: []string{scope.ScopeGoogleCrossClient + "client_a"}, clientID: "client_b", wantCode: http.StatusBadRequest, }, { - scopes: []string{ScopeGoogleCrossClient + "client_b"}, + scopes: []string{scope.ScopeGoogleCrossClient + "client_b"}, clientID: "client_a", wantCode: http.StatusFound, }, { - scopes: []string{ScopeGoogleCrossClient + "client_b"}, + scopes: []string{scope.ScopeGoogleCrossClient + "client_b"}, clientID: "client_a", wantCode: http.StatusFound, }, { - scopes: []string{ScopeGoogleCrossClient + "client_c"}, + scopes: []string{scope.ScopeGoogleCrossClient + "client_c"}, clientID: "client_a", wantCode: http.StatusFound, }, { // Two clients that client_a is authorized to mint tokens for. scopes: []string{ - ScopeGoogleCrossClient + "client_c", - ScopeGoogleCrossClient + "client_b", + scope.ScopeGoogleCrossClient + "client_c", + scope.ScopeGoogleCrossClient + "client_b", }, clientID: "client_a", wantCode: http.StatusFound, @@ -163,8 +165,8 @@ func TestHandleAuthCrossClient(t *testing.T) { { // Two clients that client_a is authorized to mint tokens for. scopes: []string{ - ScopeGoogleCrossClient + "client_c", - ScopeGoogleCrossClient + "client_a", + scope.ScopeGoogleCrossClient + "client_c", + scope.ScopeGoogleCrossClient + "client_a", }, clientID: "client_b", wantCode: http.StatusBadRequest, @@ -200,3 +202,129 @@ func TestHandleAuthCrossClient(t *testing.T) { } } + +func TestServerCodeTokenCrossClient(t *testing.T) { + f, err := makeCrossClientTestFixtures() + if err != nil { + t.Fatalf("Error creating test fixtures: %v", err) + } + sm := f.sessionManager + + tests := []struct { + clientID string + offline bool + refreshToken string + crossClients []string + + wantErr bool + wantAUD []string + wantAZP string + }{ + // First test the non-cross-client cases, make sure they're undisturbed: + { + // No 'offline_access' in scope, should get empty refresh token. + clientID: testClientID, + refreshToken: "", + + wantAUD: []string{testClientID}, + }, + { + // Have 'offline_access' in scope, should get non-empty refresh token. + clientID: testClientID, + offline: true, + refreshToken: fmt.Sprintf("1/%s", base64.URLEncoding.EncodeToString([]byte("refresh-1"))), + + wantAUD: []string{testClientID}, + }, + // Now test cross-client cases: + { + clientID: "client_a", + crossClients: []string{"client_b"}, + + wantAUD: []string{"client_b"}, + wantAZP: "client_a", + }, + { + clientID: "client_a", + crossClients: []string{"client_b", "client_a"}, + + wantAUD: []string{"client_a", "client_b"}, + wantAZP: "client_a", + }, + } + + for i, tt := range tests { + scopes := []string{"openid"} + if tt.offline { + scopes = append(scopes, "offline_access") + } + for _, client := range tt.crossClients { + scopes = append(scopes, scope.ScopeGoogleCrossClient+client) + } + + sessionID, err := sm.NewSession("bogus_idpc", tt.clientID, "bogus", url.URL{}, "", false, scopes) + if err != nil { + t.Fatalf("case %d: unexpected error: %v", i, err) + } + _, err = sm.AttachRemoteIdentity(sessionID, oidc.Identity{}) + if err != nil { + t.Fatalf("case %d: unexpected error: %v", i, err) + } + + _, err = sm.AttachUser(sessionID, "ID-1") + if err != nil { + t.Fatalf("case %d: unexpected error: %v", i, err) + } + + key, err := sm.NewSessionKey(sessionID) + if err != nil { + t.Fatalf("case %d: unexpected error: %v", i, err) + } + + jwt, token, err := f.srv.CodeToken(f.clientCreds[tt.clientID], key) + if err != nil { + t.Fatalf("case %d: unexpected error: %v", i, err) + } + if jwt == nil { + t.Fatalf("case %d: expect non-nil jwt", i) + } + if token != tt.refreshToken { + t.Errorf("case %d: expect refresh token %q, got %q", i, tt.refreshToken, token) + } + + claims, err := jwt.Claims() + if err != nil { + t.Fatalf("case %d: unexpected error getting claims: %v", i, err) + } + + var gotAUD []string + if len(tt.wantAUD) < 2 { + aud, _, err := claims.StringClaim("aud") + if err != nil { + t.Fatalf("case %d: unexpected error getting 'aud': %q: raw: %v", i, err, claims["aud"]) + } + gotAUD = []string{aud} + } else { + gotAUD, _, err = claims.StringsClaim("aud") + if err != nil { + t.Fatalf("case %d: unexpected error getting 'aud': %v", i, err) + } + } + + sort.Strings(gotAUD) + if diff := pretty.Compare(tt.wantAUD, gotAUD); diff != "" { + t.Fatalf("case %d: pretty.Compare(tt.wantAUD, gotAUD): %v", i, diff) + } + + gotAZP, _, err := claims.StringClaim("azp") + if err != nil { + if err != nil { + t.Fatalf("case %d: unexpected error getting 'aud': %v", i, err) + } + } + + if gotAZP != tt.wantAZP { + t.Errorf("case %d: wantAZP=%v, gotAZP=%v", i, tt.wantAZP, gotAZP) + } + } +} diff --git a/server/http.go b/server/http.go index 2753806a..98da6de5 100644 --- a/server/http.go +++ b/server/http.go @@ -7,7 +7,6 @@ import ( "io" "net/http" "net/url" - "sort" "strings" "time" @@ -22,6 +21,7 @@ import ( "github.com/coreos/dex/connector" phttp "github.com/coreos/dex/pkg/http" "github.com/coreos/dex/pkg/log" + "github.com/coreos/dex/scope" ) const ( @@ -392,20 +392,18 @@ func handleAuthFunc(srv DexServer, idpcs []connector.Connector, tpl *template.Te func validateScopes(srv DexServer, clientID string, scopes []string) error { foundOpenIDScope := false - sort.Strings(scopes) - for i, scope := range scopes { - if i > 0 && scope == scopes[i-1] { + for i, curScope := range scopes { + if i > 0 && curScope == scopes[i-1] { err := oauth2.NewError(oauth2.ErrorInvalidRequest) err.Description = fmt.Sprintf( "Duplicate scopes are not allowed: %q", - scope) + curScope) return err } switch { - case strings.HasPrefix(scope, ScopeGoogleCrossClient): - otherClient := scope[len(ScopeGoogleCrossClient):] - + case strings.HasPrefix(curScope, scope.ScopeGoogleCrossClient): + otherClient := curScope[len(scope.ScopeGoogleCrossClient):] var allowed bool var err error if otherClient == clientID { @@ -424,11 +422,11 @@ func validateScopes(srv DexServer, clientID string, scopes []string) error { clientID, otherClient) return err } - case scope == "openid": + case curScope == "openid": foundOpenIDScope = true - case scope == "profile": - case scope == "email": - case scope == "offline_access": + case curScope == "profile": + case curScope == "email": + case curScope == "offline_access": // According to the spec, for offline_access scope, the client must // use a response_type value that would result in an Authorization // Code. Currently oauth2.ResponseTypeCode is the only supported @@ -439,7 +437,7 @@ func validateScopes(srv DexServer, clientID string, scopes []string) error { default: // Reject all other scopes. err := oauth2.NewError(oauth2.ErrorInvalidRequest) - err.Description = fmt.Sprintf("%q is not a recognized scope", scope) + err.Description = fmt.Sprintf("%q is not a recognized scope", curScope) return err } } diff --git a/server/http_test.go b/server/http_test.go index 67b9e105..180993d3 100644 --- a/server/http_test.go +++ b/server/http_test.go @@ -18,6 +18,7 @@ import ( "github.com/coreos/dex/client" "github.com/coreos/dex/connector" + "github.com/coreos/dex/scope" "github.com/coreos/go-oidc/jose" "github.com/coreos/go-oidc/oauth2" "github.com/coreos/go-oidc/oidc" @@ -346,21 +347,21 @@ func TestValidateScopes(t *testing.T) { { // ERR: invalid cross client auth clientID: "XXX", - scopes: []string{"openid", ScopeGoogleCrossClient + "client_a"}, + scopes: []string{"openid", scope.ScopeGoogleCrossClient + "client_a"}, wantErr: true, }, { // OK: valid cross client auth (though perverse - a client // requesting cross-client auth for itself) clientID: "client_a", - scopes: []string{"openid", ScopeGoogleCrossClient + "client_a"}, + scopes: []string{"openid", scope.ScopeGoogleCrossClient + "client_a"}, wantErr: false, }, { // OK: valid cross client auth clientID: "client_a", - scopes: []string{"openid", ScopeGoogleCrossClient + "client_b"}, + scopes: []string{"openid", scope.ScopeGoogleCrossClient + "client_b"}, wantErr: false, }, { @@ -368,8 +369,8 @@ func TestValidateScopes(t *testing.T) { // ERR: valid cross client auth...but duplicated scope. clientID: "client_a", scopes: []string{"openid", - ScopeGoogleCrossClient + "client_b", - ScopeGoogleCrossClient + "client_b", + scope.ScopeGoogleCrossClient + "client_b", + scope.ScopeGoogleCrossClient + "client_b", }, wantErr: true, }, @@ -378,9 +379,9 @@ func TestValidateScopes(t *testing.T) { clientID: "client_a", scopes: []string{ "openid", - ScopeGoogleCrossClient + "client_a", - ScopeGoogleCrossClient + "client_b", - ScopeGoogleCrossClient + "client_c", + scope.ScopeGoogleCrossClient + "client_a", + scope.ScopeGoogleCrossClient + "client_b", + scope.ScopeGoogleCrossClient + "client_c", }, wantErr: false, }, @@ -388,9 +389,9 @@ func TestValidateScopes(t *testing.T) { // ERR: valid cross client auth with >1 clients including itself...but no openid! clientID: "client_a", scopes: []string{ - ScopeGoogleCrossClient + "client_a", - ScopeGoogleCrossClient + "client_b", - ScopeGoogleCrossClient + "client_c", + scope.ScopeGoogleCrossClient + "client_a", + scope.ScopeGoogleCrossClient + "client_b", + scope.ScopeGoogleCrossClient + "client_c", }, wantErr: true, }, diff --git a/server/server.go b/server/server.go index 6eaf8b5e..8cb8d8b6 100644 --- a/server/server.go +++ b/server/server.go @@ -39,10 +39,6 @@ const ( ResetPasswordTemplateName = "reset-password.html" APIVersion = "v1" - - // Scope prefix which indicates initiation of a cross-client authentication flow. - // See https://developers.google.com/identity/protocols/CrossClientAuth - ScopeGoogleCrossClient = "audience:server:client_id:" ) type OIDCServer interface { @@ -454,6 +450,36 @@ func (s *Server) CodeToken(creds oidc.ClientCredentials, sessionKey string) (*jo claims := ses.Claims(s.IssuerURL.String()) user.AddToClaims(claims) + crossClientIDs := ses.Scope.CrossClientIDs() + if len(crossClientIDs) > 0 { + var aud []string + for _, id := range crossClientIDs { + if ses.ClientID == id { + aud = append(aud, id) + continue + } + allowed, err := s.CrossClientAuthAllowed(ses.ClientID, id) + if err != nil { + log.Errorf("Failed to check cross client auth. reqClientID %v; authClient:ID %v; err: %v", ses.ClientID, id, err) + return nil, "", oauth2.NewError(oauth2.ErrorServerError) + } + if !allowed { + err := oauth2.NewError(oauth2.ErrorInvalidRequest) + err.Description = fmt.Sprintf( + "%q is not authorized to perform cross-client requests for %q", + ses.ClientID, id) + return nil, "", err + } + aud = append(aud, id) + } + if len(aud) == 1 { + claims.Add("aud", aud[0]) + } else { + claims.Add("aud", aud) + } + claims.Add("azp", ses.ClientID) + } + jwt, err := jose.NewSignedJWT(claims, signer) if err != nil { log.Errorf("Failed to generate ID token: %v", err) @@ -538,7 +564,7 @@ func (s *Server) RefreshToken(creds oidc.ClientCredentials, token string) (*jose } func (s *Server) CrossClientAuthAllowed(requestingClientID, authorizingClientID string) (bool, error) { - alloweds, err := s.ClientRepo.GetTrustedPeers(authorizingClientID) + alloweds, err := s.ClientRepo.GetTrustedPeers(nil, authorizingClientID) if err != nil { return false, err } diff --git a/server/server_test.go b/server/server_test.go index f0eb30ce..1650d37f 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -9,16 +9,17 @@ import ( "testing" "time" - "github.com/coreos/dex/client" - "github.com/coreos/dex/db" - "github.com/coreos/dex/refresh/refreshtest" - "github.com/coreos/dex/session/manager" - "github.com/coreos/dex/user" "github.com/coreos/go-oidc/jose" "github.com/coreos/go-oidc/key" "github.com/coreos/go-oidc/oauth2" "github.com/coreos/go-oidc/oidc" "github.com/kylelemons/godebug/pretty" + + "github.com/coreos/dex/client" + "github.com/coreos/dex/db" + "github.com/coreos/dex/refresh/refreshtest" + "github.com/coreos/dex/session/manager" + "github.com/coreos/dex/user" ) var validRedirURL = url.URL{ @@ -266,6 +267,12 @@ func TestServerLoginDisabledUser(t *testing.T) { } func TestServerCodeToken(t *testing.T) { + f, err := makeTestFixtures() + if err != nil { + t.Fatalf("Error creating test fixtures: %v", err) + } + sm := f.sessionManager + tests := []struct { scope []string refreshToken string @@ -277,21 +284,14 @@ func TestServerCodeToken(t *testing.T) { }, // Have 'offline_access' in scope, should get non-empty refresh token. { - // NOTE(ericchiang): This test assumes that the database ID of the first - // refresh token will be "1". + // NOTE(ericchiang): This test assumes that the database ID of the + // first refresh token will be "1". scope: []string{"openid", "offline_access"}, refreshToken: fmt.Sprintf("1/%s", base64.URLEncoding.EncodeToString([]byte("refresh-1"))), }, } for i, tt := range tests { - f, err := makeTestFixtures() - if err != nil { - t.Fatalf("error making test fixtures: %v", err) - } - f.srv.RefreshTokenRepo = refreshtest.NewTestRefreshTokenRepo() - - sm := f.sessionManager sessionID, err := sm.NewSession("bogus_idpc", testClientID, "bogus", url.URL{}, "", false, tt.scope) if err != nil { t.Fatalf("case %d: unexpected error: %v", i, err) @@ -311,11 +311,9 @@ func TestServerCodeToken(t *testing.T) { t.Fatalf("case %d: unexpected error: %v", i, err) } - jwt, token, err := f.srv.CodeToken( - oidc.ClientCredentials{ - ID: testClientID, - Secret: clientTestSecret, - }, key) + jwt, token, err := f.srv.CodeToken(oidc.ClientCredentials{ + ID: testClientID, + Secret: clientTestSecret}, key) if err != nil { t.Fatalf("case %d: unexpected error: %v", i, err) } diff --git a/server/testutil.go b/server/testutil.go index be794a44..2a4ce254 100644 --- a/server/testutil.go +++ b/server/testutil.go @@ -14,6 +14,7 @@ import ( "github.com/coreos/dex/connector" "github.com/coreos/dex/db" "github.com/coreos/dex/email" + "github.com/coreos/dex/refresh/refreshtest" sessionmanager "github.com/coreos/dex/session/manager" "github.com/coreos/dex/user" useremail "github.com/coreos/dex/user/email" @@ -83,6 +84,11 @@ var ( } testPrivKey, _ = key.GeneratePrivateKey() + + testClientCreds = oidc.ClientCredentials{ + ID: testClientID, + Secret: base64.URLEncoding.EncodeToString([]byte("secret")), + } ) type testFixtures struct { @@ -93,6 +99,7 @@ type testFixtures struct { redirectURL url.URL clientRepo client.ClientRepo clientManager *clientmanager.ClientManager + clientCreds map[string]oidc.ClientCredentials } type testFixtureOptions struct { @@ -150,6 +157,8 @@ func makeTestFixturesWithOptions(options testFixtureOptions) (*testFixtures, err sessionManager := sessionmanager.NewSessionManager(db.NewSessionRepo(db.NewMemDB()), db.NewSessionKeyRepo(db.NewMemDB())) sessionManager.GenerateCode = sequentialGenerateCodeFunc() + refreshTokenRepo := refreshtest.NewTestRefreshTokenRepo() + emailer, err := email.NewTemplatizedEmailerFromGlobs( emailTemplatesLocation+"/*.txt", emailTemplatesLocation+"/*.html", @@ -210,6 +219,7 @@ func makeTestFixturesWithOptions(options testFixtureOptions) (*testFixtures, err UserManager: userManager, ClientManager: clientManager, KeyManager: km, + RefreshTokenRepo: refreshTokenRepo, } err = setTemplates(srv, tpl) @@ -243,5 +253,8 @@ func makeTestFixturesWithOptions(options testFixtureOptions) (*testFixtures, err emailer: emailer, clientRepo: clientRepo, clientManager: clientManager, + clientCreds: map[string]oidc.ClientCredentials{ + testClientID: testClientCreds, + }, }, nil } diff --git a/session/session.go b/session/session.go index ab35cfd4..050d60a8 100644 --- a/session/session.go +++ b/session/session.go @@ -6,6 +6,8 @@ import ( "github.com/coreos/go-oidc/jose" "github.com/coreos/go-oidc/oidc" + + "github.com/coreos/dex/scope" ) const ( @@ -46,11 +48,13 @@ type Session struct { // Regsiter indicates that this session is a registration flow. Register bool - // Nonce is optionally provided in the initial authorization request, and propogated in such cases to the generated claims. + // Nonce is optionally provided in the initial authorization request, and + // propogated in such cases to the generated claims. Nonce string - // Scope is the 'scope' field in the authentication request. Example scopes are 'openid', 'email', 'offline', etc. - Scope []string + // Scope is the 'scope' field in the authentication request. Example scopes + // are 'openid', 'email', 'offline', etc. + Scope scope.Scopes } // Claims returns a new set of Claims for the current session.