From 1e5133a98df19fb6e6107d68a19552413071abdf Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Sat, 8 Oct 2016 11:38:43 -0700 Subject: [PATCH 1/2] Documentation/proposals: add a proposal for keeping data in-sync during refreshes --- .../proposals/upstream-refreshing.md | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 Documentation/proposals/upstream-refreshing.md diff --git a/Documentation/proposals/upstream-refreshing.md b/Documentation/proposals/upstream-refreshing.md new file mode 100644 index 00000000..628ae89e --- /dev/null +++ b/Documentation/proposals/upstream-refreshing.md @@ -0,0 +1,150 @@ +# Proposal: upstream refreshing + +## TL;DR + +Today, if a user deletes their Google account, dex will keep allowing clients to +refresh tokens on that user's behalf because dex never checks back in with +Google. + +This is a proposal to change the connector package so the dex can check back +in with Google. + +## The problem + +When dex is federaing to an upstream identity provider (IDP), we want to ensure +claims being passed onto clients remain fresh. This includes data such as Google +accounts display names, LDAP group membership, account deactivations. Changes to +these on an upstream IDP should always be reflected in the claims dex passes to +its own clients. + +Refresh tokens make this complicated. When refreshing a token, unlike normal +logins, dex doesn't have the opportunity to prompt for user interaction. For +example, if dex is proxying to a LDAP server, it won't have the user's username +and passwords. + +Dex can't do this today because connectors have no concept of checking back in +with an upstream provider (with the sole exception of groups). They're only +called during the initial login, and never consulted when dex needs to mint a +new refresh token for a client. Additionally, connectors aren't actually aware +of the scopes being requested by the client, so they don't know when they should +setup the ability to check back in and have to treat every request identically. + +## Changes to the connector package + +The biggest changes proposed impact the connector package and connector +implementations. + +1. Connectors should be consulted when dex attempts to refresh a token. +2. Connectors should be aware of the scopes requested by the client. + +The second bullet is important because of the first. If a client isn't +requesting a refresh token, the connector shouldn't do the extra work, such as +requesting additional upstream scopes. + +to address the first point, a top level `Scopes` object will be added to the +connector package to express the scopes requested by the client. The +`CallbackConnector` and `PasswordConnector` will be updated accordingly. + +```go +// Scopes represents additional data requested by the clients about the end user. +type Scopes struct{ + // The client has requested a refresh token from the server. + OfflineAccess bool + + // The client has requested group information about the end user. + Groups bool +} + +// CallbackConnector is an interface implemented by connectors which use an OAuth +// style redirect flow to determine user information. +type CallbackConnector interface { + // The initial URL to redirect the user to. + // + // OAuth2 implementations should request different scopes from the upstream + // identity provider based on the scopes requested by the downstream client. + // For example, if the downstream client requests a refresh token from the + // server, the connector should also request a token from the provider. + LoginURL(s Scopes, callbackURL, state string) (string, error) + + // Handle the callback to the server and return an identity. + HandleCallback(s Scopes, r *http.Request) (identity Identity, state string, err error) +} + +// PasswordConnector is an interface implemented by connectors which take a +// username and password. +type PasswordConnector interface { + Login(s Scopes, username, password string) (identity Identity, validPassword bool, err error) +} +``` + +The existing `GroupsConnector` plays two roles. + +1. The connector only attempts to grab groups when the downstream client requests it. +2. Allow group information to be refreshed. + +The first issue is remedied by the added `Scopes` struct. This proposal also +hopes to generalize the need of the second role by adding a more general +`RefreshConnector`: + +```go +type Identity struct { + // Existing fields... + + // Groups are added to the identity object, since connectors are now told + // if they're being requested. + + // The set of groups a user is a member of. + Groups []string +} + + +// RefreshConnector is a connector that can update the client claims. +type RefreshConnector interface { + // Refresh is called when a client attempts to claim a refresh token. The + // connector should attempt to update the identity object to reflect any + // changes since the token was last refreshed. + Refresh(s Scopes, identity Identity) (Identity, error) + + // TODO(ericchiang): Should we allow connectors to indicate that the user has + // been delete or an upstream token has been revoked? This would allow us to + // know when we should remove the downstream refresh token, and when there was + // just a server error, but might be hard to determine for certain protocols. + // Might be safer to always delete the downstream token if the Refresh() + // method returns an error. +} +``` + +## Example changes to the "passwordDB" connector + +The `passwordDB` connector is the internal connector maintained by the server. +As an example, these are the changes to that connector if this change was +accepted. + +```go +func (db passwordDB) Login(s connector.Scopes, username, password string) (connector.Identity, bool, error) { + // No change to existing implementation. Scopes can be ignored since we'll + // always have access to the password objects. + +} + +func (db passwordDB) Refresh(s connector.Scopes, identity connector.Identity) (connector.Identity, error) { + // If the user has been deleted, the refresh token will be rejected. + p, err := db.s.GetPassword(identity.Email) + if err != nil { + if err == storage.ErrNotFound { + return connector.Identity{}, errors.New("user not found") + } + return connector.Identity{}, fmt.Errorf("get password: %v", err) + } + + // User removed but a new user with the same email exists. + if p.UserID != identity.UserID { + return connector.Identity{}, errors.New("user not found") + } + + // If a user has updated their username, that will be reflected in the + // refreshed token. + identity.Username = p.Username + return identity, nil +} +``` From 774242f750e4d5b624e9f84b8161435fcb7c2a3f Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 17 Oct 2016 11:53:46 -0700 Subject: [PATCH 2/2] Documentation/proposals: added a caveats section to upstream refreshing proposal --- .../proposals/upstream-refreshing.md | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/Documentation/proposals/upstream-refreshing.md b/Documentation/proposals/upstream-refreshing.md index 628ae89e..019bd9f8 100644 --- a/Documentation/proposals/upstream-refreshing.md +++ b/Documentation/proposals/upstream-refreshing.md @@ -2,12 +2,12 @@ ## TL;DR -Today, if a user deletes their Google account, dex will keep allowing clients to +Today, if a user deletes their GitHub account, dex will keep allowing clients to refresh tokens on that user's behalf because dex never checks back in with -Google. +GitHub. This is a proposal to change the connector package so the dex can check back -in with Google. +in with GitHub. ## The problem @@ -148,3 +148,18 @@ func (db passwordDB) Refresh(s connector.Scopes, identity connector.Identity) (c return identity, nil } ``` + +## Caveats + +Certain providers, such as Google, will only grant a single refresh token for each +client + end user pair. The second time one's requested, no refresh token is +returned. This means refresh tokens must be stored by dex as objects on an +upstream identity rather than part of a downstream refresh even. + +Right now `ConnectorData` is too general for this since it is only stored with a +refresh token and can't be shared between sessions. This should be rethought in +combination with the [`user-object.md`](./user-object.md) proposal to see if +there are reasonable ways for us to do this. + +This isn't a problem for providers like GitHub because they return the same +refresh token every time. We don't need to track a token per client.