Update PKI to natively use time.Duration (#4493)

* Update PKI to natively use time.Duration

Among other things this now means PKI will output durations in seconds
like other backends, instead of as Go strings.

* Add a warning when refusing to blow away an existing root instead of just returning success

* Fix another issue found while debugging this...

The reason it wasn't caught on tests in the first place is that the ttl
and max ttl were only being compared if in addition to a provided csr, a
role was also provided. This was because the check was in the role !=
nil block instead of outside of it. This has been fixed, which made the
problem occur in all sign-verbatim cases and the changes in this PR have
now verified the fix.

builtin/logical/pki/backend_test.go

@@ -1487,7 +1487,7 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int
 // Generates steps to test out various role permutations
 func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep {
 	roleVals := roleEntry{
-		MaxTTL:    "12h",
+		MaxTTL:    12 * time.Hour,
 		KeyType:   "rsa",
 		KeyBits:   2048,
 		RequireCN: true,
@@ -1866,7 +1866,7 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep {
 
 			issueTestStep.ErrorOk = !allowed
 
-			validity, _ := time.ParseDuration(roleVals.MaxTTL)
+			validity := roleVals.MaxTTL
 
 			var testBitSize int
 
@@ -2088,16 +2088,16 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep {
 	{
 		roleTestStep.ErrorOk = true
 		roleVals.Lease = ""
-		roleVals.MaxTTL = ""
+		roleVals.MaxTTL = 0
 		addTests(nil)
 
 		roleVals.Lease = "12h"
-		roleVals.MaxTTL = "6h"
+		roleVals.MaxTTL = 6 * time.Hour
 		addTests(nil)
 
 		roleTestStep.ErrorOk = false
 		roleVals.TTL = ""
-		roleVals.MaxTTL = "12h"
+		roleVals.MaxTTL = 12 * time.Hour
 	}
 
 	// Listing test

builtin/logical/pki/ca_util.go

@@ -29,7 +29,7 @@ func (b *backend) getGenerationParams(
 	}
 
 	role = &roleEntry{
-		TTL:              (time.Duration(data.Get("ttl").(int)) * time.Second).String(),
+		TTL:              time.Duration(data.Get("ttl").(int)) * time.Second,
 		KeyType:          data.Get("key_type").(string),
 		KeyBits:          data.Get("key_bits").(int),
 		AllowLocalhost:   true,

builtin/logical/pki/cert_util.go

@@ -23,7 +23,6 @@ import (
 	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/vault/helper/certutil"
 	"github.com/hashicorp/vault/helper/errutil"
-	"github.com/hashicorp/vault/helper/parseutil"
 	"github.com/hashicorp/vault/helper/strutil"
 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
@@ -860,24 +859,12 @@ func generateCreationBundle(b *backend, data *dataBundle) error {
 	{
 		ttl = time.Duration(data.apiData.Get("ttl").(int)) * time.Second
 
-		if ttl == 0 {
-			roleTTL, err := parseutil.ParseDurationSecond(data.role.TTL)
-			if err != nil {
-				return errutil.UserError{Err: fmt.Sprintf(
-					"invalid role ttl: %s", err)}
-			}
-			if roleTTL != 0 {
-				ttl = roleTTL
-			}
+		if ttl == 0 && data.role.TTL > 0 {
+			ttl = data.role.TTL
 		}
 
-		roleMaxTTL, err := parseutil.ParseDurationSecond(data.role.MaxTTL)
-		if err != nil {
-			return errutil.UserError{Err: fmt.Sprintf(
-				"invalid role max_ttl: %s", err)}
-		}
-		if roleMaxTTL != 0 {
-			maxTTL = roleMaxTTL
+		if data.role.MaxTTL > 0 {
+			maxTTL = data.role.MaxTTL
 		}
 
 		if ttl == 0 {

builtin/logical/pki/fields.go

@@ -81,7 +81,7 @@ email addresses.`,
 sets the expiration date. If not specified
 the role default, backend default, or system
 default TTL is used, in that order. Cannot
-be later than the role max TTL.`,
+be larger than the role max TTL.`,
 	}
 
 	return fields

builtin/logical/pki/path_issue_sign.go

@@ -9,7 +9,6 @@ import (
 	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/vault/helper/certutil"
 	"github.com/hashicorp/vault/helper/errutil"
-	"github.com/hashicorp/vault/helper/parseutil"
 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
 )
@@ -125,8 +124,8 @@ func (b *backend) pathSignVerbatim(ctx context.Context, req *logical.Request, da
 	}
 
 	entry := &roleEntry{
-		TTL:              b.System().DefaultLeaseTTL().String(),
-		MaxTTL:           b.System().MaxLeaseTTL().String(),
+		TTL:              b.System().DefaultLeaseTTL(),
+		MaxTTL:           b.System().MaxLeaseTTL(),
 		AllowLocalhost:   true,
 		AllowAnyName:     true,
 		AllowIPSANs:      true,
@@ -137,34 +136,23 @@ func (b *backend) pathSignVerbatim(ctx context.Context, req *logical.Request, da
 		GenerateLease:    new(bool),
 	}
 
+	*entry.GenerateLease = false
+
 	if role != nil {
-		if role.TTL != "" {
-			ttl, err := parseutil.ParseDurationSecond(role.TTL)
-			if err != nil {
-				return logical.ErrorResponse(fmt.Sprintf("could not parse role ttl: %s", err)), nil
-			}
-			if ttl != 0 {
-				entry.TTL = role.TTL
-			}
+		if role.TTL > 0 {
+			entry.TTL = role.TTL
 		}
-		if role.MaxTTL != "" {
-			ttl, err := parseutil.ParseDurationSecond(role.MaxTTL)
-			if err != nil {
-				return logical.ErrorResponse(fmt.Sprintf("could not parse role max ttl: %s", err)), nil
-			}
-			if ttl != 0 {
-				entry.MaxTTL = role.MaxTTL
-			}
+		if role.MaxTTL > 0 {
+			entry.MaxTTL = role.MaxTTL
 		}
-		if entry.TTL > entry.MaxTTL {
-			return logical.ErrorResponse(fmt.Sprintf("requested ttl of %s is greater than max ttl of %s", entry.TTL, entry.MaxTTL)), nil
+		if role.GenerateLease != nil {
+			*entry.GenerateLease = *role.GenerateLease
 		}
 		entry.NoStore = role.NoStore
 	}
 
-	*entry.GenerateLease = false
-	if role != nil && role.GenerateLease != nil {
-		*entry.GenerateLease = *role.GenerateLease
+	if entry.MaxTTL > 0 && entry.TTL > entry.MaxTTL {
+		return logical.ErrorResponse(fmt.Sprintf("requested ttl of %s is greater than max ttl of %s", entry.TTL, entry.MaxTTL)), nil
 	}
 
 	return b.pathIssueSignCert(ctx, req, data, entry, true, true)

builtin/logical/pki/path_roles.go

@@ -289,16 +289,34 @@ func (b *backend) getRole(ctx context.Context, s logical.Storage, n string) (*ro
 
 	// Migrate existing saved entries and save back if changed
 	modified := false
-	if len(result.TTL) == 0 && len(result.Lease) != 0 {
-		result.TTL = result.Lease
+	if len(result.DeprecatedTTL) == 0 && len(result.Lease) != 0 {
+		result.DeprecatedTTL = result.Lease
 		result.Lease = ""
 		modified = true
 	}
-	if len(result.MaxTTL) == 0 && len(result.LeaseMax) != 0 {
-		result.MaxTTL = result.LeaseMax
+	if result.TTL == 0 && len(result.DeprecatedTTL) != 0 {
+		parsed, err := parseutil.ParseDurationSecond(result.DeprecatedTTL)
+		if err != nil {
+			return nil, err
+		}
+		result.TTL = parsed
+		result.DeprecatedTTL = ""
+		modified = true
+	}
+	if len(result.DeprecatedMaxTTL) == 0 && len(result.LeaseMax) != 0 {
+		result.DeprecatedMaxTTL = result.LeaseMax
 		result.LeaseMax = ""
 		modified = true
 	}
+	if result.MaxTTL == 0 && len(result.DeprecatedMaxTTL) != 0 {
+		parsed, err := parseutil.ParseDurationSecond(result.DeprecatedMaxTTL)
+		if err != nil {
+			return nil, err
+		}
+		result.MaxTTL = parsed
+		result.DeprecatedMaxTTL = ""
+		modified = true
+	}
 	if result.AllowBaseDomain {
 		result.AllowBaseDomain = false
 		result.AllowBareDomains = true
@@ -394,19 +412,6 @@ func (b *backend) pathRoleRead(ctx context.Context, req *logical.Request, data *
 		return nil, nil
 	}
 
-	hasMax := true
-	if len(role.MaxTTL) == 0 {
-		role.MaxTTL = "(system default)"
-		hasMax = false
-	}
-	if len(role.TTL) == 0 {
-		if hasMax {
-			role.TTL = "(system default, capped to role max)"
-		} else {
-			role.TTL = "(system default)"
-		}
-	}
-
 	resp := &logical.Response{
 		Data: role.ToResponseData(),
 	}
@@ -427,8 +432,8 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data
 	name := data.Get("name").(string)
 
 	entry := &roleEntry{
-		MaxTTL:                        (time.Duration(data.Get("max_ttl").(int)) * time.Second).String(),
-		TTL:                           (time.Duration(data.Get("ttl").(int)) * time.Second).String(),
+		MaxTTL:                        time.Duration(data.Get("max_ttl").(int)) * time.Second,
+		TTL:                           time.Duration(data.Get("ttl").(int)) * time.Second,
 		AllowLocalhost:                data.Get("allow_localhost").(bool),
 		AllowedDomains:                data.Get("allowed_domains").([]string),
 		AllowBareDomains:              data.Get("allow_bare_domains").(bool),
@@ -480,17 +485,7 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data
 		return logical.ErrorResponse("RSA keys < 2048 bits are unsafe and not supported"), nil
 	}
 
-	maxTTL, err := parseutil.ParseDurationSecond(entry.MaxTTL)
-	if err != nil {
-		return logical.ErrorResponse(fmt.Sprintf(
-			"Invalid max ttl: %s", err)), nil
-	}
-	ttl, err := parseutil.ParseDurationSecond(entry.TTL)
-	if err != nil {
-		return logical.ErrorResponse(fmt.Sprintf(
-			"Invalid ttl: %s", err)), nil
-	}
-	if maxTTL != 0 && ttl > maxTTL {
+	if entry.MaxTTL > 0 && entry.TTL > entry.MaxTTL {
 		return logical.ErrorResponse(
 			`"ttl" value must be less than "max_ttl" value`,
 		), nil
@@ -550,48 +545,50 @@ func parseKeyUsages(input []string) int {
 }
 
 type roleEntry struct {
-	LeaseMax                      string   `json:"lease_max"`
-	Lease                         string   `json:"lease"`
-	MaxTTL                        string   `json:"max_ttl" mapstructure:"max_ttl"`
-	TTL                           string   `json:"ttl" mapstructure:"ttl"`
-	AllowLocalhost                bool     `json:"allow_localhost" mapstructure:"allow_localhost"`
-	AllowedBaseDomain             string   `json:"allowed_base_domain" mapstructure:"allowed_base_domain"`
-	AllowedDomainsOld             string   `json:"allowed_domains,omit_empty"`
-	AllowedDomains                []string `json:"allowed_domains_list" mapstructure:"allowed_domains"`
-	AllowBaseDomain               bool     `json:"allow_base_domain"`
-	AllowBareDomains              bool     `json:"allow_bare_domains" mapstructure:"allow_bare_domains"`
-	AllowTokenDisplayName         bool     `json:"allow_token_displayname" mapstructure:"allow_token_displayname"`
-	AllowSubdomains               bool     `json:"allow_subdomains" mapstructure:"allow_subdomains"`
-	AllowGlobDomains              bool     `json:"allow_glob_domains" mapstructure:"allow_glob_domains"`
-	AllowAnyName                  bool     `json:"allow_any_name" mapstructure:"allow_any_name"`
-	EnforceHostnames              bool     `json:"enforce_hostnames" mapstructure:"enforce_hostnames"`
-	AllowIPSANs                   bool     `json:"allow_ip_sans" mapstructure:"allow_ip_sans"`
-	ServerFlag                    bool     `json:"server_flag" mapstructure:"server_flag"`
-	ClientFlag                    bool     `json:"client_flag" mapstructure:"client_flag"`
-	CodeSigningFlag               bool     `json:"code_signing_flag" mapstructure:"code_signing_flag"`
-	EmailProtectionFlag           bool     `json:"email_protection_flag" mapstructure:"email_protection_flag"`
-	UseCSRCommonName              bool     `json:"use_csr_common_name" mapstructure:"use_csr_common_name"`
-	UseCSRSANs                    bool     `json:"use_csr_sans" mapstructure:"use_csr_sans"`
-	KeyType                       string   `json:"key_type" mapstructure:"key_type"`
-	KeyBits                       int      `json:"key_bits" mapstructure:"key_bits"`
-	MaxPathLength                 *int     `json:",omitempty" mapstructure:"max_path_length"`
-	KeyUsageOld                   string   `json:"key_usage,omitempty"`
-	KeyUsage                      []string `json:"key_usage_list" mapstructure:"key_usage"`
-	OUOld                         string   `json:"ou,omitempty"`
-	OU                            []string `json:"ou_list" mapstructure:"ou"`
-	OrganizationOld               string   `json:"organization,omitempty"`
-	Organization                  []string `json:"organization_list" mapstructure:"organization"`
-	Country                       []string `json:"country" mapstructure:"country"`
-	Locality                      []string `json:"locality" mapstructure:"locality"`
-	Province                      []string `json:"province" mapstructure:"province"`
-	StreetAddress                 []string `json:"street_address" mapstructure:"street_address"`
-	PostalCode                    []string `json:"postal_code" mapstructure:"postal_code"`
-	GenerateLease                 *bool    `json:"generate_lease,omitempty"`
-	NoStore                       bool     `json:"no_store" mapstructure:"no_store"`
-	RequireCN                     bool     `json:"require_cn" mapstructure:"require_cn"`
-	AllowedOtherSANs              []string `json:"allowed_other_sans" mapstructure:"allowed_other_sans"`
-	PolicyIdentifiers             []string `json:"policy_identifiers" mapstructure:"policy_identifiers"`
-	BasicConstraintsValidForNonCA bool     `json:"basic_constraints_valid_for_non_ca" mapstructure:"basic_constraints_valid_for_non_ca"`
+	LeaseMax                      string        `json:"lease_max"`
+	Lease                         string        `json:"lease"`
+	DeprecatedMaxTTL              string        `json:"max_ttl" mapstructure:"max_ttl"`
+	DeprecatedTTL                 string        `json:"ttl" mapstructure:"ttl"`
+	TTL                           time.Duration `json:"ttl_duration" mapstructure:"ttl_duration"`
+	MaxTTL                        time.Duration `json:"max_ttl_duration" mapstructure:"max_ttl_duration"`
+	AllowLocalhost                bool          `json:"allow_localhost" mapstructure:"allow_localhost"`
+	AllowedBaseDomain             string        `json:"allowed_base_domain" mapstructure:"allowed_base_domain"`
+	AllowedDomainsOld             string        `json:"allowed_domains,omit_empty"`
+	AllowedDomains                []string      `json:"allowed_domains_list" mapstructure:"allowed_domains"`
+	AllowBaseDomain               bool          `json:"allow_base_domain"`
+	AllowBareDomains              bool          `json:"allow_bare_domains" mapstructure:"allow_bare_domains"`
+	AllowTokenDisplayName         bool          `json:"allow_token_displayname" mapstructure:"allow_token_displayname"`
+	AllowSubdomains               bool          `json:"allow_subdomains" mapstructure:"allow_subdomains"`
+	AllowGlobDomains              bool          `json:"allow_glob_domains" mapstructure:"allow_glob_domains"`
+	AllowAnyName                  bool          `json:"allow_any_name" mapstructure:"allow_any_name"`
+	EnforceHostnames              bool          `json:"enforce_hostnames" mapstructure:"enforce_hostnames"`
+	AllowIPSANs                   bool          `json:"allow_ip_sans" mapstructure:"allow_ip_sans"`
+	ServerFlag                    bool          `json:"server_flag" mapstructure:"server_flag"`
+	ClientFlag                    bool          `json:"client_flag" mapstructure:"client_flag"`
+	CodeSigningFlag               bool          `json:"code_signing_flag" mapstructure:"code_signing_flag"`
+	EmailProtectionFlag           bool          `json:"email_protection_flag" mapstructure:"email_protection_flag"`
+	UseCSRCommonName              bool          `json:"use_csr_common_name" mapstructure:"use_csr_common_name"`
+	UseCSRSANs                    bool          `json:"use_csr_sans" mapstructure:"use_csr_sans"`
+	KeyType                       string        `json:"key_type" mapstructure:"key_type"`
+	KeyBits                       int           `json:"key_bits" mapstructure:"key_bits"`
+	MaxPathLength                 *int          `json:",omitempty" mapstructure:"max_path_length"`
+	KeyUsageOld                   string        `json:"key_usage,omitempty"`
+	KeyUsage                      []string      `json:"key_usage_list" mapstructure:"key_usage"`
+	OUOld                         string        `json:"ou,omitempty"`
+	OU                            []string      `json:"ou_list" mapstructure:"ou"`
+	OrganizationOld               string        `json:"organization,omitempty"`
+	Organization                  []string      `json:"organization_list" mapstructure:"organization"`
+	Country                       []string      `json:"country" mapstructure:"country"`
+	Locality                      []string      `json:"locality" mapstructure:"locality"`
+	Province                      []string      `json:"province" mapstructure:"province"`
+	StreetAddress                 []string      `json:"street_address" mapstructure:"street_address"`
+	PostalCode                    []string      `json:"postal_code" mapstructure:"postal_code"`
+	GenerateLease                 *bool         `json:"generate_lease,omitempty"`
+	NoStore                       bool          `json:"no_store" mapstructure:"no_store"`
+	RequireCN                     bool          `json:"require_cn" mapstructure:"require_cn"`
+	AllowedOtherSANs              []string      `json:"allowed_other_sans" mapstructure:"allowed_other_sans"`
+	PolicyIdentifiers             []string      `json:"policy_identifiers" mapstructure:"policy_identifiers"`
+	BasicConstraintsValidForNonCA bool          `json:"basic_constraints_valid_for_non_ca" mapstructure:"basic_constraints_valid_for_non_ca"`
 
 	// Used internally for signing intermediates
 	AllowExpirationPastCA bool
@@ -599,8 +596,8 @@ type roleEntry struct {
 
 func (r *roleEntry) ToResponseData() map[string]interface{} {
 	responseData := map[string]interface{}{
-		"ttl":                                r.TTL,
-		"max_ttl":                            r.MaxTTL,
+		"ttl":                                int64(r.TTL.Seconds()),
+		"max_ttl":                            int64(r.MaxTTL.Seconds()),
 		"allow_localhost":                    r.AllowLocalhost,
 		"allowed_domains":                    r.AllowedDomains,
 		"allow_bare_domains":                 r.AllowBareDomains,

builtin/logical/pki/path_roles_test.go

@@ -3,6 +3,7 @@ package pki
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/vault/helper/strutil"
 	"github.com/hashicorp/vault/logical"
@@ -63,6 +64,11 @@ func TestPki_RoleGenerateLease(t *testing.T) {
 		t.Fatalf("generate_lease should not be set by default")
 	}
 
+	// Update values due to switching of ttl type
+	resp.Data["ttl_duration"] = resp.Data["ttl"]
+	resp.Data["ttl"] = (time.Duration(resp.Data["ttl"].(int64)) * time.Second).String()
+	resp.Data["max_ttl_duration"] = resp.Data["max_ttl"]
+	resp.Data["max_ttl"] = (time.Duration(resp.Data["max_ttl"].(int64)) * time.Second).String()
 	// role.GenerateLease will be nil after the decode
 	var role roleEntry
 	err = mapstructure.Decode(resp.Data, &role)
@@ -156,6 +162,11 @@ func TestPki_RoleKeyUsage(t *testing.T) {
 		t.Fatalf("key_usage should have 2 values")
 	}
 
+	// Update values due to switching of ttl type
+	resp.Data["ttl_duration"] = resp.Data["ttl"]
+	resp.Data["ttl"] = (time.Duration(resp.Data["ttl"].(int64)) * time.Second).String()
+	resp.Data["max_ttl_duration"] = resp.Data["max_ttl"]
+	resp.Data["max_ttl"] = (time.Duration(resp.Data["max_ttl"].(int64)) * time.Second).String()
 	// Check that old key usage value is nil
 	var role roleEntry
 	err = mapstructure.Decode(resp.Data, &role)
@@ -249,6 +260,11 @@ func TestPki_RoleOUOrganizationUpgrade(t *testing.T) {
 		t.Fatalf("organization should have 2 values")
 	}
 
+	// Update values due to switching of ttl type
+	resp.Data["ttl_duration"] = resp.Data["ttl"]
+	resp.Data["ttl"] = (time.Duration(resp.Data["ttl"].(int64)) * time.Second).String()
+	resp.Data["max_ttl_duration"] = resp.Data["max_ttl"]
+	resp.Data["max_ttl"] = (time.Duration(resp.Data["max_ttl"].(int64)) * time.Second).String()
 	// Check that old key usage value is nil
 	var role roleEntry
 	err = mapstructure.Decode(resp.Data, &role)
@@ -351,6 +367,11 @@ func TestPki_RoleAllowedDomains(t *testing.T) {
 		t.Fatalf("allowed_domains should have 2 values")
 	}
 
+	// Update values due to switching of ttl type
+	resp.Data["ttl_duration"] = resp.Data["ttl"]
+	resp.Data["ttl"] = (time.Duration(resp.Data["ttl"].(int64)) * time.Second).String()
+	resp.Data["max_ttl_duration"] = resp.Data["max_ttl"]
+	resp.Data["max_ttl"] = (time.Duration(resp.Data["max_ttl"].(int64)) * time.Second).String()
 	// Check that old key usage value is nil
 	var role roleEntry
 	err = mapstructure.Decode(resp.Data, &role)

builtin/logical/pki/path_root.go

@@ -123,7 +123,9 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request,
 		return nil, err
 	}
 	if entry != nil {
-		return nil, nil
+		resp := &logical.Response{}
+		resp.AddWarning(fmt.Sprintf("Refusing to generate a root certificate over an existing root certificate. If you really want to destroy the original root certificate, please issue a delete against %sroot.", req.MountPoint))
+		return resp, nil
 	}
 
 	exported, format, role, errorResp := b.getGenerationParams(data)
@@ -260,7 +262,7 @@ func (b *backend) pathCASignIntermediate(ctx context.Context, req *logical.Reque
 		Province:              data.Get("province").([]string),
 		StreetAddress:         data.Get("street_address").([]string),
 		PostalCode:            data.Get("postal_code").([]string),
-		TTL:                   (time.Duration(data.Get("ttl").(int)) * time.Second).String(),
+		TTL:                   time.Duration(data.Get("ttl").(int)) * time.Second,
 		AllowLocalhost:        true,
 		AllowAnyName:          true,
 		AllowIPSANs:           true,