Add ability to create tokens #498

dthomson25 · 2018-08-07T00:25:35Z

Implemented Project Tokens (#472) using #228 as the overall design. In order to make it easier to add IAM authorization in the future, I created a more generic project role struct instead just creating a project token stuct. This will allow us to extend the project role to include IAMs specific roles without having to refactor the similar Jwt token work.

jessesuen · 2018-08-07T00:28:07Z

pkg/apis/application/v1alpha1/types.go

@@ -428,7 +429,7 @@ type RepositoryList struct {
 // AppProjectList is list of AppProject resources
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 type AppProjectList struct {
-	metav1.TypeMeta `json:",inline"`
+	metav1.TypeMeta `json:",inline""`


This extra quote is going to cause problems

jessesuen · 2018-08-07T00:28:58Z

pkg/apis/application/v1alpha1/types.go

@@ -453,6 +463,36 @@ type AppProjectSpec struct {

 	// Description contains optional project description
 	Description string `json:"description,omitempty" protobuf:"bytes,3,opt,name=description"`
+
+	Roles []ProjectRole `protobuf:"bytes,4,rep,name=roles"`


This needs the json annotation.

jessesuen · 2018-08-07T00:33:03Z

pkg/apis/application/v1alpha1/types.go

+}
+
+// GetRoleIndex looks up the index of a role in a project by the name
+func (proj *AppProject) GetRoleIndex(name string) (int, error) {


GetRoleIndex is not very useful as an exported method on project. It's really just implementation detail. Can we make this a unexported function inside the project app server instead? Or at least someplace other than types.go, which we should try to business logic to a minimum (unless we really feel every user of the type will need it).

I might feel better about it, it it were more useful like GetRoleByName().

Moved GetRoleByName to a separate file in the utils folder to get it out of the types.go.

jessesuen · 2018-08-07T00:34:26Z

util/rbac/rbac.go

@@ -19,6 +20,7 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/cache"

+	appclientset "github.com/argoproj/argo-cd/pkg/client/clientset/versioned"


The fact that we have to import an appclientset in the RBAC utility library, means we're not organizing the code very well. We should remove this import somehow, since this library is expected to be generic, and we have one internal project that depends on this RBAC library. In fact we eventually want to move the entire rbac library under argoproj/pkg instead of argoproj/argo-cd since we may need RBAC in other projects.

jessesuen · 2018-08-07T00:39:01Z

util/rbac/rbac.go

@@ -122,10 +131,47 @@ func (e *Enforcer) defaultEnforceClaims(rvals ...interface{}) bool {
 		}
 	}
 	user := jwtutil.GetField(mapClaims, "sub")
+	if strings.HasPrefix(user, "proj:") {


To fix the import issue, our custom defaultEnforceClaims can be moved to an ArgoCD specific function, which can be supplied when instantiating the enforcer using NewEnforcer.

I changed the enforcer so that if a claims enforcer function isn't provided, then we will always return false

jessesuen · 2018-08-07T00:46:34Z

cmd/argocd/commands/project.go

+	tokenCommand.AddCommand(NewProjectDeleteTokenCommand(clientOpts))
+	tokenCommand.AddCommand(NewProjectAddTokenPolicyCommand(clientOpts))
+	tokenCommand.AddCommand(NewProjectRemoveTokenPolicyCommand(clientOpts))
+	command.AddCommand(tokenCommand)


Per our previous discussion, what users will be managing, are roles in project. And tokens will be one mechanism to authenticate to a role. IAM whitelist will be an eventual second. We need to rework the terminology on the CLI to reflect this.

I changed the cli command from token to role to address this issue.

jessesuen · 2018-08-07T00:58:38Z

pkg/apis/application/v1alpha1/types.go

+// ProjectRoleMetatdata represents all the different types of roles a project can have
+// ProjectRoleMetatdata only one of its members may be specified for a specific role
+type ProjectRoleMetatdata struct {
+	JwtToken *JwtTokenMetadata `protobuf:"bytes,1,opt,name=jwtToken"`


Let's move JWTToken directly under ProjectRole and get rid of ProjectRoleMetatdata. After thinking about it some more, the one of property between JWT and IAM is not really necessary. We can support the mode where the same project role can be accessed by both an IAM role as well as JWT token.

Done, but I'm going to refactor it to allow a list of tokens as per our previous discussions

jessesuen · 2018-08-07T01:03:00Z

util/session/sessionmanager.go

@@ -43,6 +43,12 @@ const (
 	badUserError       = "Bad local superuser username"
 )

+// JwtToken the metadata of a token
+type JwtToken struct {


This datastructure is redundant. The string returned by SessionManager.Create() is already a JWT token with information about when it was issued.

jessesuen · 2018-08-07T01:03:42Z

util/session/sessionmanager.go

+	if err != nil {
+		return nil, err
+	}
+	return &JwtToken{Token: token, IssuedAt: now.Unix()}, nil


See previous comment. IssuedAt is already being generated in the claims so we don't need a wrapper around the token string.

jessesuen · 2018-08-13T22:17:03Z

cmd/argocd/commands/project.go

+	roleCommand.AddCommand(NewProjectRoleDeleteTokenCommand(clientOpts))
+	roleCommand.AddCommand(NewProjectRoleAddPolicyCommand(clientOpts))
+	roleCommand.AddCommand(NewProjectRoleRemovePolicyCommand(clientOpts))
+	command.AddCommand(roleCommand)


All of these roleCommand.AddCommand should be happening in the NewProjectRoleCommand() function. So then the only thing that happens here is:

command.AddCommand(NewProjectRoleCommand(clientOpts))

jessesuen · 2018-08-13T22:17:41Z

pkg/apis/application/v1alpha1/generated.proto

@@ -351,7 +370,7 @@ message SyncOperation {
  // Prune deletes resources that are no longer tracked in git
  optional bool prune = 2;

-  // DryRun will perform a `kubectl apply --dry-run` without actually performing the sync
+  // DryRun will perform a `kubectl apply --dry-rudn` without actually performing the sync


jessesuen · 2018-08-13T22:18:34Z

pkg/apis/application/v1alpha1/types.go

@@ -443,6 +443,15 @@ type AppProject struct {
 	Spec              AppProjectSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"`
 }

+//ProjectPoliciesString returns Casabin formated string of a project's polcies for each role


Casabin* typo. Also space after //

jessesuen · 2018-08-13T22:19:40Z

pkg/apis/application/v1alpha1/types.go

+	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
+	// Policies Stores a list of casbin formated strings that define access policies for the role in the project.
+	Policies  []string   `json:"policies" protobuf:"bytes,2,rep,name=policies"`
+	JWTTokens []JWTToken `json:"JWTTokens" protobuf:"bytes,3,rep,name=JWTTokens"`


The golang field name should be JWTTokens. But the JSON/proto field name should be lowercase jwtTokens.

jessesuen · 2018-08-13T22:21:18Z

Gopkg.lock

  name = "github.com/GeertJohan/go.rice"
  packages = [
    ".",
-    "embedded",
+    "embedded"


Are you running older version of dep? I think you need to upgrade to dep 0.5 because otherwise we will flap between dep formats.

Yes I was on 4.1. Let me know if the new changes from the dep ensure are a problem

jessesuen · 2018-08-13T23:39:43Z

cmd/argocd/commands/project.go

+			fmt.Print(token.Token)
+		},
+	}
+	command.Flags().StringVarP(&timeBeforeExpiry, "timeBeforeExpiry", "s", "0s", "Time before the token will expire. (Default: No expiration)")


Lets rename to expires-in per my other comment. Also "Duration" will be a better description than time.

In general, we shouldn't use camelCase for CLI flags, preferring dashes.

jessesuen · 2018-08-13T23:41:36Z

cmd/argocd/commands/project.go

+		timeBeforeExpiry string
+	)
+	var command = &cobra.Command{
+		Use:   "create-token PROJECT TOKEN-NAME [--seconds seconds]",


--seconds is inconsistent with the actual flag name, which I'm proposing to change to --expires-in to match the API field name.

jessesuen · 2018-08-13T23:43:53Z

cmd/argocd/commands/project.go

+// NewProjectRoleDeleteTokenCommand returns a new instance of an `argocd proj role delete-token` command
+func NewProjectRoleDeleteTokenCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command {
+	var command = &cobra.Command{
+		Use:   "delete-token PROJECT ROLE-NAME ISSUED_AT",


Inconsistency between dashes and underscore with ROLE-NAME ISSUED_AT. Lets choose dashes.

jessesuen · 2018-08-13T23:46:20Z

cmd/argocd/commands/project.go

+				fmt.Fprintf(w, "%s\n", role.Name)
+				if role.JWTTokens != nil {
+					for _, token := range role.JWTTokens {
+						fmt.Fprintf(w, "%s\t%d\t%d\n", role.Name, token.IssuedAt, token.ExpiresAt)


If a token does not expire, I think this is going to print epoch zero (1970). Instead we should print <none>

Actually I see that it will print 0 since this is '%d'. Instead can we print the issued at and expiration time in a human readable format. e.g. 2018-08-15T12:13:14?

Addressed this issue by breaking the list role command into a list role and a get role command. The list role command would list all the roles and their descriptions. The get role command would list the name, description, policies, and JWT tokens of a role in a project. With the JWT token, the cli lists the issue-at value as a ID to used in the delete-token and two human readable tokens for issued-at and expires-at.

Example output:

./dist/argocd proj role list guestbook ROLE-NAME Description test desc asdfghjklasdfghjklasdfghjkl desc1

./dist/argocd proj role get guestbook test Role Name: test Description:desc Policies: p, proj:guestbook:test, applications, get, guestbook/guestbook-test, allow p, proj:guestbook:test, applications, get, guestbook/*, allow Jwt Tokens: ID ISSUED-AT EXPIRES-AT 1534270415 Tue Aug 14 11:13:35 -0700 (4 hours ago) <none> 1534286505 2018-08-14T15:41:45-07:00 (16 minutes ago) 2018-08-24T15:41:45-07:00 (1 week from now) 1534286515 2018-08-14T15:41:55-07:00 (16 minutes ago) 2018-09-03T15:41:55-07:00 (2 weeks from now) 1534286520 2018-08-14T15:42:00-07:00 (16 minutes ago) 2018-09-04T15:42:00-07:00 (2 weeks from now) 1534286527 2018-08-14T15:42:07-07:00 (16 minutes ago) 2018-09-05T15:42:07-07:00 (3 weeks from now) 1534286539 2018-08-14T15:42:19-07:00 (15 minutes ago) 2018-10-13T15:42:19-07:00 (1 month from now) 1534286552 2018-08-14T15:42:32-07:00 (15 minutes ago) 2018-08-14T15:43:02-07:00 (15 minutes ago) 1534286554 2018-08-14T15:42:34-07:00 (15 minutes ago) 2018-08-14T15:43:04-07:00 (15 minutes ago) 1534286558 2018-08-14T15:42:38-07:00 (15 minutes ago) 2018-08-14T15:43:08-07:00 (15 minutes ago)```

jessesuen · 2018-08-13T23:47:48Z

pkg/apis/application/v1alpha1/generated.proto

@@ -283,6 +292,16 @@ message OperationState {
  optional k8s.io.apimachinery.pkg.apis.meta.v1.Time finishedAt = 7;
 }

+// ProjectRole represents a role that has access to a project
+message ProjectRole {
+  optional string name = 1;


I think I requested this earlier, but roles will need a description so that humans know the purpose of a role.

ammend

jessesuen

LGTM. Please squash and shorten the git commit message so it's not just a huge list of bullet points, before pushing.

dthomson25 requested review from alexmt and jessesuen August 7, 2018 00:25

jessesuen requested changes Aug 7, 2018

View reviewed changes

dthomson25 force-pushed the project-tokens branch 6 times, most recently from 79e4b07 to 9e317f8 Compare August 9, 2018 22:11

dthomson25 added 21 commits August 9, 2018 16:05

WIP[SPIKE]: Create project token and policies

ed0177f

Accept proj tokens based on createdAt time

63fecea

Modify CLI to have token subcommand

e8c18a0

Add tests for create token and create token Policies

66b5beb

Push all create token logic server side

8de9f28

Refactor create policy token to use update endpoint

b77b745

Add Policy delete and token delete cli commands

1d2f846

Add policy validation

c72fab0

Fix broken tests

f5f5973

Refactor token to use more generic role

94eaf6f

Move argocd specific enforcement to projects server

ba92991

Refactor cli to leverage roles instead of tokens

e307e40

Remove RoleMetadata to only use JwtToken

0c0b60a

Move argocd specific rbac impl to server

c8b9c72

Restrict tokens to applications instead of projects

49bb3ba

Move GetRoleIndexByName to a util lib

07e94dd

Add create/delete role commands to project cli

369c6a1

Refactor project errors to send status instead of regular errors

46ee59e

Fix formating

4c8c3b7

Add ability to stop multiple tokens

ec0fee6

Add expiresAt field to JwtToken

4a91e71

dthomson25 added 10 commits August 13, 2018 11:26

Renew JwtToken to JWTToken to find golang standard

6299abc

Refactor create/delete token api

fdb6a40

Rename imports to not use camelCase

84a377e

Remove missed created_at

28188f5

Reorder imports to fix style convention

37c1036

Refactor out GetInt64Field

3640cd1

Add Expires at to role list

8b866ae

Make error message more clear

72eefaf

Add comment to policy in types.go

e810855

Make create/delete token idempotent

7f1722f

jessesuen requested changes Aug 13, 2018

View reviewed changes

dthomson25 force-pushed the project-tokens branch 3 times, most recently from 7a582b0 to f3b04fb Compare August 14, 2018 04:35

Refactor all role commands into single method

51c3209

dthomson25 force-pushed the project-tokens branch from f3b04fb to 49a6c27 Compare August 14, 2018 04:38

dthomson25 added 3 commits August 13, 2018 21:45

Fix typos

b387c23

ammend

Refactor to make errors cleaner

2841b89

Rerun dep ensure after upgrading v5

b2218a4

dthomson25 force-pushed the project-tokens branch from 49a6c27 to b2218a4 Compare August 14, 2018 16:44

dthomson25 added 3 commits August 14, 2018 10:50

Rename cli variable for better consistency

3d390f2

Remove duplicate validation in CLI

b394685

Add description to the role

b90118e

dthomson25 force-pushed the project-tokens branch from 9e3cae3 to b90118e Compare August 14, 2018 18:15

dthomson25 added 2 commits August 14, 2018 15:44

Refactor CLI to getting role cleaner

d6d5fce

Humanize IssueAt and ExpiredAt values in CLI

1a9d159

dthomson25 force-pushed the project-tokens branch from 05022c2 to 1a9d159 Compare August 14, 2018 23:09

jessesuen approved these changes Aug 14, 2018

View reviewed changes

dthomson25 merged commit 66f64fb into argoproj:master Aug 15, 2018

dthomson25 deleted the project-tokens branch August 15, 2018 19:54

Add ability to create tokens #498

Add ability to create tokens #498

Conversation

dthomson25 commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessesuen Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessesuen left a comment

Choose a reason for hiding this comment

jessesuen Aug 14, 2018 •

edited

Loading