Optionally allow OIDC password grant for CLI-based login experience #778

cfryanr · 2021-08-12T17:53:50Z

This PR addresses #686 to support non-interactive password-based OIDC logins (e.g., from CI). It implements the proposed solution described by the comments of #686.

Add AllowPasswordGrant boolean field to OIDCIdentityProvider's spec
The oidc upstream watcher controller copies the value of AllowPasswordGrant into the configuration of the cached provider
Add password grant to the UpstreamOIDCIdentityProviderI interface which is implemented by the cached provider instance for use in the authorization endpoint
Enhance the IDP discovery endpoint to return the supported client "flows" for each IDP ("cli_password" and/or "browser_authcode")
Enhance pinniped get kubeconfig to help the user choose the desired flow for the selected IDP, and to write the flow into the resulting kubeconfg. The command offers a new --upstream-identity-provider-flow flag which can be used to resolve ambiguity when there is more than one client flow available for the selected upstream IDP.
Enhance pinniped login oidc to have a new --upstream-identity-provider-flow flag to tell it which client-side flow it should use for auth (CLI-based or browser-based) along with a client-determined default flow for each IDP type (browser for OIDC and CLI-based for LDAP)
Enhance the authorize endpoint to perform password grant when requested by the incoming headers. This commit does not include unit tests for the enhancements to the authorize endpoint, which will come in the next commit
Extract some shared helpers from the callback endpoint to share the code with the authorize endpoint
In the Dex config, allow the resource owner password grant, which Dex implements to also return ID tokens, for use in integration tests
Log the private debug message when logging Fosite errors
Add new integration tests. These depend on this Dex bug fix being included in the version of Dex used by the integration tests: Return valid JWT access token from password grant dexidp/dex#2234

Release note:

Adds `AllowPasswordGrant` boolean field to `OIDCIdentityProvider` spec.
If your OIDC provider server supports the OAuth 2.0 resource owner password credentials grant such that
it returns an ID token along with the usual other response values when the `openid` scope is requested,
then you may optionally choose to configure `AllowPasswordGrant` to `true` to allow clients to perform
this type of authentication. Clients will be prompted for their username and password on the
command-line without opening a browser window. See #686 for some discussion of scenarios where you
might like to enable or disable this capability.

- Add `AllowPasswordGrant` boolean field to OIDCIdentityProvider's spec - The oidc upstream watcher controller copies the value of `AllowPasswordGrant` into the configuration of the cached provider - Add password grant to the UpstreamOIDCIdentityProviderI interface which is implemented by the cached provider instance for use in the authorization endpoint - Enhance the IDP discovery endpoint to return the supported "flows" for each IDP ("cli_password" and/or "browser_authcode") - Enhance `pinniped get kubeconfig` to help the user choose the desired flow for the selected IDP, and to write the flow into the resulting kubeconfg - Enhance `pinniped login oidc` to have a flow flag to tell it which client-side flow it should use for auth (CLI-based or browser-based) - In the Dex config, allow the resource owner password grant, which Dex implements to also return ID tokens, for use in integration tests - Enhance the authorize endpoint to perform password grant when requested by the incoming headers. This commit does not include unit tests for the enhancements to the authorize endpoint, which will come in the next commit - Extract some shared helpers from the callback endpoint to share the code with the authorize endpoint - Add new integration tests

After merging the new Kube 1.22 ExecCredential changes from main into this feature branch, some of the new units test on this feature branch needed to be update to account for the new ExecCredential "interactive" field.

codecov · 2021-08-12T20:41:24Z

Codecov Report

Merging #778 (3077034) into main (89cef2e) will increase coverage by 0.04%.
The diff coverage is 93.58%.

@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
+ Coverage   79.37%   79.42%   +0.04%     
==========================================
  Files         127      127              
  Lines        8655     8676      +21     
==========================================
+ Hits         6870     6891      +21     
  Misses       1562     1562              
  Partials      223      223

