Init create #3

nate-yocom · 2017-12-07T21:24:12Z

Updated PR after addressing review feedback from #2

…t config to match

…ng-sdk rather than internal lib

… lookup to get resolved username/uuid of user from directory service.

- Don't default mode in cli, path_login handles this already - Don't apply a default policy - Add an existence check - Config not found should return same as empty config - Snake case struct field tags

jefferai · 2017-12-07T21:25:49Z

@nate-yocom You don't need to close PRs and open new ones -- this makes it impossible to track comments as you make changes. You can just push new commits to the branch the PR is based off of.

nate-yocom · 2017-12-07T21:31:40Z

Ah, good to know. Apologies. However you'd like to proceed let me know.

jefferai · 2017-12-07T21:42:21Z

path_config.go

+	} else if req.Operation == logical.CreateOperation {
+		config.AppID = data.Get("app_id").(string)
+	}
+	if config.AppID == "" {


You should either do the same thing as above, where if this is a required parameter you error out if it's not set, or remove this code -- but this basically sets defaults, which the framework will already do for you when you call data.Get().

Ah yeah, good catch - i'd added this before realizing defaults could be set by framework. Thanks.

jefferai · 2017-12-07T21:43:24Z

path_config.go

+	}
+
+	// Munge on the service a little bit, force it to have no trailing / and always start with https://
+	var normalizedService = strings.TrimPrefix(config.ServiceURL, "http://")


Please use https://golang.org/pkg/net/url/ for this -- you can parse it (while making sure it's a valid URL), then manually adjust the scheme and get a string representation.

jefferai · 2017-12-07T21:44:45Z

path_config.go

+	if err != nil {
+		return nil, err
+	}
+


It's preferable to return nil, nil at this point if entry == nil. That gives more flexibility to other code trying to understand whether a config existed or not (e.g. in the create or update path).

jefferai · 2017-12-07T21:50:15Z

path_config.go

