Skip to content

Commit 4a849d2

Browse files
committed
provide a way to request escalating scopes
1 parent 3edbe7f commit 4a849d2

File tree

3 files changed

+122
-13
lines changed

3 files changed

+122
-13
lines changed

pkg/authorization/authorizer/scope/converter.go

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -140,43 +140,56 @@ func (clusterRoleEvaluator) Handles(scope string) bool {
140140
}
141141

142142
func (e clusterRoleEvaluator) Validate(scope string) error {
143-
_, _, err := e.getRoleNamespace(scope)
143+
_, _, _, err := e.parseScope(scope)
144144
return err
145145
}
146146

147-
func (e clusterRoleEvaluator) getRoleNamespace(scope string) (string, string, error) {
147+
// parseScope parses the requested scope, determining the requested role name, namespace, and if
148+
// access to escalating objects is required. It will return an error if it doesn't parse cleanly
149+
func (e clusterRoleEvaluator) parseScope(scope string) (string /*role name*/, string /*namespace*/, bool /*escalating*/, error) {
148150
if !e.Handles(scope) {
149-
return "", "", fmt.Errorf("bad format for scope %v", scope)
151+
return "", "", false, fmt.Errorf("bad format for scope %v", scope)
150152
}
153+
escalating := false
154+
if strings.HasSuffix(scope, ":!") {
155+
escalating = true
156+
// clip that last segment before parsing the rest
157+
scope = scope[:strings.LastIndex(scope, ":")]
158+
}
159+
151160
tokens := strings.SplitN(scope, ":", 2)
152161
if len(tokens) != 2 {
153-
return "", "", fmt.Errorf("bad format for scope %v", scope)
162+
return "", "", false, fmt.Errorf("bad format for scope %v", scope)
154163
}
155164

156165
// namespaces can't have colons, but roles can. pick last.
157166
lastColonIndex := strings.LastIndex(tokens[1], ":")
158167
if lastColonIndex <= 0 || lastColonIndex == (len(tokens[1])-1) {
159-
return "", "", fmt.Errorf("bad format for scope %v", scope)
168+
return "", "", false, fmt.Errorf("bad format for scope %v", scope)
160169
}
161170

162-
return tokens[1][0:lastColonIndex], tokens[1][lastColonIndex+1:], nil
171+
return tokens[1][0:lastColonIndex], tokens[1][lastColonIndex+1:], escalating, nil
163172
}
164173

165174
func (e clusterRoleEvaluator) Describe(scope string) string {
166-
roleName, scopeNamespace, err := e.getRoleNamespace(scope)
175+
roleName, scopeNamespace, escalating, err := e.parseScope(scope)
167176
if err != nil {
168177
return err.Error()
169178
}
179+
escalatingPhrase := "including any escalating resources like secrets"
180+
if !escalating {
181+
escalatingPhrase = "excluding any escalating resources like secrets"
182+
}
170183

171184
if scopeNamespace == authorizationapi.ScopesAllNamespaces {
172-
return roleName + " access in all projects"
185+
return roleName + " access in all projects, " + escalatingPhrase
173186
}
174187

175-
return roleName + " access in the " + scopeNamespace + " project"
188+
return roleName + " access in the " + scopeNamespace + " project, " + escalatingPhrase
176189
}
177190

178191
func (e clusterRoleEvaluator) ResolveRules(scope, namespace string, clusterPolicyGetter rulevalidation.ClusterPolicyGetter) ([]authorizationapi.PolicyRule, error) {
179-
roleName, scopeNamespace, err := e.getRoleNamespace(scope)
192+
roleName, scopeNamespace, escalating, err := e.parseScope(scope)
180193
if err != nil {
181194
return nil, err
182195
}
@@ -198,21 +211,25 @@ func (e clusterRoleEvaluator) ResolveRules(scope, namespace string, clusterPolic
198211

199212
rules := []authorizationapi.PolicyRule{}
200213
for _, rule := range role.Rules {
214+
if escalating {
215+
rules = append(rules, rule)
216+
continue
217+
}
218+
201219
// rules with unbounded access shouldn't be allowed in scopes.
202220
if rule.Verbs.Has(authorizationapi.VerbAll) || rule.Resources.Has(authorizationapi.ResourceAll) || getAPIGroupSet(rule).Has(authorizationapi.APIGroupAll) {
203221
continue
204222
}
205-
206223
// rules that allow escalating resource access should be cleaned.
207224
safeRule := removeEscalatingResources(rule)
208-
209225
rules = append(rules, safeRule)
210226
}
211227

212228
return rules, nil
213229
}
214230

215-
// removeEscalatingResources has coarse logic for now. It is possible to rewrite one rule into many for the finest grain control
231+
// removeEscalatingResources inspects a PolicyRule and removes any references to escalating resources.
232+
// It has coarse logic for now. It is possible to rewrite one rule into many for the finest grain control
216233
// but removing the entire matching resource regardless of verb or secondary group is cheaper, easier, and errs on the side removing
217234
// too much, not too little
218235
func removeEscalatingResources(in authorizationapi.PolicyRule) authorizationapi.PolicyRule {

pkg/authorization/authorizer/scope/converter_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,17 @@ func TestEscalationProtection(t *testing.T) {
251251
expectedRules: []authorizationapi.PolicyRule{authorizationapi.DiscoveryRule, {APIGroups: []string{"", "and-foo"}, Resources: sets.NewString("pods")}},
252252
scopes: []string{ClusterRoleIndicator + "admin:*"},
253253
},
254+
{
255+
name: "allow the escalation",
256+
clusterRoles: []authorizationapi.ClusterRole{
257+
{
258+
ObjectMeta: kapi.ObjectMeta{Name: "admin"},
259+
Rules: []authorizationapi.PolicyRule{{APIGroups: []string{""}, Resources: sets.NewString("pods", "secrets")}},
260+
},
261+
},
262+
expectedRules: []authorizationapi.PolicyRule{authorizationapi.DiscoveryRule, {APIGroups: []string{""}, Resources: sets.NewString("pods", "secrets")}},
263+
scopes: []string{ClusterRoleIndicator + "admin:*:!"},
264+
},
254265
}
255266

256267
for _, tc := range testCases {

test/integration/scopes_test.go

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
kapi "k8s.io/kubernetes/pkg/api"
99
kapierrors "k8s.io/kubernetes/pkg/api/errors"
10+
kclient "k8s.io/kubernetes/pkg/client/unversioned"
1011
"k8s.io/kubernetes/pkg/serviceaccount"
1112

1213
"github.com/openshift/origin/pkg/authorization/authorizer/scope"
@@ -92,3 +93,83 @@ func TestScopedTokens(t *testing.T) {
9293
t.Fatalf("missing error: %v got user %#v", err, impersonatedUser)
9394
}
9495
}
96+
97+
func TestScopeEscalations(t *testing.T) {
98+
testutil.RequireEtcd(t)
99+
_, clusterAdminKubeConfig, err := testserver.StartTestMasterAPI()
100+
if err != nil {
101+
t.Fatalf("unexpected error: %v", err)
102+
}
103+
104+
clusterAdminClient, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
105+
if err != nil {
106+
t.Fatalf("unexpected error: %v", err)
107+
}
108+
109+
clusterAdminClientConfig, err := testutil.GetClusterAdminClientConfig(clusterAdminKubeConfig)
110+
if err != nil {
111+
t.Fatalf("unexpected error: %v", err)
112+
}
113+
114+
projectName := "hammer-project"
115+
userName := "harold"
116+
haroldClient, err := testserver.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, projectName, userName)
117+
if err != nil {
118+
t.Fatalf("unexpected error: %v", err)
119+
}
120+
121+
if _, err := haroldClient.Builds(projectName).List(kapi.ListOptions{}); err != nil {
122+
t.Fatalf("unexpected error: %v", err)
123+
}
124+
125+
haroldUser, err := haroldClient.Users().Get("~")
126+
if err != nil {
127+
t.Fatalf("unexpected error: %v", err)
128+
}
129+
130+
nonEscalatingEditToken := &oauthapi.OAuthAccessToken{
131+
ObjectMeta: kapi.ObjectMeta{Name: "non-escalating-edit-plus-some-padding-here-to-make-the-limit"},
132+
ClientName: "any-client",
133+
ExpiresIn: 200,
134+
Scopes: []string{scope.ClusterRoleIndicator + "edit:*"},
135+
UserName: userName,
136+
UserUID: string(haroldUser.UID),
137+
}
138+
if _, err := clusterAdminClient.OAuthAccessTokens().Create(nonEscalatingEditToken); err != nil {
139+
t.Fatalf("unexpected error: %v", err)
140+
}
141+
142+
nonEscalatingEditConfig := clientcmd.AnonymousClientConfig(clusterAdminClientConfig)
143+
nonEscalatingEditConfig.BearerToken = nonEscalatingEditToken.Name
144+
nonEscalatingEditClient, err := kclient.New(&nonEscalatingEditConfig)
145+
if err != nil {
146+
t.Fatalf("unexpected error: %v", err)
147+
}
148+
149+
if _, err := nonEscalatingEditClient.Secrets(projectName).List(kapi.ListOptions{}); !kapierrors.IsForbidden(err) {
150+
t.Fatalf("unexpected error: %v", err)
151+
}
152+
153+
escalatingEditToken := &oauthapi.OAuthAccessToken{
154+
ObjectMeta: kapi.ObjectMeta{Name: "escalating-edit-plus-some-padding-here-to-make-the-limit"},
155+
ClientName: "any-client",
156+
ExpiresIn: 200,
157+
Scopes: []string{scope.ClusterRoleIndicator + "edit:*:!"},
158+
UserName: userName,
159+
UserUID: string(haroldUser.UID),
160+
}
161+
if _, err := clusterAdminClient.OAuthAccessTokens().Create(escalatingEditToken); err != nil {
162+
t.Fatalf("unexpected error: %v", err)
163+
}
164+
165+
escalatingEditConfig := clientcmd.AnonymousClientConfig(clusterAdminClientConfig)
166+
escalatingEditConfig.BearerToken = escalatingEditToken.Name
167+
escalatingEditClient, err := kclient.New(&escalatingEditConfig)
168+
if err != nil {
169+
t.Fatalf("unexpected error: %v", err)
170+
}
171+
172+
if _, err := escalatingEditClient.Secrets(projectName).List(kapi.ListOptions{}); err != nil {
173+
t.Fatalf("unexpected error: %v", err)
174+
}
175+
}

0 commit comments

Comments
 (0)