Support for multiple client IDs in the verifier

[aramase]

We use github.com/coreos/go-oidc. IDTokenVerifier in the Kubernetes authenticator package.

I'm working on supporting Structured Authentication Configuration in Kubernetes. As part of this feature, we're going to support configuring multiple client IDs in the JWT authenticator.

For this, I would like to propose a change in this package to support checking for multiple client IDs against the audiences as part of Verify.

Here is the diff

diff --git a/oidc/verify.go b/oidc/verify.go
index 0bca49a..cc67e64 100644
--- a/oidc/verify.go
+++ b/oidc/verify.go
@@ -83,6 +83,15 @@ type Config struct {
 	//
 	// If not provided, users must explicitly set SkipClientIDCheck.
 	ClientID string
+
+	// Expected audiences of the token. For a majority of the cases this is expected to be
+	// the ID of the client that initialized the login flow. It may occasionally differ if
+	// the provider supports the authorizing party (azp) claim.
+	//
+	// This field is mutually exclusive with ClientID.
+	// Only use this field if you need to support multiple audiences.
+	// If not provided, users must explicitly set SkipClientIDCheck.
+	ClientIDs []string
 	// If specified, only this set of algorithms may be used to sign the JWT.
 	//
 	// If the IDTokenVerifier is created from a provider with (*Provider).Verifier, this
@@ -268,16 +277,28 @@ func (v *IDTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*IDTok
 		}
 	}
 
-	// If a client ID has been provided, make sure it's part of the audience. SkipClientIDCheck must be true if ClientID is empty.
+	// If client ID(s) has been provided, make sure it's part of the audience. SkipClientIDCheck must be true if ClientID and ClientIDs is empty.
 	//
 	// This check DOES NOT ensure that the ClientID is the party to which the ID Token was issued (i.e. Authorized party).
 	if !v.config.SkipClientIDCheck {
+		if v.config.ClientID == "" && len(v.config.ClientIDs) == 0 {
+			return nil, fmt.Errorf("oidc: invalid configuration, clientID or clientIDs must be provided or SkipClientIDCheck must be set")
+		}
+		if v.config.ClientID != "" && len(v.config.ClientIDs) > 0 {
+			return nil, fmt.Errorf("oidc: invalid configuration, ClientID and ClientIDs cannot both be set")
+		}
+
+		var clientIDs []string
 		if v.config.ClientID != "" {
-			if !contains(t.Audience, v.config.ClientID) {
-				return nil, fmt.Errorf("oidc: expected audience %q got %q", v.config.ClientID, t.Audience)
-			}
+			clientIDs = []string{v.config.ClientID}
 		} else {
-			return nil, fmt.Errorf("oidc: invalid configuration, clientID must be provided or SkipClientIDCheck must be set")
+			clientIDs = v.config.ClientIDs
+		}
+
+		for _, clientID := range clientIDs {
+			if !contains(t.Audience, clientID) {
+				return nil, fmt.Errorf("oidc: expected audience %q got %q", clientID, t.Audience)
+			}
 		}
 	}
 

@ericchiang I've implemented this change in a branch and would like to get your feedback. Is this change you would be willing to review and merge? For context, without this change, we would need to perform the audience match in the authenticator after Verify.

[ericchiang]

Some previous links:

#334 (comment)
#355 (comment)

Kubernetes should be able to do all of this without changes to go-oidc using the SkipClientIDCheck. So I don't believe we're blocking your KEP.

If we're going to add an API here, I'm still not clear on:

• What kind of scenarios this is relevant, and what providers support this
• What the correct thing to do with authorized parties is Verify azp claim  #355
• What security objectives are being achieved with this check and what the docs should recommend

For now, is this a dup of #355?

[aramase]

Thanks for the response!

Kubernetes should be able to do all of this without changes to go-oidc using the SkipClientIDCheck. So I don't believe we're blocking your KEP.

Right, this change is not blocking the KEP. SkipClientIDCheck is what I've been looking at but wanted to propose the change here to see if it's a valid use-case.

What kind of scenarios this is relevant, and what providers support this

I'm trying to find more information on this.

For now, is this a dup of #355?

I'm not sure if this is a dup of #355, because we aren't changing anything related to azp claims?