Nomad Secret Backend integration #3401

ncorrare · 2017-09-29T12:15:18Z

Considering that Nomad has shipped an ACL system it makes sense that Vault would be able to bridge the gap between the identity providers and the Nomad ACL system through the use of dynamic short-lived tokens.

Taking the Consul backend as an example, and refactoring to match the Nomad API data model (and with help from @chrishoffman whose input was far from negligible, and immensely appreciated), I've added a Nomad Secret Backend

… creation)

jefferai · 2017-09-29T12:31:43Z

Hi Nic,

Thanks for working on this!

This should be refactored on top of the combined database backend. Shouldn't be too hard though.

ncorrare · 2017-10-02T15:36:22Z

After talking with @schmichael apparently the failure in the tests was caused by hashicorp/nomad#3302 which is merged now.

chrishoffman

Overall, this is looking pretty good. There are a lot of comments but mostly relating to new conventions. The backend that this was modeled after was one of the oldest backends and much has not been changed for backwards compatibility reasonse.

chrishoffman · 2017-10-19T22:02:18Z

builtin/logical/nomad/path_config.go

+	}
+}
+
+func readConfigAccess(storage logical.Storage) (*accessConfig, error, error) {


I think you only need to have a single error. I'm not sure the differentiation between the two error returns but I'm not sure it is necessary.

I honestly don't see why in this case an error isn't just an error, and we need to differentiate between userErr and intErr. Maybe in an old Vault version something allowed to like "enable" the backend without configuration? In any case, I refactored it so an error is just an error, looks cleaner. Fixed in ca92922

chrishoffman · 2017-10-31T14:39:38Z

builtin/logical/nomad/secret_token.go

+	}
+
+	tokenRaw, ok := req.Secret.InternalData["accessor_id"]
+	if !ok {


This should never be true since we are always setting the accessor_id in the response data.

Fixed in 15de885

chrishoffman · 2017-10-31T14:43:03Z

builtin/logical/nomad/path_token.go

+		"secret_id":   token.SecretID,
+		"accessor_id": token.AccessorID,
+	}, map[string]interface{}{
+		"secret_id":   token.SecretID,


Although this is never exposed to the user, there is no reason to keep secret_id around since Vault will never be making calls in behalf the the user.

Fixed in 7015139

chrishoffman · 2017-10-31T14:44:39Z

builtin/logical/nomad/path_config.go

+				Type:        framework.TypeString,
+				Description: "URI scheme for the Nomad address",
+
+				// https would be a better default but Consul on its own


s/Consul/Nomad/

Fixed in 482d73a

chrishoffman · 2017-10-31T14:45:18Z

builtin/logical/nomad/path_config.go

+				// https would be a better default but Consul on its own
+				// defaults to HTTP access, and when HTTPS is enabled it
+				// disables HTTP, so there isn't really any harm done here.
+				Default: "http",


I think we should default to https and have to specify explicitly if they want http.

Fixed in 482d73a

chrishoffman · 2017-10-31T15:32:16Z

builtin/logical/nomad/path_roles.go

+	global := d.Get("global").(bool)
+	policy := d.Get("policy").([]string)
+	var err error
+	if tokenType != "management" {


Can a management token have associated policies? Should you verify the management token has zero policies?

Should be fixed in ffb9343

chrishoffman · 2017-10-31T15:32:29Z

builtin/logical/nomad/path_roles.go

+
+	entry, err := logical.StorageEntryJSON("policy/"+name, roleConfig{
+		Policy:    policy,
+		Lease:     lease,


s/Lease/TTL/

chrishoffman · 2017-10-31T15:37:19Z

builtin/logical/nomad/path_roles.go

+
+type roleConfig struct {
+	Policy    []string      `json:"policy"`
+	Lease     time.Duration `json:"lease"`


s/Lease/TTL/

chrishoffman · 2017-10-31T15:38:44Z

builtin/logical/nomad/secret_token.go

+func (b *backend) secretTokenRenew(
+	req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
+
+	return framework.LeaseExtend(0, 0, b.System())(req, d)


If you add a config/lease endpoint you are going to want to pass in the configured values instead of 0.

chrishoffman · 2017-10-31T15:40:09Z

builtin/logical/nomad/backend.go

+	var b backend
+	b.Backend = &framework.Backend{
+		Paths: []*framework.Path{
+			pathConfigAccess(),


Please add a config/lease path for configuring the ttl and max_ttl for the backend.

Fixed in d1e3eff

…ies attached

jefferai · 2017-12-14T21:57:54Z

builtin/logical/nomad/path_config_access.go

+
+	conf := &accessConfig{}
+	if err := entry.DecodeJSON(conf); err != nil {
+		return nil, fmt.Errorf("error reading nomad access configuration: %s", err)


Please use errwrap here.

jefferai · 2017-12-14T22:03:28Z

builtin/logical/nomad/path_config_access.go

+func (b *backend) pathConfigAccessWrite(
+	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+	address := data.Get("address").(string)
+	if address == "" {


See earlier comments; I don't think this (or token) should be required.

jefferai · 2017-12-14T22:04:29Z

builtin/logical/nomad/path_config_access.go

+		Callbacks: map[logical.Operation]framework.OperationFunc{
+			logical.ReadOperation:   b.pathConfigAccessRead,
+			logical.UpdateOperation: b.pathConfigAccessWrite,
+		},


If the client can be successful through env vars alone, there should be a DeleteOperation to clear out the access information if it's not needed or if it's conflicting.

jefferai · 2017-12-14T22:06:33Z

builtin/logical/nomad/path_config_lease.go

+
+	return &logical.Response{
+		Data: map[string]interface{}{
+			"ttl":     lease.TTL.Seconds(),


Please int64() these values.

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

builtin/logical/nomad/path_creds_create.go

+
+	role, err := b.Role(req.Storage, name)
+	if err != nil {
+		return nil, fmt.Errorf("error retrieving role: %v", err)


Please use errwrap here

jefferai · 2017-12-14T22:08:33Z

builtin/logical/nomad/path_creds_create.go

+		return nil, fmt.Errorf("error retrieving role: %v", err)
+	}
+	if role == nil {
+		return logical.ErrorResponse(fmt.Sprintf("Role %q not found", name)), nil


Please start the error with a lowercase letter (Go convention)

jefferai · 2017-12-14T22:09:30Z

builtin/logical/nomad/path_creds_create.go

+	}
+
+	// Generate a name for the token
+	tokenName := fmt.Sprintf("Vault-%s-%s-%d", name, req.DisplayName, time.Now().UnixNano())


s/Vault/vault

jefferai · 2017-12-14T22:11:30Z

builtin/logical/nomad/path_roles.go

+
+			"policies": &framework.FieldSchema{
+				Type:        framework.TypeCommaStringSlice,
+				Description: "Comma separated list of policies as previously created in Nomad. Required",


Please update this description to make it clear that it can be comma-separated or an array of strings.

Also, if this is going to be two separate sentences, please put a period after "Required".

jefferai · 2017-12-14T22:12:21Z

builtin/logical/nomad/path_roles.go

+			"global": &framework.FieldSchema{
+				Type:        framework.TypeBool,
+				Default:     false,
+				Description: "Boolean value describing if the token should be global or not. Defaults to false",


Same with "Defaults to false"

jefferai · 2017-12-14T22:13:27Z

builtin/logical/nomad/path_roles.go

+
+		Callbacks: map[logical.Operation]framework.OperationFunc{
+			logical.ReadOperation:   b.pathRolesRead,
+			logical.UpdateOperation: b.pathRolesWrite,


Please add CreateOperation and an existence check and update pathRolesWrite accordingly.

jefferai · 2017-12-14T22:13:40Z

builtin/logical/nomad/path_roles.go

+func (b *backend) Role(storage logical.Storage, name string) (*roleConfig, error) {
+	entry, err := storage.Get("role/" + name)
+	if err != nil {
+		return nil, fmt.Errorf("error retrieving role: %s", err)


Use errwrap please.

jefferai · 2017-12-14T22:14:48Z

builtin/logical/nomad/path_roles.go

+		}
+	default:
+		return logical.ErrorResponse(
+			"type must be \"client\" or \"management\""), nil


You can use `` for the string delimeter to avoid needing to escape quotes here

jefferai · 2017-12-14T22:16:49Z

builtin/logical/nomad/secret_token.go

+	}
+
+	if c == nil {
+		return nil, fmt.Errorf("Error connecting with Nomad")


lowercase the start of the error please

Also I think the error should be more descriptive...it's not an error connecting, it's an error getting a client.

jefferai · 2017-12-14T22:17:54Z

builtin/logical/nomad/secret_token.go

+		return nil, fmt.Errorf("accessor_id is missing on the lease")
+	}
+
+	_, err = c.ACLTokens().Delete(accessorIDRaw.(string), nil)


If for some reason it's not a string, this will panic. Please separate out the string conversion into its own guarded check.

* oss/master: Defer reader.Close that is used to determine sha256 changelog++ Avoid unseal failure if plugin backends fail to setup during postUnseal (#3686) Add logic for using Auth.Period when handling auth login/renew requests (#3677) plugins/database: use context with plugins that use database/sql package (#3691) changelog++ Fix plaintext backup in transit (#3692) Database gRPC plugins (#3666)

* oss/master: Add support for encrypted TLS key files (#3685)

ncorrare added 10 commits September 20, 2017 15:59

MVP of working Nomad Secret Backend

4cda42a

Fixing data model

393e7bf

Adding Nomad secret backend documentation

bcd1477

Adding Nomad Secret Backend API documentation

bc1ea9a

Added tests

9338277

Adding Global tokens to the data model

ca9ad73

Various fixes (Null pointer, wait for Nomad go up, Auth before policy…

bf68079

… creation)

Working tests

7e5c465

Removing ignore to cleanup function

222b9d1

Updated API Docs with the Global Token Parameter

b581716

ncorrare requested review from jefferai and chrishoffman September 29, 2017 12:15

ncorrare added 2 commits September 29, 2017 14:39

Adding vendor dependency

84b57b2

Adding vendor dir

ff4f920

ncorrare and others added 5 commits October 2, 2017 16:48

Added nomad as dependency after include fix

331068d

Adding further nomad deps

6a33bf0

fixing dependencies

44aed8f

fixing dependencies

a393b20

Adding Nomad docs to the nav. Minor cosmetics fixes

72b0a2f

ncorrare requested a review from dadgar October 6, 2017 16:02

ncorrare mentioned this pull request Oct 6, 2017

Vault Secret Backend hashicorp/nomad#3334

Merged

chrishoffman suggested changes Oct 31, 2017

View reviewed changes

ncorrare added 2 commits October 31, 2017 19:11

Minor/Cosmetic fixes

482d73a

Unifying Storage and API path in role

3a0d7ac

ncorrare force-pushed the f-nomad branch from ee54c12 to 3a0d7ac Compare October 31, 2017 21:06

ncorrare added 3 commits October 31, 2017 21:12

Should return an error if trying create a management token with polic…

ffb9343

…ies attached

tokenType can never be nil/empty string as there are default values

5d3513b

Ignoring userErr as it will be nil anyway

c4bf80c

jefferai reviewed Dec 14, 2017

View reviewed changes

hashicorp deleted a comment from vishalnayak Dec 14, 2017

hashicorp deleted a comment from ncorrare Dec 14, 2017

jefferai reviewed Dec 14, 2017

View reviewed changes

Chris Hoffman added 2 commits December 15, 2017 17:05

address some feedback

152b6e4

chrishoffman dismissed their stale review via 152b6e4 December 15, 2017 22:07

hashicorp deleted a comment from jefferai Dec 15, 2017

Chris Hoffman added 5 commits December 15, 2017 19:18

adding access config existence check and delete endpoint

b82493f

adding existence check for roles

20aac4d

Merge remote-tracking branch 'oss/master' into f-nomad

6c19fa3

* oss/master: Add support for encrypted TLS key files (#3685)

fixing up config to allow environment vars supported by api client

737dbca

use defaultconfig as base, adding env var test

abbb1c6

jefferai approved these changes Dec 18, 2017

View reviewed changes

Merge branch 'master' into f-nomad

4f31ee7

jefferai merged commit 7fd6103 into master Dec 18, 2017

jefferai deleted the f-nomad branch December 18, 2017 17:24

Nomad Secret Backend integration #3401

Nomad Secret Backend integration #3401

Conversation

ncorrare commented Sep 29, 2017

jefferai commented Sep 29, 2017

ncorrare commented Oct 2, 2017

chrishoffman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncorrare Nov 1, 2017 • 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

ncorrare Nov 3, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncorrare Nov 1, 2017 • 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

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

ncorrare Nov 1, 2017 •

edited

Loading

ncorrare Nov 3, 2017 •

edited

Loading

ncorrare Nov 1, 2017 •

edited

Loading