Skip to content

Commit 7563b46

Browse files
majewskymajewsky
committed
add keppel.RBACPolicy.ForbiddenPermissions
1 parent 1225895 commit 7563b46

5 files changed

Lines changed: 138 additions & 38 deletions

File tree

‎docs/api-spec.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ The following fields may be returned:
144144
| `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. |
145145
| `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. |
146146
| `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. |
147+
| `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. |
147148
| `accounts[].replication` | object or omitted | Replication configuration for this account, if any. [See below](#replication-strategies) for details. |
148149
| `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). |
149150
| `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‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,11 @@ var (
7777
UserNamePattern: "correct.*",
7878
Permissions: []keppel.RBACPermission{keppel.GrantsPull},
7979
}
80+
policyForbidPush = keppel.RBACPolicy{
81+
RepositoryPattern: "fo+",
82+
UserNamePattern: "correct.*",
83+
ForbiddenPermissions: []keppel.RBACPermission{keppel.GrantsPush},
84+
}
8085
policyPushMatches = keppel.RBACPolicy{
8186
RepositoryPattern: "fo+",
8287
UserNamePattern: "correct.*",
@@ -414,6 +419,19 @@ var testCases = []TestCase{
414419
{Scope: "repository:test1/foo:delete",
415420
RBACPolicy: &policyDeleteMatches,
416421
GrantedActions: "delete"},
422+
// negative RBAC policies can take away permissions
423+
{Scope: "repository:test1/foo:pull",
424+
RBACPolicy: &policyForbidPush,
425+
GrantedActions: "pull"},
426+
{Scope: "repository:test1/foo:push",
427+
RBACPolicy: &policyForbidPush,
428+
GrantedActions: ""},
429+
{Scope: "repository:test1/foo:pull,push",
430+
RBACPolicy: &policyForbidPush,
431+
GrantedActions: "pull"},
432+
{Scope: "repository:test1/foo:delete",
433+
RBACPolicy: &policyForbidPush,
434+
GrantedActions: "delete"},
417435
}
418436

419437
// TODO expect refresh_token when offline_token=true is given

‎internal/api/keppel/accounts_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -802,6 +802,8 @@ func TestPutAccountErrorCases(t *testing.T) {
802802
RBACPolicyJSON assert.JSONObject
803803
ErrorMessage string
804804
}{
805+
// NOTE: Many testcases come in pairs where the problematic permission is
806+
// in `permissions` the first time and in `forbidden_permissions` the second time.
805807
{
806808
RBACPolicyJSON: assert.JSONObject{
807809
"match_repository": "library/.+",
@@ -815,6 +817,22 @@ func TestPutAccountErrorCases(t *testing.T) {
815817
},
816818
ErrorMessage: `"foo" is not a valid RBAC policy permission`,
817819
},
820+
{
821+
RBACPolicyJSON: assert.JSONObject{
822+
"match_repository": "library/.+",
823+
"permissions": []string{"pull"},
824+
"forbidden_permissions": []string{"push", "foo"},
825+
},
826+
ErrorMessage: `"foo" is not a valid RBAC policy permission`,
827+
},
828+
{
829+
RBACPolicyJSON: assert.JSONObject{
830+
"match_repository": "library/.+",
831+
"permissions": []string{"pull"},
832+
"forbidden_permissions": []string{"pull", "push"},
833+
},
834+
ErrorMessage: `"pull" cannot be granted and forbidden by the same RBAC policy`,
835+
},
818836
{
819837
RBACPolicyJSON: assert.JSONObject{
820838
"permissions": []string{"anonymous_pull"},
@@ -829,20 +847,42 @@ func TestPutAccountErrorCases(t *testing.T) {
829847
},
830848
ErrorMessage: `RBAC policy with "anonymous_pull" or "anonymous_first_pull" may not have the "match_username" attribute`,
831849
},
850+
{
851+
RBACPolicyJSON: assert.JSONObject{
852+
"match_repository": "library/.+",
853+
"match_username": "foo",
854+
"forbidden_permissions": []string{"anonymous_pull"},
855+
},
856+
ErrorMessage: `RBAC policy with "anonymous_pull" or "anonymous_first_pull" may not have the "match_username" attribute`,
857+
},
832858
{
833859
RBACPolicyJSON: assert.JSONObject{
834860
"match_repository": "library/.+",
835861
"permissions": []string{"pull"},
836862
},
837863
ErrorMessage: `RBAC policy with "pull" must have the "match_cidr" or "match_username" attribute`,
838864
},
865+
{
866+
RBACPolicyJSON: assert.JSONObject{
867+
"match_repository": "library/.+",
868+
"forbidden_permissions": []string{"pull"},
869+
},
870+
ErrorMessage: `RBAC policy with "pull" must have the "match_cidr" or "match_username" attribute`,
871+
},
839872
{
840873
RBACPolicyJSON: assert.JSONObject{
841874
"match_repository": "library/.+",
842875
"permissions": []string{"delete"},
843876
},
844877
ErrorMessage: `RBAC policy with "delete" must have the "match_username" attribute`,
845878
},
879+
{
880+
RBACPolicyJSON: assert.JSONObject{
881+
"match_repository": "library/.+",
882+
"forbidden_permissions": []string{"delete"},
883+
},
884+
ErrorMessage: `RBAC policy with "delete" must have the "match_username" attribute`,
885+
},
846886
{
847887
RBACPolicyJSON: assert.JSONObject{
848888
"match_repository": "library/.+",
@@ -872,13 +912,28 @@ func TestPutAccountErrorCases(t *testing.T) {
872912
},
873913
ErrorMessage: `RBAC policy with "anonymous_pull" or "anonymous_first_pull" may not have the "match_username" attribute`,
874914
},
915+
{
916+
RBACPolicyJSON: assert.JSONObject{
917+
"match_repository": "library/.+",
918+
"match_username": "foo",
919+
"forbidden_permissions": []string{"anonymous_first_pull"},
920+
},
921+
ErrorMessage: `RBAC policy with "anonymous_pull" or "anonymous_first_pull" may not have the "match_username" attribute`,
922+
},
875923
{
876924
RBACPolicyJSON: assert.JSONObject{
877925
"match_repository": "library/.+",
878926
"permissions": []string{"anonymous_first_pull"},
879927
},
880928
ErrorMessage: `RBAC policy with "anonymous_first_pull" may only be for external replica accounts`,
881929
},
930+
{
931+
RBACPolicyJSON: assert.JSONObject{
932+
"match_repository": "library/.+",
933+
"forbidden_permissions": []string{"anonymous_first_pull"},
934+
},
935+
ErrorMessage: `RBAC policy with "anonymous_first_pull" may only be for external replica accounts`,
936+
},
882937
{
883938
RBACPolicyJSON: assert.JSONObject{
884939
"match_repository": "*/library",

‎internal/auth/filter.go‎

Lines changed: 36 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"errors"
2525
"fmt"
2626

27+
. "github.com/majewsky/gg/option"
2728
"github.com/sapcc/go-bits/httpext"
2829

2930
"github.com/sapcc/keppel/internal/keppel"
@@ -188,46 +189,55 @@ func filterRepoActions(ip string, scope Scope, uid keppel.UserIdentity, audience
188189
return nil, err
189190
}
190191

191-
isAllowedAction := map[string]bool{
192-
"pull": uid.HasPermission(keppel.CanPullFromAccount, authTenantID),
193-
"push": uid.HasPermission(keppel.CanPushToAccount, authTenantID),
194-
"delete": uid.HasPermission(keppel.CanDeleteFromAccount, authTenantID),
195-
}
196-
192+
// collect permission overrides from matching RBAC policies
197193
policies, err := keppel.ParseRBACPoliciesField(rbacPoliciesJSON)
198194
if err != nil {
199195
return nil, fmt.Errorf("while parsing account RBAC policies: %w", err)
200196
}
197+
permOverride := make(map[keppel.RBACPermission]Option[bool])
201198
userName := uid.UserName()
202199
for _, policy := range policies {
203200
if !policy.Matches(ip, repoScope.RepositoryName, userName) {
204201
continue
205202
}
206-
207-
hasPerm := make(map[keppel.RBACPermission]bool)
203+
// NOTE: forbidding overrides take precedence over granting overrides
208204
for _, perm := range policy.Permissions {
209-
hasPerm[perm] = true
210-
}
211-
212-
if hasPerm[keppel.GrantsAnonymousPull] {
213-
isAllowedAction["pull"] = true
214-
}
215-
if hasPerm[keppel.GrantsAnonymousFirstPull] {
216-
isAllowedAction["anonymous_first_pull"] = true
217-
}
218-
if uid.UserType() != keppel.AnonymousUser {
219-
if hasPerm[keppel.GrantsPull] {
220-
isAllowedAction["pull"] = true
221-
}
222-
if hasPerm[keppel.GrantsPush] {
223-
isAllowedAction["push"] = true
224-
}
225-
if hasPerm[keppel.GrantsDelete] {
226-
isAllowedAction["delete"] = true
205+
if permOverride[perm] != Some(false) {
206+
permOverride[perm] = Some(true)
227207
}
228208
}
209+
for _, perm := range policy.ForbiddenPermissions {
210+
permOverride[perm] = Some(false)
211+
}
212+
}
213+
214+
// certain policies can never be granted to anonymous users by an RBAC policy
215+
if uid.UserType() == keppel.AnonymousUser {
216+
delete(permOverride, keppel.GrantsPull)
217+
delete(permOverride, keppel.GrantsPush)
218+
delete(permOverride, keppel.GrantsDelete)
219+
}
220+
221+
// evaluate final permission set
222+
isAllowedAction := map[string]bool{
223+
"pull": permOverride[keppel.GrantsPull].UnwrapOr(
224+
uid.HasPermission(keppel.CanPullFromAccount, authTenantID),
225+
),
226+
"push": permOverride[keppel.GrantsPush].UnwrapOr(
227+
uid.HasPermission(keppel.CanPushToAccount, authTenantID),
228+
),
229+
"delete": permOverride[keppel.GrantsDelete].UnwrapOr(
230+
uid.HasPermission(keppel.CanDeleteFromAccount, authTenantID),
231+
),
232+
}
233+
if permOverride[keppel.GrantsAnonymousPull].UnwrapOr(false) {
234+
isAllowedAction["pull"] = true
235+
}
236+
if isAllowedAction["pull"] {
237+
isAllowedAction["anonymous_first_pull"] = permOverride[keppel.GrantsAnonymousFirstPull].UnwrapOr(false)
229238
}
230239

240+
// grant requested actions as possible
231241
var result []string
232242
for _, action := range scope.Actions {
233243
if isAllowedAction[action] {

‎internal/keppel/rbac_policy.go‎

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,11 @@ import (
3333
// RBACPolicy is a policy granting user-defined access to repos in an account.
3434
// It is stored in serialized form in the RBACPoliciesJSON field of type Account.
3535
type RBACPolicy struct {
36-
CidrPattern string `json:"match_cidr,omitempty"`
37-
RepositoryPattern regexpext.BoundedRegexp `json:"match_repository,omitempty"`
38-
UserNamePattern regexpext.BoundedRegexp `json:"match_username,omitempty"`
39-
Permissions []RBACPermission `json:"permissions"`
36+
CidrPattern string `json:"match_cidr,omitempty"`
37+
RepositoryPattern regexpext.BoundedRegexp `json:"match_repository,omitempty"`
38+
UserNamePattern regexpext.BoundedRegexp `json:"match_username,omitempty"`
39+
Permissions []RBACPermission `json:"permissions"`
40+
ForbiddenPermissions []RBACPermission `json:"forbidden_permissions,omitempty"`
4041
}
4142

4243
// RBACPermission enumerates permissions that can be granted by an RBAC policy.
@@ -93,33 +94,48 @@ func (r *RBACPolicy) ValidateAndNormalize(strategy ReplicationStrategy) error {
9394
}
9495
}
9596

96-
hasPerm := make(map[RBACPermission]bool)
97+
grantsPerm := make(map[RBACPermission]bool)
98+
forbidsPerm := make(map[RBACPermission]bool)
99+
refersToPerm := make(map[RBACPermission]bool)
97100
for _, perm := range r.Permissions {
98101
if !isRBACPermission[perm] {
99102
return fmt.Errorf("%q is not a valid RBAC policy permission", perm)
100103
}
101-
hasPerm[perm] = true
104+
grantsPerm[perm] = true
105+
forbidsPerm[perm] = false
106+
refersToPerm[perm] = true
107+
}
108+
for _, perm := range r.ForbiddenPermissions {
109+
if !isRBACPermission[perm] {
110+
return fmt.Errorf("%q is not a valid RBAC policy permission", perm)
111+
}
112+
if grantsPerm[perm] {
113+
return fmt.Errorf("%q cannot be granted and forbidden by the same RBAC policy", perm)
114+
}
115+
grantsPerm[perm] = false
116+
forbidsPerm[perm] = true
117+
refersToPerm[perm] = true
102118
}
103119

104-
if len(r.Permissions) == 0 {
120+
if len(r.Permissions) == 0 && len(r.ForbiddenPermissions) == 0 {
105121
return errors.New(`RBAC policy must grant at least one permission`)
106122
}
107123
if r.CidrPattern == "" && r.UserNamePattern == "" && r.RepositoryPattern == "" {
108124
return errors.New(`RBAC policy must have at least one "match_..." attribute`)
109125
}
110-
if (hasPerm[GrantsAnonymousPull] || hasPerm[GrantsAnonymousFirstPull]) && r.UserNamePattern != "" {
126+
if (refersToPerm[GrantsAnonymousPull] || refersToPerm[GrantsAnonymousFirstPull]) && r.UserNamePattern != "" {
111127
return errors.New(`RBAC policy with "anonymous_pull" or "anonymous_first_pull" may not have the "match_username" attribute`)
112128
}
113-
if hasPerm[GrantsPull] && r.CidrPattern == "" && r.UserNamePattern == "" {
129+
if refersToPerm[GrantsPull] && r.CidrPattern == "" && r.UserNamePattern == "" {
114130
return errors.New(`RBAC policy with "pull" must have the "match_cidr" or "match_username" attribute`)
115131
}
116-
if hasPerm[GrantsPush] && !hasPerm[GrantsPull] {
132+
if grantsPerm[GrantsPush] && !grantsPerm[GrantsPull] {
117133
return errors.New(`RBAC policy with "push" must also grant "pull"`)
118134
}
119-
if hasPerm[GrantsDelete] && r.UserNamePattern == "" {
135+
if refersToPerm[GrantsDelete] && r.UserNamePattern == "" {
120136
return errors.New(`RBAC policy with "delete" must have the "match_username" attribute`)
121137
}
122-
if hasPerm[GrantsAnonymousFirstPull] && strategy != FromExternalOnFirstUseStrategy {
138+
if refersToPerm[GrantsAnonymousFirstPull] && strategy != FromExternalOnFirstUseStrategy {
123139
return errors.New(`RBAC policy with "anonymous_first_pull" may only be for external replica accounts`)
124140
}
125141

0 commit comments

Comments
 (0)