Skip to content

Commit

Permalink
apiv2: accept cookie auth when header is non-empty
Browse files Browse the repository at this point in the history
In order to make use of HTTP endpoints under `/api/v2` in the DB Console
it is necessary to support cookie-based authentication for ergonomic
Javascript use.

Previously, header-based auth was not possible to use in the DB Console
because the login endpoint we use returns the session in a Cookie.
Moving this cookie into a header would require us to read into a
less-secure storage method (local storage, redux, etc.) instead of
keeping it secure in the browser's cookie storage.

We implement a suggestion to rely on Cookie auth by requiring the
presence of the auth header with a magic value of `"cookie"` that tells the
server to look for the session in the session cookie. This forces the caller
to modify the request via JS, which protects us from CSRF since
cross-origin requests can only be "simple". See the issue for further
discussion.

Resolves #84311

Release note (security update): The HTTP endpoints under the `/api/v2` prefix
will now accept cookie-based authentication similar to other HTTP endpoints
used by the DB Console. The encoded session *must* be in a cookie named
`"session"`, and the `"X-Cockroach-API-Session"` header is required to be set
to `"cookie"` for the session to be read from the cookie header. A cookie
provided without the custom header present will be ignored.
  • Loading branch information
dhartunian committed Aug 8, 2022
1 parent 033c911 commit 6d36dfc
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 11 deletions.
57 changes: 46 additions & 11 deletions pkg/server/api_v2_auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -277,32 +277,67 @@ func newAuthenticationV2Mux(s *authenticationV2Server, inner http.Handler) *auth
}
}

// getSession decodes the cookie from the request, looks up the corresponding session, and
// returns the logged in user name. If there's an error, it returns an error value and
// also sends the error over http using w.
// apiV2UseCookieBasedAuth is a magic value of the auth header that
// tells us to look for the session in the cookie. This can be used by
// frontend code to maintain cookie-based auth while interacting with
// the API.
const apiV2UseCookieBasedAuth = "cookie"

