Skip to content

Commit

Permalink
Merge #102635
Browse files Browse the repository at this point in the history 
102635: server, tenant: gate process debugging behind capability r=knz,abarganier a=dhartunian

Previously, all tenant servers were started with a debug server
that granted process-manipulating power via pprof and vmodule HTTP
endpoints.

This implementation is fine when servers serve just 1 tenant in
a given process; that tenant then legitimately has access to all
process-level control. However, it becomes a problem in shared-process
multitenancy, when the same process is shared by multiple tenants. In
that case, it is undesirable for 1 tenant to have access to & control
process properties, as it could influence the well-functioning of
other tenants or potentially leak data across tenant boundaries.

This commit gates access to the debug server behind a capability
**only with shared process multitenancy**. Tenant servers running
within their own process will contain a debug server with no
capability gating since they own their process.

The gating is implemented via a tenant authorizer function backed
by the capability store that is injected into the debug server
upon startup. Shared process tenants are provided this authorizer,
while separate process tenants and the system tenant use the no-op
authorizer.

Epic: CRDB-12100
Resolves: #97946

Release note: None

Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
  • Loading branch information
@dhartunian
craig[bot] and dhartunian committed Jul 5, 2023
2 parents 9e84bfe + cc16c03 commit 5b3cd0d
Show file tree
Hide file tree
Showing 48 changed files with 472 additions and 160 deletions.
15 changes: 15 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/tenant_capability
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -61,6 +62,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -79,6 +81,7 @@ can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -104,6 +107,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -129,6 +133,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -154,6 +159,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info true
can_view_tsdb_metrics false
Expand All @@ -172,6 +178,7 @@ can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -190,6 +197,7 @@ can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -208,6 +216,7 @@ can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -227,6 +236,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit true
can_check_consistency true
can_debug_process true
can_use_nodelocal_storage true
can_view_node_info true
can_view_tsdb_metrics true
Expand Down Expand Up @@ -268,6 +278,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand Down Expand Up @@ -297,6 +308,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand Down Expand Up @@ -331,6 +343,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -349,6 +362,7 @@ can_admin_scatter false
can_admin_split false
can_admin_unsplit false
can_check_consistency false
can_debug_process false
can_use_nodelocal_storage false
can_view_node_info false
can_view_tsdb_metrics false
Expand All @@ -367,6 +381,7 @@ can_admin_scatter true
can_admin_split true
can_admin_unsplit true
can_check_consistency true
can_debug_process true
can_use_nodelocal_storage true
can_view_node_info true
can_view_tsdb_metrics true
Expand Down
6 changes: 3 additions & 3 deletions pkg/ccl/oidcccl/authentication_oidc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import (
"github.com/stretchr/testify/require"
)

