From 82861ae8d7a169d872457913d3d19b5f02eb6246 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 2 Aug 2022 09:32:09 -0400 Subject: [PATCH] secure vars: enforce ENT quotas (OSS work) (#13951) Move the secure variables quota enforcement calls into the state store to ensure quota checks are atomic with quota updates (in the same transaction). Switch to a machine-size int instead of a uint64 for quota tracking. The ENT-side quota spec is described as int, and negative values have a meaning as "not permitted at all". Using the same type for tracking will make it easier to the math around checks, and uint64 is infeasibly large anyways. Add secure vars to quota HTTP API and CLI outputs and API docs. --- api/quota.go | 6 ++ command/agent/search_endpoint_test.go | 12 ++-- command/quota_apply.go | 1 + command/quota_init.go | 4 +- command/quota_status.go | 10 +++- nomad/secure_variables_endpoint.go | 15 +++-- nomad/secure_variables_endpoint_oss.go | 10 ---- nomad/state/state_store_secure_variables.go | 58 ++++++++++++------- .../state/state_store_secure_variables_oss.go | 8 +++ .../state_store_secure_variables_test.go | 37 ++++++++---- nomad/structs/secure_variables.go | 11 ++-- website/content/api-docs/quotas.mdx | 6 +- 12 files changed, 115 insertions(+), 63 deletions(-) delete mode 100644 nomad/secure_variables_endpoint_oss.go create mode 100644 nomad/state/state_store_secure_variables_oss.go diff --git a/api/quota.go b/api/quota.go index be4e46c7e95d..ecfb82e8e31e 100644 --- a/api/quota.go +++ b/api/quota.go @@ -126,6 +126,12 @@ type QuotaLimit struct { // useful for once we support GPUs RegionLimit *Resources + // SecureVariablesLimit is the maximum total size of all secure + // variables SecureVariable.EncryptedData. A value of zero is + // treated as unlimited and a negative value is treated as fully + // disallowed. + SecureVariablesLimit *int `mapstructure:"secure_variables_limit" hcl:"secure_variables_limit,optional"` + // Hash is the hash of the object and is used to make replication efficient. Hash []byte } diff --git a/command/agent/search_endpoint_test.go b/command/agent/search_endpoint_test.go index f538552437ef..d90602f5432f 100644 --- a/command/agent/search_endpoint_test.go +++ b/command/agent/search_endpoint_test.go @@ -701,9 +701,9 @@ func TestHTTP_PrefixSearch_SecureVariables_ACL(t *testing.T) { sv2 := sv1.Copy() sv2.Namespace = ns.Name - _ = state.UpsertNamespaces(7000, []*structs.Namespace{ns}) - _ = state.UpsertSecureVariables(structs.MsgTypeTestSetup, 8000, []*structs.SecureVariableEncrypted{sv1}) - _ = state.UpsertSecureVariables(structs.MsgTypeTestSetup, 8001, []*structs.SecureVariableEncrypted{&sv2}) + require.NoError(t, state.UpsertNamespaces(7000, []*structs.Namespace{ns})) + require.NoError(t, state.UpsertSecureVariables(structs.MsgTypeTestSetup, 8000, []*structs.SecureVariableEncrypted{sv1})) + require.NoError(t, state.UpsertSecureVariables(structs.MsgTypeTestSetup, 8001, []*structs.SecureVariableEncrypted{&sv2})) rootToken := s.RootToken @@ -807,9 +807,9 @@ func TestHTTP_FuzzySearch_SecureVariables_ACL(t *testing.T) { sv2 := sv1.Copy() sv2.Namespace = ns.Name - _ = state.UpsertNamespaces(7000, []*structs.Namespace{mock.Namespace()}) - _ = state.UpsertSecureVariables(structs.MsgTypeTestSetup, 8000, []*structs.SecureVariableEncrypted{sv1}) - _ = state.UpsertSecureVariables(structs.MsgTypeTestSetup, 8001, []*structs.SecureVariableEncrypted{&sv2}) + require.NoError(t, state.UpsertNamespaces(7000, []*structs.Namespace{ns})) + require.NoError(t, state.UpsertSecureVariables(structs.MsgTypeTestSetup, 8000, []*structs.SecureVariableEncrypted{sv1})) + require.NoError(t, state.UpsertSecureVariables(structs.MsgTypeTestSetup, 8001, []*structs.SecureVariableEncrypted{&sv2})) rootToken := s.RootToken defNSToken := mock.CreatePolicyAndToken(t, state, 8002, "default", mock.NamespacePolicy("default", "read", nil)) diff --git a/command/quota_apply.go b/command/quota_apply.go index 90bedbdfd3dd..a60a0187680e 100644 --- a/command/quota_apply.go +++ b/command/quota_apply.go @@ -198,6 +198,7 @@ func parseQuotaLimits(result *[]*api.QuotaLimit, list *ast.ObjectList) error { valid := []string{ "region", "region_limit", + "secure_variables_limit", } if err := helper.CheckHCLKeys(o.Val, valid); err != nil { return err diff --git a/command/quota_init.go b/command/quota_init.go index 01b420d26296..62254ce8e9d8 100644 --- a/command/quota_init.go +++ b/command/quota_init.go @@ -121,6 +121,7 @@ limit { memory = 1000 memory_max = 1000 } + secure_variables_limit = 1000 } `) @@ -135,7 +136,8 @@ var defaultJsonQuotaSpec = strings.TrimSpace(` "CPU": 2500, "MemoryMB": 1000, "MemoryMaxMB": 1000 - } + }, + "SecureVariablesLimit": 1000 } ] } diff --git a/command/quota_status.go b/command/quota_status.go index 4e9434e4ce67..b87b839fec8a 100644 --- a/command/quota_status.go +++ b/command/quota_status.go @@ -158,7 +158,7 @@ func formatQuotaLimits(spec *api.QuotaSpec, usages map[string]*api.QuotaUsage) s sort.Sort(api.QuotaLimitSort(spec.Limits)) limits := make([]string, len(spec.Limits)+1) - limits[0] = "Region|CPU Usage|Memory Usage|Memory Max Usage|Network Usage" + limits[0] = "Region|CPU Usage|Memory Usage|Memory Max Usage|Network Usage|Secure Variables Usage" i := 0 for _, specLimit := range spec.Limits { i++ @@ -185,7 +185,9 @@ func formatQuotaLimits(spec *api.QuotaSpec, usages map[string]*api.QuotaUsage) s memory := fmt.Sprintf("- / %s", formatQuotaLimitInt(specLimit.RegionLimit.MemoryMB)) memoryMax := fmt.Sprintf("- / %s", formatQuotaLimitInt(specLimit.RegionLimit.MemoryMaxMB)) net := fmt.Sprintf("- / %s", formatQuotaLimitInt(&specBits)) - limits[i] = fmt.Sprintf("%s|%s|%s|%s|%s", specLimit.Region, cpu, memory, memoryMax, net) + + vars := fmt.Sprintf("- / %s", formatQuotaLimitInt(specLimit.SecureVariablesLimit)) + limits[i] = fmt.Sprintf("%s|%s|%s|%s|%s|%s", specLimit.Region, cpu, memory, memoryMax, net, vars) continue } @@ -204,7 +206,9 @@ func formatQuotaLimits(spec *api.QuotaSpec, usages map[string]*api.QuotaUsage) s if len(used.RegionLimit.Networks) == 1 { net = fmt.Sprintf("%d / %s", *used.RegionLimit.Networks[0].MBits, formatQuotaLimitInt(&specBits)) } - limits[i] = fmt.Sprintf("%s|%s|%s|%s|%s", specLimit.Region, cpu, memory, memoryMax, net) + + vars := fmt.Sprintf("%d / %s", orZero(used.SecureVariablesLimit), formatQuotaLimitInt(specLimit.SecureVariablesLimit)) + limits[i] = fmt.Sprintf("%s|%s|%s|%s|%s|%s", specLimit.Region, cpu, memory, memoryMax, net, vars) } return formatList(limits) diff --git a/nomad/secure_variables_endpoint.go b/nomad/secure_variables_endpoint.go index 9db43bf15a71..093f30a75fb1 100644 --- a/nomad/secure_variables_endpoint.go +++ b/nomad/secure_variables_endpoint.go @@ -79,6 +79,16 @@ func (sv *SecureVariables) Upsert( mErr.Errors = append(mErr.Errors, err) continue } + + ns, err := sv.srv.State().NamespaceByName(nil, v.Namespace) + if err != nil { + return err + } + if ns == nil { + return fmt.Errorf("secure variable %q is in nonexistent namespace %q", + v.Path, v.Namespace) + } + if args.CheckIndex != nil { var conflict *structs.SecureVariableDecrypted if err := sv.validateCASUpdate(*args.CheckIndex, v, &conflict); err != nil { @@ -106,11 +116,6 @@ func (sv *SecureVariables) Upsert( return &mErr } - // TODO: This should be done on each Data in uArgs. - if err := sv.enforceQuota(uArgs); err != nil { - return err - } - // Update via Raft. out, index, err := sv.srv.raftApply(structs.SecureVariableUpsertRequestType, uArgs) if err != nil { diff --git a/nomad/secure_variables_endpoint_oss.go b/nomad/secure_variables_endpoint_oss.go deleted file mode 100644 index 46c88aeda61b..000000000000 --- a/nomad/secure_variables_endpoint_oss.go +++ /dev/null @@ -1,10 +0,0 @@ -//go:build !ent -// +build !ent - -package nomad - -import "github.com/hashicorp/nomad/nomad/structs" - -func (sv *SecureVariables) enforceQuota(uArgs structs.SecureVariablesEncryptedUpsertRequest) error { - return nil -} diff --git a/nomad/state/state_store_secure_variables.go b/nomad/state/state_store_secure_variables.go index 281c6a6f3011..12a874858761 100644 --- a/nomad/state/state_store_secure_variables.go +++ b/nomad/state/state_store_secure_variables.go @@ -2,6 +2,7 @@ package state import ( "fmt" + "math" "time" "github.com/hashicorp/go-memdb" @@ -146,7 +147,7 @@ func (s *StateStore) upsertSecureVariableImpl(index uint64, txn *txn, sv *struct return fmt.Errorf("secure variable quota lookup failed: %v", err) } - var quotaChange int + var quotaChange int64 // Setup the indexes correctly nowNano := time.Now().UnixNano() @@ -160,13 +161,13 @@ func (s *StateStore) upsertSecureVariableImpl(index uint64, txn *txn, sv *struct sv.CreateTime = exist.CreateTime sv.ModifyIndex = index sv.ModifyTime = nowNano - quotaChange = len(sv.Data) - len(exist.Data) + quotaChange = int64(len(sv.Data) - len(exist.Data)) } else { sv.CreateIndex = index sv.CreateTime = nowNano sv.ModifyIndex = index sv.ModifyTime = nowNano - quotaChange = len(sv.Data) + quotaChange = int64(len(sv.Data)) } // Insert the secure variable @@ -174,24 +175,41 @@ func (s *StateStore) upsertSecureVariableImpl(index uint64, txn *txn, sv *struct return fmt.Errorf("secure variable insert failed: %v", err) } - if quotaChange != 0 { - // Track quota usage - var quotaUsed *structs.SecureVariablesQuota - if existingQuota != nil { - quotaUsed = existingQuota.(*structs.SecureVariablesQuota) - quotaUsed = quotaUsed.Copy() - } else { - quotaUsed = &structs.SecureVariablesQuota{ - Namespace: sv.Namespace, - CreateIndex: index, - } + // Track quota usage + var quotaUsed *structs.SecureVariablesQuota + if existingQuota != nil { + quotaUsed = existingQuota.(*structs.SecureVariablesQuota) + quotaUsed = quotaUsed.Copy() + } else { + quotaUsed = &structs.SecureVariablesQuota{ + Namespace: sv.Namespace, + CreateIndex: index, } + } + + if quotaChange > math.MaxInt64-quotaUsed.Size { + // this limit is actually shared across all namespaces in the region's + // quota (if there is one), but we need this check here to prevent + // overflow as well + return fmt.Errorf("secure variables can store a maximum of %d bytes of encrypted data per namespace", math.MaxInt) + } + + if quotaChange > 0 { + quotaUsed.Size += quotaChange + } else if quotaChange < 0 { + quotaUsed.Size -= helper.Min(quotaUsed.Size, -quotaChange) + } + + err = s.enforceSecureVariablesQuota(index, txn, sv.Namespace, quotaChange) + if err != nil { + return err + } + + // we check enforcement above even if there's no change because another + // namespace may have used up quota to make this no longer valid, but we + // only update the table if this namespace has changed + if quotaChange != 0 { quotaUsed.ModifyIndex = index - if quotaChange > 0 { - quotaUsed.Size += uint64(quotaChange) - } else { - quotaUsed.Size -= uint64(helper.MinInt(int(quotaUsed.Size), -quotaChange)) - } if err := txn.Insert(TableSecureVariablesQuotas, quotaUsed); err != nil { return fmt.Errorf("secure variable quota insert failed: %v", err) } @@ -275,7 +293,7 @@ func (s *StateStore) DeleteSecureVariableTxn(index uint64, namespace, path strin quotaUsed := existingQuota.(*structs.SecureVariablesQuota) quotaUsed = quotaUsed.Copy() sv := existing.(*structs.SecureVariableEncrypted) - quotaUsed.Size -= uint64(len(sv.Data)) + quotaUsed.Size -= helper.Min(quotaUsed.Size, int64(len(sv.Data))) quotaUsed.ModifyIndex = index if err := txn.Insert(TableSecureVariablesQuotas, quotaUsed); err != nil { return fmt.Errorf("secure variable quota insert failed: %v", err) diff --git a/nomad/state/state_store_secure_variables_oss.go b/nomad/state/state_store_secure_variables_oss.go new file mode 100644 index 000000000000..1ef8c772e4ad --- /dev/null +++ b/nomad/state/state_store_secure_variables_oss.go @@ -0,0 +1,8 @@ +//go:build !ent +// +build !ent + +package state + +func (s *StateStore) enforceSecureVariablesQuota(_ uint64, _ *txn, _ string, _ int64) error { + return nil +} diff --git a/nomad/state/state_store_secure_variables_test.go b/nomad/state/state_store_secure_variables_test.go index bd4c53882861..a768faa9f8aa 100644 --- a/nomad/state/state_store_secure_variables_test.go +++ b/nomad/state/state_store_secure_variables_test.go @@ -34,9 +34,9 @@ func TestStateStore_UpsertSecureVariables(t *testing.T) { t.Log(printSecureVariables(svs)) insertIndex := uint64(20) - var expectedQuotaSize uint64 + var expectedQuotaSize int for _, v := range svs { - expectedQuotaSize += uint64(len(v.Data)) + expectedQuotaSize += len(v.Data) } // Ensure new secure variables are inserted as expected with their @@ -78,7 +78,7 @@ func TestStateStore_UpsertSecureVariables(t *testing.T) { quotaUsed, err := testState.SecureVariablesQuotaByNamespace(ws, structs.DefaultNamespace) require.NoError(t, err) - require.Equal(t, expectedQuotaSize, quotaUsed.Size) + require.Equal(t, int64(expectedQuotaSize), quotaUsed.Size) }) svs = svm.List() @@ -102,7 +102,7 @@ func TestStateStore_UpsertSecureVariables(t *testing.T) { quotaUsed, err := testState.SecureVariablesQuotaByNamespace(ws, structs.DefaultNamespace) require.NoError(t, err) - require.Equal(t, expectedQuotaSize, quotaUsed.Size) + require.Equal(t, int64(expectedQuotaSize), quotaUsed.Size) }) // Modify a single one of the previously inserted secure variables @@ -158,7 +158,7 @@ func TestStateStore_UpsertSecureVariables(t *testing.T) { quotaUsed, err := testState.SecureVariablesQuotaByNamespace(ws, structs.DefaultNamespace) require.NoError(t, err) - require.Equal(t, expectedQuotaSize+1, quotaUsed.Size) + require.Equal(t, int64(expectedQuotaSize+1), quotaUsed.Size) }) svs = svm.List() @@ -219,7 +219,7 @@ func TestStateStore_UpsertSecureVariables(t *testing.T) { quotaUsed, err := testState.SecureVariablesQuotaByNamespace(ws, structs.DefaultNamespace) require.NoError(t, err) - require.Equal(t, expectedQuotaSize+1, quotaUsed.Size) + require.Equal(t, int64(expectedQuotaSize+1), quotaUsed.Size) }) } @@ -250,6 +250,11 @@ func TestStateStore_DeleteSecureVariable(t *testing.T) { // remaining is left as expected. t.Run("2 upsert variable and delete", func(t *testing.T) { + ns := mock.Namespace() + ns.Name = svs[0].Namespace + require.NoError(t, testState.UpsertNamespaces(initialIndex, []*structs.Namespace{ns})) + + initialIndex++ require.NoError(t, testState.UpsertSecureVariables( structs.MsgTypeTestSetup, initialIndex, svs)) @@ -270,19 +275,19 @@ func TestStateStore_DeleteSecureVariable(t *testing.T) { require.NoError(t, err) var delete1Count int - var expectedQuotaSize uint64 + var expectedQuotaSize int // Iterate all the stored variables and assert we have the expected // number. for raw := iter.Next(); raw != nil; raw = iter.Next() { delete1Count++ v := raw.(*structs.SecureVariableEncrypted) - expectedQuotaSize += uint64(len(v.Data)) + expectedQuotaSize += len(v.Data) } require.Equal(t, 1, delete1Count, "unexpected number of variables in table") quotaUsed, err := testState.SecureVariablesQuotaByNamespace(ws, structs.DefaultNamespace) require.NoError(t, err) - require.Equal(t, expectedQuotaSize, quotaUsed.Size) + require.Equal(t, int64(expectedQuotaSize), quotaUsed.Size) }) t.Run("3 delete remaining variable", func(t *testing.T) { @@ -310,7 +315,7 @@ func TestStateStore_DeleteSecureVariable(t *testing.T) { quotaUsed, err := testState.SecureVariablesQuotaByNamespace(ws, structs.DefaultNamespace) require.NoError(t, err) - require.Equal(t, uint64(0), quotaUsed.Size) + require.Equal(t, int64(0), quotaUsed.Size) }) } @@ -318,10 +323,15 @@ func TestStateStore_GetSecureVariables(t *testing.T) { ci.Parallel(t) testState := testStateStore(t) + ns := mock.Namespace() + ns.Name = "~*magical*~" + initialIndex := uint64(10) + require.NoError(t, testState.UpsertNamespaces(initialIndex, []*structs.Namespace{ns})) + // Generate some test secure variables and upsert them. svs, _ := mockSecureVariables(2) svs[0].Namespace = "~*magical*~" - initialIndex := uint64(10) + initialIndex++ require.NoError(t, testState.UpsertSecureVariables(structs.MsgTypeTestSetup, initialIndex, svs)) // Look up secure variables using the namespace of the first mock variable. @@ -386,7 +396,12 @@ func TestStateStore_ListSecureVariablesByNamespaceAndPrefix(t *testing.T) { svs[5].Namespace = "other" svs[5].Path = "a/z/z" + ns := mock.Namespace() + ns.Name = "other" initialIndex := uint64(10) + require.NoError(t, testState.UpsertNamespaces(initialIndex, []*structs.Namespace{ns})) + + initialIndex++ require.NoError(t, testState.UpsertSecureVariables(structs.MsgTypeTestSetup, initialIndex, svs)) t.Run("ByNamespace", func(t *testing.T) { diff --git a/nomad/structs/secure_variables.go b/nomad/structs/secure_variables.go index d5af53ce66ab..60ab12408018 100644 --- a/nomad/structs/secure_variables.go +++ b/nomad/structs/secure_variables.go @@ -214,13 +214,14 @@ func (sv SecureVariableMetadata) GetCreateIndex() uint64 { return sv.CreateIndex } -// SecureVariablesQuota is used to track the total size of secure -// variables entries per namespace. The total length of -// SecureVariable.EncryptedData will be added to the SecureVariablesQuota -// table in the same transaction as a write, update, or delete. +// SecureVariablesQuota is used to track the total size of secure variables +// entries per namespace. The total length of SecureVariable.EncryptedData in +// bytes will be added to the SecureVariablesQuota table in the same transaction +// as a write, update, or delete. This tracking effectively caps the maximum +// size of secure variables in a given namespace to MaxInt64 bytes. type SecureVariablesQuota struct { Namespace string - Size uint64 + Size int64 CreateIndex uint64 ModifyIndex uint64 } diff --git a/website/content/api-docs/quotas.mdx b/website/content/api-docs/quotas.mdx index edeed1de87db..c452ad264bfc 100644 --- a/website/content/api-docs/quotas.mdx +++ b/website/content/api-docs/quotas.mdx @@ -73,7 +73,8 @@ $ curl \ "ReservedPorts": null } ] - } + }, + "SecureVariablesLimit": 1000 } ], "ModifyIndex": 56, @@ -136,7 +137,8 @@ $ curl \ "ReservedPorts": null } ] - } + }, + "SecureVariablesLimit": 1000 } ], "ModifyIndex": 56,