Impacted Files	Coverage Δ
internal/oidc/oidc.go	`0.00% <0.00%> (ø)`
...nal/oidc/provider/dynamic_upstream_idp_provider.go	`0.00% <ø> (ø)`
internal/oidc/auth/auth_handler.go	`96.95% <87.93%> (-3.05%)`	⬇️
cmd/pinniped/cmd/kubeconfig.go	`83.09% <100.00%> (+1.26%)`	⬆️
cmd/pinniped/cmd/login_oidc.go	`91.74% <100.00%> (+1.24%)`	⬆️
...onfig/oidcupstreamwatcher/oidc_upstream_watcher.go	`98.00% <100.00%> (+0.01%)`	⬆️
internal/oidc/callback/callback_handler.go	`100.00% <100.00%> (+5.94%)`	⬆️
internal/oidc/discovery/discovery_handler.go	`81.25% <100.00%> (+2.67%)`	⬆️
...nternal/oidc/idpdiscovery/idp_discovery_handler.go	`83.78% <100.00%> (+6.00%)`	⬆️
internal/upstreamoidc/upstreamoidc.go	`95.50% <100.00%> (+0.83%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89cef2e...3077034. Read the comment docs.

enj · 2021-08-12T23:53:36Z

Do you need to rebase on main? 69964fc should not be needed since #770 added 5678fc6.

cfryanr · 2021-08-13T00:42:37Z

Do you need to rebase on main? 69964fc should not be needed since #770 added 5678fc6.

I don't think so. See commit comment for that commit for details. TLDR: I think I needed those changes because I wrote new tests before I merged, so those new tests needed to be updated after the merge to be consistent with what's on main.

enj · 2021-08-17T01:51:05Z

apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl

-	// request flow with an OIDC identity provider. By default only the "openid" scope will be requested.
+	// request flow with an OIDC identity provider.
+	// In the case of a Resource Owner Password Credentials Grant flow, AdditionalScopes are the scopes
+	// in addition to "openid" that will be requested as part of the token request (see also the AllowPasswordGrant field).


Generally you want to refer to the JSON tag.

Suggested change

// in addition to "openid" that will be requested as part of the token request (see also the AllowPasswordGrant field).

// in addition to "openid" that will be requested as part of the token request (see also the allowPasswordGrant field).

apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl

enj · 2021-08-17T02:00:06Z

cmd/pinniped/cmd/kubeconfig.go

 type pinnipedIDPResponse struct {
-	Name string `json:"name"`
-	Type string `json:"type"`
+	Name  string   `json:"name"`
+	Type  string   `json:"type"`
+	Flows []string `json:"flows,omitempty"`
 }


Please create a public struct in our apis package and share it between the client and server. This is part of our public API, just like our rest APIs.

This is not a Kubernetes API, so we don't currently have a place to put it. Everything in the /apis directory is a .tmpl file for code generation including Kubernetes client-go code across all supported Kubernetes versions, which is not what's needed here.

We could make a new public package for this stuff. Maybe /pkg/supervisorapis or something like that?

I put it into the /apis directory as a .tmpl file and ran the code generator. It’s kind of strange to have a copy of the file per kube version when it is not needed, but it doesn’t hurt anything.

enj · 2021-08-17T02:06:06Z

internal/oidc/oidc.go

@@ -251,11 +251,17 @@ func FositeErrorForLog(err error) []interface{} {
 	rfc6749Error := fosite.ErrorToRFC6749Error(err)


We seem to be calling this FositeErrorForLog function at the Info level but Debug seems more appropriate. Also does this have the potential to leak any secrets into logs?

The use of Info predates this PR, but I think the reasoning for Info was trying to follow these logging guidelines:

pinniped/internal/plog/plog.go

Lines 13 to 17 in b5889f3

// info should be reserved for "nice to know" information. It should be possible to run a production

// pinniped server at the info log level with no performance degradation due to high log volume.

// debug should be used for information targeted at developers and to aid in support cases. Care must

// be taken at this level to not leak any secrets into the log stream. That is, even though debug may

// cause performance issues in production, it must not cause security issues in production.

According to that guideline, Info is intended to be stuff that would be useful for regular users to see to help them debug their configuration, as opposed to Debug which is probably only useful for Pinniped contributors. Messages that could help you debug why your OIDCIdentityProvider is causing failed logins feels more like an Info log according to that guideline.

As far as I know, there should be no potential to leak secrets here under normal circumstances. Neither fosite error messages nor upstream OIDC/LDAP network responses should be putting secrets into error messages. The debug field inside of the Fosite error struct is intended to be things to log on the server without sending to the client, as opposed to the "hint" field which is returned to the client.

enj · 2021-08-17T02:09:01Z

internal/upstreamoidc/upstreamoidc.go

+
+	// There is no nonce to validate for a resource owner password credentials grant because it skips using
+	// the authorize endpoint and goes straight to the token endpoint.
+	skipNonceValidation := nonce.Nonce("")


nit:

Suggested change

skipNonceValidation := nonce.Nonce("")

const skipNonceValidation nonce.Nonce = ""

internal/upstreamoidc/upstreamoidc_test.go

internal/oidc/callback/callback_handler_test.go

enj · 2021-08-17T16:32:35Z

internal/oidc/auth/auth_handler_test.go

+	upstreamOIDCIdentityProvider := func() *oidctestutil.TestUpstreamOIDCIdentityProvider {
+		return oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+			WithName(oidcUpstreamName).
+			WithClientID("some-client-id").
+			WithAuthorizationURL(*upstreamAuthURL).
+			WithScopes([]string{"scope1", "scope2"}). // the scopes to request when starting the upstream authorization flow
+			WithAllowPasswordGrant(false).
+			WithPasswordGrantError(errors.New("should not have used password grant on this instance")).
+			Build()
+	}
+
+	passwordGrantUpstreamOIDCIdentityProviderBuilder := func() *oidctestutil.TestUpstreamOIDCIdentityProviderBuilder {
+		return oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+			WithName(oidcPasswordGrantUpstreamName).
+			WithClientID("some-client-id").
+			WithAuthorizationURL(*upstreamAuthURL).
+			WithScopes([]string{"scope1", "scope2"}). // the scopes to request when starting the upstream authorization flow
+			WithAllowPasswordGrant(false).
+			WithUsernameClaim(oidcUpstreamUsernameClaim).
+			WithGroupsClaim(oidcUpstreamGroupsClaim).
+			WithIDTokenClaim("iss", oidcUpstreamIssuer).
+			WithIDTokenClaim("sub", oidcUpstreamSubject).
+			WithIDTokenClaim(oidcUpstreamUsernameClaim, oidcUpstreamUsername).
+			WithIDTokenClaim(oidcUpstreamGroupsClaim, oidcUpstreamGroupMembership).
+			WithIDTokenClaim("other-claim", "should be ignored").
+			WithAllowPasswordGrant(true).
+			WithUpstreamAuthcodeExchangeError(errors.New("should not have tried to exchange upstream authcode on this instance"))
 	}


Please make regular functions for this kind of stuff. Over use of closures is an anti pattern from the spec tests.

Prefer the following approaches over closures:

func doSomething(t, a, b, c, d) { ... }

type foo struct { t, a, b, c, d } func (f *foo) doSomething() { ... }

I agree that over use of closures can make for confusing code, and I'm sure that I'm guilty of overdoing it in some of our older tests.

However, in this case every variable used inside the closures is either const or is effectively const (e.g. upstreamAuthURL which would be a const except that it needs to be the result of url.Parse), and the functions simply return new structs. Using a simple closure as a test helper to create a struct containing fake test data seems reasonable and helps keep each test in the test table brief and expressive.

keep each test in the test table brief and expressive

I think about this differently - I want the table test to be verbose and straightforward, because that is the general nature of table tests - repetition is both fine and generally preferred. So even in this case, a regular func passwordGrantUpstreamOIDCIdentityProviderBuilder(... params here...) is preferred over the closure. But a conversation for another day.

cmd/pinniped/cmd/kubeconfig_test.go

vmwclabot added the cla-not-required label Aug 12, 2021

cfryanr marked this pull request as draft August 12, 2021 18:09

Merge branch 'main' into oidc_password_grant

5b96d01

enj mentioned this pull request Aug 12, 2021

[stub] CLI SSO - Kerberos support on CLI (possibly with Active Directory) #551

Open

cfryanr added 5 commits August 12, 2021 17:53

First unit test for auth endpoint's password grant and related refactor

50085a5

Merge branch 'main' into oidc_password_grant

71d6281

More unit tests and small error handling changes for OIDC password grant

52cb0bb

Extract private helper in auth_handler.go

91c8a3e

Merge branch 'main' into oidc_password_grant

52409f8

cfryanr marked this pull request as ready for review August 16, 2021 22:20

cfryanr changed the title ~~WIP: Optionally allow OIDC password grant for CLI-based login experience~~ Optionally allow OIDC password grant for CLI-based login experience Aug 16, 2021

Update expected error message in e2e integration test

3fb683f

enj suggested changes Aug 17, 2021

View reviewed changes

cfryanr added 12 commits August 17, 2021 10:45

Merge branch 'main' into oidc_password_grant

a7c88b5

Some refactors based on PR feedback from @enj

964d161

Extract Supervisor IDP discovery endpoint types into apis package

96474b3

Merge branch 'main' into oidc_password_grant

62c6d53

Extract Supervisor IDP discovery endpoint string constants into apis pkg

0089540

Extract Supervisor authorize endpoint string constants into apis pkg

04b8f0b

Refactor some authorize and callback error handling, and add more tests

61c21d2

Update login.md doc to mention OIDC CLI-based flow

42d31a7

Merge branch 'main' into oidc_password_grant

cf627a8

Merge branch 'main' into oidc_password_grant

e4d418a

remove one nolint:unparam comment

6239a56

Log auth endpoint errors with stack traces

211f4b2

Merge branch 'main' into oidc_password_grant

3077034

enj approved these changes Aug 24, 2021

View reviewed changes

enj enabled auto-merge August 24, 2021 16:24

enj merged commit f7751d1 into main Aug 24, 2021

enj deleted the oidc_password_grant branch August 24, 2021 17:02

cfryanr mentioned this pull request Apr 29, 2022

Advertise browser_authcode flow in ldap idp discovery #1132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally allow OIDC password grant for CLI-based login experience #778

Optionally allow OIDC password grant for CLI-based login experience #778

cfryanr commented Aug 12, 2021 •

edited

codecov bot commented Aug 12, 2021 •

edited

enj commented Aug 12, 2021

cfryanr commented Aug 13, 2021

enj Aug 17, 2021

cfryanr Aug 17, 2021

enj Aug 17, 2021

cfryanr Aug 17, 2021

cfryanr Aug 17, 2021

enj Aug 17, 2021

cfryanr Aug 17, 2021

enj Aug 17, 2021

cfryanr Aug 17, 2021

enj Aug 17, 2021

cfryanr Aug 17, 2021

enj Aug 20, 2021

	// in addition to "openid" that will be requested as part of the token request (see also the AllowPasswordGrant field).
	// in addition to "openid" that will be requested as part of the token request (see also the allowPasswordGrant field).

		@@ -251,11 +251,17 @@ func FositeErrorForLog(err error) []interface{} {
		rfc6749Error := fosite.ErrorToRFC6749Error(err)

	// info should be reserved for "nice to know" information. It should be possible to run a production
	// pinniped server at the info log level with no performance degradation due to high log volume.
	// debug should be used for information targeted at developers and to aid in support cases. Care must
	// be taken at this level to not leak any secrets into the log stream. That is, even though debug may
	// cause performance issues in production, it must not cause security issues in production.

	skipNonceValidation := nonce.Nonce("")
	const skipNonceValidation nonce.Nonce = ""

Optionally allow OIDC password grant for CLI-based login experience #778

Optionally allow OIDC password grant for CLI-based login experience #778

Conversation

cfryanr commented Aug 12, 2021 • edited

codecov bot commented Aug 12, 2021 • edited

Codecov Report

enj commented Aug 12, 2021

cfryanr commented Aug 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfryanr commented Aug 12, 2021 •

edited

codecov bot commented Aug 12, 2021 •

edited