Merge cockroachdb#95880 cockroachdb#96152

95880: ui: move jobs details to keyed reducer, simplify polling r=ericharmeling a=ericharmeling This commit moves the jobs details to a keyed reducer and removes the invalidating receivedJobSaga, in favor of the component-level refresh interval. Loom: https://www.loom.com/share/67dbe4970d4d41419dbcdf5376ec8904 Epic: None Release note: None 96152: rpc: prevent cross-tenant RPCs r=jaylim-crl,JeffSwenson a=knz Fixes cockroachdb#96150. Prior to this patch, it was possible for a server for tenant 123 to perform RPCs to a server for enant 456. This patch disables that. Release note: None Epic: CRDB-23559 Co-authored-by: Eric Harmeling <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
Shivs11 · Jan 30, 2023 · 10ef5d9 · 10ef5d9
3 parents fefa5fe + d5770bb + 7c2d43a
commit 10ef5d9
Show file tree

Hide file tree

Showing 12 changed files with 269 additions and 127 deletions.
diff --git a/pkg/rpc/auth.go b/pkg/rpc/auth.go
@@ -143,8 +143,20 @@ func (a kvAuth) authenticate(ctx context.Context) (roachpb.TenantID, error) {
 		//
 		// Is this a connection from another SQL tenant server?
 		if security.IsTenantCertificate(clientCert) {
-			// Incoming connection originating from a tenant SQL server. Let through.
-			// The other server is able to use any of this server's RPCs.
+			// Incoming connection originating from a tenant SQL server.
+			tid, err := tenantFromCommonName(clientCert.Subject.CommonName)
+			if err != nil {
+				return roachpb.TenantID{}, err
+			}
+			// Verify that our peer is a service for the same tenant
+			// as ourselves (we don't want to allow tenant 123 to
+			// serve requests for a client coming from tenant 456).
+			if tid != a.tenant.tenantID {
+				return roachpb.TenantID{}, authErrorf("this tenant (%v) cannot serve requests from a server for tenant %v", a.tenant.tenantID, tid)
+			}
+
+			// We return an unset tenant ID, to bypass authorization checks:
+			// the other server is able to use any of this server's RPCs.
 			return roachpb.TenantID{}, nil
 		}
 	}

diff --git a/pkg/rpc/auth_test.go b/pkg/rpc/auth_test.go
@@ -15,6 +15,7 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"fmt"
 	"io"
 	"testing"
 
@@ -90,40 +91,48 @@ func TestWrappedServerStream(t *testing.T) {
 	require.Equal(t, 3, recv)
 }
 
