From c0ea5c101193ca212165ac14cff5a480f7414c85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Wei=C3=9Fe?= <66256922+daniel-weisse@users.noreply.github.com> Date: Tue, 14 Jan 2025 08:46:36 +0100 Subject: [PATCH] coordinator: threshold manifest update support (#785) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Weiße --- coordinator/manifest/manifest.go | 17 +++ coordinator/manifest/manifest_test.go | 176 +++++++++++++++++++++++-- docs/docs/workflows/define-manifest.md | 4 + docs/docs/workflows/update-manifest.md | 5 +- 4 files changed, 188 insertions(+), 14 deletions(-) diff --git a/coordinator/manifest/manifest.go b/coordinator/manifest/manifest.go index ee49e712..d5a438f9 100644 --- a/coordinator/manifest/manifest.go +++ b/coordinator/manifest/manifest.go @@ -75,6 +75,9 @@ type Config struct { SealMode string // FeatureGates is a list of additional features to enable on the Coordinator. FeatureGates []string + // UpdateThreshold is the amount of acknowledgements required to perform a multi party manifest update. + // If set to 0, all users with the update permission are required to acknowledge an update before it is applied. + UpdateThreshold uint } // Marble describes a service in the mesh that should be handled and verified by the Coordinator. @@ -496,6 +499,20 @@ func (m Manifest) Check(zaplogger *zap.Logger) error { } } + var manifestUpdaters uint + for _, mrUser := range m.Users { + for _, roleName := range mrUser.Roles { + if m.Roles[roleName].ResourceType == "Manifest" && len(m.Roles[roleName].Actions) > 0 && + strings.ToLower(m.Roles[roleName].Actions[0]) == user.PermissionUpdateManifest { + manifestUpdaters++ + break // Avoid counting the same user multiple times if they are assigned more than one role with update permission + } + } + } + if manifestUpdaters < m.Config.UpdateThreshold { + return fmt.Errorf("not enough users with manifest update permissions (%d) to meet the threshold of %d", manifestUpdaters, m.Config.UpdateThreshold) + } + switch m.Config.SealMode { case "", "ProductKey", "UniqueKey", "Disabled": default: diff --git a/coordinator/manifest/manifest_test.go b/coordinator/manifest/manifest_test.go index abec189f..dea1223b 100644 --- a/coordinator/manifest/manifest_test.go +++ b/coordinator/manifest/manifest_test.go @@ -11,6 +11,7 @@ import ( "crypto/x509" "encoding/json" "encoding/pem" + "strings" "testing" "github.com/edgelesssys/marblerun/coordinator/quote" @@ -369,22 +370,173 @@ func TestTemplateDryRun(t *testing.T) { } func TestManifestCheck(t *testing.T) { - assert := assert.New(t) - require := require.New(t) + testCases := map[string]struct { + manifest func(*require.Assertions) Manifest + wantErr bool + }{ + "valid default manifest": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.ManifestJSON), &manifest)) + return manifest + }, + }, + "valid manifest with recovery key": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest)) + return manifest + }, + }, + "valid integration test manifest": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.IntegrationManifestJSON), &manifest)) + return manifest + }, + }, + "update threshold 0 with one update-manifest user": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest)) - var manifest Manifest - err := json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest) - require.NoError(err) + manifest.Roles["updateManifest"] = Role{ + ResourceType: "Manifest", + ResourceNames: []string{}, + Actions: []string{user.PermissionUpdateManifest}, + } + adminUser := manifest.Users["admin"] + adminUser.Roles = append(adminUser.Roles, "updateManifest") + manifest.Users["admin"] = adminUser - log := zaptest.NewLogger(t) - err = manifest.Check(log) - assert.NoError(err) + manifest.Config.UpdateThreshold = 0 + return manifest + }, + }, + "update threshold equal to allowed users": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest)) - manifest.Users["anotherUser"] = User{ - Certificate: manifest.Users["admin"].Certificate, + manifest.Roles["updateManifest"] = Role{ + ResourceType: "Manifest", + ResourceNames: []string{}, + Actions: []string{user.PermissionUpdateManifest}, + } + adminUser := manifest.Users["admin"] + adminUser.Roles = append(adminUser.Roles, "updateManifest") + manifest.Users["admin"] = adminUser + + manifest.Config.UpdateThreshold = 1 + return manifest + }, + }, + "update threshold lower than allowed users": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest)) + + manifest.Roles["updateManifest"] = Role{ + ResourceType: "Manifest", + ResourceNames: []string{}, + Actions: []string{user.PermissionUpdateManifest}, + } + adminUser := manifest.Users["admin"] + adminUser.Roles = append(adminUser.Roles, "updateManifest") + manifest.Users["admin"] = adminUser + + // Set up a second user from a copy of the first + // The certificate is not a valid x509 cert, but that is not checked for in this test + adminUser2 := adminUser + adminUser2.Certificate = strings.ReplaceAll(adminUser.Certificate, "q", "b") + manifest.Users["admin2"] = adminUser2 + + manifest.Config.UpdateThreshold = 1 + return manifest + }, + }, + "update threshold higher than allowed users": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest)) + + manifest.Roles["updateManifest"] = Role{ + ResourceType: "Manifest", + ResourceNames: []string{}, + Actions: []string{user.PermissionUpdateManifest}, + } + adminUser := manifest.Users["admin"] + adminUser.Roles = append(adminUser.Roles, "updateManifest") + manifest.Users["admin"] = adminUser + + // Set up a second user from a copy of the first + // The certificate is not a valid x509 cert, but that is not checked for in this test + adminUser2 := adminUser + adminUser2.Certificate = strings.ReplaceAll(adminUser.Certificate, "q", "b") + manifest.Users["admin2"] = adminUser2 + + manifest.Config.UpdateThreshold = 3 + return manifest + }, + wantErr: true, + }, + "user with multiple update manifest roles is only counted once for the threshold": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest)) + + manifest.Roles["updateManifest"] = Role{ + ResourceType: "Manifest", + ResourceNames: []string{}, + Actions: []string{user.PermissionUpdateManifest}, + } + manifest.Roles["updateManifest2"] = Role{ + ResourceType: "Manifest", + ResourceNames: []string{}, + Actions: []string{user.PermissionUpdateManifest}, + } + manifest.Roles["updateManifest3"] = Role{ + ResourceType: "Manifest", + ResourceNames: []string{}, + Actions: []string{user.PermissionUpdateManifest}, + } + + adminUser := manifest.Users["admin"] + adminUser.Roles = append(adminUser.Roles, "updateManifest", "updateManifest2", "updateManifest3") + manifest.Users["admin"] = adminUser + + manifest.Config.UpdateThreshold = 3 + return manifest + }, + wantErr: true, + }, + "users without unique certificates": { + manifest: func(require *require.Assertions) Manifest { + var manifest Manifest + require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest)) + + manifest.Users["anotherUser"] = User{ + Certificate: manifest.Users["admin"].Certificate, + } + return manifest + }, + wantErr: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + err := tc.manifest(require).Check(zaptest.NewLogger(t)) + if tc.wantErr { + assert.Error(err) + } else { + assert.NoError(err) + } + }) } - err = manifest.Check(log) - assert.Error(err) } func TestCertificate(t *testing.T) { diff --git a/docs/docs/workflows/define-manifest.md b/docs/docs/workflows/define-manifest.md index 0d76970d..e11400db 100644 --- a/docs/docs/workflows/define-manifest.md +++ b/docs/docs/workflows/define-manifest.md @@ -522,6 +522,7 @@ The optional entry `Config` holds configuration settings for the Coordinator. "Config": { "SealMode": "ProductKey", + "UpdateThreshold": 5, "FeatureGates": [] } //... @@ -536,6 +537,9 @@ The optional entry `Config` holds configuration settings for the Coordinator. See the section on [seal key types](../architecture/security.md#seal-key) for more information. +`UpdateThreshold` specifies the number of acknowledgments required for a [multi-party manifest update](./update-manifest.md#full-update). +If not set, all users with roles that have the `UpdateManifest` action need to acknowledge the update. + `FeatureGates` allows you to opt-in to additional features that may be useful for certain use cases. The following features are available: * `SignQuoteEndpoint`: enables the [sign-quote endpoint](../reference/coordinator.md#verify-and-sign-an-sgx-quote) diff --git a/docs/docs/workflows/update-manifest.md b/docs/docs/workflows/update-manifest.md index 3a66dad2..0a06359d 100644 --- a/docs/docs/workflows/update-manifest.md +++ b/docs/docs/workflows/update-manifest.md @@ -54,7 +54,8 @@ Don't define other values except the `SecurityVersion` value for a package, as M Some deployment scenarios require more flexibility regarding changes to the manifest. To this end, MarbleRun also allows uploading a full manifest. User-defined secrets and secrets of type `symmetric-key` are retained if their definition doesn't change. To deploy a new manifest, your user must have a [role assigned that contains the `UpdateManifest` action](define-manifest.md#roles). -If multiple users have such a role assigned, each of them must [acknowledge](#acknowledging-a-multi-party-update) the new manifest. +If more than one user have such a role assigned, the manifest field `.Config.UpdateThreshold` specifies how many of them must [acknowledge](#acknowledging-a-multi-party-update) the new manifest. +If not set, all must acknowledge the new manifest. ## Deploying an update @@ -68,7 +69,7 @@ On success, no message will be returned and your MarbleRun logs should highlight ## Acknowledging a multi-party update -All users that have the `UpdateManifest` permission are required to acknowledge a full manifest update. +Users that have the `UpdateManifest` permission are required to acknowledge a full manifest update. To this end, they need to upload the same manifest. This proves that they have knowledge of and agree on this manifest.