Skip to content

Commit

Permalink
Merge pull request from GHSA-q9hr-j4rf-8fjc
Browse files Browse the repository at this point in the history 
* fix: verify audience claim

Co-Authored-By: Vladimir Pouzanov <farcaller@gmail.com>
Signed-off-by: CI <350466+crenshaw-dev@users.noreply.github.com>

* fix lint

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix handling of expired token error

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* go mod tidy

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* handle single aud claim marshaled as a string

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

Signed-off-by: CI <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Vladimir Pouzanov <farcaller@gmail.com>
  • Loading branch information
@crenshaw-dev @farcaller
crenshaw-dev and farcaller committed Jan 25, 2023
1 parent 95da518 commit b38bc00
Show file tree
Hide file tree
Showing 14 changed files with 774 additions and 63 deletions.
2 changes: 1 addition & 1 deletion cmd/argocd/commands/admin/settings.go
View file
Expand Up @@ -206,7 +206,7 @@ var validatorsByGroup = map[string]settingValidator{
}
ssoProvider = "Dex"
} else if general.OIDCConfigRAW != "" {
if _, err := settings.UnmarshalOIDCConfig(general.OIDCConfigRAW); err != nil {
if err := settings.ValidateOIDCConfig(general.OIDCConfigRAW); err != nil {
return "", fmt.Errorf("invalid oidc.config: %v", err)
}
ssoProvider = "OIDC"
Expand Down
6 changes: 6 additions & 0 deletions common/common.go
View file
@@ -1,6 +1,7 @@
package common

import (
"errors"
"os"
"path/filepath"
"strconv"
Expand Down Expand Up @@ -316,3 +317,8 @@ const (
SecurityMedium = 2 // Could indicate malicious events, but has a high likelihood of being user/system error (i.e. access denied)
SecurityLow = 1 // Unexceptional entries (i.e. successful access logs)
)

// Common error messages
const TokenVerificationError = "failed to verify the token"

var TokenVerificationErr = errors.New(TokenVerificationError)
12 changes: 12 additions & 0 deletions docs/operator-manual/user-management/index.md
View file
Expand Up @@ -301,6 +301,18 @@ data:
issuer: https://dev-123456.oktapreview.com
clientID: aaaabbbbccccddddeee
clientSecret: $oidc.okta.clientSecret
# Optional list of allowed aud claims. If omitted or empty, defaults to the clientID value above. If you specify a
# list and want the clientD to be allowed, you must explicitly include it in the list.
# Token verification will pass if any of the token's audiences matches any of the audiences in this list.
allowedAudiences:
- aaaabbbbccccddddeee
- qqqqwwwweeeerrrrttt
# Optional. If false, tokens without an audience will always fail validation. If true, tokens without an audience
# will always pass validation.
# Defaults to true for Argo CD < 2.6.0. Defaults to false for Argo CD >= 2.6.0.
skipAudienceCheckWhenTokenHasNoAudience: true
# Optional set of OIDC scopes to request. If omitted, defaults to: ["openid", "profile", "email", "groups"]
requestedScopes: ["openid", "profile", "email", "groups"]
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Expand Up @@ -99,7 +99,7 @@ require (
require (
github.com/gfleury/go-bitbucket-v1 v0.0.0-20220301131131-8e7ed04b843e
github.com/stretchr/objx v0.5.0 // indirect
gopkg.in/square/go-jose.v2 v2.6.0 // indirect
gopkg.in/square/go-jose.v2 v2.6.0
k8s.io/apiserver v0.24.2
)

Expand Down
131 changes: 112 additions & 19 deletions server/server_test.go
View file
Expand Up @@ -34,6 +34,7 @@ import (
appstatecache "github.com/argoproj/argo-cd/v2/util/cache/appstate"
"github.com/argoproj/argo-cd/v2/util/rbac"
settings_util "github.com/argoproj/argo-cd/v2/util/settings"
testutil "github.com/argoproj/argo-cd/v2/util/test"
)

func fakeServer() (*ArgoCDServer, func()) {
Expand Down Expand Up @@ -522,7 +523,7 @@ func dexMockHandler(t *testing.T, url string) func(http.ResponseWriter, *http.Re
}
}

func getTestServer(t *testing.T, anonymousEnabled bool, withFakeSSO bool) (argocd *ArgoCDServer, dexURL string) {
func getTestServer(t *testing.T, anonymousEnabled bool, withFakeSSO bool, useDexForSSO bool) (argocd *ArgoCDServer, oidcURL string) {
cm := test.NewFakeConfigMap()
if anonymousEnabled {
cm.Data["users.anonymous.enabled"] = "true"
Expand All @@ -533,9 +534,14 @@ func getTestServer(t *testing.T, anonymousEnabled bool, withFakeSSO bool) (argoc
ts.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
dexMockHandler(t, ts.URL)(w, r)
})
oidcServer := ts
if !useDexForSSO {
oidcServer = testutil.GetOIDCTestServer(t)
}
if withFakeSSO {
cm.Data["url"] = ts.URL
cm.Data["dex.config"] = `
if useDexForSSO {
cm.Data["dex.config"] = `
connectors:
# OIDC
- type: OIDC
Expand All @@ -545,6 +551,19 @@ connectors:
issuer: https://auth.example.gom
clientID: test-client
clientSecret: $dex.oidc.clientSecret`
} else {
oidcConfig := settings_util.OIDCConfig{
Name: "Okta",
Issuer: oidcServer.URL,
ClientID: "argo-cd",
ClientSecret: "$oidc.okta.clientSecret",
}
oidcConfigString, err := yaml.Marshal(oidcConfig)
require.NoError(t, err)
cm.Data["oidc.config"] = string(oidcConfigString)
// Avoid bothering with certs for local tests.
cm.Data["oidc.tls.insecure.skip.verify"] = "true"
}
}
secret := test.NewFakeSecret()
kubeclientset := fake.NewSimpleClientset(cm, secret)
Expand All @@ -556,27 +575,32 @@ connectors:
AppClientset: appClientSet,
RepoClientset: mockRepoClient,
}
if withFakeSSO {
if withFakeSSO && useDexForSSO {
argoCDOpts.DexServerAddr = ts.URL
}
argocd = NewServer(context.Background(), argoCDOpts)
return argocd, ts.URL
return argocd, oidcServer.URL
}

func TestAuthenticate_3rd_party_JWTs(t *testing.T) {
// Marshaling single strings to strings is typical, so we test for this relatively common behavior.
jwt.MarshalSingleStringAsArray = false

type testData struct {
test string
anonymousEnabled bool
claims jwt.RegisteredClaims
expectedErrorContains string
expectedClaims interface{}
useDex bool
}
var tests = []testData{
// Dex
{
test: "anonymous disabled, no audience",
anonymousEnabled: false,
claims: jwt.RegisteredClaims{},
expectedErrorContains: "no audience found in the token",
claims: jwt.RegisteredClaims{ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
expectedErrorContains: common.TokenVerificationError,
expectedClaims: nil,
},
{
Expand All @@ -589,31 +613,95 @@ func TestAuthenticate_3rd_party_JWTs(t *testing.T) {
{
test: "anonymous disabled, unexpired token, admin claim",
anonymousEnabled: false,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"test-client"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
expectedErrorContains: "id token signed with unsupported algorithm",
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
expectedErrorContains: common.TokenVerificationError,
expectedClaims: nil,
},
{
test: "anonymous enabled, unexpired token, admin claim",
anonymousEnabled: true,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"test-client"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
expectedErrorContains: "",
expectedClaims: "",
},
{
test: "anonymous disabled, expired token, admin claim",
anonymousEnabled: false,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"test-client"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())},
expectedErrorContains: "token is expired",
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())},
expectedErrorContains: common.TokenVerificationError,
expectedClaims: jwt.RegisteredClaims{Issuer: "sso"},
},
{
test: "anonymous enabled, expired token, admin claim",
anonymousEnabled: true,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"test-client"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())},
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())},
expectedErrorContains: "",
expectedClaims: "",
},
{
test: "anonymous disabled, unexpired token, admin claim, incorrect audience",
anonymousEnabled: false,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"incorrect-audience"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
expectedErrorContains: common.TokenVerificationError,
expectedClaims: nil,
},
// External OIDC (not bundled Dex)
{
test: "external OIDC: anonymous disabled, no audience",
anonymousEnabled: false,
claims: jwt.RegisteredClaims{ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
useDex: true,
expectedErrorContains: common.TokenVerificationError,
expectedClaims: nil,
},
{
test: "external OIDC: anonymous enabled, no audience",
anonymousEnabled: true,
claims: jwt.RegisteredClaims{},
useDex: true,
expectedErrorContains: "",
expectedClaims: "",
},
{
test: "external OIDC: anonymous disabled, unexpired token, admin claim",
anonymousEnabled: false,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
useDex: true,
expectedErrorContains: common.TokenVerificationError,
expectedClaims: nil,
},
{
test: "external OIDC: anonymous enabled, unexpired token, admin claim",
anonymousEnabled: true,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
useDex: true,
expectedErrorContains: "",
expectedClaims: "",
},
{
test: "external OIDC: anonymous disabled, expired token, admin claim",
anonymousEnabled: false,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())},
useDex: true,
expectedErrorContains: common.TokenVerificationError,
expectedClaims: jwt.RegisteredClaims{Issuer: "sso"},
},
{
test: "external OIDC: anonymous enabled, expired token, admin claim",
anonymousEnabled: true,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())},
useDex: true,
expectedErrorContains: "",
expectedClaims: "",
},
{
test: "external OIDC: anonymous disabled, unexpired token, admin claim, incorrect audience",
anonymousEnabled: false,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"incorrect-audience"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))},
useDex: true,
expectedErrorContains: common.TokenVerificationError,
expectedClaims: nil,
},
}

for _, testData := range tests {
Expand All @@ -625,8 +713,13 @@ func TestAuthenticate_3rd_party_JWTs(t *testing.T) {
// Must be declared here to avoid race.
ctx := context.Background() //nolint:ineffassign,staticcheck

argocd, dexURL := getTestServer(t, testDataCopy.anonymousEnabled, true)
testDataCopy.claims.Issuer = fmt.Sprintf("%s/api/dex", dexURL)
argocd, oidcURL := getTestServer(t, testDataCopy.anonymousEnabled, true, testDataCopy.useDex)

if testDataCopy.useDex {
testDataCopy.claims.Issuer = fmt.Sprintf("%s/api/dex", oidcURL)
} else {
testDataCopy.claims.Issuer = oidcURL
}
token := jwt.NewWithClaims(jwt.SigningMethodHS256, testDataCopy.claims)
tokenString, err := token.SignedString([]byte("key"))
require.NoError(t, err)
Expand Down Expand Up @@ -676,7 +769,7 @@ func TestAuthenticate_no_request_metadata(t *testing.T) {
t.Run(testDataCopy.test, func(t *testing.T) {
t.Parallel()

argocd, _ := getTestServer(t, testDataCopy.anonymousEnabled, true)
argocd, _ := getTestServer(t, testDataCopy.anonymousEnabled, true, true)
ctx := context.Background()

ctx, err := argocd.Authenticate(ctx)
Expand Down Expand Up @@ -722,7 +815,7 @@ func TestAuthenticate_no_SSO(t *testing.T) {
// Must be declared here to avoid race.
ctx := context.Background() //nolint:ineffassign,staticcheck

argocd, dexURL := getTestServer(t, testDataCopy.anonymousEnabled, false)
argocd, dexURL := getTestServer(t, testDataCopy.anonymousEnabled, false, true)
token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.RegisteredClaims{Issuer: fmt.Sprintf("%s/api/dex", dexURL)})
tokenString, err := token.SignedString([]byte("key"))
require.NoError(t, err)
Expand Down Expand Up @@ -795,7 +888,7 @@ func TestAuthenticate_bad_request_metadata(t *testing.T) {
test: "anonymous disabled, bad auth header",
anonymousEnabled: false,
metadata: metadata.MD{"authorization": []string{"Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiJ9.TGGTTHuuGpEU8WgobXxkrBtW3NiR3dgw5LR-1DEW3BQ"}},
expectedErrorMessage: "no audience found in the token",
expectedErrorMessage: common.TokenVerificationError,
expectedClaims: nil,
},
{
Expand All @@ -809,7 +902,7 @@ func TestAuthenticate_bad_request_metadata(t *testing.T) {
test: "anonymous disabled, bad auth cookie",
anonymousEnabled: false,
metadata: metadata.MD{"grpcgateway-cookie": []string{"argocd.token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiJ9.TGGTTHuuGpEU8WgobXxkrBtW3NiR3dgw5LR-1DEW3BQ"}},
expectedErrorMessage: "no audience found in the token",
expectedErrorMessage: common.TokenVerificationError,
expectedClaims: nil,
},
{
Expand All @@ -830,7 +923,7 @@ func TestAuthenticate_bad_request_metadata(t *testing.T) {
// Must be declared here to avoid race.
ctx := context.Background() //nolint:ineffassign,staticcheck

argocd, _ := getTestServer(t, testDataCopy.anonymousEnabled, true)
argocd, _ := getTestServer(t, testDataCopy.anonymousEnabled, true, true)
ctx = metadata.NewIncomingContext(context.Background(), testDataCopy.metadata)

ctx, err := argocd.Authenticate(ctx)
Expand Down
7 changes: 5 additions & 2 deletions util/oidc/oidc.go
View file
Expand Up @@ -353,9 +353,12 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
http.Error(w, "no id_token in token response", http.StatusInternalServerError)
return
}
idToken, err := a.provider.Verify(a.clientID, idTokenRAW)

idToken, err := a.provider.Verify(idTokenRAW, a.settings)

if err != nil {
http.Error(w, fmt.Sprintf("invalid session token: %v", err), http.StatusInternalServerError)
log.Warnf("Failed to verify token: %s", err)
http.Error(w, common.TokenVerificationError, http.StatusInternalServerError)
return
}
path := "/"
Expand Down
2 changes: 1 addition & 1 deletion util/oidc/oidc_test.go
View file
Expand Up @@ -90,7 +90,7 @@ func (p *fakeProvider) ParseConfig() (*OIDCConfiguration, error) {
return nil, nil
}

func (p *fakeProvider) Verify(_, _ string) (*gooidc.IDToken, error) {
func (p *fakeProvider) Verify(_ string, _ *settings.ArgoCDSettings) (*gooidc.IDToken, error) {
return nil, nil
}

Expand Down

0 comments on commit b38bc00

Please sign in to comment.