Skip to content

Commit

Permalink
fix: Adds proper CSRF protection to ArgoCD API
Browse files Browse the repository at this point in the history
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
  • Loading branch information
alexmt committed Jan 12, 2024
1 parent b12630c commit ab8bb46
Show file tree
Hide file tree
Showing 25 changed files with 167 additions and 16 deletions.
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")
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 _, err := r.Cookie(common.AuthCookieName); err != nil {
if authHdr := r.Header.Get("Authorization"); strings.HasPrefix(authHdr, "Bearer ") {
r = csrf.UnsafeSkipCheck(r)
}
} else if r.URL.Path == "/api/v1/session" && r.Method == "POST" {
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
Loading

0 comments on commit ab8bb46

Please sign in to comment.