Skip to content

Commit 39c2ca5

Browse files
bastjanbastjan
authored
Validate invitation targetRefs to prevent privilege escalation (#107)
* Invitation validate direct user references * Deduplicate getting objects from targetRef * Update readme and local setup with added hook
1 parent 433f03f commit 39c2ca5

File tree

9 files changed

+741
-68
lines changed

9 files changed

+741
-68
lines changed

‎README.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,16 @@ EOF
163163
kubectl patch validatingwebhookconfiguration validating-webhook-configuration \
164164
-p '{
165165
"webhooks": [
166+
{
167+
"name": "validate-invitations.user.appuio.io",
168+
"clientConfig": {
169+
"caBundle": "'"$(base64 -w0 "./local-env/webhook-certs/tls.crt)"'",
170+
"service": {
171+
"namespace": "default",
172+
"port": 9444
173+
}
174+
}
175+
},
166176
{
167177
"name": "validate-users.appuio.io",
168178
"clientConfig": {

‎config/webhook/manifests.yaml

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,26 @@ metadata:
55
creationTimestamp: null
66
name: validating-webhook-configuration
77
webhooks:
8+
- admissionReviewVersions:
9+
- v1
10+
clientConfig:
11+
service:
12+
name: webhook-service
13+
namespace: system
14+
path: /validate-user-appuio-io-v1-invitation
15+
failurePolicy: Fail
16+
name: validate-invitations.user.appuio.io
17+
rules:
18+
- apiGroups:
19+
- user.appuio.io
20+
apiVersions:
21+
- v1
22+
operations:
23+
- CREATE
24+
- UPDATE
25+
resources:
26+
- invitations
27+
sideEffects: None
828
- admissionReviewVersions:
929
- v1
1030
clientConfig:

‎controllers/invitation_redeem_controller.go

Lines changed: 12 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,8 @@ package controllers
33
import (
44
"context"
55
"errors"
6-
"fmt"
76

87
"go.uber.org/multierr"
9-
rbacv1 "k8s.io/api/rbac/v1"
108
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
119
"k8s.io/apimachinery/pkg/runtime"
1210
"k8s.io/client-go/tools/record"
@@ -15,7 +13,7 @@ import (
1513
"sigs.k8s.io/controller-runtime/pkg/log"
1614

1715
userv1 "github.com/appuio/control-api/apis/user/v1"
18-
controlv1 "github.com/appuio/control-api/apis/v1"
16+
"github.com/appuio/control-api/controllers/targetref"
1917
)
2018

2119
// InvitationRedeemReconciler reconciles invitations and adds a token to the status if required.
@@ -94,72 +92,20 @@ func (r *InvitationRedeemReconciler) SetupWithManager(mgr ctrl.Manager) error {
9492
}
9593

9694
func addUserToTarget(ctx context.Context, c client.Client, user string, target userv1.TargetRef) error {
97-
switch {
98-
case target.APIGroup == "appuio.io" && target.Kind == "OrganizationMembers":
99-
om := controlv1.OrganizationMembers{}
100-
if err := c.Get(ctx, client.ObjectKey{Name: target.Name, Namespace: target.Namespace}, &om); err != nil {
101-
return err
102-
}
103-
refs, added := ensure(om.Spec.UserRefs, controlv1.UserRef{Name: user})
104-
om.Spec.UserRefs = refs
105-
if added {
106-
return c.Update(ctx, &om)
107-
}
108-
return nil
109-
case target.APIGroup == "appuio.io" && target.Kind == "Team":
110-
te := controlv1.Team{}
111-
if err := c.Get(ctx, client.ObjectKey{Name: target.Name, Namespace: target.Namespace}, &te); err != nil {
112-
return err
113-
}
114-
refs, added := ensure(te.Spec.UserRefs, controlv1.UserRef{Name: user})
115-
te.Spec.UserRefs = refs
116-
if added {
117-
return c.Update(ctx, &te)
118-
}
119-
return nil
120-
case target.APIGroup == rbacv1.GroupName && target.Kind == "ClusterRoleBinding":
121-
crb := rbacv1.ClusterRoleBinding{}
122-
if err := c.Get(ctx, client.ObjectKey{Name: target.Name}, &crb); err != nil {
123-
return err
124-
}
125-
subjects, added := ensure(crb.Subjects, newSubject(user))
126-
crb.Subjects = subjects
127-
if added {
128-
return c.Update(ctx, &crb)
129-
}
130-
return nil
131-
case target.APIGroup == rbacv1.GroupName && target.Kind == "RoleBinding":
132-
rb := rbacv1.RoleBinding{}
133-
if err := c.Get(ctx, client.ObjectKey{Name: target.Name, Namespace: target.Namespace}, &rb); err != nil {
134-
return err
135-
}
136-
subjects, added := ensure(rb.Subjects, newSubject(user))
137-
rb.Subjects = subjects
138-
if added {
139-
return c.Update(ctx, &rb)
140-
}
141-
return nil
95+
o, err := targetref.GetTarget(ctx, c, target)
96+
if err != nil {
97+
return err
14298
}
14399

144-
return fmt.Errorf("unsupported target %q.%q", target.APIGroup, target.Kind)
145-
}
146-
147-
// ensure ensures that the given element is present in the given slice.
148-
// If the element is already present, the original slice is returned.
149-
// If the element is not present, a new slice is returned with the element appended.
150-
func ensure[T comparable](s []T, e T) (ret []T, added bool) {
151-
for _, v := range s {
152-
if v == e {
153-
return s, false
154-
}
100+
a, err := targetref.NewUserAccessor(o)
101+
if err != nil {
102+
return err
155103
}
156-
return append(s, e), true
157-
}
158104

159-
func newSubject(user string) rbacv1.Subject {
160-
return rbacv1.Subject{
161-
Kind: rbacv1.UserKind,
162-
APIGroup: rbacv1.GroupName,
163-
Name: user,
105+
added := a.EnsureUser(user)
106+
if added {
107+
return c.Update(ctx, o)
164108
}
109+
110+
return nil
165111
}

‎controllers/targetref/targetref.go

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
package targetref
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
rbacv1 "k8s.io/api/rbac/v1"
8+
"sigs.k8s.io/controller-runtime/pkg/client"
9+
10+
userv1 "github.com/appuio/control-api/apis/user/v1"
11+
controlv1 "github.com/appuio/control-api/apis/v1"
12+
)
13+
14+
// GetTarget returns the target object for the given TargetRef.
15+
// Returns an error if the target is not supported.
16+
func GetTarget(ctx context.Context, c client.Client, target userv1.TargetRef) (client.Object, error) {
17+
var obj client.Object
18+
switch {
19+
case target.APIGroup == "appuio.io" && target.Kind == "OrganizationMembers":
20+
obj = &controlv1.OrganizationMembers{}
21+
case target.APIGroup == "appuio.io" && target.Kind == "Team":
22+
obj = &controlv1.Team{}
23+
case target.APIGroup == rbacv1.GroupName && target.Kind == "ClusterRoleBinding":
24+
obj = &rbacv1.ClusterRoleBinding{}
25+
case target.APIGroup == rbacv1.GroupName && target.Kind == "RoleBinding":
26+
obj = &rbacv1.RoleBinding{}
27+
default:
28+
return nil, fmt.Errorf("unsupported target %q.%q", target.APIGroup, target.Kind)
29+
}
30+
31+
err := c.Get(ctx, client.ObjectKey{Name: target.Name, Namespace: target.Namespace}, obj)
32+
return obj, err
33+
}
34+
35+
// UserAccessor is an interface for accessing users from objects supported by this project.
36+
type UserAccessor interface {
37+
// EnsureUser adds the user to the object if it is not already present.
38+
// Returns true if the user was added.
39+
EnsureUser(user string) (added bool)
40+
// HasUser returns true if the user is present in the object.
41+
HasUser(user string) bool
42+
}
43+
44+
// NewUserAccessor returns a UserAccessor for the given object or an error if the object is not supported.
45+
func NewUserAccessor(obj client.Object) (UserAccessor, error) {
46+
switch o := obj.(type) {
47+
case *controlv1.OrganizationMembers:
48+
return &controlv1UserRefAccessor{userRefs: &o.Spec.UserRefs}, nil
49+
case *controlv1.Team:
50+
return &controlv1UserRefAccessor{userRefs: &o.Spec.UserRefs}, nil
51+
case *rbacv1.ClusterRoleBinding:
52+
return &rbacv1SubjectAccessor{subjects: &o.Subjects}, nil
53+
case *rbacv1.RoleBinding:
54+
return &rbacv1SubjectAccessor{subjects: &o.Subjects}, nil
55+
default:
56+
return nil, fmt.Errorf("unsupported object %T", obj)
57+
}
58+
}
59+
60+
type controlv1UserRefAccessor struct {
61+
userRefs *[]controlv1.UserRef
62+
}
63+
64+
var _ UserAccessor = &controlv1UserRefAccessor{}
65+
66+
func (s *controlv1UserRefAccessor) HasUser(user string) bool {
67+
return isInSlice(*s.userRefs, controlv1.UserRef{Name: user})
68+
}
69+
70+
func (s *controlv1UserRefAccessor) EnsureUser(user string) (added bool) {
71+
*s.userRefs, added = ensure(*s.userRefs, controlv1.UserRef{Name: user})
72+
return
73+
}
74+
75+
type rbacv1SubjectAccessor struct {
76+
subjects *[]rbacv1.Subject
77+
}
78+
79+
var _ UserAccessor = &rbacv1SubjectAccessor{}
80+
81+
func (s *rbacv1SubjectAccessor) HasUser(user string) bool {
82+
return isInSlice(*s.subjects, newSubject(user))
83+
}
84+
85+
func (s *rbacv1SubjectAccessor) EnsureUser(user string) (added bool) {
86+
*s.subjects, added = ensure(*s.subjects, newSubject(user))
87+
return
88+
}
89+
90+
func newSubject(user string) rbacv1.Subject {
91+
return rbacv1.Subject{
92+
Kind: rbacv1.UserKind,
93+
APIGroup: rbacv1.GroupName,
94+
Name: user,
95+
}
96+
}
97+
98+
func ensure[T comparable](s []T, e T) (ret []T, added bool) {
99+
for _, v := range s {
100+
if v == e {
101+
return s, false
102+
}
103+
}
104+
return append(s, e), true
105+
}
106+
107+
func isInSlice[T comparable](s []T, e T) (found bool) {
108+
for _, v := range s {
109+
if v == e {
110+
return true
111+
}
112+
}
113+
return false
114+
}

0 commit comments

Comments
 (0)