Merge pull request #514 from sapcc/negative-rbac

add keppel.RBACPolicy.ForbiddenPermissions

Author: majewsky

cmd/api/gui_redirect.go

@@ -77,7 +77,7 @@ func (g *guiRedirecter) tryRedirectToGUI(w http.ResponseWriter, r *http.Request)
 		return
 	}
 	for _, policy := range policies {
-		if !slices.Contains(policy.Permissions, keppel.GrantsAnonymousPull) {
+		if !slices.Contains(policy.Permissions, keppel.RBACAnonymousPullPermission) {
 			continue
 		}
 		ip := httpext.GetRequesterIPFor(r)

docs/api-spec.md

@@ -144,6 +144,7 @@ The following fields may be returned:
 | `accounts[].rbac_policies[].match_repository` | string | The RBAC policy applies to all repositories in this account whose name matches this regex. The leading account name and slash is stripped from the repository name before matching. The notes on regexes below apply. |
 | `accounts[].rbac_policies[].match_username` | string | The RBAC policy applies to all users whose name matches this regex. Refer to the [documentation of your auth driver](./drivers/) for the syntax of usernames. The notes on regexes below apply. |
 | `accounts[].rbac_policies[].permissions` | list of strings | The permissions granted by the RBAC policy. Acceptable values include `pull`, `push`, `delete`, `anonymous_pull` and `anonymous_first_pull`. When `pull`, `push` or `delete` are included, `match_username` is not empty. When `anonymous_pull` or `anonymous_first_pull` is included, `match_username` is empty. `anonymous_first_pull` is only relevant for external replica accounts and allows unauthenticated users to replicate tags. It should always be combined with an appropriate `match_*` rule. |
+| `accounts[].rbac_policies[].forbidden_permissions` | list of strings | The permissions forbidden by the RBAC policy. Acceptable values are the same as for the `permissions` field. This field takes precedence over `permissions`: Any permission listed here will never be given to matching users, even if another matching policy would grant it. |
 | `accounts[].replication` | object or omitted | Replication configuration for this account, if any. [See below](#replication-strategies) for details. |
 | `accounts[].platform_filter` | list of objects or omitted | Only allowed for replica accounts. If not empty, when replicating an image list manifest (i.e. a multi-architecture image), only submanifests matching one of the given platforms will be replicated. Each entry must have the same format as the `manifests[].platform` field in the [OCI Image Index Specification](https://github.com/opencontainers/image-spec/blob/master/image-index.md). |
 | `accounts[].validation` | object or omitted | Validation rules for this account. When included, pushing blobs and manifests not satisfying these validation rules may be rejected. |

internal/api/auth/api_test.go

@@ -66,41 +66,46 @@ type TestCase struct {
 var (
 	policyAnonPull = keppel.RBACPolicy{
 		RepositoryPattern: "fo+",
-		Permissions:       []keppel.RBACPermission{keppel.GrantsAnonymousPull},
+		Permissions:       []keppel.RBACPermission{keppel.RBACAnonymousPullPermission},
 	}
 	policyAnonFirstPull = keppel.RBACPolicy{
 		RepositoryPattern: "fo+",
-		Permissions:       []keppel.RBACPermission{keppel.GrantsAnonymousPull, keppel.GrantsAnonymousFirstPull},
+		Permissions:       []keppel.RBACPermission{keppel.RBACAnonymousPullPermission, keppel.RBACAnonymousFirstPullPermission},
 	}
 	policyPullMatches = keppel.RBACPolicy{
 		RepositoryPattern: "fo+",
 		UserNamePattern:   "correct.*",
-		Permissions:       []keppel.RBACPermission{keppel.GrantsPull},
+		Permissions:       []keppel.RBACPermission{keppel.RBACPullPermission},
+	}
+	policyForbidPush = keppel.RBACPolicy{
+		RepositoryPattern:    "fo+",
+		UserNamePattern:      "correct.*",
+		ForbiddenPermissions: []keppel.RBACPermission{keppel.RBACPushPermission},
 	}
 	policyPushMatches = keppel.RBACPolicy{
 		RepositoryPattern: "fo+",
 		UserNamePattern:   "correct.*",
-		Permissions:       []keppel.RBACPermission{keppel.GrantsPull, keppel.GrantsPush},
+		Permissions:       []keppel.RBACPermission{keppel.RBACPullPermission, keppel.RBACPushPermission},
 	}
 	policyDeleteMatches = keppel.RBACPolicy{
 		RepositoryPattern: "fo+",
 		UserNamePattern:   "correct.*",
-		Permissions:       []keppel.RBACPermission{keppel.GrantsPull, keppel.GrantsDelete},
+		Permissions:       []keppel.RBACPermission{keppel.RBACPullPermission, keppel.RBACDeletePermission},
 	}
 	policyPullDoesNotMatch = keppel.RBACPolicy{
 		RepositoryPattern: "fo+",
 		UserNamePattern:   "doesnotmatch",
-		Permissions:       []keppel.RBACPermission{keppel.GrantsPull},
+		Permissions:       []keppel.RBACPermission{keppel.RBACPullPermission},
 	}
 	policyPushDoesNotMatch = keppel.RBACPolicy{
 		RepositoryPattern: "doesnotmatch",
 		UserNamePattern:   "correct.*",
-		Permissions:       []keppel.RBACPermission{keppel.GrantsPull, keppel.GrantsPush},
+		Permissions:       []keppel.RBACPermission{keppel.RBACPullPermission, keppel.RBACPushPermission},
 	}
 	policyDeleteDoesNotMatch = keppel.RBACPolicy{
 		RepositoryPattern: "fo+",
 		UserNamePattern:   "doesnotmatch",
-		Permissions:       []keppel.RBACPermission{keppel.GrantsPull, keppel.GrantsDelete},
+		Permissions:       []keppel.RBACPermission{keppel.RBACPullPermission, keppel.RBACDeletePermission},
 	}
 )
 
@@ -414,6 +419,19 @@ var testCases = []TestCase{
 	{Scope: "repository:test1/foo:delete",
 		RBACPolicy:     &policyDeleteMatches,
 		GrantedActions: "delete"},
+	// negative RBAC policies can take away permissions
+	{Scope: "repository:test1/foo:pull",
+		RBACPolicy:     &policyForbidPush,
+		GrantedActions: "pull"},
+	{Scope: "repository:test1/foo:push",
+		RBACPolicy:     &policyForbidPush,
+		GrantedActions: ""},
+	{Scope: "repository:test1/foo:pull,push",
+		RBACPolicy:     &policyForbidPush,
+		GrantedActions: "pull"},
+	{Scope: "repository:test1/foo:delete",
+		RBACPolicy:     &policyForbidPush,
+		GrantedActions: "delete"},
 }
 
 // TODO expect refresh_token when offline_token=true is given