func tenantOrSystemAdminURL(s serverutils.TestServerInterface) string {
func tenantOrSystemAdminURL(s serverutils.TestServerInterface) *serverutils.TestURL {
if len(s.TestTenants()) > 0 {
return s.TestTenants()[0].AdminURL()
}
Expand Down Expand Up @@ -71,7 +71,7 @@ func TestOIDCBadRequestIfDisabled(t *testing.T) {
client, err := testCertsContext.GetHTTPClient()
require.NoError(t, err)

resp, err := client.Get(tenantOrSystemAdminURL(s) + "/oidc/v1/login")
resp, err := client.Get(tenantOrSystemAdminURL(s).WithPath("/oidc/v1/login").String())
if err != nil {
t.Fatalf("could not issue GET request to admin server: %s", err)
}
Expand Down Expand Up @@ -196,7 +196,7 @@ func TestOIDCEnabled(t *testing.T) {
}

t.Run("login redirect", func(t *testing.T) {
resp, err := client.Get(tenantOrSystemAdminURL(s) + "/oidc/v1/login")
resp, err := client.Get(tenantOrSystemAdminURL(s).WithPath("/oidc/v1/login").String())
if err != nil {
t.Fatalf("could not issue GET request to admin server: %s", err)
}
Expand Down
28 changes: 12 additions & 16 deletions pkg/ccl/serverccl/server_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"io"
"net/http"
"net/http/cookiejar"
"net/url"
"testing"
"time"

Expand Down Expand Up @@ -167,7 +166,6 @@ func TestServerControllerHTTP(t *testing.T) {

// Retrieve a privileged HTTP client. NB: this also populates
// system.web_sessions.
aurl := s.AdminURL()
client, err := s.GetAdminHTTPClient()
require.NoError(t, err)

Expand Down Expand Up @@ -252,8 +250,10 @@ VALUES($1, $2, $3, $4, $5, (SELECT user_id FROM system.users WHERE username = $3
return &ls, err
}

var aurl *serverutils.TestURL
newreq := func() *http.Request {
req, err := http.NewRequest("GET", aurl+"/_status/sessions", nil)
aurl = s.AdminURL()
req, err := http.NewRequest("GET", aurl.WithPath("/_status/sessions").String(), nil)
require.NoError(t, err)
return req
}
Expand Down Expand Up @@ -289,9 +289,7 @@ VALUES($1, $2, $3, $4, $5, (SELECT user_id FROM system.users WHERE username = $3
HttpOnly: true,
Secure: true,
}
purl, err := url.Parse(aurl)
require.NoError(t, err)
client.Jar.SetCookies(purl, []*http.Cookie{c})
client.Jar.SetCookies(aurl.URL, []*http.Cookie{c})

req = newreq()
body, err = get(req)
Expand All @@ -303,7 +301,7 @@ VALUES($1, $2, $3, $4, $5, (SELECT user_id FROM system.users WHERE username = $3
t.Logf("retrieving session list from test tenant via cookie")

c.Value = "hello"
client.Jar.SetCookies(purl, []*http.Cookie{c})
client.Jar.SetCookies(aurl.URL, []*http.Cookie{c})
req = newreq()
body, err = get(req)
require.NoError(t, err)
Expand Down Expand Up @@ -351,7 +349,7 @@ func TestServerControllerDefaultHTTPTenant(t *testing.T) {
client, err := s.GetUnauthenticatedHTTPClient()
require.NoError(t, err)

resp, err := client.Post(s.AdminURL()+"/login",
resp, err := client.Post(s.AdminURL().WithPath("/login").String(),
"application/json",
bytes.NewBuffer([]byte("{\"username\":\"foo\",\"password\":\"cockroach\"})")),
)
Expand Down Expand Up @@ -392,15 +390,15 @@ func TestServerControllerBadHTTPCookies(t *testing.T) {
Secure: true,
}

req, err := http.NewRequest("GET", s.AdminURL()+"/", nil)
req, err := http.NewRequest("GET", s.AdminURL().WithPath("/").String(), nil)
require.NoError(t, err)
req.AddCookie(c)
resp, err := client.Do(req)
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, 200, resp.StatusCode)

req, err = http.NewRequest("GET", s.AdminURL()+"/bundle.js", nil)
req, err = http.NewRequest("GET", s.AdminURL().WithPath("/bundle.js").String(), nil)
require.NoError(t, err)
req.AddCookie(c)
resp, err = client.Do(req)
Expand Down Expand Up @@ -569,7 +567,7 @@ func TestServerControllerLoginLogout(t *testing.T) {
client, err := s.GetAuthenticatedHTTPClient(false, serverutils.SingleTenantSession)
require.NoError(t, err)

resp, err := client.Post(s.AdminURL()+"/logout", "", nil)
resp, err := client.Post(s.AdminURL().WithPath("/logout").String(), "", nil)
require.NoError(t, err)
defer resp.Body.Close()

Expand All @@ -590,7 +588,7 @@ func TestServerControllerLoginLogout(t *testing.T) {
clientMT, err := s2.GetAuthenticatedHTTPClient(false, serverutils.MultiTenantSession)
require.NoError(t, err)

respMT, err := clientMT.Get(s.AdminURL() + "/logout")
respMT, err := clientMT.Get(s.AdminURL().WithPath("/logout").String())
require.NoError(t, err)
defer respMT.Body.Close()

Expand All @@ -605,19 +603,17 @@ func TestServerControllerLoginLogout(t *testing.T) {
require.ElementsMatch(t, []string{"", ""}, cookieValues)

// Now using manual clients to simulate states that might be invalid
url, err := url.Parse(s2.AdminURL())
require.NoError(t, err)
cookieJar, err := cookiejar.New(nil)
require.NoError(t, err)
cookieJar.SetCookies(url, []*http.Cookie{
cookieJar.SetCookies(s2.AdminURL().URL, []*http.Cookie{
{
Name: "multitenant-session",
Value: "abc-123",
},
})
clientMT.Jar = cookieJar

respBadCookie, err := clientMT.Get(s.AdminURL() + "/logout")
respBadCookie, err := clientMT.Get(s.AdminURL().WithPath("/logout").String())
require.NoError(t, err)
defer respBadCookie.Body.Close()

Expand Down
Loading

0 comments on commit 5b3cd0d

Please sign in to comment.