coordinator: threshold manifest update support (#785)

Signed-off-by: Daniel Weiße <[email protected]>
edgelesssys · Jan 14, 2025 · c0ea5c1 · c0ea5c1
1 parent 55456de
commit c0ea5c1
Show file tree

Hide file tree

Showing 4 changed files with 188 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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.