Add period and max_ttl to cert role creation #3642

calvn · 2017-12-03T21:25:35Z

jefferai · 2017-12-04T17:50:50Z

builtin/credential/cert/path_certs.go

 	return &logical.Response{
 		Data: map[string]interface{}{
 			"certificate":  cert.Certificate,
 			"display_name": cert.DisplayName,
 			"policies":     cert.Policies,
-			"ttl":          duration / time.Second,


I think we should actually not display ttl, max_ttl, or period if they're not set. This is because the value will vary according to the system/mount tuning, so returning 0 (or not returning the key at all) keeps people from making assumptions based on snapshots of the current values.

Period will be 0 if that's not set. I can remove ttl and max_ttl from using the sys values if those are zero so that they return zero as well. AFAIK the other backends return ttl, max_ttl, and period all the time so if we do decide to remove them if unset we would have to do so for other backends as well, though this would could be a breaking change on people using these values from the response.

If this value will vary and we are returning 0 or hiding it if it's unset, where can the user/operator find out what these values are, would it be by doing a token lookup directly?

jefferai · 2017-12-04T17:52:17Z

builtin/credential/cert/path_certs.go

+	if ttl == 0 {
+		ttl = time.Second * time.Duration(d.Get("lease").(int))
+	}
+	if ttl > systemDefaultTTL {


Don't check against system TTL/max TTL. We'll automatically truncate the time for issued credentials and this can cause subtle issues. At the most, I'd make this a warning instead of an error.

I've changed it to be a warning instead of error below.

jefferai · 2017-12-04T17:52:57Z

builtin/credential/cert/path_certs.go

+	if maxTTL != 0 {
+		// If ttl is 0, set the ttl as the max ttl
+		if ttl == 0 {
+			ttl = maxTTL


Don't do this; this will happen automatically in core if TTL is zero and there is a max TTL.

jefferai · 2017-12-04T17:54:02Z

builtin/credential/cert/path_certs.go

+
+	// If a period is provided, use that as the ttl instead, ignoring previously set ttl
+	if period > 0 {
+		ttl = period


Also don't do this. Set according to period at token issue or renew time. Don't modify the input parameters.

jefferai · 2017-12-04T17:59:05Z

builtin/credential/cert/path_login.go

@@ -77,16 +78,21 @@ func (b *backend) pathLogin(
 		ttl = b.System().DefaultLeaseTTL()
 	}

+	maxTTL := matched.Entry.MaxTTL
+	if maxTTL == 0 {
+		maxTTL = b.System().MaxLeaseTTL()


You don't need to do this, the max TTL at the mount/system level will be applied automatically. However, what you do need to do is check if maxTTL is zero, and if it isn't, ensure that the TTL value is capped to it.

Basically: if both are zero, leave it at zero. If both exist, ensure that the TTL is less than the MaxTTL. If only TTL exists, leave things alone, and if only max TTL exists, set TTL to the system default and then cap it to max TTL if max TTL is less.

jefferai · 2017-12-04T18:02:04Z

builtin/credential/cert/path_login.go

 	resp := &logical.Response{
 		Auth: &logical.Auth{
+			Period: matched.Entry.Period,


This actually isn't handled automatically (possibly/probably it should) -- if a period is non-zero, you should explicitly set the TTL to the period after the other TTL machinations are in place.

This is handled explicitly below now.

No longer handled explicitly at the backend level, check is now performed in core after #3677

vishalnayak · 2017-12-07T00:10:24Z

@calvn I don't have any new comments on top of Jeff's comments. This is looking good.

calvn · 2017-12-11T23:47:19Z

@jefferai this should be good for a second look

jefferai · 2017-12-18T17:30:28Z

builtin/credential/cert/path_certs.go

-		duration = b.System().DefaultLeaseTTL()
+	ttl := cert.TTL
+	if ttl == 0 {
+		ttl = b.System().DefaultLeaseTTL()


When returning the cert info we should return the actual stored info, not the effective info. Let the cert.TTL and cert.MaxTTL bits stay at zero if they're zero, because that indicates that they weren't configured.

Ok, I'd guess the same applies for max_ttl on https://github.com/hashicorp/vault/pull/3642/files/4043a5f8dda025e58ab7c399126f3a7a9b229396#diff-881681caf9da9a575287a64096baa902R146

jefferai · 2017-12-18T17:31:18Z

builtin/credential/cert/path_certs.go

+		resp.AddWarning(fmt.Sprintf("Given max_ttl of %d seconds is greater than current mount/system default of %d seconds", maxTTL/time.Second, systemMaxTTL/time.Second))
+	}
+
+	if maxTTL < time.Duration(0) {


Should probably have a similar check for ttl

Handled up top as well now

jefferai · 2017-12-18T17:31:40Z

builtin/credential/cert/path_certs.go

+	// Parse period
+	period := time.Duration(d.Get("period").(int)) * time.Second
+	if period > systemMaxTTL {
+		return logical.ErrorResponse(fmt.Sprintf("Given period of %d seconds is greater than the backend's maximum TTL of %d seconds", period/time.Second, systemMaxTTL/time.Second)), nil


This should be a warning, not an error.

jefferai · 2017-12-18T17:32:36Z

builtin/credential/cert/path_login.go

@@ -79,14 +80,14 @@ func (b *backend) pathLogin(

 	clientCerts := req.Connection.ConnState.PeerCertificates
 	if len(clientCerts) == 0 {
-		return logical.ErrorResponse("no client certificate found"), nil
+		return nil, fmt.Errorf("no client certificate found")


Why change this? This is more likely to be user error than an internal error.

I'll revert this. Changed https://github.com/hashicorp/vault/pull/3642/files#diff-0f3ca593a1923ab0963f7b2f7e165b17R154 to be logical.ErrorResponse

chrishoffman · 2017-12-18T18:53:08Z

builtin/credential/cert/path_login.go

@@ -135,7 +152,7 @@ func (b *backend) pathLoginRenew(

 		clientCerts := req.Connection.ConnState.PeerCertificates
 		if len(clientCerts) == 0 {
-			return nil, fmt.Errorf("no client certificate found")
+			return logical.ErrorResponse("no client certificate found"), nil


Does this need a typed error return, maybe logical.ErrInvalidRequest?

ErrInvalidRequest is actually the default for a logical.ErrorResponse with no typed error.

Good to know!

chrishoffman · 2017-12-18T18:58:46Z

website/source/api/auth/cert/index.html.md

+  (`3600`) or an integer time unit (`60m`) after which the issued token can no
+  longer be renewed.
+- `period` `(string: "")` - Duration in either an integer number of seconds
+  (3600) or an integer time unit (60m). If set, the generated token is a


Integer time unit is not used anywhere else in the docs and is a little confusing. I've seen a duration with hours as the largest suffix as well as some other ones. We should probably make the docs consistent for ttl, max_ttl, and period.

chrishoffman · 2017-12-18T19:00:42Z

builtin/credential/cert/path_certs.go

+	systemDefaultTTL := b.System().DefaultLeaseTTL()
+	ttl := time.Duration(d.Get("ttl").(int)) * time.Second
+	if ttl == 0 {
+		ttl = time.Second * time.Duration(d.Get("lease").(int))


minor, the previous statement is ttl * time.Second while this one is time.Second * lease.

It should definitely be ttl, nice catch!

That is for backwards compatibility with the old term for it, lease. I was just commenting on the ordering of the time math which makes it harder to scan through.

Yea you're right. I'll swap the ordering and keep lease.

chrishoffman

A few minor comments, otherwise looks good!

briankassouf

👍 , looks good!

calvn added 6 commits November 10, 2017 11:33

Initial changes for adding period to cert auth backend

9d0ef5b

Merge branch 'master-oss' into tls-auth-periodic-tokens

9e0b87a

Merge branch 'master-oss' into tls-auth-periodic-tokens

81ae8d5

Add max_ttl parameter, change ttl to fall back to DefaultLeaseTTL

d96b8ad

Add max_ttl and period to docs

3b96fde

Merge branch 'master-oss' into tls-auth-periodic-tokens

2a7b58b

calvn requested review from jefferai and vishalnayak December 3, 2017 21:25

jefferai reviewed Dec 4, 2017

View reviewed changes

calvn added 2 commits December 6, 2017 12:39

Update logic on pathCertWrite and pathLogin

19ffaf8

Fix pathLogin logic if period is set

7bc8ec5

jefferai added this to the 0.9.1 milestone Dec 14, 2017

calvn added 5 commits December 15, 2017 13:40

Merge branch 'master-oss' into tls-auth-periodic-tokens

c6e5e72

Remove explicit logic check on period value

a0816c6

Merge branch 'master-oss' into tls-auth-periodic-tokens

61c2994

Use resp.Auth.TTL instead

dc6ba94

Update maxTTL logic to on login

4043a5f

jefferai reviewed Dec 18, 2017

View reviewed changes

calvn added 2 commits December 18, 2017 12:38

Address review feedback

1d3d574

Address review feedback

0181a96

calvn and others added 3 commits December 18, 2017 13:25

Merge branch 'master-oss' into tls-auth-periodic-tokens

1542868

Merge branch 'master' into tls-auth-periodic-tokens

36d3bd9

Guard against negative value on period

e04d5ca

chrishoffman reviewed Dec 18, 2017

View reviewed changes

chrishoffman previously approved these changes Dec 18, 2017

View reviewed changes

briankassouf previously approved these changes Dec 18, 2017

View reviewed changes

Address review feedback

8620a95

calvn dismissed stale reviews from briankassouf and chrishoffman via 8620a95 December 18, 2017 19:29

jefferai approved these changes Dec 18, 2017

View reviewed changes

jefferai merged commit 40b8314 into master Dec 18, 2017

dominikmueller mentioned this pull request Jan 12, 2018

add allowed_names to cert-responses (cli and API) #3779

Merged

calvn deleted the tls-auth-periodic-tokens branch February 12, 2018 05:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add period and max_ttl to cert role creation #3642

Add period and max_ttl to cert role creation #3642

calvn commented Dec 3, 2017

jefferai Dec 4, 2017

calvn Dec 7, 2017

jefferai Dec 4, 2017

calvn Dec 7, 2017

jefferai Dec 4, 2017

jefferai Dec 4, 2017

jefferai Dec 4, 2017

jefferai Dec 4, 2017

calvn Dec 7, 2017

calvn Dec 15, 2017

vishalnayak commented Dec 7, 2017

calvn commented Dec 11, 2017

jefferai Dec 18, 2017

calvn Dec 18, 2017

jefferai Dec 18, 2017

jefferai Dec 18, 2017

calvn Dec 18, 2017

jefferai Dec 18, 2017

jefferai Dec 18, 2017

calvn Dec 18, 2017

chrishoffman Dec 18, 2017

jefferai Dec 18, 2017

chrishoffman Dec 18, 2017

chrishoffman Dec 18, 2017

chrishoffman Dec 18, 2017

calvn Dec 18, 2017

chrishoffman Dec 18, 2017

calvn Dec 18, 2017

chrishoffman left a comment

briankassouf left a comment

Add period and max_ttl to cert role creation #3642

Add period and max_ttl to cert role creation #3642

Conversation

calvn commented Dec 3, 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

vishalnayak commented Dec 7, 2017

calvn commented Dec 11, 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

chrishoffman left a comment

Choose a reason for hiding this comment

briankassouf left a comment

Choose a reason for hiding this comment