Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use TypeDurationSecond for TTL values in PKI. #3270

Merged
merged 3 commits into from
Aug 31, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions builtin/logical/pki/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ func checkCertsAndPrivateKey(keyType string, key crypto.Signer, usage x509.KeyUs
}

if math.Abs(float64(time.Now().Add(validity).Unix()-cert.NotAfter.Unix())) > 10 {
return nil, fmt.Errorf("Validity period of %d too large vs max of 10", cert.NotAfter.Unix())
return nil, fmt.Errorf("Certificate validity end: %s; expected within 10 seconds of %s", cert.NotAfter.Format(time.RFC3339), time.Now().Add(validity).Format(time.RFC3339))
}

return parsedCertBundle, nil
Expand Down Expand Up @@ -1845,6 +1845,8 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep {
addTests(nil)

roleTestStep.ErrorOk = false
roleVals.TTL = ""
roleVals.MaxTTL = "12h"
}

// Listing test
Expand Down Expand Up @@ -2126,12 +2128,31 @@ func TestBackend_SignVerbatim(t *testing.T) {
"ttl": "12h",
},
})
if resp != nil && !resp.IsError() {
t.Fatalf("sign-verbatim signed too-large-ttl'd CSR: %#v", *resp)
if err != nil {
t.Fatal(err)
}
if resp != nil && resp.IsError() {
t.Fatalf(resp.Error().Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need for double Error() here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe if I switch it to t.Fatal -- t.Fatalf expects a string, and resp.Error() returns an error

}
if resp.Data == nil || resp.Data["certificate"] == nil {
t.Fatal("did not get expected data")
}
certString := resp.Data["certificate"].(string)
block, _ := pem.Decode([]byte(certString))
if block == nil {
t.Fatal("nil pem block")
}
certs, err := x509.ParseCertificates(block.Bytes)
if err != nil {
t.Fatal(err)
}
if len(certs) != 1 {
t.Fatalf("expected a single cert, got %d", len(certs))
}
cert := certs[0]
if math.Abs(float64(time.Now().Add(12*time.Hour).Unix()-cert.NotAfter.Unix())) < 10 {
t.Fatalf("sign-verbatim did not properly cap validiaty period on signed CSR")
}

// now check that if we set generate-lease it takes it from the role and the TTLs match
roleData = map[string]interface{}{
Expand Down Expand Up @@ -2326,6 +2347,7 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {

// Direct issuing from root
_, err = client.Logical().Write("root/root/generate/internal", map[string]interface{}{
"ttl": "40h",
"common_name": "myvault.com",
"permitted_dns_domains": []string{"foobar.com", ".zipzap.com"},
})
Expand Down
4 changes: 3 additions & 1 deletion builtin/logical/pki/ca_util.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package pki

import (
"time"

"github.com/hashicorp/vault/logical"
"github.com/hashicorp/vault/logical/framework"
)
Expand All @@ -27,7 +29,7 @@ func (b *backend) getGenerationParams(
}

role = &roleEntry{
TTL: data.Get("ttl").(string),
TTL: (time.Duration(data.Get("ttl").(int)) * time.Second).String(),
KeyType: data.Get("key_type").(string),
KeyBits: data.Get("key_bits").(int),
AllowLocalhost: true,
Expand Down
42 changes: 16 additions & 26 deletions builtin/logical/pki/cert_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -728,48 +728,38 @@ func generateCreationBundle(b *backend,
}

// Get the TTL and very it against the max allowed
var ttlField string
var ttl time.Duration
var maxTTL time.Duration
var notAfter time.Time
var ttlFieldInt interface{}
{
ttlFieldInt, ok = data.GetOk("ttl")
if !ok {
ttlField = role.TTL
} else {
ttlField = ttlFieldInt.(string)
}
ttl = time.Duration(data.Get("ttl").(int)) * time.Second

if len(ttlField) == 0 {
ttl = b.System().DefaultLeaseTTL()
} else {
ttl, err = parseutil.ParseDurationSecond(ttlField)
if err != nil {
return nil, errutil.UserError{Err: fmt.Sprintf(
"invalid requested ttl: %s", err)}
if ttl == 0 {
if role.TTL != "" {
ttl, err = parseutil.ParseDurationSecond(role.TTL)
if err != nil {
return nil, errutil.UserError{Err: fmt.Sprintf(
"invalid requested ttl: %s", err)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be a requested ttl. Its coming from role. Can we change the error message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah

}
}
}

if len(role.MaxTTL) == 0 {
maxTTL = b.System().MaxLeaseTTL()
} else {
if role.MaxTTL != "" {
maxTTL, err = parseutil.ParseDurationSecond(role.MaxTTL)
if err != nil {
return nil, errutil.UserError{Err: fmt.Sprintf(
"invalid ttl: %s", err)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_ttl

}
}

if ttl == 0 {
ttl = b.System().DefaultLeaseTTL()
}
if maxTTL == 0 {
maxTTL = b.System().MaxLeaseTTL()
}
if ttl > maxTTL {
// Don't error if they were using system defaults, only error if
// they specifically chose a bad TTL
if len(ttlField) == 0 {
ttl = maxTTL
} else {
return nil, errutil.UserError{Err: fmt.Sprintf(
"ttl is larger than maximum allowed (%d)", maxTTL/time.Second)}
}
ttl = maxTTL
}

notAfter = time.Now().Add(ttl)
Expand Down
4 changes: 2 additions & 2 deletions builtin/logical/pki/fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ email addresses.`,
}

fields["ttl"] = &framework.FieldSchema{
Type: framework.TypeString,
Type: framework.TypeDurationSecond,
Description: `The requested Time To Live for the certificate;
sets the expiration date. If not specified
the role default, backend default, or system
Expand Down Expand Up @@ -92,7 +92,7 @@ must still be specified in alt_names or ip_sans.`,
}

fields["ttl"] = &framework.FieldSchema{
Type: framework.TypeString,
Type: framework.TypeDurationSecond,
Description: `The requested Time To Live for the certificate;
sets the expiration date. If not specified
the role default, backend default, or system
Expand Down
4 changes: 2 additions & 2 deletions builtin/logical/pki/path_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func pathRoles(b *backend) *framework.Path {
},

"ttl": &framework.FieldSchema{
Type: framework.TypeString,
Type: framework.TypeDurationSecond,
Default: "",
Description: `The lease duration if no specific lease duration is
requested. The lease duration controls the expiration
Expand Down Expand Up @@ -383,7 +383,7 @@ func (b *backend) pathRoleCreate(

entry := &roleEntry{
MaxTTL: data.Get("max_ttl").(string),
TTL: data.Get("ttl").(string),
TTL: (time.Duration(data.Get("ttl").(int)) * time.Second).String(),
AllowLocalhost: data.Get("allow_localhost").(bool),
AllowedDomains: data.Get("allowed_domains").(string),
AllowBareDomains: data.Get("allow_bare_domains").(bool),
Expand Down
45 changes: 43 additions & 2 deletions builtin/logical/pki/path_root.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package pki
import (
"encoding/base64"
"fmt"
"time"

"github.com/hashicorp/vault/helper/errutil"
"github.com/hashicorp/vault/logical"
Expand Down Expand Up @@ -81,6 +82,34 @@ the non-repudiation flag.`,
return ret
}

/*
func pathSignSelfIssued(b *backend) *framework.Path {
ret := &framework.Path{
Pattern: "root/sign-self-issued",

Callbacks: map[logical.Operation]framework.OperationFunc{
logical.UpdateOperation: b.pathCASignIntermediate,
},

HelpSynopsis: pathSignSelfIssuedHelpSyn,
HelpDescription: pathSignSelfIssuedHelpDesc,
}

ret.Fields["certificate"] = &framework.FieldSchema{
Type: framework.TypeString,
Default: "",
Description: `PEM-format self-issued certificate to be signed.`,
}

ret.Fields["ttl"] = &framework.FieldSchema{
Type: framework.TypeDurationSecond,
Description: `Time-to-live for the signed certificate. This is not bounded by the lifetime of this root CA.`,
}

return ret
}
*/

func (b *backend) pathCADeleteRoot(
req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
return nil, req.Storage.Delete("config/ca_bundle")
Expand Down Expand Up @@ -214,7 +243,7 @@ func (b *backend) pathCASignIntermediate(
}

role := &roleEntry{
TTL: data.Get("ttl").(string),
TTL: (time.Duration(data.Get("ttl").(int)) * time.Second).String(),
AllowLocalhost: true,
AllowAnyName: true,
AllowIPSANs: true,
Expand Down Expand Up @@ -340,5 +369,17 @@ Issue an intermediate CA certificate based on the provided CSR.
`

const pathSignIntermediateHelpDesc = `
See the API documentation for more information.
see the API documentation for more information.
`

/*
const pathSignSelfIssuedHelpSyn = `
Signs another CA's self-issued certificate.
`

const pathSignSelfIssuedHelpDesc = `
Signs another CA's self-issued certificate. This is most often used for rolling roots; unless you know you need this you probably want to use sign-intermediate instead.

Note that this is a very "god-mode" operation and should be extremely restricted in terms of who is allowed to use it. All values will be taken directly from the incoming certificate and no verification of host names, path lengths, or any other values will be performed.
`
*/
3 changes: 3 additions & 0 deletions helper/parseutil/parseutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ func ParseDurationSecond(in interface{}) (time.Duration, error) {
switch in.(type) {
case string:
inp := in.(string)
if inp == "" {
return time.Duration(0), nil
}
var err error
// Look for a suffix otherwise its a plain second value
if strings.HasSuffix(inp, "s") || strings.HasSuffix(inp, "m") || strings.HasSuffix(inp, "h") {
Expand Down