From 91924ec685cde368117b9189d4ac98f6a3f20aee Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 10 Jan 2022 17:03:31 -0800 Subject: [PATCH] Revert adding allowAccessTokenBasedRefresh flag to OIDCIdentityProvider Signed-off-by: Margo Crawford --- .../types_oidcidentityprovider.go.tmpl | 7 -- ...or.pinniped.dev_oidcidentityproviders.yaml | 11 --- generated/1.17/README.adoc | 1 - .../v1alpha1/types_oidcidentityprovider.go | 7 -- ...or.pinniped.dev_oidcidentityproviders.yaml | 11 --- generated/1.18/README.adoc | 1 - .../v1alpha1/types_oidcidentityprovider.go | 7 -- ...or.pinniped.dev_oidcidentityproviders.yaml | 11 --- generated/1.19/README.adoc | 1 - .../v1alpha1/types_oidcidentityprovider.go | 7 -- ...or.pinniped.dev_oidcidentityproviders.yaml | 11 --- generated/1.20/README.adoc | 1 - .../v1alpha1/types_oidcidentityprovider.go | 7 -- ...or.pinniped.dev_oidcidentityproviders.yaml | 11 --- .../v1alpha1/types_oidcidentityprovider.go | 7 -- .../oidc_upstream_watcher.go | 11 +-- .../mockupstreamoidcidentityprovider.go | 14 --- internal/oidc/auth/auth_handler.go | 46 ++++----- internal/oidc/auth/auth_handler_test.go | 99 +++++++++++-------- .../provider/dynamic_upstream_idp_provider.go | 4 - internal/psession/pinniped_session.go | 9 +- .../testutil/oidctestutil/oidctestutil.go | 88 +++++++---------- internal/upstreamoidc/upstreamoidc.go | 25 ++--- pkg/oidcclient/login_test.go | 8 +- test/integration/supervisor_login_test.go | 3 +- 25 files changed, 147 insertions(+), 261 deletions(-) diff --git a/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl index 971a6639..798275a9 100644 --- a/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl @@ -111,13 +111,6 @@ type OIDCAuthorizationConfig struct { // allowPasswordGrant defaults to false. // +optional AllowPasswordGrant bool `json:"allowPasswordGrant,omitempty"` - - // allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access - // token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token - // from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend - // updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths - // will be tied to this access token lifetime. - AllowAccessTokenBasedRefresh bool `json:"allowAccessTokenBasedRefresh,omitempty"` } // Parameter is a key/value pair which represents a parameter in an HTTP request. diff --git a/deploy/supervisor/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml b/deploy/supervisor/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml index 8bdf8819..8ebd5eb5 100644 --- a/deploy/supervisor/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml +++ b/deploy/supervisor/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml @@ -151,17 +151,6 @@ spec: items: type: string type: array - allowAccessTokenBasedRefresh: - description: allowAccessTokenBasedRefresh, when true, will allow - a user to refresh their tokens by checking their upstream access - token against the user info endpoint, but skipping the refresh - token flow. If it is possible to acquire a refresh token from - your identity provider, you should do so. But if you can't, - this option allows the refresh flow to work. We recommend updating - the access token lifetime in your identity provider to at least - an hour, or up to 9 hours long. Users session lengths will be - tied to this access token lifetime. - type: boolean allowPasswordGrant: description: allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index 9dbb50f4..d18e7610 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -1102,7 +1102,6 @@ OIDCAuthorizationConfig provides information about how to form the OAuth2 author | *`additionalScopes`* __string array__ | additionalScopes are the additional scopes that will be requested from your OIDC provider in the authorization request during an OIDC Authorization Code Flow and in the token request during a Resource Owner Password Credentials Grant. Note that the "openid" scope will always be requested regardless of the value in this setting, since it is always required according to the OIDC spec. By default, when this field is not set, the Supervisor will request the following scopes: "openid", "offline_access", "email", and "profile". See https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims for a description of the "profile" and "email" scopes. See https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess for a description of the "offline_access" scope. This default value may change in future versions of Pinniped as the standard evolves, or as common patterns used by providers who implement the standard in the ecosystem evolve. By setting this list to anything other than an empty list, you are overriding the default value, so you may wish to include some of "offline_access", "email", and "profile" in your override list. If you do not want any of these scopes to be requested, you may set this list to contain only "openid". Some OIDC providers may also require a scope to get access to the user's group membership, in which case you may wish to include it in this list. Sometimes the scope to request the user's group membership is called "groups", but unfortunately this is not specified in the OIDC standard. Generally speaking, you should include any scopes required to cause the appropriate claims to be the returned by your OIDC provider in the ID token or userinfo endpoint results for those claims which you would like to use in the oidcClaims settings to determine the usernames and group memberships of your Kubernetes users. See your OIDC provider's documentation for more information about what scopes are available to request claims. Additionally, the Pinniped Supervisor requires that your OIDC provider returns refresh tokens to the Supervisor from these authorization flows. For most OIDC providers, the scope required to receive refresh tokens will be "offline_access". See the documentation of your OIDC provider's authorization and token endpoints for its requirements for what to include in the request in order to receive a refresh token in the response, if anything. Note that it may be safe to send "offline_access" even to providers which do not require it, since the provider may ignore scopes that it does not understand or require (see https://datatracker.ietf.org/doc/html/rfc6749#section-3.3). In the unusual case that you must avoid sending the "offline_access" scope, then you must override the default value of this setting. This is required if your OIDC provider will reject the request when it includes "offline_access" (e.g. GitLab's OIDC provider). | *`additionalAuthorizeParameters`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-parameter[$$Parameter$$] array__ | additionalAuthorizeParameters are extra query parameters that should be included in the authorize request to your OIDC provider in the authorization request during an OIDC Authorization Code Flow. By default, no extra parameters are sent. The standard parameters that will be sent are "response_type", "scope", "client_id", "state", "nonce", "code_challenge", "code_challenge_method", and "redirect_uri". These parameters cannot be included in this setting. Additionally, the "hd" parameter cannot be included in this setting at this time. The "hd" parameter is used by Google's OIDC provider to provide a hint as to which "hosted domain" the user should use during login. However, Pinniped does not yet support validating the hosted domain in the resulting ID token, so it is not yet safe to use this feature of Google's OIDC provider with Pinniped. This setting does not influence the parameters sent to the token endpoint in the Resource Owner Password Credentials Grant. The Pinniped Supervisor requires that your OIDC provider returns refresh tokens to the Supervisor from the authorization flows. Some OIDC providers may require a certain value for the "prompt" parameter in order to properly request refresh tokens. See the documentation of your OIDC provider's authorization endpoint for its requirements for what to include in the request in order to receive a refresh token in the response, if anything. If your provider requires the prompt parameter to request a refresh token, then include it here. Also note that most providers also require a certain scope to be requested in order to receive refresh tokens. See the additionalScopes setting for more information about using scopes to request refresh tokens. | *`allowPasswordGrant`* __boolean__ | allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see https://datatracker.ietf.org/doc/html/rfc6749#section-4.3) to authenticate to the OIDC provider using a username and password without a web browser, in addition to the usual browser-based OIDC Authorization Code Flow. The Resource Owner Password Credentials Grant is not officially part of the OIDC specification, so it may not be supported by your OIDC provider. If your OIDC provider supports returning ID tokens from a Resource Owner Password Credentials Grant token request, then you can choose to set this field to true. This will allow end users to choose to present their username and password to the kubectl CLI (using the Pinniped plugin) to authenticate to the cluster, without using a web browser to log in as is customary in OIDC Authorization Code Flow. This may be convenient for users, especially for identities from your OIDC provider which are not intended to represent a human actor, such as service accounts performing actions in a CI/CD environment. Even if your OIDC provider supports it, you may wish to disable this behavior by setting this field to false when you prefer to only allow users of this OIDCIdentityProvider to log in via the browser-based OIDC Authorization Code Flow. Using the Resource Owner Password Credentials Grant means that the Pinniped CLI and Pinniped Supervisor will directly handle your end users' passwords (similar to LDAPIdentityProvider), and you will not be able to require multi-factor authentication or use the other web-based login features of your OIDC provider during Resource Owner Password Credentials Grant logins. allowPasswordGrant defaults to false. -| *`allowAccessTokenBasedRefresh`* __boolean__ | allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths will be tied to this access token lifetime. |=== diff --git a/generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index 971a6639..798275a9 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go @@ -111,13 +111,6 @@ type OIDCAuthorizationConfig struct { // allowPasswordGrant defaults to false. // +optional AllowPasswordGrant bool `json:"allowPasswordGrant,omitempty"` - - // allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access - // token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token - // from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend - // updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths - // will be tied to this access token lifetime. - AllowAccessTokenBasedRefresh bool `json:"allowAccessTokenBasedRefresh,omitempty"` } // Parameter is a key/value pair which represents a parameter in an HTTP request. diff --git a/generated/1.17/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml b/generated/1.17/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml index 8bdf8819..8ebd5eb5 100644 --- a/generated/1.17/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml +++ b/generated/1.17/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml @@ -151,17 +151,6 @@ spec: items: type: string type: array - allowAccessTokenBasedRefresh: - description: allowAccessTokenBasedRefresh, when true, will allow - a user to refresh their tokens by checking their upstream access - token against the user info endpoint, but skipping the refresh - token flow. If it is possible to acquire a refresh token from - your identity provider, you should do so. But if you can't, - this option allows the refresh flow to work. We recommend updating - the access token lifetime in your identity provider to at least - an hour, or up to 9 hours long. Users session lengths will be - tied to this access token lifetime. - type: boolean allowPasswordGrant: description: allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index c47f632e..417b57ac 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -1102,7 +1102,6 @@ OIDCAuthorizationConfig provides information about how to form the OAuth2 author | *`additionalScopes`* __string array__ | additionalScopes are the additional scopes that will be requested from your OIDC provider in the authorization request during an OIDC Authorization Code Flow and in the token request during a Resource Owner Password Credentials Grant. Note that the "openid" scope will always be requested regardless of the value in this setting, since it is always required according to the OIDC spec. By default, when this field is not set, the Supervisor will request the following scopes: "openid", "offline_access", "email", and "profile". See https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims for a description of the "profile" and "email" scopes. See https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess for a description of the "offline_access" scope. This default value may change in future versions of Pinniped as the standard evolves, or as common patterns used by providers who implement the standard in the ecosystem evolve. By setting this list to anything other than an empty list, you are overriding the default value, so you may wish to include some of "offline_access", "email", and "profile" in your override list. If you do not want any of these scopes to be requested, you may set this list to contain only "openid". Some OIDC providers may also require a scope to get access to the user's group membership, in which case you may wish to include it in this list. Sometimes the scope to request the user's group membership is called "groups", but unfortunately this is not specified in the OIDC standard. Generally speaking, you should include any scopes required to cause the appropriate claims to be the returned by your OIDC provider in the ID token or userinfo endpoint results for those claims which you would like to use in the oidcClaims settings to determine the usernames and group memberships of your Kubernetes users. See your OIDC provider's documentation for more information about what scopes are available to request claims. Additionally, the Pinniped Supervisor requires that your OIDC provider returns refresh tokens to the Supervisor from these authorization flows. For most OIDC providers, the scope required to receive refresh tokens will be "offline_access". See the documentation of your OIDC provider's authorization and token endpoints for its requirements for what to include in the request in order to receive a refresh token in the response, if anything. Note that it may be safe to send "offline_access" even to providers which do not require it, since the provider may ignore scopes that it does not understand or require (see https://datatracker.ietf.org/doc/html/rfc6749#section-3.3). In the unusual case that you must avoid sending the "offline_access" scope, then you must override the default value of this setting. This is required if your OIDC provider will reject the request when it includes "offline_access" (e.g. GitLab's OIDC provider). | *`additionalAuthorizeParameters`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-parameter[$$Parameter$$] array__ | additionalAuthorizeParameters are extra query parameters that should be included in the authorize request to your OIDC provider in the authorization request during an OIDC Authorization Code Flow. By default, no extra parameters are sent. The standard parameters that will be sent are "response_type", "scope", "client_id", "state", "nonce", "code_challenge", "code_challenge_method", and "redirect_uri". These parameters cannot be included in this setting. Additionally, the "hd" parameter cannot be included in this setting at this time. The "hd" parameter is used by Google's OIDC provider to provide a hint as to which "hosted domain" the user should use during login. However, Pinniped does not yet support validating the hosted domain in the resulting ID token, so it is not yet safe to use this feature of Google's OIDC provider with Pinniped. This setting does not influence the parameters sent to the token endpoint in the Resource Owner Password Credentials Grant. The Pinniped Supervisor requires that your OIDC provider returns refresh tokens to the Supervisor from the authorization flows. Some OIDC providers may require a certain value for the "prompt" parameter in order to properly request refresh tokens. See the documentation of your OIDC provider's authorization endpoint for its requirements for what to include in the request in order to receive a refresh token in the response, if anything. If your provider requires the prompt parameter to request a refresh token, then include it here. Also note that most providers also require a certain scope to be requested in order to receive refresh tokens. See the additionalScopes setting for more information about using scopes to request refresh tokens. | *`allowPasswordGrant`* __boolean__ | allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see https://datatracker.ietf.org/doc/html/rfc6749#section-4.3) to authenticate to the OIDC provider using a username and password without a web browser, in addition to the usual browser-based OIDC Authorization Code Flow. The Resource Owner Password Credentials Grant is not officially part of the OIDC specification, so it may not be supported by your OIDC provider. If your OIDC provider supports returning ID tokens from a Resource Owner Password Credentials Grant token request, then you can choose to set this field to true. This will allow end users to choose to present their username and password to the kubectl CLI (using the Pinniped plugin) to authenticate to the cluster, without using a web browser to log in as is customary in OIDC Authorization Code Flow. This may be convenient for users, especially for identities from your OIDC provider which are not intended to represent a human actor, such as service accounts performing actions in a CI/CD environment. Even if your OIDC provider supports it, you may wish to disable this behavior by setting this field to false when you prefer to only allow users of this OIDCIdentityProvider to log in via the browser-based OIDC Authorization Code Flow. Using the Resource Owner Password Credentials Grant means that the Pinniped CLI and Pinniped Supervisor will directly handle your end users' passwords (similar to LDAPIdentityProvider), and you will not be able to require multi-factor authentication or use the other web-based login features of your OIDC provider during Resource Owner Password Credentials Grant logins. allowPasswordGrant defaults to false. -| *`allowAccessTokenBasedRefresh`* __boolean__ | allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths will be tied to this access token lifetime. |=== diff --git a/generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index 971a6639..798275a9 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go @@ -111,13 +111,6 @@ type OIDCAuthorizationConfig struct { // allowPasswordGrant defaults to false. // +optional AllowPasswordGrant bool `json:"allowPasswordGrant,omitempty"` - - // allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access - // token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token - // from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend - // updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths - // will be tied to this access token lifetime. - AllowAccessTokenBasedRefresh bool `json:"allowAccessTokenBasedRefresh,omitempty"` } // Parameter is a key/value pair which represents a parameter in an HTTP request. diff --git a/generated/1.18/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml b/generated/1.18/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml index 8bdf8819..8ebd5eb5 100644 --- a/generated/1.18/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml +++ b/generated/1.18/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml @@ -151,17 +151,6 @@ spec: items: type: string type: array - allowAccessTokenBasedRefresh: - description: allowAccessTokenBasedRefresh, when true, will allow - a user to refresh their tokens by checking their upstream access - token against the user info endpoint, but skipping the refresh - token flow. If it is possible to acquire a refresh token from - your identity provider, you should do so. But if you can't, - this option allows the refresh flow to work. We recommend updating - the access token lifetime in your identity provider to at least - an hour, or up to 9 hours long. Users session lengths will be - tied to this access token lifetime. - type: boolean allowPasswordGrant: description: allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index a981cb54..0317faed 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -1102,7 +1102,6 @@ OIDCAuthorizationConfig provides information about how to form the OAuth2 author | *`additionalScopes`* __string array__ | additionalScopes are the additional scopes that will be requested from your OIDC provider in the authorization request during an OIDC Authorization Code Flow and in the token request during a Resource Owner Password Credentials Grant. Note that the "openid" scope will always be requested regardless of the value in this setting, since it is always required according to the OIDC spec. By default, when this field is not set, the Supervisor will request the following scopes: "openid", "offline_access", "email", and "profile". See https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims for a description of the "profile" and "email" scopes. See https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess for a description of the "offline_access" scope. This default value may change in future versions of Pinniped as the standard evolves, or as common patterns used by providers who implement the standard in the ecosystem evolve. By setting this list to anything other than an empty list, you are overriding the default value, so you may wish to include some of "offline_access", "email", and "profile" in your override list. If you do not want any of these scopes to be requested, you may set this list to contain only "openid". Some OIDC providers may also require a scope to get access to the user's group membership, in which case you may wish to include it in this list. Sometimes the scope to request the user's group membership is called "groups", but unfortunately this is not specified in the OIDC standard. Generally speaking, you should include any scopes required to cause the appropriate claims to be the returned by your OIDC provider in the ID token or userinfo endpoint results for those claims which you would like to use in the oidcClaims settings to determine the usernames and group memberships of your Kubernetes users. See your OIDC provider's documentation for more information about what scopes are available to request claims. Additionally, the Pinniped Supervisor requires that your OIDC provider returns refresh tokens to the Supervisor from these authorization flows. For most OIDC providers, the scope required to receive refresh tokens will be "offline_access". See the documentation of your OIDC provider's authorization and token endpoints for its requirements for what to include in the request in order to receive a refresh token in the response, if anything. Note that it may be safe to send "offline_access" even to providers which do not require it, since the provider may ignore scopes that it does not understand or require (see https://datatracker.ietf.org/doc/html/rfc6749#section-3.3). In the unusual case that you must avoid sending the "offline_access" scope, then you must override the default value of this setting. This is required if your OIDC provider will reject the request when it includes "offline_access" (e.g. GitLab's OIDC provider). | *`additionalAuthorizeParameters`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-parameter[$$Parameter$$] array__ | additionalAuthorizeParameters are extra query parameters that should be included in the authorize request to your OIDC provider in the authorization request during an OIDC Authorization Code Flow. By default, no extra parameters are sent. The standard parameters that will be sent are "response_type", "scope", "client_id", "state", "nonce", "code_challenge", "code_challenge_method", and "redirect_uri". These parameters cannot be included in this setting. Additionally, the "hd" parameter cannot be included in this setting at this time. The "hd" parameter is used by Google's OIDC provider to provide a hint as to which "hosted domain" the user should use during login. However, Pinniped does not yet support validating the hosted domain in the resulting ID token, so it is not yet safe to use this feature of Google's OIDC provider with Pinniped. This setting does not influence the parameters sent to the token endpoint in the Resource Owner Password Credentials Grant. The Pinniped Supervisor requires that your OIDC provider returns refresh tokens to the Supervisor from the authorization flows. Some OIDC providers may require a certain value for the "prompt" parameter in order to properly request refresh tokens. See the documentation of your OIDC provider's authorization endpoint for its requirements for what to include in the request in order to receive a refresh token in the response, if anything. If your provider requires the prompt parameter to request a refresh token, then include it here. Also note that most providers also require a certain scope to be requested in order to receive refresh tokens. See the additionalScopes setting for more information about using scopes to request refresh tokens. | *`allowPasswordGrant`* __boolean__ | allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see https://datatracker.ietf.org/doc/html/rfc6749#section-4.3) to authenticate to the OIDC provider using a username and password without a web browser, in addition to the usual browser-based OIDC Authorization Code Flow. The Resource Owner Password Credentials Grant is not officially part of the OIDC specification, so it may not be supported by your OIDC provider. If your OIDC provider supports returning ID tokens from a Resource Owner Password Credentials Grant token request, then you can choose to set this field to true. This will allow end users to choose to present their username and password to the kubectl CLI (using the Pinniped plugin) to authenticate to the cluster, without using a web browser to log in as is customary in OIDC Authorization Code Flow. This may be convenient for users, especially for identities from your OIDC provider which are not intended to represent a human actor, such as service accounts performing actions in a CI/CD environment. Even if your OIDC provider supports it, you may wish to disable this behavior by setting this field to false when you prefer to only allow users of this OIDCIdentityProvider to log in via the browser-based OIDC Authorization Code Flow. Using the Resource Owner Password Credentials Grant means that the Pinniped CLI and Pinniped Supervisor will directly handle your end users' passwords (similar to LDAPIdentityProvider), and you will not be able to require multi-factor authentication or use the other web-based login features of your OIDC provider during Resource Owner Password Credentials Grant logins. allowPasswordGrant defaults to false. -| *`allowAccessTokenBasedRefresh`* __boolean__ | allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths will be tied to this access token lifetime. |=== diff --git a/generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index 971a6639..798275a9 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go @@ -111,13 +111,6 @@ type OIDCAuthorizationConfig struct { // allowPasswordGrant defaults to false. // +optional AllowPasswordGrant bool `json:"allowPasswordGrant,omitempty"` - - // allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access - // token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token - // from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend - // updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths - // will be tied to this access token lifetime. - AllowAccessTokenBasedRefresh bool `json:"allowAccessTokenBasedRefresh,omitempty"` } // Parameter is a key/value pair which represents a parameter in an HTTP request. diff --git a/generated/1.19/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml b/generated/1.19/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml index 8bdf8819..8ebd5eb5 100644 --- a/generated/1.19/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml +++ b/generated/1.19/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml @@ -151,17 +151,6 @@ spec: items: type: string type: array - allowAccessTokenBasedRefresh: - description: allowAccessTokenBasedRefresh, when true, will allow - a user to refresh their tokens by checking their upstream access - token against the user info endpoint, but skipping the refresh - token flow. If it is possible to acquire a refresh token from - your identity provider, you should do so. But if you can't, - this option allows the refresh flow to work. We recommend updating - the access token lifetime in your identity provider to at least - an hour, or up to 9 hours long. Users session lengths will be - tied to this access token lifetime. - type: boolean allowPasswordGrant: description: allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see diff --git a/generated/1.20/README.adoc b/generated/1.20/README.adoc index 3de7b4b0..faf7ad54 100644 --- a/generated/1.20/README.adoc +++ b/generated/1.20/README.adoc @@ -1102,7 +1102,6 @@ OIDCAuthorizationConfig provides information about how to form the OAuth2 author | *`additionalScopes`* __string array__ | additionalScopes are the additional scopes that will be requested from your OIDC provider in the authorization request during an OIDC Authorization Code Flow and in the token request during a Resource Owner Password Credentials Grant. Note that the "openid" scope will always be requested regardless of the value in this setting, since it is always required according to the OIDC spec. By default, when this field is not set, the Supervisor will request the following scopes: "openid", "offline_access", "email", and "profile". See https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims for a description of the "profile" and "email" scopes. See https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess for a description of the "offline_access" scope. This default value may change in future versions of Pinniped as the standard evolves, or as common patterns used by providers who implement the standard in the ecosystem evolve. By setting this list to anything other than an empty list, you are overriding the default value, so you may wish to include some of "offline_access", "email", and "profile" in your override list. If you do not want any of these scopes to be requested, you may set this list to contain only "openid". Some OIDC providers may also require a scope to get access to the user's group membership, in which case you may wish to include it in this list. Sometimes the scope to request the user's group membership is called "groups", but unfortunately this is not specified in the OIDC standard. Generally speaking, you should include any scopes required to cause the appropriate claims to be the returned by your OIDC provider in the ID token or userinfo endpoint results for those claims which you would like to use in the oidcClaims settings to determine the usernames and group memberships of your Kubernetes users. See your OIDC provider's documentation for more information about what scopes are available to request claims. Additionally, the Pinniped Supervisor requires that your OIDC provider returns refresh tokens to the Supervisor from these authorization flows. For most OIDC providers, the scope required to receive refresh tokens will be "offline_access". See the documentation of your OIDC provider's authorization and token endpoints for its requirements for what to include in the request in order to receive a refresh token in the response, if anything. Note that it may be safe to send "offline_access" even to providers which do not require it, since the provider may ignore scopes that it does not understand or require (see https://datatracker.ietf.org/doc/html/rfc6749#section-3.3). In the unusual case that you must avoid sending the "offline_access" scope, then you must override the default value of this setting. This is required if your OIDC provider will reject the request when it includes "offline_access" (e.g. GitLab's OIDC provider). | *`additionalAuthorizeParameters`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-parameter[$$Parameter$$] array__ | additionalAuthorizeParameters are extra query parameters that should be included in the authorize request to your OIDC provider in the authorization request during an OIDC Authorization Code Flow. By default, no extra parameters are sent. The standard parameters that will be sent are "response_type", "scope", "client_id", "state", "nonce", "code_challenge", "code_challenge_method", and "redirect_uri". These parameters cannot be included in this setting. Additionally, the "hd" parameter cannot be included in this setting at this time. The "hd" parameter is used by Google's OIDC provider to provide a hint as to which "hosted domain" the user should use during login. However, Pinniped does not yet support validating the hosted domain in the resulting ID token, so it is not yet safe to use this feature of Google's OIDC provider with Pinniped. This setting does not influence the parameters sent to the token endpoint in the Resource Owner Password Credentials Grant. The Pinniped Supervisor requires that your OIDC provider returns refresh tokens to the Supervisor from the authorization flows. Some OIDC providers may require a certain value for the "prompt" parameter in order to properly request refresh tokens. See the documentation of your OIDC provider's authorization endpoint for its requirements for what to include in the request in order to receive a refresh token in the response, if anything. If your provider requires the prompt parameter to request a refresh token, then include it here. Also note that most providers also require a certain scope to be requested in order to receive refresh tokens. See the additionalScopes setting for more information about using scopes to request refresh tokens. | *`allowPasswordGrant`* __boolean__ | allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see https://datatracker.ietf.org/doc/html/rfc6749#section-4.3) to authenticate to the OIDC provider using a username and password without a web browser, in addition to the usual browser-based OIDC Authorization Code Flow. The Resource Owner Password Credentials Grant is not officially part of the OIDC specification, so it may not be supported by your OIDC provider. If your OIDC provider supports returning ID tokens from a Resource Owner Password Credentials Grant token request, then you can choose to set this field to true. This will allow end users to choose to present their username and password to the kubectl CLI (using the Pinniped plugin) to authenticate to the cluster, without using a web browser to log in as is customary in OIDC Authorization Code Flow. This may be convenient for users, especially for identities from your OIDC provider which are not intended to represent a human actor, such as service accounts performing actions in a CI/CD environment. Even if your OIDC provider supports it, you may wish to disable this behavior by setting this field to false when you prefer to only allow users of this OIDCIdentityProvider to log in via the browser-based OIDC Authorization Code Flow. Using the Resource Owner Password Credentials Grant means that the Pinniped CLI and Pinniped Supervisor will directly handle your end users' passwords (similar to LDAPIdentityProvider), and you will not be able to require multi-factor authentication or use the other web-based login features of your OIDC provider during Resource Owner Password Credentials Grant logins. allowPasswordGrant defaults to false. -| *`allowAccessTokenBasedRefresh`* __boolean__ | allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths will be tied to this access token lifetime. |=== diff --git a/generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index 971a6639..798275a9 100644 --- a/generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go @@ -111,13 +111,6 @@ type OIDCAuthorizationConfig struct { // allowPasswordGrant defaults to false. // +optional AllowPasswordGrant bool `json:"allowPasswordGrant,omitempty"` - - // allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access - // token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token - // from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend - // updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths - // will be tied to this access token lifetime. - AllowAccessTokenBasedRefresh bool `json:"allowAccessTokenBasedRefresh,omitempty"` } // Parameter is a key/value pair which represents a parameter in an HTTP request. diff --git a/generated/1.20/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml b/generated/1.20/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml index 8bdf8819..8ebd5eb5 100644 --- a/generated/1.20/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml +++ b/generated/1.20/crds/idp.supervisor.pinniped.dev_oidcidentityproviders.yaml @@ -151,17 +151,6 @@ spec: items: type: string type: array - allowAccessTokenBasedRefresh: - description: allowAccessTokenBasedRefresh, when true, will allow - a user to refresh their tokens by checking their upstream access - token against the user info endpoint, but skipping the refresh - token flow. If it is possible to acquire a refresh token from - your identity provider, you should do so. But if you can't, - this option allows the refresh flow to work. We recommend updating - the access token lifetime in your identity provider to at least - an hour, or up to 9 hours long. Users session lengths will be - tied to this access token lifetime. - type: boolean allowPasswordGrant: description: allowPasswordGrant, when true, will allow the use of OAuth 2.0's Resource Owner Password Credentials Grant (see diff --git a/generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go b/generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go index 971a6639..798275a9 100644 --- a/generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go +++ b/generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go @@ -111,13 +111,6 @@ type OIDCAuthorizationConfig struct { // allowPasswordGrant defaults to false. // +optional AllowPasswordGrant bool `json:"allowPasswordGrant,omitempty"` - - // allowAccessTokenBasedRefresh, when true, will allow a user to refresh their tokens by checking their upstream access - // token against the user info endpoint, but skipping the refresh token flow. If it is possible to acquire a refresh token - // from your identity provider, you should do so. But if you can't, this option allows the refresh flow to work. We recommend - // updating the access token lifetime in your identity provider to at least an hour, or up to 9 hours long. Users session lengths - // will be tied to this access token lifetime. - AllowAccessTokenBasedRefresh bool `json:"allowAccessTokenBasedRefresh,omitempty"` } // Parameter is a key/value pair which represents a parameter in an HTTP request. diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index af2b23a2..4669cdc3 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -210,12 +210,11 @@ func (c *oidcWatcherController) validateUpstream(ctx controllerlib.Context, upst Config: &oauth2.Config{ Scopes: computeScopes(authorizationConfig.AdditionalScopes), }, - UsernameClaim: upstream.Spec.Claims.Username, - GroupsClaim: upstream.Spec.Claims.Groups, - AllowPasswordGrant: authorizationConfig.AllowPasswordGrant, - AllowAccessTokenBasedRefresh: authorizationConfig.AllowAccessTokenBasedRefresh, - AdditionalAuthcodeParams: additionalAuthcodeAuthorizeParameters, - ResourceUID: upstream.UID, + UsernameClaim: upstream.Spec.Claims.Username, + GroupsClaim: upstream.Spec.Claims.Groups, + AllowPasswordGrant: authorizationConfig.AllowPasswordGrant, + AdditionalAuthcodeParams: additionalAuthcodeAuthorizeParameters, + ResourceUID: upstream.UID, } conditions := []*v1alpha1.Condition{ diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 8dd90af0..07bff23e 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -45,20 +45,6 @@ func (m *MockUpstreamOIDCIdentityProviderI) EXPECT() *MockUpstreamOIDCIdentityPr return m.recorder } -// AllowsAccessTokenBasedRefresh mocks base method. -func (m *MockUpstreamOIDCIdentityProviderI) AllowsAccessTokenBasedRefresh() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AllowsAccessTokenBasedRefresh") - ret0, _ := ret[0].(bool) - return ret0 -} - -// AllowsAccessTokenBasedRefresh indicates an expected call of AllowsAccessTokenBasedRefresh. -func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) AllowsAccessTokenBasedRefresh() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AllowsAccessTokenBasedRefresh", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).AllowsAccessTokenBasedRefresh)) -} - // AllowsPasswordGrant mocks base method. func (m *MockUpstreamOIDCIdentityProviderI) AllowsPasswordGrant() bool { m.ctrl.T.Helper() diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index df6fdd92..737567a9 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -174,17 +174,6 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( fosite.ErrAccessDenied.WithDebug(err.Error()), true) // WithDebug hides the error from the client } - if token.RefreshToken == nil || token.RefreshToken.Token == "" && !oidcUpstream.AllowsAccessTokenBasedRefresh() { - // TODO change warning message - plog.Warning("refresh token not returned by upstream provider during password grant, "+ - "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI", - "upstreamName", oidcUpstream.GetName(), - "scopes", oidcUpstream.GetScopes()) - return writeAuthorizeError(w, oauthHelper, authorizeRequester, - fosite.ErrAccessDenied.WithHint( - "Refresh token not returned by upstream provider during password grant."), true) - } - subject, username, groups, err := downstreamsession.GetDownstreamIdentityFromUpstreamIDToken(oidcUpstream, token.IDToken.Claims) if err != nil { // Return a user-friendly error for this case which is entirely within our control. @@ -212,22 +201,33 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( ProviderName: oidcUpstream.GetName(), ProviderType: psession.ProviderTypeOIDC, OIDC: &psession.OIDCSessionData{ - UpstreamRefreshToken: token.RefreshToken.Token, - UpstreamIssuer: upstreamIssuer, - UpstreamSubject: upstreamSubject, + UpstreamIssuer: upstreamIssuer, + UpstreamSubject: upstreamSubject, }, } - if token.RefreshToken == nil || token.RefreshToken.Token == "" { - if token.AccessToken == nil || token.AccessToken.Token == "" { - return writeAuthorizeError(w, oauthHelper, authorizeRequester, - fosite.ErrAccessDenied.WithHint( - "Access token not returned by upstream provider during password grant."), true) - } - customSessionData.OIDC = &psession.OIDCSessionData{ - UpstreamAccessToken: token.AccessToken.Token, - } + hasRefreshToken := token.RefreshToken != nil && token.RefreshToken.Token != "" + hasAccessToken := token.AccessToken != nil && token.AccessToken.Token != "" + switch { + case hasRefreshToken: // we prefer refresh tokens, so check for this first + customSessionData.OIDC.UpstreamRefreshToken = token.RefreshToken.Token + case hasAccessToken: + plog.Info("refresh token not returned by upstream provider during password grant, using access token instead. "+ + "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI "+ + "and try to get a refresh token if possible", + "upstreamName", oidcUpstream.GetName(), + "scopes", oidcUpstream.GetScopes()) + customSessionData.OIDC.UpstreamAccessToken = token.AccessToken.Token + default: + plog.Warning("refresh token and access token not returned by upstream provider during password grant, "+ + "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI", + "upstreamName", oidcUpstream.GetName(), + "scopes", oidcUpstream.GetScopes()) + return writeAuthorizeError(w, oauthHelper, authorizeRequester, + fosite.ErrAccessDenied.WithHint( + "Neither access token nor refresh token returned by upstream provider during password grant."), true) } + return makeDownstreamSessionAndReturnAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, subject, username, groups, customSessionData) } diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index c481207b..1cb4a778 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -56,6 +56,7 @@ func TestAuthorizationEndpoint(t *testing.T) { oidcUpstreamUsernameClaim = "the-user-claim" oidcUpstreamGroupsClaim = "the-groups-claim" oidcPasswordGrantUpstreamRefreshToken = "some-opaque-token" //nolint: gosec + oidcUpstreamAccessToken = "some-access-token" downstreamIssuer = "https://my-downstream-issuer.com/some-path" downstreamRedirectURI = "http://127.0.0.1/callback" @@ -154,15 +155,9 @@ func TestAuthorizationEndpoint(t *testing.T) { "state": happyState, } - fositeAccessDeniedWithMissingRefreshTokenErrorQuery = map[string]string{ - "error": "access_denied", - "error_description": "The resource owner or authorization server denied the request. Refresh token not returned by upstream provider during password grant.", - "state": happyState, - } - fositeAccessDeniedWithMissingAccessTokenErrorQuery = map[string]string{ "error": "access_denied", - "error_description": "The resource owner or authorization server denied the request. Access token not returned by upstream provider during password grant.", + "error_description": "The resource owner or authorization server denied the request. Neither access token nor refresh token returned by upstream provider during password grant.", "state": happyState, } @@ -486,7 +481,9 @@ func TestAuthorizationEndpoint(t *testing.T) { ProviderName: oidcPasswordGrantUpstreamName, ProviderType: psession.ProviderTypeOIDC, OIDC: &psession.OIDCSessionData{ - UpstreamAccessToken: "some-access-token", + UpstreamAccessToken: oidcUpstreamAccessToken, + UpstreamSubject: oidcUpstreamSubject, + UpstreamIssuer: oidcUpstreamIssuer, }, } @@ -889,8 +886,30 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyStringWithLocationInHref: true, }, { - name: "doesn't return an error if allowAccessTokenRefresh is set when upstream IDP did not return a refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithAllowAccessTokenBasedRefresh(true).WithEmptyRefreshToken().WithAccessToken("some-access-token").Build()), + name: "OIDC password grant happy path when upstream IDP returned empty refresh token but it did return an access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: htmlContentType, + wantRedirectLocationRegexp: happyAuthcodeDownstreamRedirectLocationRegexp, + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamRedirectURI: downstreamRedirectURI, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: expectedHappyOIDCPasswordGrantCustomSessionWithAccessToken, + }, + { + name: "OIDC password grant happy path when upstream IDP did not return a refresh token but it did return an access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).Build()), method: http.MethodGet, path: happyGetRequestPath, customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), @@ -1052,34 +1071,8 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyString: "", }, { - name: "return an error when upstream IDP did not return a refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().Build()), - method: http.MethodGet, - path: happyGetRequestPath, - customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), - customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), - wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, - wantStatus: http.StatusFound, - wantContentType: "application/json; charset=utf-8", - wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingRefreshTokenErrorQuery), - wantBodyString: "", - }, - { - name: "return an error when upstream IDP did not return a refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().Build()), - method: http.MethodGet, - path: happyGetRequestPath, - customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), - customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), - wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, - wantStatus: http.StatusFound, - wantContentType: "application/json; charset=utf-8", - wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingRefreshTokenErrorQuery), - wantBodyString: "", - }, - { - name: "return an error when upstream IDP did not return an access or refresh token and allowAccessTokenBasedRefresh is true", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAllowAccessTokenBasedRefresh(true).WithEmptyAccessToken().Build()), + name: "return an error when upstream IDP returns empty refresh token and empty access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithEmptyAccessToken().Build()), method: http.MethodGet, path: happyGetRequestPath, customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), @@ -1091,8 +1084,34 @@ func TestAuthorizationEndpoint(t *testing.T) { wantBodyString: "", }, { - name: "return an error when upstream IDP did not return an access or refresh token and allowAccessTokenBasedRefresh is true", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAllowAccessTokenBasedRefresh(true).WithoutAccessToken().Build()), + name: "return an error when upstream IDP returns no refresh and no access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithoutAccessToken().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery), + wantBodyString: "", + }, + { + name: "return an error when upstream IDP returns no refresh token and empty access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithEmptyAccessToken().Build()), + method: http.MethodGet, + path: happyGetRequestPath, + customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), + customPasswordHeader: pointer.StringPtr(oidcUpstreamPassword), + wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, + wantStatus: http.StatusFound, + wantContentType: "application/json; charset=utf-8", + wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery), + wantBodyString: "", + }, + { + name: "return an error when upstream IDP returns empty refresh token and no access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithoutAccessToken().Build()), method: http.MethodGet, path: happyGetRequestPath, customUsernameHeader: pointer.StringPtr(oidcUpstreamUsername), diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 7ac0edba..24b821e7 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -46,10 +46,6 @@ type UpstreamOIDCIdentityProviderI interface { // flow with this upstream provider. When false, it should not be allowed. AllowsPasswordGrant() bool - // AllowsAccessTokenBasedRefresh returns true if the supervisor should be allowed to refresh upstream - // users with an access token rather than a refresh token. - AllowsAccessTokenBasedRefresh() bool - // GetAdditionalAuthcodeParams returns additional params to be sent on authcode requests. GetAdditionalAuthcodeParams() map[string]string diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index 3a5855da..3ed39ee0 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -74,9 +74,14 @@ type OIDCSessionData struct { // non-empty, then this field should be empty, indicating that we should use the upstream refresh token during // downstream refresh. UpstreamAccessToken string `json:"upstreamAccessToken"` - // TODO describe these + + // UpstreamSubject is the "sub" claim from the upstream identity provider from the user's initial login. We store this + // so that we can validate that it does not change upon refresh. UpstreamSubject string `json:"upstreamSubject"` - UpstreamIssuer string `json:"upstreamIssuer"` + + // UpstreamIssuer is the "iss" claim from the upstream identity provider from the user's initial login. We store this + // so that we can validate that it does not change upon refresh. + UpstreamIssuer string `json:"upstreamIssuer"` } // LDAPSessionData is the additional data needed by Pinniped when the upstream IDP is an LDAP provider. diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index dab0cf92..567212d1 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -146,17 +146,16 @@ func (u *TestUpstreamLDAPIdentityProvider) PerformRefreshArgs(call int) *Perform } type TestUpstreamOIDCIdentityProvider struct { - Name string - ClientID string - ResourceUID types.UID - AuthorizationURL url.URL - RevocationURL *url.URL - UsernameClaim string - GroupsClaim string - Scopes []string - AdditionalAuthcodeParams map[string]string - AllowPasswordGrant bool - AllowAccessTokenBasedRefresh bool + Name string + ClientID string + ResourceUID types.UID + AuthorizationURL url.URL + RevocationURL *url.URL + UsernameClaim string + GroupsClaim string + Scopes []string + AdditionalAuthcodeParams map[string]string + AllowPasswordGrant bool ExchangeAuthcodeAndValidateTokensFunc func( ctx context.Context, @@ -231,10 +230,6 @@ func (u *TestUpstreamOIDCIdentityProvider) AllowsPasswordGrant() bool { return u.AllowPasswordGrant } -func (u *TestUpstreamOIDCIdentityProvider) AllowsAccessTokenBasedRefresh() bool { - return u.AllowAccessTokenBasedRefresh -} - func (u *TestUpstreamOIDCIdentityProvider) PasswordCredentialsGrantAndValidateTokens(ctx context.Context, username, password string) (*oidctypes.Token, error) { u.passwordCredentialsGrantAndValidateTokensCallCount++ u.passwordCredentialsGrantAndValidateTokensArgs = append(u.passwordCredentialsGrantAndValidateTokensArgs, &PasswordCredentialsGrantAndValidateTokensArgs{ @@ -605,26 +600,25 @@ func NewUpstreamIDPListerBuilder() *UpstreamIDPListerBuilder { } type TestUpstreamOIDCIdentityProviderBuilder struct { - name string - resourceUID types.UID - clientID string - scopes []string - idToken map[string]interface{} - refreshToken *oidctypes.RefreshToken - accessToken *oidctypes.AccessToken - usernameClaim string - groupsClaim string - refreshedTokens *oauth2.Token - validatedTokens *oidctypes.Token - authorizationURL url.URL - additionalAuthcodeParams map[string]string - allowPasswordGrant bool - allowAccessTokenBasedRefresh bool - authcodeExchangeErr error - passwordGrantErr error - performRefreshErr error - revokeRefreshTokenErr error - validateTokenErr error + name string + resourceUID types.UID + clientID string + scopes []string + idToken map[string]interface{} + refreshToken *oidctypes.RefreshToken + accessToken *oidctypes.AccessToken + usernameClaim string + groupsClaim string + refreshedTokens *oauth2.Token + validatedTokens *oidctypes.Token + authorizationURL url.URL + additionalAuthcodeParams map[string]string + allowPasswordGrant bool + authcodeExchangeErr error + passwordGrantErr error + performRefreshErr error + revokeRefreshTokenErr error + validateTokenErr error } func (u *TestUpstreamOIDCIdentityProviderBuilder) WithName(value string) *TestUpstreamOIDCIdentityProviderBuilder { @@ -652,11 +646,6 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAllowPasswordGrant(value b return u } -func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAllowAccessTokenBasedRefresh(value bool) *TestUpstreamOIDCIdentityProviderBuilder { - u.allowAccessTokenBasedRefresh = value - return u -} - func (u *TestUpstreamOIDCIdentityProviderBuilder) WithScopes(values []string) *TestUpstreamOIDCIdentityProviderBuilder { u.scopes = values return u @@ -766,16 +755,15 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeRefreshTokenError(er func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdentityProvider { return &TestUpstreamOIDCIdentityProvider{ - Name: u.name, - ClientID: u.clientID, - ResourceUID: u.resourceUID, - UsernameClaim: u.usernameClaim, - GroupsClaim: u.groupsClaim, - Scopes: u.scopes, - AllowPasswordGrant: u.allowPasswordGrant, - AllowAccessTokenBasedRefresh: u.allowAccessTokenBasedRefresh, - AuthorizationURL: u.authorizationURL, - AdditionalAuthcodeParams: u.additionalAuthcodeParams, + Name: u.name, + ClientID: u.clientID, + ResourceUID: u.resourceUID, + UsernameClaim: u.usernameClaim, + GroupsClaim: u.groupsClaim, + Scopes: u.scopes, + AllowPasswordGrant: u.allowPasswordGrant, + AuthorizationURL: u.authorizationURL, + AdditionalAuthcodeParams: u.additionalAuthcodeParams, ExchangeAuthcodeAndValidateTokensFunc: func(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { if u.authcodeExchangeErr != nil { return nil, u.authcodeExchangeErr diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 989b53de..b9d371ed 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -35,17 +35,16 @@ func New(config *oauth2.Config, provider *coreosoidc.Provider, client *http.Clie // ProviderConfig holds the active configuration of an upstream OIDC provider. type ProviderConfig struct { - Name string - ResourceUID types.UID - UsernameClaim string - GroupsClaim string - Config *oauth2.Config - Client *http.Client - AllowPasswordGrant bool - AllowAccessTokenBasedRefresh bool - AdditionalAuthcodeParams map[string]string - RevocationURL *url.URL // will commonly be nil: many providers do not offer this - Provider interface { + Name string + ResourceUID types.UID + UsernameClaim string + GroupsClaim string + Config *oauth2.Config + Client *http.Client + AllowPasswordGrant bool + AdditionalAuthcodeParams map[string]string + RevocationURL *url.URL // will commonly be nil: many providers do not offer this + Provider interface { Verifier(*coreosoidc.Config) *coreosoidc.IDTokenVerifier Claims(v interface{}) error UserInfo(ctx context.Context, tokenSource oauth2.TokenSource) (*coreosoidc.UserInfo, error) @@ -95,10 +94,6 @@ func (p *ProviderConfig) AllowsPasswordGrant() bool { return p.AllowPasswordGrant } -func (p *ProviderConfig) AllowsAccessTokenBasedRefresh() bool { - return p.AllowAccessTokenBasedRefresh -} - func (p *ProviderConfig) PasswordCredentialsGrantAndValidateTokens(ctx context.Context, username, password string) (*oidctypes.Token, error) { // Disallow this grant when requested. if !p.AllowPasswordGrant { diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index e55f8e03..f29250e8 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidcclient @@ -406,7 +406,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). + ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). @@ -453,7 +453,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). + ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(nil, fmt.Errorf("some validation error")) mock.EXPECT(). PerformRefresh(gomock.Any(), "test-refresh-token-returning-invalid-id-token"). @@ -1648,7 +1648,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). + ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index aefdd69f..5fdd6060 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -149,8 +149,7 @@ func TestSupervisorLogin(t *testing.T) { Groups: env.SupervisorUpstreamOIDC.GroupsClaim, }, AuthorizationConfig: idpv1alpha1.OIDCAuthorizationConfig{ - AdditionalScopes: []string{"email"}, // does not ask for offline_access. - AllowAccessTokenBasedRefresh: true, + AdditionalScopes: []string{"email"}, // does not ask for offline_access. }, }, idpv1alpha1.PhaseReady) return oidcIDP.Name