+}
+
+type config struct {
+	ClientID        string   `json:"client_id" structs:"client_id" mapstructure:"client_id"`


You don't need the structs/mapstructure bits unless you specifically know you're using those things. (We're actually divesting the Vault codebase of their usage as much as possible these days.)

agreed, removed.

jefferai · 2017-12-07T21:51:25Z

path_login.go

+	"github.com/hashicorp/vault/logical/framework"
+)
+
+const defaultAuthMode = "ro"


You can set the Default option for mode in the field schema instead of doing this. A const can be nice if you're using it in multiple places, but you're not currently.

jefferai · 2017-12-07T22:22:37Z

path_login.go

+
+	if mode == "cc" {
+		oclient, err = oauth.GetNewConfidentialClient(config.ServiceURL, username, password, cleanhttp.DefaultClient)
+		oclient.SourceHeader = "vault-auth-plugin"


It'd be preferable to name this the same as the repo name for clarity -- vault-plugin-auth-centrify

done (everywhere)

jefferai · 2017-12-07T22:24:08Z

path_login.go

+		token, failure, err = oclient.ClientCredentials(config.AppID, config.Scope)
+	} else if mode == "ro" {
+		oclient, err = oauth.GetNewConfidentialClient(config.ServiceURL, config.ClientID, config.ClientSecret, cleanhttp.DefaultClient)
+		oclient.SourceHeader = "vault-auth-plugin"


jefferai · 2017-12-07T22:26:02Z

path_login.go

+	}
+
+	restClient.Headers["Authorization"] = accessToken.TokenType + " " + accessToken.AccessToken
+	restClient.SourceHeader = "vault-auth-plugin"


jefferai · 2017-12-07T22:27:39Z

path_login.go

+	resp := &logical.Response{
+		Auth: &logical.Auth{
+			InternalData: map[string]interface{}{
+				"access_token": token,


If you aren't supporting renewal, you don't need to store this -- InternalData is just there to round-trip things back to the renew function.

We don't support it today, but this token does contain a refresh token (optionally based on cloud side service configuration), so we may support it later. leaving this for future proofing.

It might be safer and a bit easier later on if you just store the items from the TokenResponse that may be required for renewal, or possibly each field separately. If you store the original response object you will need to use something like mapstructure or Marshal/Unmarshal to get this in a usable form for further processing.

jefferai · 2017-12-07T22:32:39Z

path_login.go

+			Alias: &logical.Alias{
+				Name: username,
+			},
+			EntityID: strings.ToLower(uinfo.uuid),


This doesn't need to be set (and will be ignored).

jefferai · 2017-12-07T22:39:59Z

README.md

+    - Client ID Type -> Confidential (must be OAuth client)
+  - Tokens:
+    - Token Type: JwtRS256


Do you actually care about the contents of the JWT or just pass it around as a token? I didn't see JWT parsing code here or in the SDK; if there is any you are using I'd like to skim it real quick.

Nope, not parsing it today

- Default mode via Fields default - Defaults are handled via framework, no need to force them in config set - Use net/url to parse/validate service_url and force scheme/normalize path - return nil for config if not-existent, so exist check actually works :) - remove mapstructure/structs tags - rest client source-header set to name of repo - don't set EntityID on Auth result

calvn · 2018-01-04T16:31:48Z

path_login.go

+func (b *backend) getUserInfo(accessToken *oauth.TokenResponse, serviceUrl string) (*userinfo, error) {
+	uinfo := &userinfo{}
+
+	restClient, err := restapi.GetNewRestClient(serviceUrl, cleanhttp.DefaultClient)


restClient *restapi.RestClient could be part of type backend struct, and this call to instantiate a client can be made in Backend() to avoid creating a client on every login request. However, you'll need to flush the token after the request since all login requests will share the same client.

I think it is likely safer to create a new client for each request.

Yeah that's probably the right call. I was looking at auth/aws which stores a map of EC2/IAM clients to avoid recreating clients on renewal, but this backend doesn't support renewals yet. If the discussion on renewal above means that there's will be support for it in the future, then we could store a map of RestClient instead of storing the token in InternalData.

Yes, keeping it simple for now - new client for each request means no potential for crossing streams (accidentally sharing headers etc), allows our cloud service to best load balance connections, and keeps it simple. We can get fancier if/when renewal support comes online and if optimization here then makes sense.

chrishoffman · 2018-01-04T21:44:48Z

path_login.go

+		oclient, err = oauth.GetNewConfidentialClient(config.ServiceURL, username, password, cleanhttp.DefaultClient)
+		oclient.SourceHeader = "vault-plugin-auth-centrify"
+		if err != nil {
+			log.Fatal(err)


This log.Fatal will shutdown the plugin. This should probably just return nil, err.

chrishoffman · 2018-01-04T21:45:16Z

path_login.go

+		oclient, err = oauth.GetNewConfidentialClient(config.ServiceURL, config.ClientID, config.ClientSecret, cleanhttp.DefaultClient)
+		oclient.SourceHeader = "vault-plugin-auth-centrify"
+		if err != nil {
+			log.Fatal(err)


Same comment as above.

chrishoffman · 2018-01-04T21:46:14Z

path_login.go

+	var token *oauth.TokenResponse
+	var failure *oauth.ErrorResponse
+
+	if mode == "cc" {


Minor: This would read a little better as a switch statement on mode instead of if, else if...

chrishoffman · 2018-01-04T21:48:35Z

path_login.go

+	}
+
+	uinfo, err := b.getUserInfo(token, config.ServiceURL)
+	b.Logger().Trace("centrify authenticated user", "userinfo", uinfo)


This probably makes sense to move after the error checking since it will log the nil user on every failure here.

chrishoffman · 2018-01-04T21:50:18Z

path_login.go

+func (b *backend) getUserInfo(accessToken *oauth.TokenResponse, serviceUrl string) (*userinfo, error) {
+	uinfo := &userinfo{}
+
+	restClient, err := restapi.GetNewRestClient(serviceUrl, cleanhttp.DefaultClient)


I think it is likely safer to create a new client for each request.

chrishoffman · 2018-01-04T22:02:02Z

path_login.go

+		return nil, errors.New("centrify auth plugin configuration not set")
+	}
+
+	var oclient *oauth.OauthClient


It looks like you can scope oclient to the conditional below. This value is not used outside that scope.

chrishoffman · 2018-01-04T22:12:31Z

path_login.go

+	resp := &logical.Response{
+		Auth: &logical.Auth{
+			InternalData: map[string]interface{}{
+				"access_token": token,


It might be safer and a bit easier later on if you just store the items from the TokenResponse that may be required for renewal, or possibly each field separately. If you store the original response object you will need to use something like mapstructure or Marshal/Unmarshal to get this in a usable form for further processing.

chrishoffman · 2018-01-04T22:21:51Z

path_login.go

+
+	var rolePolicies []string
+	if config.RolesAsPolicies {
+		for _, role := range uinfo.roles {


I see that this is configurable but having implicit policies based on role name still seems a little risky to me. This could lead someone that is administering Centrify roles to accidentally escalate privileges to a user not knowing that a policy may exists with the same name in Vault. I would suggest providing an explicit mappings from roles to policies. An example would be ldap groups https://www.vaultproject.io/api/auth/ldap/index.html#read-ldap-group.

Actually, we're returning our roles as groups in Auth.GroupAliases - is there a generic vault means by which policies can be assigned via group alias? if so, this obviates the need for either RolesAsPolicies or a new group->policy mapping indirection.

@nate-yocom Indeed, you can assign policies to groups. I think you're right that this obviates the need for RolesAsPolicies. It's very new-style but it's the direction we're headed!

Cool, i'll remove the RolesAsPolicies - is the group alias -> policy stuff doc'd somewhere? I can update our doc to link to the same. Thanks!

See https://www.vaultproject.io/api/secret/identity/group-alias.html

Basically the admin of Vault needs to create the group aliases. The reason is that if we do it opportunistically we can easily end up creating 1000 aliases when 5 are needed and pollute the Identity store. But it's relatively easy to write a script to do it (one that lives in your plugin repo would probably be super nice to have as a reference!).

chrishoffman · 2018-01-04T22:23:48Z

path_login.go

+		token, failure, err = oclient.ClientCredentials(config.AppID, config.Scope)
+	} else if mode == "ro" {
+		oclient, err = oauth.GetNewConfidentialClient(config.ServiceURL, config.ClientID, config.ClientSecret, cleanhttp.DefaultClient)
+		oclient.SourceHeader = "vault-plugin-auth-centrify"


See comment above.

chrishoffman · 2018-01-04T22:29:35Z

path_login.go

+			uinfo.roles = append(uinfo.roles, row["Name"].(string))
+		}
+	} else {
+		b.Logger().Error("centrify: failed to get user roles", "error", rolesAndRightsResult.Message)


Just want to check if you intend to continue when this call result returns unsuccessful and just logs to vault's log.

Yes, authentication has succeeded - inability to get roles may, or may not, mean the user can do anything however - so it seems appropriate to continue, having logged an error.

- replace log.Fatal with err return - Inline error checking in a couple places - const for client source header - change if/elseif/else to a switch statement - don't trace user if we cant get info on them - remove use of logical.Response.Auth.InternalData for now (renewal not supported)

…preferred

* commit '412b98b0d536c855829996e31d895eb50f436a94': Address review feedback from PR #3: # Conflicts: # path_login.go

chrishoffman

Looks good!

Nate Yocom added 6 commits November 16, 2017 17:19

Initial commit

6a483ef

Update README.md with cloud and plugin configuration details + defaul…

cb171cf

…t config to match

Update with cloud version requirement

ce82a4b

Update to use centrify golang sdk from github.com/centrify/cloud-gola…

a1e26ee

…ng-sdk rather than internal lib

Fix 'policies' config so it round-trips, and add additional user info…

cb08090

… lookup to get resolved username/uuid of user from directory service.

Address review feedback from #2 =>

80f356f

- Don't default mode in cli, path_login handles this already - Don't apply a default policy - Add an existence check - Config not found should return same as empty config - Snake case struct field tags

nate-yocom requested review from jefferai and calvn December 7, 2017 21:24

jefferai reviewed Dec 7, 2017

View reviewed changes

calvn reviewed Jan 4, 2018

View reviewed changes

chrishoffman reviewed Jan 4, 2018

View reviewed changes

Nate Yocom added 3 commits January 8, 2018 07:33

Address review feedback from PR #3 - scope OauthClient with usage

0a970a1

Remove RolesAsPolicies option - indirection through group aliases is …

fe8bfb1

…preferred

nate-yocom pushed a commit that referenced this pull request Jan 8, 2018

Merge commit '412b98b0d536c855829996e31d895eb50f436a94' into local-wip

f82a24e

* commit '412b98b0d536c855829996e31d895eb50f436a94': Address review feedback from PR #3: # Conflicts: # path_login.go

chrishoffman approved these changes Jan 18, 2018

View reviewed changes

jefferai merged commit 50def81 into master Jan 19, 2018

jefferai deleted the init-create branch January 19, 2018 06:04

Init create #3

Init create #3

Conversation

nate-yocom commented Dec 7, 2017

jefferai commented Dec 7, 2017

nate-yocom commented Dec 7, 2017

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

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

chrishoffman left a comment

Choose a reason for hiding this comment