Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Adds proper CSRF protection to ArgoCD API #16856

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions applicationset/webhook/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,7 @@ func newFakeClient(ns string) *kubefake.Clientset {
},
Data: map[string][]byte{
"server.secretkey": nil,
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
})
}
3 changes: 3 additions & 0 deletions cmd/argocd-server/commands/argocd_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func NewCommand() *cobra.Command {
repoServerAddress string
dexServerAddress string
disableAuth bool
disableCsrf bool
enableGZip bool
tlsConfigCustomizerSrc func() (tls.ConfigCustomizer, error)
cacheSrc func() (*servercache.Cache, error)
Expand Down Expand Up @@ -185,6 +186,7 @@ func NewCommand() *cobra.Command {
DexServerAddr: dexServerAddress,
DexTLSConfig: dexTlsConfig,
DisableAuth: disableAuth,
DisableCsrf: disableCsrf,
EnableGZip: enableGZip,
TLSConfigCustomizer: tlsConfigCustomizer,
Cache: cache,
Expand Down Expand Up @@ -240,6 +242,7 @@ func NewCommand() *cobra.Command {
command.Flags().StringVar(&repoServerAddress, "repo-server", env.StringFromEnv("ARGOCD_SERVER_REPO_SERVER", common.DefaultRepoServerAddr), "Repo server address")
command.Flags().StringVar(&dexServerAddress, "dex-server", env.StringFromEnv("ARGOCD_SERVER_DEX_SERVER", common.DefaultDexServerAddr), "Dex server address")
command.Flags().BoolVar(&disableAuth, "disable-auth", env.ParseBoolFromEnv("ARGOCD_SERVER_DISABLE_AUTH", false), "Disable client authentication")
command.Flags().BoolVar(&disableCsrf, "disable-csrf", false, "Disable API CSRF protection")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make this configurable via argocd-cmd-params-cm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

command.Flags().BoolVar(&enableGZip, "enable-gzip", env.ParseBoolFromEnv("ARGOCD_SERVER_ENABLE_GZIP", true), "Enable GZIP compression")
command.AddCommand(cli.NewVersionCmd(cliName))
command.Flags().StringVar(&listenHost, "address", env.StringFromEnv("ARGOCD_SERVER_LISTEN_ADDRESS", common.DefaultAddressAPIServer), "Listen on given address")
Expand Down
1 change: 1 addition & 0 deletions cmd/argocd/commands/admin/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ func (opts *settingsOpts) createSettingsManager(ctx context.Context) (*settings.
Data: map[string][]byte{
"admin.password": []byte("test"),
"server.secretkey": []byte("test"),
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
}
Expand Down
1 change: 1 addition & 0 deletions cmd/argocd/commands/admin/settings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ func newSettingsManager(data map[string]string) *settings.SettingsManager {
Data: map[string][]byte{
"admin.password": []byte("test"),
"server.secretkey": []byte("test"),
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
})
return settings.NewSettingsManager(ctx, clientset, "default")
Expand Down
1 change: 1 addition & 0 deletions controller/clusterinfoupdater_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func TestClusterSecretUpdater(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
kubeclientset := fake.NewSimpleClientset(emptyArgoCDConfigMap, argoCDSecret)
Expand Down
4 changes: 4 additions & 0 deletions docs/operator-manual/argocd-secret.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ data:
# Autogenerated when missing.
server.secretkey:

# the 32 bytes long authentication key used for CSRF protection
# Autogenerated when missing.
server.csrfkey:

# Shared secrets for authenticating GitHub, GitLab, BitBucket webhook events (optional).
# See https://github.com/argoproj/argo-cd/blob/master/docs/operator-manual/webhook.md for additional details.
# github webhook secret
Expand Down
1 change: 1 addition & 0 deletions docs/operator-manual/server-commands/argocd-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ argocd-server [flags]
--dex-server-strict-tls Perform strict validation of TLS certificates when connecting to dex server
--disable-auth Disable client authentication
--disable-compression If true, opt-out of response compression for all requests to the server
--disable-csrf Disable API CSRF protection
--enable-gzip Enable GZIP compression (default true)
--enable-proxy-extension Enable Proxy Extension feature
--gloglevel int Set the glog logging level
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ require (
github.com/google/go-jsonnet v0.20.0
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
github.com/google/uuid v1.3.1
github.com/gorilla/csrf v1.7.2
github.com/gorilla/handlers v1.5.1
github.com/gorilla/websocket v1.5.0
github.com/gosimple/slug v1.13.1
Expand Down Expand Up @@ -131,6 +132,7 @@ require (
github.com/google/s2a-go v0.1.4 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.2.5 // indirect
github.com/googleapis/gax-go/v2 v2.12.0 // indirect
github.com/gorilla/securecookie v1.1.2 // indirect
github.com/kylelemons/godebug v1.1.0 // indirect
github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 // indirect
github.com/tidwall/gjson v1.14.4 // indirect
Expand Down
4 changes: 4 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1197,10 +1197,14 @@ github.com/gopackage/ddp v0.0.0-20170117053602-652027933df4 h1:4EZlYQIiyecYJlUbV
github.com/gopackage/ddp v0.0.0-20170117053602-652027933df4/go.mod h1:lEO7XoHJ/xNRBCxrn4h/CEB67h0kW1B0t4ooP2yrjUA=
github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
github.com/gorilla/context v1.1.1/go.mod h1:kBGZzfjB9CEq2AlWe17Uuf7NDRt0dE0s8S51q0aT7Yg=
github.com/gorilla/csrf v1.7.2 h1:oTUjx0vyf2T+wkrx09Trsev1TE+/EbDAeHtSTbtC2eI=
github.com/gorilla/csrf v1.7.2/go.mod h1:F1Fj3KG23WYHE6gozCmBAezKookxbIvUJT+121wTuLk=
github.com/gorilla/handlers v1.5.1 h1:9lRY6j8DEeeBT10CvO9hGW0gmky0BprnvDI5vfhUHH4=
github.com/gorilla/handlers v1.5.1/go.mod h1:t8XrUpc4KVXb7HGyJ4/cEnwQiaxrX/hz1Zv/4g96P1Q=
github.com/gorilla/mux v1.6.2/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs=
github.com/gorilla/mux v1.7.3/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs=
github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kXD8ePA=
github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pwzwo4h3eOamfo=
github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ=
github.com/gorilla/websocket v1.4.0/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ=
github.com/gorilla/websocket v1.4.1/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE=
Expand Down
1 change: 1 addition & 0 deletions server/account/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func newTestAccountServerExt(ctx context.Context, enforceFn rbac.ClaimsEnforcerF
Data: map[string][]byte{
"admin.password": []byte(bcrypt),
"server.secretkey": []byte("test"),
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
for i := range opts {
Expand Down
1 change: 1 addition & 0 deletions server/application/application_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ func newTestAppServerWithEnforcerConfigure(f func(*rbac.Enforcer), t *testing.T,
Data: map[string][]byte{
"admin.password": []byte("test"),
"server.secretkey": []byte("test"),
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
})
ctx := context.Background()
Expand Down
1 change: 1 addition & 0 deletions server/applicationset/applicationset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ func newTestAppSetServerWithEnforcerConfigure(f func(*rbac.Enforcer), namespace
Data: map[string][]byte{
"admin.password": []byte("test"),
"server.secretkey": []byte("test"),
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
})
ctx := context.Background()
Expand Down
1 change: 1 addition & 0 deletions server/badge/badge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ var (
Data: map[string][]byte{
"admin.password": []byte("test"),
"server.secretkey": []byte("test"),
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
argoCDCm = corev1.ConfigMap{
Expand Down
22 changes: 12 additions & 10 deletions server/cluster/cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,17 @@ import (
"testing"
"time"

"github.com/argoproj/gitops-engine/pkg/utils/kube/kubetest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/fake"
"k8s.io/utils/pointer"

"github.com/argoproj/argo-cd/v2/common"
"github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster"
clusterapi "github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster"
Expand All @@ -21,16 +32,6 @@ import (
dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks"
"github.com/argoproj/argo-cd/v2/util/rbac"
"github.com/argoproj/argo-cd/v2/util/settings"
"github.com/argoproj/gitops-engine/pkg/utils/kube/kubetest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/fake"
"k8s.io/utils/pointer"
)

func newServerInMemoryCache() *servercache.Cache {
Expand Down Expand Up @@ -478,6 +479,7 @@ func getClientset(config map[string]string, ns string, objects ...runtime.Object
Data: map[string][]byte{
"admin.password": []byte("test"),
"server.secretkey": []byte("test"),
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
cm := corev1.ConfigMap{
Expand Down
5 changes: 5 additions & 0 deletions server/logout/logout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var (
expectedNonOIDCLogoutURL = "http://localhost:4000"
expectedOIDCLogoutURL = "https://dev-5695098.okta.com/oauth2/v1/logout?id_token_hint=" + oidcToken + "&post_logout_redirect_uri=" + baseURL
expectedOIDCLogoutURLWithRootPath = "https://dev-5695098.okta.com/oauth2/v1/logout?id_token_hint=" + oidcToken + "&post_logout_redirect_uri=" + baseURL + "/" + rootPath
testCSRFKey = []byte("12345678901234567890123456789012")
)

func TestConstructLogoutURL(t *testing.T) {
Expand Down Expand Up @@ -114,6 +115,7 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": testCSRFKey,
},
},
)
Expand Down Expand Up @@ -146,6 +148,7 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": testCSRFKey,
},
},
)
Expand Down Expand Up @@ -177,6 +180,7 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": testCSRFKey,
},
},
)
Expand Down Expand Up @@ -204,6 +208,7 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": testCSRFKey,
},
},
)
Expand Down
1 change: 1 addition & 0 deletions server/project/project_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func TestProjectServer(t *testing.T) {
Data: map[string][]byte{
"admin.password": []byte("test"),
"server.secretkey": []byte("test"),
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
})
settingsMgr := settings.NewSettingsManager(context.Background(), kubeclientset, testNamespace)
Expand Down
63 changes: 60 additions & 3 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

// nolint:staticcheck
golang_proto "github.com/golang/protobuf/proto"
"github.com/gorilla/csrf"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/selection"

Expand Down Expand Up @@ -197,6 +198,7 @@ type ArgoCDServer struct {

type ArgoCDServerOpts struct {
DisableAuth bool
DisableCsrf bool
EnableGZip bool
Insecure bool
StaticAssetsDir string
Expand Down Expand Up @@ -982,14 +984,29 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl
// golang/protobuf. Which does not support types such as time.Time. gogo/protobuf does support
// time.Time, but does not support custom UnmarshalJSON() and MarshalJSON() methods. Therefore
// we use our own Marshaler
gwMuxOpts := runtime.WithMarshalerOption(runtime.MIMEWildcard, new(grpc_util.JSONMarshaler))
gwCookieOpts := runtime.WithForwardResponseOption(a.translateGrpcCookieHeader)
gwmux := runtime.NewServeMux(gwMuxOpts, gwCookieOpts)
gwOpts := []runtime.ServeMuxOption{
runtime.WithMarshalerOption(runtime.MIMEWildcard, new(grpc_util.JSONMarshaler)),
runtime.WithForwardResponseOption(a.translateGrpcCookieHeader),
}
if !a.DisableCsrf {
gwOpts = append(gwOpts, runtime.WithForwardResponseOption(func(ctx context.Context, w http.ResponseWriter, resp golang_proto.Message) error {
r := http.Request{}
token := csrf.Token(r.WithContext(ctx))
if token != "" {
w.Header().Set("x-csrf-token", token)
}
return nil
}))
}
gwmux := runtime.NewServeMux(gwOpts...)

var handler http.Handler = gwmux
if a.EnableGZip {
handler = compressHandler(handler)
}
if !a.DisableCsrf {
handler = csrf.Protect(a.settings.CsrfKey, csrf.Secure(a.useTLS()), csrf.Path("/api/v1"))(handler)
}
mux.Handle("/api/", handler)

terminal := application.NewHandler(a.appLister, a.Namespace, a.ApplicationNamespaces, a.db, a.enf, a.Cache, appResourceTreeFn, a.settings.ExecShells, *a.sessionMgr).
Expand Down Expand Up @@ -1380,6 +1397,13 @@ func getToken(md metadata.MD) string {
return ""
}

// Add JSON API paths to NoCsrfHeaderPatterns slice that allow unauthenticated
// GET requests, so that they will not return X-CSRF-Token header on response.
var NoCsrfHeaderPattern = []*regexp.Regexp{
regexp.MustCompile(`/api/version.*`),
regexp.MustCompile(`/api/v1/settings.*`),
}

type handlerSwitcher struct {
handler http.Handler
urlToHandler map[string]http.Handler
Expand All @@ -1392,6 +1416,39 @@ func (s *handlerSwitcher) ServeHTTP(w http.ResponseWriter, r *http.Request) {
} else if contentHandler, ok := s.contentTypeToHandler[r.Header.Get("content-type")]; ok {
contentHandler.ServeHTTP(w, r)
} else {
// If we have a valid authorization header for API access, skip CSRF protection for this request
// This is accomplished by setting the key gorilla.csrf.Skip in the request's context.
//
// We don't validate the JWT token here - this is done at API level. But we make sure that the
// request does not carry a cookie named "argocd.token" as sent by the browser. This would
// circumvent CSRF protection.
//
// API access from scripts or other clients therefore MUST always set "Authorization" header
// and MUST NOT send the JWT in a cookie (or MUST ALSO send the X-CSRF-Token header along)
//
// We do allow a POST on /api/v1/session to get a valid JWT token without having to also
// supply the X-CSRF-Token header, as long as the argocd.token is not set.
//
if r.URL.Path == "/api/v1/session" && r.Method == "POST" {
r = csrf.UnsafeSkipCheck(r)
} else if _, err := r.Cookie(common.AuthCookieName); err != nil {
if authHdr := r.Header.Get("Authorization"); authHdr != "" {
r = csrf.UnsafeSkipCheck(r)
}
}

// We do not want to send X-CSRF-Token on unauthenticated requests, so we skip CSRF checks
// on safe requests to our unauthenticated URLs
switch strings.ToUpper(r.Method) {
case "GET", "HEAD", "OPTIONS", "TRACE":
for _, pattern := range NoCsrfHeaderPattern {
if pattern.MatchString(r.URL.Path) {
r = csrf.UnsafeSkipCheck(r)
break
}
}
}
// gorilla-csrf takes care that this request is properly protected
s.handler.ServeHTTP(w, r)
}
}
Expand Down
4 changes: 1 addition & 3 deletions test/e2e/fixture/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import (
"io"
"net/http"
"net/url"

"github.com/argoproj/argo-cd/v2/common"
)

// DoHttpRequest executes a http request against the Argo CD API server
Expand All @@ -27,7 +25,7 @@ func DoHttpRequest(method string, path string, data ...byte) (*http.Response, er
if err != nil {
return nil, err
}
req.AddCookie(&http.Cookie{Name: common.AuthCookieName, Value: token})
req.Header.Add("Authorization", "Bearer "+token)

httpClient := &http.Client{
Transport: &http.Transport{
Expand Down
10 changes: 10 additions & 0 deletions ui/src/app/shared/services/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,17 @@ function apiRoot(): string {
return toAbsURL('/api/v1');
}

const csrfHeaderName = 'x-csrf-token';
let crsfToken: string;

function initHandlers(req: agent.Request) {
req.set(csrfHeaderName, crsfToken || '');
req.on('response', res => {
const val = res.header['x-csrf-token'];
if (val) {
crsfToken = val;
}
});
req.on('error', err => onError.next(err));
return req;
}
Expand Down
2 changes: 2 additions & 0 deletions util/db/cluster_norace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func TestWatchClusters_CreateRemoveCluster(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
kubeclientset := fake.NewSimpleClientset(emptyArgoCDConfigMap, argoCDSecret)
Expand Down Expand Up @@ -101,6 +102,7 @@ func TestWatchClusters_LocalClusterModifications(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
kubeclientset := fake.NewSimpleClientset(emptyArgoCDConfigMap, argoCDSecret)
Expand Down
2 changes: 2 additions & 0 deletions util/db/cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ func TestRejectCreationForInClusterWhenDisabled(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
kubeclientset := fake.NewSimpleClientset(argoCDConfigMapWithInClusterServerAddressDisabled, argoCDSecret)
Expand Down Expand Up @@ -332,6 +333,7 @@ func TestListClusters(t *testing.T) {
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
"server.csrfkey": []byte("12345678901234567890123456789012"),
},
}
secretForServerWithInClusterAddr := &v1.Secret{
Expand Down