// getSession decodes the cookie from the request, looks up the corresponding
// session, and returns the logged-in username. The session can be looked up
// either from a session cookie as used in the non-v2 API server, or via the
// session header. In order for us to use the cookie as the session source, the
// header `"X-Cockroach-API-Session"` must be set to `"cookie"` (This is to
// guard against CSRF attacks in the browser since it forces the caller to use
// javascript to set the header). If there's an error, it returns an error value
// and also sends the error over http using w.
func (a *authenticationV2Mux) getSession(
w http.ResponseWriter, req *http.Request,
) (string, *serverpb.SessionCookie, error) {
// Validate the returned cookie.
ctx := req.Context()
// Validate the returned session header or cookie.
rawSession := req.Header.Get(apiV2AuthHeader)
if len(rawSession) == 0 {
err := errors.New("invalid session header")
http.Error(w, err.Error(), http.StatusUnauthorized)
return "", nil, err
}

possibleSessions := []string{}
if rawSession == apiV2UseCookieBasedAuth {
cookies := req.Cookies()
for _, c := range cookies {
if c.Name != SessionCookieName {
continue
}
possibleSessions = append(possibleSessions, c.Value)
}
} else {
possibleSessions = append(possibleSessions, rawSession)
}

sessionCookie := &serverpb.SessionCookie{}
decoded, err := base64.StdEncoding.DecodeString(rawSession)
if err != nil {
err := errors.New("invalid session header")
http.Error(w, err.Error(), http.StatusBadRequest)
return "", nil, err
var decoded []byte
var err error
for i := range possibleSessions {
decoded, err = base64.StdEncoding.DecodeString(possibleSessions[i])
if err != nil {
log.Warningf(ctx, "attempted to decode session but failed: %v", err)
continue
}
err = protoutil.Unmarshal(decoded, sessionCookie)
if err != nil {
log.Warningf(ctx, "attempted to unmarshal session but failed: %v", err)
continue
}
// We've successfully decoded a session from cookie or header.
break
}
if err := protoutil.Unmarshal(decoded, sessionCookie); err != nil {
if err != nil {
err := errors.New("invalid session header")
http.Error(w, err.Error(), http.StatusBadRequest)
return "", nil, err
}

valid, username, err := a.s.authServer.verifySession(req.Context(), sessionCookie)
if err != nil {
apiV2InternalError(req.Context(), err, w)
Expand Down
81 changes: 81 additions & 0 deletions pkg/server/api_v2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package server
import (
"context"
gosql "database/sql"
"encoding/base64"
"encoding/json"
"io/ioutil"
"net/http"
Expand All @@ -27,6 +28,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/metric"
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
"github.com/stretchr/testify/require"
"gopkg.in/yaml.v2"
)
Expand Down Expand Up @@ -183,3 +185,82 @@ func TestRulesV2(t *testing.T) {
require.NoError(t, yaml.NewDecoder(resp.Body).Decode(&ruleGroups))
require.NoError(t, resp.Body.Close())
}

func TestAuthV2(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

testCluster := serverutils.StartNewTestCluster(t, 3, base.TestClusterArgs{})
ctx := context.Background()
defer testCluster.Stopper().Stop(ctx)

ts := testCluster.Server(0)
client, err := ts.GetHTTPClient()
require.NoError(t, err)

session, err := ts.GetAuthSession(true)
require.NoError(t, err)
sessionBytes, err := protoutil.Marshal(session)
require.NoError(t, err)
sessionEncoded := base64.StdEncoding.EncodeToString(sessionBytes)

for _, tc := range []struct {
name string
header string
cookie string
expectedStatus int
}{
{
name: "no auth",
expectedStatus: http.StatusUnauthorized,
},
{
name: "session in header",
header: sessionEncoded,
expectedStatus: http.StatusOK,
},
{
name: "cookie auth with correct magic header",
cookie: sessionEncoded,
header: apiV2UseCookieBasedAuth,
expectedStatus: http.StatusOK,
},
{
name: "cookie auth but missing header",
cookie: sessionEncoded,
expectedStatus: http.StatusUnauthorized,
},
{
name: "cookie auth but wrong magic header",
cookie: sessionEncoded,
header: "yes",
// Bad Request and not Unauthorized because the session cannot be decoded.
expectedStatus: http.StatusBadRequest,
},
} {
t.Run(tc.name, func(t *testing.T) {
req, err := http.NewRequest("GET", ts.AdminURL()+apiV2Path+"sessions/", nil)
require.NoError(t, err)
if tc.header != "" {
req.Header.Set(apiV2AuthHeader, tc.header)
}
if tc.cookie != "" {
req.AddCookie(&http.Cookie{
Name: SessionCookieName,
Value: tc.cookie,
})
}
resp, err := client.Do(req)
require.NoError(t, err)
require.NotNil(t, resp)
defer resp.Body.Close()

if tc.expectedStatus != resp.StatusCode {
body, err := ioutil.ReadAll(resp.Body)
require.NoError(t, err)
t.Fatalf("expected status: %d but got: %d with body: %s", tc.expectedStatus, resp.StatusCode, string(body))
}
})
}

}
10 changes: 10 additions & 0 deletions pkg/server/testserver_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,16 @@ func (ts *httpTestServer) GetAuthenticatedHTTPClient(isAdmin bool) (http.Client,
return httpClient, err
}

// GetAuthenticatedHTTPClient implements the TestServerInterface.
func (ts *httpTestServer) GetAuthSession(isAdmin bool) (*serverpb.SessionCookie, error) {
authUser := authenticatedUserName()
if !isAdmin {
authUser = authenticatedUserNameNoAdmin()
}
_, cookie, err := ts.getAuthenticatedHTTPClientAndCookie(authUser, isAdmin)
return cookie, err
}

func (ts *httpTestServer) getAuthenticatedHTTPClientAndCookie(
authUser security.SQLUsername, isAdmin bool,
) (http.Client, *serverpb.SessionCookie, error) {
Expand Down
1 change: 1 addition & 0 deletions pkg/testutils/serverutils/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ go_library(
"//pkg/roachpb",
"//pkg/rpc",
"//pkg/security",
"//pkg/server/serverpb",
"//pkg/server/status",
"//pkg/settings/cluster",
"//pkg/storage",
Expand Down
4 changes: 4 additions & 0 deletions pkg/testutils/serverutils/test_tenant_shim.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/config"
"github.com/cockroachdb/cockroach/pkg/rpc"
"github.com/cockroachdb/cockroach/pkg/server/serverpb"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/util/hlc"
"github.com/cockroachdb/cockroach/pkg/util/log"
Expand Down Expand Up @@ -127,6 +128,9 @@ type TestTenantInterface interface {
// GetAuthenticatedHTTPClient returns an http client which has been
// authenticated to access Admin API methods (via a cookie).
GetAuthenticatedHTTPClient(isAdmin bool) (http.Client, error)
// GetEncodedSession returns a byte array containing a valid auth
// session.
GetAuthSession(isAdmin bool) (*serverpb.SessionCookie, error)

// DrainClients shuts down client connections.
DrainClients(ctx context.Context) error
Expand Down

0 comments on commit 6d36dfc

Please sign in to comment.