Skip to content

Commit

Permalink
Remove the remaining bits of unneeded CSRF code
Browse files Browse the repository at this point in the history
Additionally expand upon the godocs for our various middlewares
so that it's clear which ones provide CSRF protection.
  • Loading branch information
zmb3 committed Dec 21, 2024
1 parent 65b293e commit 0509984
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 57 deletions.
11 changes: 2 additions & 9 deletions integration/appaccess/pack.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ import (
"github.com/gravitational/teleport/lib/client"
"github.com/gravitational/teleport/lib/cryptosuites"
"github.com/gravitational/teleport/lib/events"
"github.com/gravitational/teleport/lib/httplib/csrf"
"github.com/gravitational/teleport/lib/httplib/reverseproxy"
"github.com/gravitational/teleport/lib/reversetunnelclient"
"github.com/gravitational/teleport/lib/service"
Expand Down Expand Up @@ -251,15 +250,9 @@ func (p *Pack) initWebSession(t *testing.T) {
req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewBuffer(csReq))
require.NoError(t, err)

// Attach CSRF token in cookie and header.
csrfToken, err := utils.CryptoRandomHex(32)
require.NoError(t, err)
req.AddCookie(&http.Cookie{
Name: csrf.CookieName,
Value: csrfToken,
})
// Set Content-Type header, otherwise Teleport's CSRF protection will
// reject the request.
req.Header.Set("Content-Type", "application/json; charset=utf-8")
req.Header.Set(csrf.HeaderName, csrfToken)

// Issue request.
client := &http.Client{
Expand Down
12 changes: 0 additions & 12 deletions integration/helpers/instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ import (
"github.com/gravitational/teleport/lib/cryptosuites"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/events"
"github.com/gravitational/teleport/lib/httplib/csrf"
"github.com/gravitational/teleport/lib/observability/tracing"
"github.com/gravitational/teleport/lib/reversetunnel"
"github.com/gravitational/teleport/lib/reversetunnelclient"
Expand Down Expand Up @@ -1531,18 +1530,7 @@ func CreateWebSession(proxyHost, user, password string) (*web.CreateSessionRespo
return nil, nil, trace.Wrap(err)
}

// Attach CSRF token in cookie and header.
csrfToken, err := utils.CryptoRandomHex(32)
if err != nil {
return nil, nil, trace.Wrap(err)
}

req.AddCookie(&http.Cookie{
Name: csrf.CookieName,
Value: csrfToken,
})
req.Header.Set("Content-Type", "application/json; charset=utf-8")
req.Header.Set(csrf.HeaderName, csrfToken)

// Issue request.
httpClient := &http.Client{
Expand Down
8 changes: 0 additions & 8 deletions integration/helpers/web.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,7 @@ func LoginWebClient(t *testing.T, host, username, password string) *WebClientPac
req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewBuffer(csReq))
require.NoError(t, err)

// Attach CSRF token in cookie and header.
csrfToken, err := utils.CryptoRandomHex(32)
require.NoError(t, err)
req.AddCookie(&http.Cookie{
Name: csrf.CookieName,
Value: csrfToken,
})
req.Header.Set("Content-Type", "application/json; charset=utf-8")
req.Header.Set(csrf.HeaderName, csrfToken)

// Issue request.
client := &http.Client{
Expand Down
17 changes: 0 additions & 17 deletions lib/client/weblogin.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ package client
import (
"bytes"
"context"
"crypto/rand"
"crypto/x509"
"encoding/hex"
"encoding/json"
"fmt"
"io"
Expand All @@ -50,7 +48,6 @@ import (
wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/httplib"
"github.com/gravitational/teleport/lib/httplib/csrf"
websession "github.com/gravitational/teleport/lib/web/session"
)

Expand Down Expand Up @@ -911,12 +908,6 @@ func SSHAgentLoginWeb(ctx context.Context, login SSHLoginDirect) (*WebClient, ty
return nil, nil, trace.Wrap(err)
}

token := make([]byte, 32)
if _, err := rand.Read(token); err != nil {
return nil, nil, trace.Wrap(err)
}

csrfToken := hex.EncodeToString(token)
resp, err := httplib.ConvertResponse(clt.RoundTrip(func() (*http.Response, error) {
var buf bytes.Buffer
if err := json.NewEncoder(&buf).Encode(&CreateWebSessionReq{
Expand All @@ -932,15 +923,7 @@ func SSHAgentLoginWeb(ctx context.Context, login SSHLoginDirect) (*WebClient, ty
return nil, err
}

cookie := &http.Cookie{
Name: csrf.CookieName,
Value: csrfToken,
}

req.AddCookie(cookie)

req.Header.Set("Content-Type", "application/json")
req.Header.Set(csrf.HeaderName, csrfToken)
return clt.HTTPClient().Do(req)
}))
if err != nil {
Expand Down
18 changes: 7 additions & 11 deletions lib/httplib/csrf/csrf.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,13 @@ import (
"github.com/gravitational/teleport/lib/utils"
)

const (
// CookieName is the name of the CSRF cookie used to protect against login
// CSRF in Teleport's SSO flows.
//
// It's prefixed with "__Host-" as an additional defense in depth measure.
// This makes sure the cookie is sent from a secure page (HTTPS),
// won't be sent to subdomains, and the path attribute is set to /.
CookieName = "__Host-grv_csrf"
// HeaderName is the default HTTP request header to inspect.
HeaderName = "X-CSRF-Token"
)
// CookieName is the name of the CSRF cookie used to protect against login
// CSRF in Teleport's SSO flows.
//
// It's prefixed with "__Host-" as an additional defense in depth measure.
// This makes sure the cookie is sent from a secure page (HTTPS),
// won't be sent to subdomains, and the path attribute is set to /.
const CookieName = "__Host-grv_csrf"

// tokenLenBytes is the length of a raw CSRF token prior to encoding
const tokenLenBytes = 32
Expand Down
12 changes: 12 additions & 0 deletions lib/web/apiserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -4399,6 +4399,8 @@ type ClusterWebsocketHandler func(w http.ResponseWriter, r *http.Request, p http
// WithClusterAuth wraps a ClusterHandler to ensure that a request is authenticated to this proxy
// (the same as WithAuth), as well as to grab the remoteSite (which can represent this local cluster
// or a remote trusted cluster) as specified by the ":site" url parameter.
//
// WithClusterAuth also provides CSRF protection by requiring the bearer token to be present.
func (h *Handler) WithClusterAuth(fn ClusterHandler) httprouter.Handle {
return httplib.MakeHandler(func(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) {
sctx, site, err := h.authenticateRequestWithCluster(w, r, p)
Expand Down Expand Up @@ -4694,6 +4696,7 @@ func (h *Handler) WithMetaRedirect(fn redirectHandlerFunc) httprouter.Handle {

// WithAuth ensures that a request is authenticated.
// Authenticated requests require both a session cookie as well as a bearer token.
// WithAuth also provides CSRF protection by requiring the bearer token to be present.
func (h *Handler) WithAuth(fn ContextHandler) httprouter.Handle {
return httplib.MakeHandler(func(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) {
sctx, err := h.AuthenticateRequest(w, r, true /* check bearer token */)
Expand All @@ -4706,6 +4709,10 @@ func (h *Handler) WithAuth(fn ContextHandler) httprouter.Handle {

// WithSession ensures that the request provides a session cookie.
// It does not check for a bearer token.
//
// WithSession does not provide CSRF protection, so it should only
// be used for non-state-changing requests or when other CSRF mitigations
// are applied.
func (h *Handler) WithSession(fn ContextHandler) httprouter.Handle {
return httplib.MakeHandler(func(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) {
sctx, err := h.AuthenticateRequest(w, r, false /* check bearer token */)
Expand Down Expand Up @@ -5118,6 +5125,11 @@ type SSOCallbackResponse struct {
// error is returned.
func SSOSetWebSessionAndRedirectURL(w http.ResponseWriter, r *http.Request, response *SSOCallbackResponse, verifyCSRF bool) error {
if verifyCSRF {
// Make sure that the CSRF token provided in this request matches
// the token that was used to initiate this SSO attempt.
//
// This ensures that an attacker cannot perform a login CSRF attack
// in order to get the victim to log in to the incorrect account.
if err := csrf.VerifyToken(response.CSRFToken, r); err != nil {
return trace.Wrap(err)
}
Expand Down

0 comments on commit 0509984

Please sign in to comment.