-
Notifications
You must be signed in to change notification settings - Fork 4.1k
kv: don't Update HLC clock in response to untrusted tenants #72663
Description
Discovered in #72278.
We currently update the HLC clock in response to BatchRequests from SQL tenant pods here:
cockroach/pkg/kv/kvserver/store_send.go
Lines 76 to 89 in 1072ae4
| // Update our clock with the incoming request timestamp. This advances the | |
| // local node's clock to a high water mark from all nodes with which it has | |
| // interacted. | |
| if baClockTS, ok := ba.Timestamp.TryToClockTimestamp(); ok { | |
| if s.cfg.TestingKnobs.DisableMaxOffsetCheck { | |
| s.cfg.Clock.Update(baClockTS) | |
| } else { | |
| // If the command appears to come from a node with a bad clock, | |
| // reject it instead of updating the local clock and proceeding. | |
| if err := s.cfg.Clock.UpdateAndCheckMaxOffset(ctx, baClockTS); err != nil { | |
| return nil, roachpb.NewError(err) | |
| } | |
| } | |
| } |
This seems like a bad idea, given our multi-tenant threat model where any SQL pod can be entirely owned and we still want to protect the KV host cluster. Currently, a malicious tenant could keep pushing the clock forward by max_offset-1 until it crashed a KV node (through (rpc.RemoteClockMonitor).VerifyClockOffset).
Now that we've convinced ourselves that this HLC channel is not necessary for correctness, we can consider removing the call to (hlc.Clock).Update here for requests originating from secondary tenants.
Jira issue: CRDB-11248