Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adds SSO control via individual namespaces. Fixes #6916 #6990

Merged
merged 43 commits into from
Nov 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
7536b52
fix: do not delete expr tag tmpl values. Fixes #6909 (#6921)
alexec Oct 14, 2021
dd1ef75
feat: enable sso control via individual namespaces
Oct 20, 2021
72fc702
feat: add comments
Oct 20, 2021
2344eb8
feat: add authorizing server for RBAC
Oct 22, 2021
3bf16d3
feat: add auth for artifact server
Oct 22, 2021
0af8f03
feat: add auth for artifact server
Oct 22, 2021
c3e4d65
feat: add auth for artifact server
Oct 22, 2021
bb1262c
feat: add auth for artifact server
Oct 22, 2021
125cd19
feat: add feature flag
Oct 23, 2021
8982818
Merge branch 'master' of https://github.com/argoproj/argo-workflows i…
Oct 23, 2021
2453b33
feat: fix signatures
Oct 23, 2021
d9953d8
feat: fix signatures
Oct 23, 2021
458578c
feat: update mocks
Oct 23, 2021
1241358
feat: remove duplicate imports
Oct 23, 2021
0a77201
feat: fix imports
Oct 23, 2021
eefb63b
feat: read flag from env
Oct 23, 2021
dedeca6
feat: add gatekeeper unit tests
Oct 23, 2021
d0b0675
feat: add artifact_server tests
Oct 23, 2021
040ca2f
feat: add artifact_server tests
Oct 23, 2021
1c256b7
feat: refactor error messages
Oct 23, 2021
fbbaced
feat: rename structs
Oct 23, 2021
0da0fe0
feat: rename structs
Oct 23, 2021
cbc135d
feat: rename structs
Oct 23, 2021
58e2142
feat: add informer for account list caching
Oct 24, 2021
2c2f801
feat: format code
Oct 24, 2021
26d777a
feat: format code
Oct 24, 2021
24c9594
feat: add cache tests
Oct 24, 2021
ae0625f
feat: format code
Oct 24, 2021
dc583f2
feat: commit for build
Oct 24, 2021
6431071
feat: address review comments
Oct 25, 2021
e47c142
feat: add logging
Oct 25, 2021
7c8ad7c
feat: add logging
Oct 25, 2021
d8ae5bb
fix: commit for build
Oct 25, 2021
dcc1992
feat: add ContextWithRequest for easier use
Oct 26, 2021
04be874
feat: address review comments
Oct 30, 2021
c51e0a8
feat: resolve merge conflicts
Oct 30, 2021
90ea1b7
feat: update manifests
Oct 30, 2021
0d870cf
feat: ad config for SSO namespace
Nov 1, 2021
e1eeba0
feat: empty commit for build
Nov 1, 2021
ec40b61
feat: empty commit for build
Nov 1, 2021
7a40f16
feat: tests failing intermittently, empty commit for build
Nov 1, 2021
51fd13e
Merge branch 'master' of https://github.com/argoproj/argo-workflows i…
Nov 1, 2021
74b95ef
Merge branch 'master' of https://github.com/argoproj/argo-workflows i…
Nov 2, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions cmd/argo/commands/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func NewServerCommand() *cobra.Command {
htst bool
namespaced bool // --namespaced
managedNamespace string // --managed-namespace
ssoNamespace string
enableOpenBrowser bool
eventOperationQueueSize int
eventWorkerCount int
Expand Down Expand Up @@ -124,11 +125,29 @@ See %s`, help.ArgoServer),
log.Warn("You are running without client authentication. Learn how to enable client authentication: https://argoproj.github.io/argo-workflows/argo-server-auth-mode/")
}

if namespaced {
// Case 1: If ssoNamespace is not specified, default it to installation namespace
if ssoNamespace == "" {
ssoNamespace = namespace
}
// Case 2: If ssoNamespace is not equal to installation or managed namespace, default it to installation namespace
if ssoNamespace != namespace && ssoNamespace != managedNamespace {
log.Warn("--sso-namespace should be equal to --managed-namespace or the installation namespace")
ssoNamespace = namespace
}
} else {
if ssoNamespace != "" {
log.Warn("ignoring --sso-namespace because --namespaced is false")
}
ssoNamespace = namespace
}
opts := apiserver.ArgoServerOpts{
BaseHRef: baseHRef,
TLSConfig: tlsConfig,
HSTS: htst,
Namespaced: namespaced,
Namespace: namespace,
SSONameSpace: ssoNamespace,
Clients: clients,
RestConfig: config,
AuthModes: modes,
Expand Down Expand Up @@ -187,6 +206,7 @@ See %s`, help.ArgoServer),
command.Flags().StringVar(&configMap, "configmap", "workflow-controller-configmap", "Name of K8s configmap to retrieve workflow controller configuration")
command.Flags().BoolVar(&namespaced, "namespaced", false, "run as namespaced mode")
command.Flags().StringVar(&managedNamespace, "managed-namespace", "", "namespace that watches, default to the installation namespace")
command.Flags().StringVar(&ssoNamespace, "sso-namespace", "", "namespace that will be used for SSO RBAC. Defaults to installation namespace. Used only in namespaced mode")
command.Flags().BoolVarP(&enableOpenBrowser, "browser", "b", false, "enable automatic launching of the browser [local mode]")
command.Flags().IntVar(&eventOperationQueueSize, "event-operation-queue-size", 16, "how many events operations that can be queued at once")
command.Flags().IntVar(&eventWorkerCount, "event-worker-count", 4, "how many event workers to run")
Expand Down
1 change: 1 addition & 0 deletions docs/cli/argo_server.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ See https://argoproj.github.io/argo-workflows/argo-server/
--managed-namespace string namespace that watches, default to the installation namespace
--namespaced run as namespaced mode
-p, --port int Port to listen on (default 2746)
--sso-namespace string namespace that will be used for SSO RBAC. Defaults to installation namespace. Used only in namespaced mode
--x-frame-options string Set X-Frame-Options header in HTTP responses. (default "DENY")
```

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ rules:
verbs:
- get
- create
- list
- watch
- apiGroups:
- ""
resources:
Expand All @@ -44,6 +46,7 @@ rules:
verbs:
- get
- list
- watch
- apiGroups:
- argoproj.io
resources:
Expand Down
3 changes: 3 additions & 0 deletions manifests/install.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,8 @@ rules:
verbs:
- get
- create
- list
- watch
- apiGroups:
- ""
resources:
Expand All @@ -498,6 +500,7 @@ rules:
verbs:
- get
- list
- watch
- apiGroups:
- argoproj.io
resources:
Expand Down
3 changes: 3 additions & 0 deletions manifests/namespace-install.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,8 @@ rules:
verbs:
- get
- create
- list
- watch
- apiGroups:
- ""
resources:
Expand All @@ -403,6 +405,7 @@ rules:
verbs:
- get
- list
- watch
- apiGroups:
- argoproj.io
resources:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ rules:
verbs:
- get
- create
- list
- watch
- apiGroups:
- ""
resources:
Expand All @@ -44,6 +46,7 @@ rules:
verbs:
- get
- list
- watch
- apiGroups:
- argoproj.io
resources:
Expand Down
3 changes: 3 additions & 0 deletions manifests/quick-start-minimal.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,8 @@ rules:
verbs:
- get
- create
- list
- watch
- apiGroups:
- ""
resources:
Expand All @@ -408,6 +410,7 @@ rules:
verbs:
- get
- list
- watch
- apiGroups:
- argoproj.io
resources:
Expand Down
3 changes: 3 additions & 0 deletions manifests/quick-start-mysql.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,8 @@ rules:
verbs:
- get
- create
- list
- watch
- apiGroups:
- ""
resources:
Expand All @@ -408,6 +410,7 @@ rules:
verbs:
- get
- list
- watch
- apiGroups:
- argoproj.io
resources:
Expand Down
3 changes: 3 additions & 0 deletions manifests/quick-start-postgres.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,8 @@ rules:
verbs:
- get
- create
- list
- watch
- apiGroups:
- ""
resources:
Expand All @@ -408,6 +410,7 @@ rules:
verbs:
- get
- list
- watch
- apiGroups:
- argoproj.io
resources:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: v1
data:
sso: |
issuer: https://dex:5556/dex
issuerAlias: https://mydex:5556/dex
issuerAlias: https://dex:5556/dex
alexec marked this conversation as resolved.
Show resolved Hide resolved
clientId:
name: argo-server-sso
key: clientID
Expand Down
2 changes: 1 addition & 1 deletion pkg/apiclient/argo-kube-client.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func newArgoKubeClient(ctx context.Context, clientConfig clientcmd.ClientConfig,
Sensor: sensorInterface,
Workflow: wfClient,
}
gatekeeper, err := auth.NewGatekeeper(auth.Modes{auth.Server: true}, clients, restConfig, nil, auth.DefaultClientForAuthorization, "unused")
gatekeeper, err := auth.NewGatekeeper(auth.Modes{auth.Server: true}, clients, restConfig, nil, auth.DefaultClientForAuthorization, "unused", "unused", false, nil)
if err != nil {
return nil, nil, err
}
Expand Down
17 changes: 16 additions & 1 deletion server/apiserver/argoserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/metadata"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/rest"
"k8s.io/utils/env"
Expand All @@ -42,6 +43,7 @@ import (
"github.com/argoproj/argo-workflows/v3/server/auth"
"github.com/argoproj/argo-workflows/v3/server/auth/sso"
"github.com/argoproj/argo-workflows/v3/server/auth/webhook"
"github.com/argoproj/argo-workflows/v3/server/cache"
"github.com/argoproj/argo-workflows/v3/server/clusterworkflowtemplate"
"github.com/argoproj/argo-workflows/v3/server/cronworkflow"
"github.com/argoproj/argo-workflows/v3/server/event"
Expand Down Expand Up @@ -81,18 +83,21 @@ type argoServer struct {
eventAsyncDispatch bool
xframeOptions string
accessControlAllowOrigin string
cache *cache.ResourceCache
}

type ArgoServerOpts struct {
BaseHRef string
TLSConfig *tls.Config
Namespaced bool
Namespace string
Clients *types.Clients
RestConfig *rest.Config
AuthModes auth.Modes
// config map name
ConfigName string
ManagedNamespace string
SSONameSpace string
HSTS bool
EventOperationQueueSize int
EventWorkerCount int
Expand All @@ -109,8 +114,16 @@ func init() {
}
}

func getResourceCacheNamespace(opts ArgoServerOpts) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you always want to use opts.managedNamespace?

Copy link
Contributor Author

@basanthjenuhb basanthjenuhb Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it.
Case 1

  • Installed argo in cluster mode
  • In this case, we need to look at service accounts of multiple namespaces
  • So use NamespaceAll for this

Case 2

  • Installed argo in namespace mode
  • In this case, we need to see service accounts of installed namespace (typically argo)
  • So use opt.Namespace for this

Case 3

  • Installed argo in managed namespace mode
  • So, in this case say, we use argo namespace for installation, and user-namespace for usage
  • So, for login, I would still need to use service accounts of argo namespace
  • Still use opt.Namespace for this

Since for case 2 and 3, we only need service accounts of a single namespace,
we will just use the installation namespace which is the namespace opt, and not managedNamespace opt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case 1:

cacheNamespace == managedNamespace == "" == NamespaceAll
namespace == "argo"

Case 2:

cacheNamespace == managedNamespace == namespace == "argo"

Case 3:

managedNamespace == userNamespace
namespace == "argo"
cacheNamespace == ???

So it is case 3 that is odd. It should be whatever it is today, which I think is "argo", which seems wrong to me. I would actually think you'd want the user namespace. But history is history.

Lets make this overridable with config, so if we are wrong, then we can change out mind.

Maybe call it ssoNamespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, we can use ssoNamespace
so for case 1 we will set ssoNamespace = NamespaceAll
For Case 2 and 3 we will set ssoNamespace = namespace (typically argo)
Will make these changes

if opts.Namespaced {
return opts.SSONameSpace
}
return v1.NamespaceAll
}

func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error) {
configController := config.NewController(opts.Namespace, opts.ConfigName, opts.Clients.Kubernetes, emptyConfigFunc)
var resourceCache *cache.ResourceCache = nil
ssoIf := sso.NullSSO
if opts.AuthModes[auth.SSO] {
c, err := configController.Get(ctx)
Expand All @@ -121,11 +134,12 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error
if err != nil {
return nil, err
}
resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, ctx, getResourceCacheNamespace(opts))
log.Info("SSO enabled")
} else {
log.Info("SSO disabled")
}
gatekeeper, err := auth.NewGatekeeper(opts.AuthModes, opts.Clients, opts.RestConfig, ssoIf, auth.DefaultClientForAuthorization, opts.Namespace)
gatekeeper, err := auth.NewGatekeeper(opts.AuthModes, opts.Clients, opts.RestConfig, ssoIf, auth.DefaultClientForAuthorization, opts.Namespace, opts.SSONameSpace, opts.Namespaced, resourceCache)
if err != nil {
return nil, err
}
Expand All @@ -145,6 +159,7 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error
eventAsyncDispatch: opts.EventAsyncDispatch,
xframeOptions: opts.XFrameOptions,
accessControlAllowOrigin: opts.AccessControlAllowOrigin,
cache: resourceCache,
}, nil
}

Expand Down
69 changes: 38 additions & 31 deletions server/artifacts/artifact_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/argoproj/argo-workflows/v3/persist/sqldb"
wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
"github.com/argoproj/argo-workflows/v3/server/auth"
"github.com/argoproj/argo-workflows/v3/server/types"
"github.com/argoproj/argo-workflows/v3/util/instanceid"
"github.com/argoproj/argo-workflows/v3/workflow/artifactrepositories"
artifact "github.com/argoproj/argo-workflows/v3/workflow/artifacts"
Expand Down Expand Up @@ -53,18 +54,17 @@ func (a *ArtifactServer) GetInputArtifact(w http.ResponseWriter, r *http.Request
}

func (a *ArtifactServer) getArtifact(w http.ResponseWriter, r *http.Request, isInput bool) {
ctx, err := a.gateKeeping(r)
requestPath := strings.SplitN(r.URL.Path, "/", 6)
namespace := requestPath[2]
workflowName := requestPath[3]
nodeId := requestPath[4]
artifactName := requestPath[5]

ctx, err := a.gateKeeping(r, types.NamespaceHolder(namespace))
Comment on lines +57 to +63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it would be better to do this with a serializer/deserlisizer that returns Go structs. This code is smelly/hacky

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True that.
I just didn't change it since it existed from before.
I can try to change it in a follow up PR

if err != nil {
w.WriteHeader(401)
_, _ = w.Write([]byte(err.Error()))
a.unauthorizedError(err, w)
return
}
path := strings.SplitN(r.URL.Path, "/", 6)

namespace := path[2]
workflowName := path[3]
nodeId := path[4]
artifactName := path[5]

log.WithFields(log.Fields{"namespace": namespace, "workflowName": workflowName, "nodeId": nodeId, "artifactName": artifactName, "isInput": isInput}).Info("Download artifact")

Expand All @@ -91,27 +91,33 @@ func (a *ArtifactServer) GetInputArtifactByUID(w http.ResponseWriter, r *http.Re
}

func (a *ArtifactServer) getArtifactByUID(w http.ResponseWriter, r *http.Request, isInput bool) {
ctx, err := a.gateKeeping(r)
requestPath := strings.SplitN(r.URL.Path, "/", 6)

uid := requestPath[2]
nodeId := requestPath[3]
artifactName := requestPath[4]
Comment on lines +94 to +98
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, Just reused the code which existed from before
Can change it in a follow up PR


// We need to know the namespace before we can do gate keeping
wf, err := a.wfArchive.GetWorkflow(uid)
if err != nil {
w.WriteHeader(401)
_, _ = w.Write([]byte(err.Error()))
a.serverInternalError(err, w)
return
}

path := strings.SplitN(r.URL.Path, "/", 6)

uid := path[2]
nodeId := path[3]
artifactName := path[4]

log.WithFields(log.Fields{"uid": uid, "nodeId": nodeId, "artifactName": artifactName, "isInput": isInput}).Info("Download artifact")
ctx, err := a.gateKeeping(r, types.NamespaceHolder(wf.GetNamespace()))
if err != nil {
a.unauthorizedError(err, w)
return
}

wf, err := a.getWorkflowByUID(ctx, uid)
// return 401 if the client does not have permission to get wf
err = a.validateAccess(ctx, wf)
if err != nil {
a.serverInternalError(err, w)
a.unauthorizedError(err, w)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this clearer error

return
}

log.WithFields(log.Fields{"uid": uid, "nodeId": nodeId, "artifactName": artifactName, "isInput": isInput}).Info("Download artifact")
err = a.returnArtifact(ctx, w, r, wf, nodeId, artifactName, isInput)

if err != nil {
Expand All @@ -120,7 +126,7 @@ func (a *ArtifactServer) getArtifactByUID(w http.ResponseWriter, r *http.Request
}
}

func (a *ArtifactServer) gateKeeping(r *http.Request) (context.Context, error) {
func (a *ArtifactServer) gateKeeping(r *http.Request, ns types.NamespacedRequest) (context.Context, error) {
token := r.Header.Get("Authorization")
if token == "" {
cookie, err := r.Cookie("authorization")
Expand All @@ -133,7 +139,12 @@ func (a *ArtifactServer) gateKeeping(r *http.Request) (context.Context, error) {
}
}
ctx := metadata.NewIncomingContext(r.Context(), metadata.MD{"authorization": []string{token}})
return a.gatekeeper.Context(ctx)
return a.gatekeeper.ContextWithRequest(ctx, ns)
}

func (a *ArtifactServer) unauthorizedError(err error, w http.ResponseWriter) {
w.WriteHeader(401)
_, _ = w.Write([]byte(err.Error()))
}

func (a *ArtifactServer) serverInternalError(err error, w http.ResponseWriter) {
Expand Down Expand Up @@ -229,17 +240,13 @@ func (a *ArtifactServer) getWorkflowAndValidate(ctx context.Context, namespace s
return wf, nil
}

func (a *ArtifactServer) getWorkflowByUID(ctx context.Context, uid string) (*wfv1.Workflow, error) {
wf, err := a.wfArchive.GetWorkflow(uid)
if err != nil {
return nil, err
}
func (a *ArtifactServer) validateAccess(ctx context.Context, wf *wfv1.Workflow) error {
allowed, err := auth.CanI(ctx, "get", "workflows", wf.Namespace, wf.Name)
if err != nil {
return nil, err
return err
}
if !allowed {
return nil, status.Error(codes.PermissionDenied, "permission denied")
return status.Error(codes.PermissionDenied, "permission denied")
}
return wf, nil
return nil
}
Loading