From 4fe6b82664de9f8ab0cbac2813cf66ddf46fe23d Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Tue, 18 Jun 2019 13:56:44 -0700 Subject: [PATCH] Restore client_id check This check was inadvertently removed for OIDC roles in https://github.com/hashicorp/vault-plugin-auth-jwt/pull/38 --- path_login.go | 7 ++- path_login_test.go | 6 +++ path_oidc_test.go | 112 +++++++++++++++++++++++++++++---------------- 3 files changed, 85 insertions(+), 40 deletions(-) diff --git a/path_login.go b/path_login.go index 7223999b..976c30aa 100644 --- a/path_login.go +++ b/path_login.go @@ -255,7 +255,12 @@ func (b *jwtAuthBackend) verifyOIDCToken(ctx context.Context, config *jwtConfig, oidcConfig := &oidc.Config{ SupportedSigningAlgs: config.JWTSupportedAlgs, - SkipClientIDCheck: true, + } + + if role.RoleType == "oidc" { + oidcConfig.ClientID = config.OIDCClientID + } else { + oidcConfig.SkipClientIDCheck = true } verifier := provider.Verifier(oidcConfig) diff --git a/path_login_test.go b/path_login_test.go index 0b3206ce..824e5290 100644 --- a/path_login_test.go +++ b/path_login_test.go @@ -991,12 +991,18 @@ func TestLogin_JWKS_Concurrent(t *testing.T) { Audience: jwt.Audience{"https://vault.plugin.auth.jwt.test"}, } + type orgs struct { + Primary string `json:"primary"` + } + privateCl := struct { User string `json:"https://vault/user"` Groups []string `json:"https://vault/groups"` + Org orgs `json:"org"` }{ "jeff", []string{"foo", "bar"}, + orgs{"engineering"}, } jwtData, _ := getTestJWT(t, ecdsaPrivKey, cl, privateCl) diff --git a/path_oidc_test.go b/path_oidc_test.go index 859082e2..4abe824f 100644 --- a/path_oidc_test.go +++ b/path_oidc_test.go @@ -239,18 +239,7 @@ func TestOIDC_Callback(t *testing.T) { nonce := getQueryParam(t, authURL, "nonce") // set provider claims that will be returned by the mock server - s.customClaims = map[string]interface{}{ - "nonce": nonce, - "email": "bob@example.com", - "COLOR": "green", - "sk": "42", - "nested": map[string]interface{}{ - "Size": "medium", - "Groups": []string{"a", "b"}, - "secret_code": "bar", - }, - "password": "foo", - } + s.customClaims = sampleClaims(nonce) // set mock provider's expected code s.code = "abc" @@ -313,7 +302,7 @@ func TestOIDC_Callback(t *testing.T) { auth := resp.Auth if !reflect.DeepEqual(auth, expected) { - t.Fatalf("expected: %v, auth: %v", expected, auth) + t.Fatalf("expected: %v, auth: %v", expected, resp) } } }) @@ -343,19 +332,7 @@ func TestOIDC_Callback(t *testing.T) { state := getQueryParam(t, authURL, "state") - // set provider claims that will be returned by the mock server - s.customClaims = map[string]interface{}{ - "nonce": "notgonnamatch", - "email": "bob@example.com", - "COLOR": "green", - "sk": "42", - "nested": map[string]interface{}{ - "Size": "medium", - "Groups": []string{"a", "b"}, - "secret_code": "bar", - }, - "password": "foo", - } + s.customClaims = sampleClaims("bad nonce") // set mock provider's expected code s.code = "abc" @@ -408,19 +385,8 @@ func TestOIDC_Callback(t *testing.T) { state := getQueryParam(t, authURL, "state") nonce := getQueryParam(t, authURL, "nonce") - // set provider claims that will be returned by the mock server - s.customClaims = map[string]interface{}{ - "nonce": nonce, - "email": "bob@example.com", - "COLOR": "green", - "sk": "43", // the pre-configured role has a bound claim of "sk"=="42" - "nested": map[string]interface{}{ - "Size": "medium", - "Groups": []string{"a", "b"}, - "secret_code": "bar", - }, - "password": "foo", - } + s.customClaims = sampleClaims(nonce) + s.customClaims["sk"] = "43" // the pre-configured role has a bound claim of "sk"=="42" // set mock provider's expected code s.code = "abc" @@ -672,6 +638,59 @@ func TestOIDC_Callback(t *testing.T) { t.Fatalf("expected invalid CIDR error, got : %v", *resp) } }) + + t.Run("test invalid client_id", func(t *testing.T) { + b, storage, s := getBackendAndServer(t, false) + defer s.server.Close() + + s.code = "abc" + s.clientID = "not_gonna_match" + + // get auth_url + data := map[string]interface{}{ + "role": "test", + "redirect_uri": "https://example.com", + } + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "oidc/auth_url", + Storage: storage, + Data: data, + } + + resp, err := b.HandleRequest(context.Background(), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v\n", err, resp) + } + + authURL := resp.Data["auth_url"].(string) + state := getQueryParam(t, authURL, "state") + nonce := getQueryParam(t, authURL, "nonce") + + // set provider claims that will be returned by the mock server + s.customClaims = sampleClaims(nonce) + + req = &logical.Request{ + Operation: logical.ReadOperation, + Path: "oidc/callback", + Storage: storage, + Data: map[string]interface{}{ + "state": state, + "code": "abc", + }, + } + resp, err = b.HandleRequest(context.Background(), req) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("nil response") + } + + if !resp.IsError() || !strings.Contains(resp.Error().Error(), `error validating signature: oidc: expected audience "abc"`) { + t.Fatalf("expected invalid client_id error, got : %v", *resp) + } + }) } // oidcProvider is local server the mocks the basis endpoints used by the @@ -913,3 +932,18 @@ func getBackendAndServer(t *testing.T, boundCIDRs bool) (logical.Backend, logica return b, storage, s } + +func sampleClaims(nonce string) map[string]interface{} { + return map[string]interface{}{ + "nonce": nonce, + "email": "bob@example.com", + "COLOR": "green", + "sk": "42", + "nested": map[string]interface{}{ + "Size": "medium", + "Groups": []string{"a", "b"}, + "secret_code": "bar", + }, + "password": "foo", + } +}