Skip to content

Commit

Permalink
Merge pull request from GHSA-2f5v-8r3f-8pww
Browse files Browse the repository at this point in the history 
* fix: application resource APIs must enforce project restrictions

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

* Fix unit tests

Signed-off-by: jannfis <jann@mistrust.net>

Co-authored-by: jannfis <jann@mistrust.net>
  • Loading branch information
@alexmt @jannfis
alexmt and jannfis committed Mar 22, 2022
1 parent b65c169 commit 21f208f
Show file tree
Hide file tree
Showing 10 changed files with 197 additions and 76 deletions.
22 changes: 18 additions & 4 deletions controller/appcontroller.go
View file
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
apiruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/wait"
Expand Down Expand Up @@ -421,8 +422,12 @@ func (ctrl *ApplicationController) getResourceTree(a *appv1.Application, managed
},
})
} else {
err := ctrl.stateCache.IterateHierarchy(a.Spec.Destination.Server, kube.GetResourceKey(live), func(child appv1.ResourceNode, appName string) {
err := ctrl.stateCache.IterateHierarchy(a.Spec.Destination.Server, kube.GetResourceKey(live), func(child appv1.ResourceNode, appName string) bool {
if !proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, a.Spec.Destination) {
return false
}
nodes = append(nodes, child)
return true
})
if err != nil {
return nil, err
Expand All @@ -432,16 +437,18 @@ func (ctrl *ApplicationController) getResourceTree(a *appv1.Application, managed
orphanedNodes := make([]appv1.ResourceNode, 0)
for k := range orphanedNodesMap {
if k.Namespace != "" && proj.IsGroupKindPermitted(k.GroupKind(), true) && !isKnownOrphanedResourceExclusion(k, proj) {
err := ctrl.stateCache.IterateHierarchy(a.Spec.Destination.Server, k, func(child appv1.ResourceNode, appName string) {
err := ctrl.stateCache.IterateHierarchy(a.Spec.Destination.Server, k, func(child appv1.ResourceNode, appName string) bool {
belongToAnotherApp := false
if appName != "" {
if _, exists, err := ctrl.appInformer.GetIndexer().GetByKey(ctrl.namespace + "/" + appName); exists && err == nil {
belongToAnotherApp = true
}
}
if !belongToAnotherApp {
orphanedNodes = append(orphanedNodes, child)
if belongToAnotherApp || !proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, a.Spec.Destination) {
return false
}
orphanedNodes = append(orphanedNodes, child)
return true
})
if err != nil {
return nil, err
Expand Down Expand Up @@ -1291,6 +1298,13 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
app.Status.Sync.Status = appv1.SyncStatusCodeUnknown
app.Status.Health.Status = health.HealthStatusUnknown
ctrl.persistAppStatus(origApp, &app.Status)

if err := ctrl.cache.SetAppResourcesTree(app.Name, &appv1.ApplicationTree{}); err != nil {
log.Warnf("failed to set app resource tree: %v", err)
}
if err := ctrl.cache.SetAppManagedResources(app.Name, nil); err != nil {
log.Warnf("failed to set app managed resources tree: %v", err)
}
return
}

Expand Down
4 changes: 2 additions & 2 deletions controller/appcontroller_test.go
View file
Expand Up @@ -136,12 +136,12 @@ func newFakeController(data *fakeData) *ApplicationController {
mockStateCache.On("GetClusterCache", mock.Anything).Return(&clusterCacheMock, nil)
mockStateCache.On("IterateHierarchy", mock.Anything, mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
key := args[1].(kube.ResourceKey)
action := args[2].(func(child argoappv1.ResourceNode, appName string))
action := args[2].(func(child argoappv1.ResourceNode, appName string) bool)
appName := ""
if res, ok := data.namespacedResources[key]; ok {
appName = res.AppName
}
action(argoappv1.ResourceNode{ResourceRef: argoappv1.ResourceRef{Kind: key.Kind, Group: key.Group, Namespace: key.Namespace, Name: key.Name}}, appName)
_ = action(argoappv1.ResourceNode{ResourceRef: argoappv1.ResourceRef{Kind: key.Kind, Group: key.Group, Namespace: key.Namespace, Name: key.Name}}, appName)
}).Return(nil)
return ctrl
}
Expand Down
8 changes: 4 additions & 4 deletions controller/cache/cache.go
View file
Expand Up @@ -104,7 +104,7 @@ type LiveStateCache interface {
// Returns synced cluster cache
GetClusterCache(server string) (clustercache.ClusterCache, error)
// Executes give callback against resource specified by the key and all its children
IterateHierarchy(server string, key kube.ResourceKey, action func(child appv1.ResourceNode, appName string)) error
IterateHierarchy(server string, key kube.ResourceKey, action func(child appv1.ResourceNode, appName string) bool) error
// Returns state of live nodes which correspond for target nodes of specified application.
GetManagedLiveObjs(a *appv1.Application, targetObjs []*unstructured.Unstructured) (map[kube.ResourceKey]*unstructured.Unstructured, error)
// IterateResources iterates all resource stored in cache
Expand Down Expand Up @@ -437,13 +437,13 @@ func (c *liveStateCache) IsNamespaced(server string, gk schema.GroupKind) (bool,
return clusterInfo.IsNamespaced(gk)
}

func (c *liveStateCache) IterateHierarchy(server string, key kube.ResourceKey, action func(child appv1.ResourceNode, appName string)) error {
func (c *liveStateCache) IterateHierarchy(server string, key kube.ResourceKey, action func(child appv1.ResourceNode, appName string) bool) error {
clusterInfo, err := c.getSyncedCluster(server)
if err != nil {
return err
}
clusterInfo.IterateHierarchy(key, func(resource *clustercache.Resource, namespaceResources map[kube.ResourceKey]*clustercache.Resource) {
action(asResourceNode(resource), getApp(resource, namespaceResources))
clusterInfo.IterateHierarchy(key, func(resource *clustercache.Resource, namespaceResources map[kube.ResourceKey]*clustercache.Resource) bool {
return action(asResourceNode(resource), getApp(resource, namespaceResources))
})
return nil
}
Expand Down
4 changes: 2 additions & 2 deletions controller/cache/mocks/LiveStateCache.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion go.mod
View file
Expand Up @@ -8,7 +8,7 @@ require (
github.com/TomOnTime/utfutil v0.0.0-20180511104225-09c41003ee1d
github.com/alicebob/miniredis v2.5.0+incompatible
github.com/alicebob/miniredis/v2 v2.14.2
github.com/argoproj/gitops-engine v0.6.0
github.com/argoproj/gitops-engine v0.6.1
github.com/argoproj/notifications-engine v0.3.1-0.20220127183449-91deed20b998
github.com/argoproj/pkg v0.11.1-0.20211203175135-36c59d8fafe0
github.com/bombsimon/logrusr/v2 v2.0.1
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Expand Up @@ -125,8 +125,8 @@ github.com/antonmedv/expr v1.8.9/go.mod h1:5qsM3oLGDND7sDmQGDXHkYfkjYMUX14qsgqmH
github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
github.com/apache/thrift v0.13.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
github.com/appscode/go v0.0.0-20190808133642-1d4ef1f1c1e0/go.mod h1:iy07dV61Z7QQdCKJCIvUoDL21u6AIceRhZzyleh2ymc=
github.com/argoproj/gitops-engine v0.6.0 h1:Tnh6kUUVuBV0m3gueYIymAeErWl9XNN9O9JcOoNM0vU=
github.com/argoproj/gitops-engine v0.6.0/go.mod h1:pRgVpLW7pZqf7n3COJ7UcDepk4cI61LAcJd64Q3Jq/c=
github.com/argoproj/gitops-engine v0.6.1 h1:fiaMQ+0OfBHQWInekgkO645i4dolvl3KA//0F7n4PK8=
github.com/argoproj/gitops-engine v0.6.1/go.mod h1:pRgVpLW7pZqf7n3COJ7UcDepk4cI61LAcJd64Q3Jq/c=
github.com/argoproj/notifications-engine v0.3.1-0.20220127183449-91deed20b998 h1:V9RDg+IZeebnm3XjkfkbN07VM21Fu1Cy/RJNoHO++VM=
github.com/argoproj/notifications-engine v0.3.1-0.20220127183449-91deed20b998/go.mod h1:5mKv7zEgI3NO0L+fsuRSwBSY9EIXSuyIsDND8O8TTIw=
github.com/argoproj/pkg v0.11.1-0.20211203175135-36c59d8fafe0 h1:Cfp7rO/HpVxnwlRqJe0jHiBbZ77ZgXhB6HWlYD02Xdc=
Expand Down
10 changes: 7 additions & 3 deletions pkg/apis/application/v1alpha1/app_project_types.go
View file
Expand Up @@ -313,11 +313,15 @@ func (proj AppProject) IsGroupKindPermitted(gk schema.GroupKind, namespaced bool

// IsLiveResourcePermitted returns whether a live resource found in the cluster is permitted by an AppProject
func (proj AppProject) IsLiveResourcePermitted(un *unstructured.Unstructured, server string, name string) bool {
if !proj.IsGroupKindPermitted(un.GroupVersionKind().GroupKind(), un.GetNamespace() != "") {
return proj.IsResourcePermitted(un.GroupVersionKind().GroupKind(), un.GetNamespace(), ApplicationDestination{Server: server, Name: name})
}

func (proj AppProject) IsResourcePermitted(groupKind schema.GroupKind, namespace string, dest ApplicationDestination) bool {
if !proj.IsGroupKindPermitted(groupKind, namespace != "") {
return false
}
if un.GetNamespace() != "" {
return proj.IsDestinationPermitted(ApplicationDestination{Server: server, Namespace: un.GetNamespace(), Name: name})
if namespace != "" {
return proj.IsDestinationPermitted(ApplicationDestination{Server: dest.Server, Name: dest.Name, Namespace: namespace})
}
return true
}
Expand Down
31 changes: 23 additions & 8 deletions server/application/application.go
View file
Expand Up @@ -488,6 +488,21 @@ func (s *Server) ListResourceEvents(ctx context.Context, q *application.Applicat
"involvedObject.namespace": a.Namespace,
}).String()
} else {
tree, err := s.getAppResources(ctx, a)
if err != nil {
return nil, err
}
found := false
for _, n := range append(tree.Nodes, tree.OrphanedNodes...) {
if n.ResourceRef.UID == q.ResourceUID && n.ResourceRef.Name == q.ResourceName && n.ResourceRef.Namespace == q.ResourceNamespace {
found = true
break
}
}
if !found {
return nil, status.Errorf(codes.InvalidArgument, "%s not found as part of application %s", q.ResourceName, *q.Name)
}

namespace = q.ResourceNamespace
var config *rest.Config
config, err = s.getApplicationClusterConfig(ctx, a)
Expand Down Expand Up @@ -937,7 +952,7 @@ func (s *Server) getAppResources(ctx context.Context, a *appv1.Application) (*ap
return &tree, err
}

func (s *Server) getAppResource(ctx context.Context, action string, q *application.ApplicationResourceRequest) (*appv1.ResourceNode, *rest.Config, *appv1.Application, error) {
func (s *Server) getAppLiveResource(ctx context.Context, action string, q *application.ApplicationResourceRequest) (*appv1.ResourceNode, *rest.Config, *appv1.Application, error) {
a, err := s.appLister.Get(*q.Name)
if err != nil {
return nil, nil, nil, err
Expand All @@ -952,7 +967,7 @@ func (s *Server) getAppResource(ctx context.Context, action string, q *applicati
}

found := tree.FindNode(q.Group, q.Kind, q.Namespace, q.ResourceName)
if found == nil {
if found == nil || found.ResourceRef.UID == "" {
return nil, nil, nil, status.Errorf(codes.InvalidArgument, "%s %s %s not found as part of application %s", q.Kind, q.Group, q.ResourceName, *q.Name)
}
config, err := s.getApplicationClusterConfig(ctx, a)
Expand All @@ -963,7 +978,7 @@ func (s *Server) getAppResource(ctx context.Context, action string, q *applicati
}

func (s *Server) GetResource(ctx context.Context, q *application.ApplicationResourceRequest) (*application.ApplicationResourceResponse, error) {
res, config, _, err := s.getAppResource(ctx, rbacpolicy.ActionGet, q)
res, config, _, err := s.getAppLiveResource(ctx, rbacpolicy.ActionGet, q)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1008,7 +1023,7 @@ func (s *Server) PatchResource(ctx context.Context, q *application.ApplicationRe
Version: q.Version,
Group: q.Group,
}
res, config, a, err := s.getAppResource(ctx, rbacpolicy.ActionUpdate, resourceRequest)
res, config, a, err := s.getAppLiveResource(ctx, rbacpolicy.ActionUpdate, resourceRequest)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1048,7 +1063,7 @@ func (s *Server) DeleteResource(ctx context.Context, q *application.ApplicationR
Version: q.Version,
Group: q.Group,
}
res, config, a, err := s.getAppResource(ctx, rbacpolicy.ActionDelete, resourceRequest)
res, config, a, err := s.getAppLiveResource(ctx, rbacpolicy.ActionDelete, resourceRequest)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1319,7 +1334,7 @@ func getSelectedPods(treeNodes []appv1.ResourceNode, q *application.ApplicationP
var pods []appv1.ResourceNode
isTheOneMap := make(map[string]bool)
for _, treeNode := range treeNodes {
if treeNode.Kind == kube.PodKind && treeNode.Group == "" {
if treeNode.Kind == kube.PodKind && treeNode.Group == "" && treeNode.UID != "" {
if isTheSelectedOne(&treeNode, q, treeNodes, isTheOneMap) {
pods = append(pods, treeNode)
}
Expand Down Expand Up @@ -1609,7 +1624,7 @@ func (s *Server) logResourceEvent(res *appv1.ResourceNode, ctx context.Context,
}

func (s *Server) ListResourceActions(ctx context.Context, q *application.ApplicationResourceRequest) (*application.ResourceActionsListResponse, error) {
res, config, _, err := s.getAppResource(ctx, rbacpolicy.ActionGet, q)
res, config, _, err := s.getAppLiveResource(ctx, rbacpolicy.ActionGet, q)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1660,7 +1675,7 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA
Group: q.Group,
}
actionRequest := fmt.Sprintf("%s/%s/%s/%s", rbacpolicy.ActionAction, q.Group, q.Kind, q.Action)
res, config, a, err := s.getAppResource(ctx, actionRequest, resourceRequest)
res, config, a, err := s.getAppLiveResource(ctx, actionRequest, resourceRequest)
if err != nil {
return nil, err
}
Expand Down

0 comments on commit 21f208f

Please sign in to comment.