-func TestTenantFromCert(t *testing.T) {
+func TestAuthenticateTenant(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	correctOU := []string{security.TenantsOU}
+	stid := roachpb.SystemTenantID
+	tenTen := roachpb.MustMakeTenantID(10)
 	for _, tc := range []struct {
+		systemID    roachpb.TenantID
 		ous         []string
 		commonName  string
 		expTenID    roachpb.TenantID
 		expErr      string
 		tenantScope uint64
 	}{
-		{ous: correctOU, commonName: "10", expTenID: roachpb.MustMakeTenantID(10)},
-		{ous: correctOU, commonName: roachpb.MinTenantID.String(), expTenID: roachpb.MinTenantID},
-		{ous: correctOU, commonName: roachpb.MaxTenantID.String(), expTenID: roachpb.MaxTenantID},
-		{ous: correctOU, commonName: roachpb.SystemTenantID.String() /* "system" */, expErr: `could not parse tenant ID from Common Name \(CN\)`},
-		{ous: correctOU, commonName: "-1", expErr: `could not parse tenant ID from Common Name \(CN\)`},
-		{ous: correctOU, commonName: "0", expErr: `invalid tenant ID 0 in Common Name \(CN\)`},
-		{ous: correctOU, commonName: "1", expErr: `invalid tenant ID 1 in Common Name \(CN\)`},
-		{ous: correctOU, commonName: "root", expErr: `could not parse tenant ID from Common Name \(CN\)`},
-		{ous: correctOU, commonName: "other", expErr: `could not parse tenant ID from Common Name \(CN\)`},
-		{ous: []string{"foo"}, commonName: "other", expErr: `client certificate CN=other,OU=foo cannot be used to perform RPC on tenant {1}`},
-		{ous: nil, commonName: "other", expErr: `client certificate CN=other cannot be used to perform RPC on tenant {1}`},
-		{ous: append([]string{"foo"}, correctOU...), commonName: "other", expErr: `could not parse tenant ID from Common Name`},
-		{ous: nil, commonName: "root"},
-		{ous: nil, commonName: "root", tenantScope: 10, expErr: "client certificate CN=root cannot be used to perform RPC on tenant {1}"},
+		{systemID: stid, ous: correctOU, commonName: "10", expTenID: tenTen},
+		{systemID: stid, ous: correctOU, commonName: roachpb.MinTenantID.String(), expTenID: roachpb.MinTenantID},
+		{systemID: stid, ous: correctOU, commonName: roachpb.MaxTenantID.String(), expTenID: roachpb.MaxTenantID},
+		{systemID: stid, ous: correctOU, commonName: roachpb.SystemTenantID.String() /* "system" */, expErr: `could not parse tenant ID from Common Name \(CN\)`},
+		{systemID: stid, ous: correctOU, commonName: "-1", expErr: `could not parse tenant ID from Common Name \(CN\)`},
+		{systemID: stid, ous: correctOU, commonName: "0", expErr: `invalid tenant ID 0 in Common Name \(CN\)`},
+		{systemID: stid, ous: correctOU, commonName: "1", expErr: `invalid tenant ID 1 in Common Name \(CN\)`},
+		{systemID: stid, ous: correctOU, commonName: "root", expErr: `could not parse tenant ID from Common Name \(CN\)`},
+		{systemID: stid, ous: correctOU, commonName: "other", expErr: `could not parse tenant ID from Common Name \(CN\)`},
+		{systemID: stid, ous: []string{"foo"}, commonName: "other", expErr: `client certificate CN=other,OU=foo cannot be used to perform RPC on tenant {1}`},
+		{systemID: stid, ous: nil, commonName: "other", expErr: `client certificate CN=other cannot be used to perform RPC on tenant {1}`},
+		{systemID: stid, ous: append([]string{"foo"}, correctOU...), commonName: "other", expErr: `could not parse tenant ID from Common Name`},
+		{systemID: stid, ous: nil, commonName: "root"},
+		{systemID: stid, ous: nil, commonName: "root", tenantScope: 10, expErr: "client certificate CN=root cannot be used to perform RPC on tenant {1}"},
+		{systemID: tenTen, ous: correctOU, commonName: "10", expTenID: roachpb.TenantID{}},
+		{systemID: tenTen, ous: correctOU, commonName: "123", expErr: `this tenant \(10\) cannot serve requests from a server for tenant 123`},
+		{systemID: tenTen, ous: correctOU, commonName: "1", expErr: `invalid tenant ID 1 in Common Name \(CN\)`},
 	} {
-		t.Run(tc.commonName, func(t *testing.T) {
+		t.Run(fmt.Sprintf("from %v to %v", tc.commonName, tc.systemID), func(t *testing.T) {
 			cert := &x509.Certificate{
 				Subject: pkix.Name{
 					CommonName:         tc.commonName,
 					OrganizationalUnit: tc.ous,
 				},
 			}
 			if tc.tenantScope > 0 {
-				tenantSANs, err := security.MakeTenantURISANs(username.MakeSQLUsernameFromPreNormalizedString(tc.commonName), []roachpb.TenantID{roachpb.MustMakeTenantID(tc.tenantScope)})
+				tenantSANs, err := security.MakeTenantURISANs(
+					username.MakeSQLUsernameFromPreNormalizedString(tc.commonName),
+					[]roachpb.TenantID{roachpb.MustMakeTenantID(tc.tenantScope)})
 				require.NoError(t, err)
 				cert.URIs = append(cert.URIs, tenantSANs...)
 			}
@@ -135,7 +144,7 @@ func TestTenantFromCert(t *testing.T) {
 			p := peer.Peer{AuthInfo: tlsInfo}
 			ctx := peer.NewContext(context.Background(), &p)
 
-			tenID, err := rpc.TestingAuthenticateTenant(ctx)
+			tenID, err := rpc.TestingAuthenticateTenant(ctx, tc.systemID)
 
 			if tc.expErr == "" {
 				require.Equal(t, tc.expTenID, tenID)

diff --git a/pkg/rpc/helpers_test.go b/pkg/rpc/helpers_test.go
@@ -36,8 +36,10 @@ func TestingNewWrappedServerStream(
 
 // TestingAuthenticateTenant performs authentication of a tenant from a context
 // for testing.
-func TestingAuthenticateTenant(ctx context.Context) (roachpb.TenantID, error) {
-	return kvAuth{tenant: tenantAuthorizer{tenantID: roachpb.SystemTenantID}}.authenticate(ctx)
+func TestingAuthenticateTenant(
+	ctx context.Context, serverTenantID roachpb.TenantID,
+) (roachpb.TenantID, error) {
+	return kvAuth{tenant: tenantAuthorizer{tenantID: serverTenantID}}.authenticate(ctx)
 }
 
 // TestingAuthorizeTenantRequest performs authorization of a tenant request

diff --git a/pkg/ui/workspaces/cluster-ui/src/api/jobsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/jobsApi.ts
@@ -19,6 +19,14 @@ export type JobsResponse = cockroach.server.serverpb.JobsResponse;
 
 export type JobRequest = cockroach.server.serverpb.JobRequest;
 export type JobResponse = cockroach.server.serverpb.JobResponse;
+export type JobResponseWithKey = {
+  jobResponse: JobResponse;
+  key: string;
+};
+export type ErrorWithKey = {
+  err: Error;
+  key: string;
+};
 
 export const getJobs = (
   req: JobsRequest,

diff --git a/pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx
@@ -76,7 +76,9 @@ export class JobDetails extends React.Component<JobDetailsProps> {
   }
 
   componentDidMount(): void {
-    this.refresh();
+    if (!this.props.job) {
+      this.refresh();
+    }
     // Refresh every 10s.
     this.refreshDataInterval = setInterval(() => this.refresh(), 10 * 1000);
   }
@@ -154,7 +156,7 @@ export class JobDetails extends React.Component<JobDetailsProps> {
 
   render(): React.ReactElement {
     const isLoading = !this.props.job || this.props.jobLoading;
-    const error = this.props.job && this.props.jobError;
+    const error = this.props.jobError;
     return (
       <div className={jobCx("job-details")}>
         <Helmet title={"Details | Job"} />

diff --git a/pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetailsConnected.tsx b/pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetailsConnected.tsx
@@ -12,7 +12,11 @@ import { connect } from "react-redux";
 import { RouteComponentProps, withRouter } from "react-router-dom";
 
 import { AppState } from "src/store";
-import { selectJobState } from "../../store/jobDetails/job.selectors";
+import {
+  selectJob,
+  selectJobLoading,
+  selectJobError,
+} from "../../store/jobDetails/job.selectors";
 import {
   JobDetailsStateProps,
   JobDetailsDispatchProps,
@@ -21,11 +25,13 @@ import {
 import { JobRequest } from "src/api/jobsApi";
 import { actions as jobActions } from "src/store/jobDetails";
 
-const mapStateToProps = (state: AppState): JobDetailsStateProps => {
-  const jobState = selectJobState(state);
-  const job = jobState ? jobState.data : null;
-  const jobLoading = jobState ? jobState.inFlight : false;
-  const jobError = jobState ? jobState.lastError : null;
+const mapStateToProps = (
+  state: AppState,
+  props: RouteComponentProps,
+): JobDetailsStateProps => {
+  const job = selectJob(state, props);
+  const jobLoading = selectJobLoading(state, props);
+  const jobError = selectJobError(state, props);
   return {
     job,
     jobLoading,

diff --git a/pkg/ui/workspaces/cluster-ui/src/store/jobDetails/job.reducer.ts b/pkg/ui/workspaces/cluster-ui/src/store/jobDetails/job.reducer.ts
@@ -9,7 +9,12 @@
 // licenses/APL.txt.
 
 import { createSlice, PayloadAction } from "@reduxjs/toolkit";
-import { JobRequest, JobResponse } from "src/api/jobsApi";
+import {
+  ErrorWithKey,
+  JobRequest,
+  JobResponse,
+  JobResponseWithKey,
+} from "src/api/jobsApi";
 import { DOMAIN_NAME } from "../utils";
 
 export type JobState = {
@@ -19,33 +24,46 @@ export type JobState = {
   inFlight: boolean;
 };
 
-const initialState: JobState = {
-  data: null,
-  lastError: null,
-  valid: true,
-  inFlight: false,
+export type JobDetailsReducerState = {
+  cachedData: {
+    [id: string]: JobState;
+  };
+};
+
+const initialState: JobDetailsReducerState = {
+  cachedData: {},
 };
 
 const JobSlice = createSlice({
   name: `${DOMAIN_NAME}/job`,
   initialState,
   reducers: {
-    received: (state, action: PayloadAction<JobResponse>) => {
-      state.data = action.payload;
-      state.valid = true;
-      state.lastError = null;
-      state.inFlight = false;
+    received: (state, action: PayloadAction<JobResponseWithKey>) => {
+      state.cachedData[action.payload.key] = {
+        data: action.payload.jobResponse,
+        valid: true,
+        lastError: null,
+        inFlight: false,
+      };
     },
-    failed: (state, action: PayloadAction<Error>) => {
-      state.valid = false;
-      state.lastError = action.payload;
+    failed: (state, action: PayloadAction<ErrorWithKey>) => {
+      state.cachedData[action.payload.key] = {
+        data: null,
+        valid: false,
+        lastError: action.payload.err,
+        inFlight: false,
+      };
     },
-    invalidated: state => {
-      state.valid = false;
+    invalidated: (state, action: PayloadAction<{ key: string }>) => {
+      state.cachedData[action.payload.key] = {
+        data: null,
+        valid: false,
+        lastError: null,
+        inFlight: false,
+      };
     },
-    refresh: (_, action: PayloadAction<JobRequest>) => {},
-    request: (_, action: PayloadAction<JobRequest>) => {},
-    reset: (_, action: PayloadAction<JobRequest>) => {},
+    refresh: (_, _action: PayloadAction<JobRequest>) => {},
+    request: (_, _action: PayloadAction<JobRequest>) => {},
   },
 });
 

diff --git a/pkg/ui/workspaces/cluster-ui/src/store/jobDetails/job.sagas.spec.ts b/pkg/ui/workspaces/cluster-ui/src/store/jobDetails/job.sagas.spec.ts
@@ -16,28 +16,46 @@ import {
 } from "redux-saga-test-plan/providers";
 import * as matchers from "redux-saga-test-plan/matchers";
 import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
-
-import { getJob } from "src/api/jobsApi";
-import { refreshJobSaga, requestJobSaga, receivedJobSaga } from "./job.sagas";
-import { actions, reducer, JobState } from "./job.reducer";
+import {
+  ErrorWithKey,
+  getJob,
+  JobRequest,
+  JobResponseWithKey,
+} from "src/api/jobsApi";
+import { refreshJobSaga, requestJobSaga } from "./job.sagas";
+import { actions, reducer, JobDetailsReducerState } from "./job.reducer";
 import { succeededJobFixture } from "../../jobs/jobsPage/jobsPage.fixture";
 import Long from "long";
+import { PayloadAction } from "@reduxjs/toolkit";
 
 describe("job sagas", () => {
   const payload = new cockroach.server.serverpb.JobRequest({
     job_id: new Long(8136728577, 70289336),
   });
+
+  const jobID = payload.job_id.toString();
+
   const jobResponse = new cockroach.server.serverpb.JobResponse(
     succeededJobFixture,
   );
 
+  const jobResponseWithKey: JobResponseWithKey = {
+    key: jobID,
+    jobResponse: jobResponse,
+  };
+
   const jobAPIProvider: (EffectProviders | StaticProvider)[] = [
     [matchers.call.fn(getJob), jobResponse],
   ];
 
+  const action: PayloadAction<JobRequest> = {
+    payload: payload,
+    type: "request",
+  };
+
   describe("refreshJobSaga", () => {
     it("dispatches refresh job action", () => {
-      return expectSaga(refreshJobSaga, actions.request(payload))
+      return expectSaga(refreshJobSaga, action)
         .provide(jobAPIProvider)
         .put(actions.request(payload))
         .run();
@@ -46,54 +64,44 @@ describe("job sagas", () => {
 
   describe("requestJobSaga", () => {
     it("successfully requests job", () => {
-      return expectSaga(requestJobSaga, actions.request(payload))
+      return expectSaga(requestJobSaga, action)
         .provide(jobAPIProvider)
-        .put(actions.received(jobResponse))
+        .put(actions.received(jobResponseWithKey))
         .withReducer(reducer)
-        .hasFinalState<JobState>({
-          data: jobResponse,
-          lastError: null,
-          valid: true,
-          inFlight: false,
+        .hasFinalState<JobDetailsReducerState>({
+          cachedData: {
+            [jobID]: {
+              data: jobResponseWithKey.jobResponse,
+              lastError: null,
+              valid: true,
+              inFlight: false,
+            },
+          },
         })
         .run();
     });
 
     it("returns error on failed request", () => {
       const error = new Error("Failed request");
-      return expectSaga(requestJobSaga, actions.request(payload))
+      const errorWithKey: ErrorWithKey = {
+        key: jobID,
+        err: error,
+      };
+      return expectSaga(requestJobSaga, action)
         .provide([[matchers.call.fn(getJob), throwError(error)]])
-        .put(actions.failed(error))
+        .put(actions.failed(errorWithKey))
         .withReducer(reducer)
-        .hasFinalState<JobState>({
-          data: null,
-          lastError: error,
-          valid: false,
-          inFlight: false,
+        .hasFinalState<JobDetailsReducerState>({
+          cachedData: {
+            [jobID]: {
+              data: null,
+              lastError: error,
+              valid: false,
+              inFlight: false,
+            },
+          },
         })
         .run();
     });
   });
-
-  describe("receivedJobSaga", () => {
-    it("sets valid status to false after specified period of time", () => {
-      const timeout = 500;
-      return expectSaga(receivedJobSaga, timeout)
-        .delay(timeout)
-        .put(actions.invalidated())
-        .withReducer(reducer, {
-          data: jobResponse,
-          lastError: null,
-          valid: true,
-          inFlight: false,
-        })
-        .hasFinalState<JobState>({
-          data: jobResponse,
-          lastError: null,
-          valid: false,
-          inFlight: false,
-        })
-        .run(1000);
-    });
-  });
 });