Megan/expiration #495

meganm53 · 2021-06-21T22:28:15Z

Fixes issue #398 , added flag that allows you to check how much longer until a certificate is about to expire, along with the color coding the outputs based on if it is about to expire/expired (red), close (yellow), or still has under 66% of its life used (green). added to description how to use this function.

CLAassistant · 2021-06-21T22:28:20Z

All committers have signed the CLA.

dopey · 2021-06-21T22:45:28Z

command/certificate/certificate.go

@@ -14,7 +14,7 @@ func init() {
 		Description: `**step certificate** command group provides facilities for creating
 certificate signing requests (CSRs), creating self-signed certificates
 (e.g., for use as a root certificate authority), generating leaf or
-intermediate CA certificate by signing a CSR, validating certificates,
+intermediate CA certificate by signing a CSR, validating certificates, check expiration of certificate,


checking certificate expiration

The tense on the other verbs all seem to end with ing, so we should be consistent.

codecov · 2021-06-21T22:49:08Z

Codecov Report

Merging #495 (1187e48) into master (0da3b28) will decrease coverage by 0.16%.
The diff coverage is 26.66%.

@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
- Coverage   63.29%   63.12%   -0.17%     
==========================================
  Files          57       57              
  Lines        6865     6894      +29     
==========================================
+ Hits         4345     4352       +7     
- Misses       2281     2303      +22     
  Partials      239      239

Impacted Files	Coverage Δ
command/certificate/verify.go	`44.60% <24.13%> (-5.40%)`	⬇️
command/certificate/certificate.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0da3b28...1187e48. Read the comment docs.

dopey · 2021-06-21T22:51:59Z

command/certificate/verify.go

+		} else if percentIntoLifeTime < 1 {
+			fmt.Println("\033[32m", "Leaf is less than 1% through its lifetime.", "\033[0m")
+		} else {
+			fmt.Println("Error")


return an errors.Errorf error here that described the issue. See errors returned in above code for examples.

…nto megan/expiration

dopey · 2021-06-21T23:08:30Z

command/certificate/verify.go

+		percentIntoLifeTime := ((totalLifeTimeOfCert.Hours() - NowTillEndOfCert.Hours()) / totalLifeTimeOfCert.Hours()) * 100
+
+		if percentIntoLifeTime >= 100 {
+			fmt.Println("\033[31m", "This certificate has already expired.", "\033[0m") //"\033[__m" are color codes


Just gonna list some bullet points here for improvements to this section:

if percent >= 100 { } else if percent >= 90 { } else if percent >= 66 { } else if percent >= 1 { } else if precent >= 0 { } else { }

Use fmt.Printf instead of fmt.Println if possible.

parameterize commonly used strings and colors so you can reuse them. e.g var colorExpired = "31m".

dopey · 2021-06-21T23:09:20Z

command/certificate/verify.go

@@ -164,6 +177,30 @@ func verifyAction(ctx *cli.Context) error {
 		}
 	}

+	if expire {
+
+		NowTillEndOfCert := time.Until(cert.NotAfter)


variable names with capital first letters are exported. Use a lower case first letter unless you need to export the variable beyond the current package.

use remainingValidity and totalValidity. The term validity window is already associated with certificates and we already frequently use the work validity in our code when doing similar calculations.

dopey · 2021-06-21T23:11:38Z

command/certificate/verify.go

+		NowTillEndOfCert := time.Until(cert.NotAfter)
+		totalLifeTimeOfCert := cert.NotAfter.Sub(cert.NotBefore)
+
+		percentIntoLifeTime := ((totalLifeTimeOfCert.Hours() - NowTillEndOfCert.Hours()) / totalLifeTimeOfCert.Hours()) * 100


Why are we specifying .Hours() here rather than using the complete duration?

You can just call this variable percent.

Generally speaking, the rule for variable names is that the length of the name should be proportionate to how far away from the instantiation of the variable it is being used. So if it's super far away, e.g. different functions, files or packages, then you want a really descriptive name like you've chosen here. But in this case, the variable and it's use are super close. So we can opt for shorter, less descriptive, more context dependent names. Lmk if that does/doesn't make sense.

dopey · 2021-06-21T23:20:16Z

command/certificate/verify.go

+	if expire {
+
+		NowTillEndOfCert := time.Until(cert.NotAfter)
+		totalLifeTimeOfCert := cert.NotAfter.Sub(cert.NotBefore)


I would check first if time.Now().Before(cert.NotBefore) because if so, then the cert is not valid yet, and that's a different message than any of the ones you have below. At the same time I would check if time.Now().After(cert.NotAfter) just to rule out both of those special cases. Then we know that the results should be within our bounds.

I would also do (1 - remainingValidity/totalValidity) * 100 because it's a bit more natural.

dopey · 2021-06-21T23:27:46Z

command/certificate/verify.go

@@ -65,12 +68,22 @@ Verify a certificate using a custom directory of root certificates for path vali
 '''
 $ step certificate verify ./certificate.crt --roots ./root-certificates/
 '''
+
+Verify the expiration time left of a certificate using a custom root certificate and host for path validation:


Verify the remaining validity of a certificate ...

dopey · 2021-06-21T23:28:28Z

command/certificate/verify.go

 `,
 		Flags: []cli.Flag{
 			cli.StringFlag{
 				Name:  "host",
 				Usage: `Check whether the certificate is for the specified host.`,
 			},
+			cli.BoolFlag{
+				Name:  "expire",
+				Usage: `Checks the certificate time till expiration`,


Checks the remaining certificate validity till expiration

dopey · 2021-06-22T05:18:01Z

command/certificate/verify.go

+
+		return nil
+	}
+


I think we should verify the certificate first cert.VerifyOpts below, and then if it's valid we can analyze how much of the lifespan has been used. The benefit of this is that we won't have to check if now < cert.NotBefore or if the cert is already expired. The VerifyOpts method already does that for us. So only if the certificate is valid do we print expiry information.

dopey · 2021-06-22T05:20:20Z

command/certificate/verify.go

+		if percentUsed >= 100 {
+			fmt.Println(red, "This certificate has already expired.", reset)
+		} else if percentUsed > 90 {
+			fmt.Printf(red,"Leaf is", int(percentUsed), "% through its lifetime.", reset)


When you use fmt.Printf you can do things like: fmt.Printf("%sLeaf is %d% through it's lifetime.", red, percentUsed). The variables at the end replace the %s and %d.

dopey · 2021-06-22T05:21:14Z

command/certificate/verify.go

+		reset := "\033[0m"
+
+		if percentUsed >= 100 {
+			fmt.Println(red, "This certificate has already expired.", reset)


We're using the word certificate in this output, but the word leaf in future outputs to refer to the same thing. I think we should consistently use the term certificate.

dopey · 2021-06-22T05:22:51Z

command/certificate/verify.go

+		reset := "\033[0m"
+
+		if percentUsed >= 100 {
+			fmt.Println(red, "This certificate has already expired.", reset)


@tashian How would you like this output to look? Your plan was to use this as input from a pipe. I recall you saying that you wanted these to be errorCodes rather than printed statements.

Yes, the exit codes are described in #398

dopey · 2021-06-22T05:24:47Z

command/certificate/verify.go

+Verify the remaining validity of a certificate using a custom root certificate and host for path validation:
+
+'''
+$ step certificate verify ./certificate.crt --host smallstep.com --expire


@mmalone @tashian what did we want this flag to be called? expire, validity, verdancy?

tashian · 2021-06-22T19:47:38Z

hi @cookie-annihilator @tommy-56 I'm excited to see this PR!
I want to explain why --verdancy is the word we landed on for this flag during design discussions.
The idea of verdancy is, it's a matter of degree: What the command is asking is "Just how green is this certificate?"
And, like a leaf on a tree, as the certificate ages it changes color.
Whereas, --expire implies a yes/no answer: Is it expired or not?
Verdancy provides that answer and more.

The use cases are:

Verdancy aids automated renewal, and once this feature is released I will immediately put it into use in our docs. We recommend people renew certificates when they are around 2/3 through their validity period, so we want an easy way to confirm that.
Expired (yes/no) could also be used in a script, if there is a chance a certificate may have expired. Because with an unexpired cert you can run step ca renew to get a new certificate, but with an expired cert you'd need to run step ca certificate.
We also discussed an expires-in flag that would allow you to pass a % or duration, and you'd get back a yes/no.

Happy to answer any other questions y'all have about this feature...

dopey · 2021-07-02T21:11:38Z

command/certificate/needsRenewal.go

+		Action:    cli.ActionFunc(needsRenewalAction),
+		Usage:     `Check if a certificate needs to be renewed`,
+		UsageText: `**step certificate needs-renewal** <crt_file or host_name> [**--expires-in <duration>]`,
+		Description: `**step certificate needs-renewal** Checks certificate expiration from file or from a host if 


step certificate needs-renewal returns '0' if the certificate needs to be renewed based on it's remaining lifetime. Returns '1' if the certificate is within it's validity lifetime bounds and does not need to be renewed. Returns '255' for any other error. By default, if a certificate "needs renewal" when it has passed 66% of it's allotted lifetime. This threshold can be adjusted using the '--expires-in' flag.

dopey · 2021-07-02T21:14:52Z

command/certificate/needsRenewal.go

+		this command will produce a non-zero return value.
+## POSITIONAL ARGUMENTS
+
+<crt_file>


Combine the positional arguments into one, as we do with the UsageText above.

<cert_file or hostname> The path to a certificate to validate OR a hostname with protocol prefix.

dopey · 2021-07-02T21:15:57Z

command/certificate/needsRenewal.go

+
+## EXIT CODES
+
+This command returns 0 if needing renewal, or returns 1 if the certificate does not need renewal. It will return 255 if any errors occurred


This command returns '0' if the certificate needs renewal, '1' if the certificate does not need renewal, and '255' for any other error.

dopey · 2021-07-02T21:16:40Z

command/certificate/needsRenewal.go

+'''
+$ step certificate needs-renewal ./certificate.crt 
+'''
+Check certificate for renewal using a host


using a hostname:

dopey · 2021-07-02T21:17:27Z

command/certificate/needsRenewal.go

+
+This command returns 0 if needing renewal, or returns 1 if the certificate does not need renewal. It will return 255 if any errors occurred
+
+## EXAMPLES


Put a colon : after each sentence prefacing the example command, like so: https://github.com/smallstep/cli/blob/master/command/certificate/verify.go#L38

dopey · 2021-07-02T21:19:28Z

command/certificate/needsRenewal.go

+				Name: "expires-in",
+				Usage: `Check if the certificate expires in given time duration
+				using <percent|duration>. For <percent>, must be followed by "%".
+				With <duration>, it is a sequence of decimal numbers, each with optional


With -> For

dopey · 2021-07-02T21:23:11Z

command/certificate/needsRenewal.go

+	)
+
+	if addr, isURL, err := trimURL(crtFile); err != nil {
+		return err


this should return 255 as well

dopey · 2021-07-02T21:55:25Z

command/certificate/needsRenewal.go

+	if addr, isURL, err := trimURL(crtFile); err != nil {
+		return err
+	} else if isURL {
+		peerCertificates, err := getPeerCertificates(addr, serverName, roots, false)


Since we're changing the operation of this command to now apply to all certificates in the bundle, you can probably just use the full list of peer certificates rather than grabbing the first index.

dopey · 2021-07-02T21:58:39Z

command/certificate/needsRenewal.go

+		// The first certificate PEM in the file is our leaf Certificate.
+		// Any certificate after the first is added to the list of Intermediate
+		// certificates used for path validation.
+		for len(crtBytes) > 0 {


Since we're changing the behavior of this command to apply to all certs in the file, what we want to do is loop over each pem block in the file (just like you're doing) and then convert it to a certificate and append it to an array of x509 certifcates.

dopey · 2021-07-02T22:00:05Z

command/certificate/needsRenewal.go

+		}
+
+	}
+	var remainingValidity = time.Until(cert.NotAfter)


This will happen in a for loop over all the certificates in the list

dopey · 2021-07-02T22:01:21Z

command/certificate/needsRenewal.go

+
+	if expiresIn != "" {
+		if strings.Contains(expiresIn, "%") {
+			percentageInput, err := strconv.Atoi(strings.ReplaceAll(expiresIn, "%", ""))


I would say just trimSuffix instead of replace all. We want people entering 50% instead of %3%5

dopey · 2021-07-02T22:02:34Z

command/certificate/needsRenewal.go

+			os.Exit(1)
+		}
+	} else {
+		if percentUsed >= 66 {


Let's parameterize this value, meaning let's make it a constant defined at the top of the file.

const defaultPercentUsedThreshold = 66

dopey · 2021-07-02T22:06:23Z

command/certificate/needsRenewal.go

+			}
+
+			if percentageInput > int(percentUsed) {
+				return nil


I think this is the case where we want to return 1.

dopey · 2021-07-02T22:52:32Z

command/certificate/needsRenewal.go

+		}
+	} else {
+		if percentUsed >= 66 {
+			return nil


It would be nice to combine this with the other percentage case so we don't copy the same conditional code.

Something like:

for _, cert := range certs { if expiredIn == "" || strings.Contains(expiresIn, "%") { var threshold int if expiresIn == "" { threshold = defaultPercentThreshold } else { threshold, err = strconv.Atoi(strings.TrimSuffix(expiresIn, "%")) if err .... } if threshold > 100 || threshold < 0 ... if int(percentUsed) >= threshold ... return nil } else { duration, err := time.ParseDuration(expiresIn) if err != nil { return errs.NewExitError(err, 255) } else if duration > remainingValidity { return nil } } } // We only get here if the certs are not expiring, so return an error to that effect. return errors.New("certificate is verdant")

dopey

left a bunch of comments.

All commented changes requested from Max have been Implemented here

dopey · 2021-07-07T22:04:20Z

command/certificate/needsRenewal.go

-		the certificate is over 66% of its lifetime. If the certificate needs renewal this command will return '0'.
-		If the certificate is not far enough into its life, it will return '1'. If validation fails, or if an error occurs, 
-		this command will produce a non-zero return value.
+		UsageText: `**step certificate needs-renewal** <crt_file or host_name> [**--expires-in**=<duration>]`,


Use cert_file here and in other places where you refer to this input. Over time we came to the custom of using cert if it's in documentation that the user sees because that's the standard other popular tools use (e.g. openssl, curl), and crt if we're just naming variables internally in our code.

dopey · 2021-07-07T22:05:13Z

command/certificate/needsRenewal.go

+		UsageText: `**step certificate needs-renewal** <crt_file or host_name> [**--expires-in**=<duration>]`,
+		Description: `**step certificate needs-renewal** returns '0' if the certificate needs to be renewed based on it's remaining lifetime. 
+		Returns '1' if the certificate is within it's validity lifetime bounds and does not need to be renewed. 
+		Returns '255' for any other error. By default, if a certificate "needs renewal" when it has passed 66% of it's allotted lifetime. 


remove the if in this line.

dopey · 2021-07-07T22:06:40Z

command/certificate/needsRenewal.go

-
-<host_name>
-: Address of remote host 
+<cert_file or hostname> The path to a certificate to validate OR a hostname with protocol prefix.


remove to validate

--roots and --servername are in with their descriptions from inspect.go. Examples for --roots were copied and adjusted for needs-renewal purposes

- change a bit of grammar in docs/examples

tashian · 2021-07-19T13:41:21Z

Thank you @tommy-56 @cookie-annihilator for this!

I've updated our systemd stuff to use needs-renewal (see smallstep/certificates#653 and smallstep/docs#105). It's so much cleaner, and it removes the dependency on jq.

meganm53 added 3 commits June 21, 2021 12:06

added expire to check verdancy

002c73e

added check expiration to description

c8bd51c

add check expiration as a flag

421d603

meganm53 requested a review from dopey June 21, 2021 22:28

github-actions bot added the needs triage Waiting for discussion / prioritization by team label Jun 21, 2021

dopey reviewed Jun 21, 2021

View reviewed changes

Go Formatting Ran

a20f560

dopey reviewed Jun 21, 2021

View reviewed changes

meganm53 and others added 3 commits June 21, 2021 15:54

Update certificate.go

d61bcc1

Fixed Error Message

3f645af

Merge branch 'megan/expiration' of https://github.com/smallstep/cli i…

c781dea

…nto megan/expiration

dopey reviewed Jun 21, 2021

View reviewed changes

added color names

181018e

dopey reviewed Jun 21, 2021

View reviewed changes

updated names and wording

1d76695

dopey reviewed Jun 22, 2021

View reviewed changes

meganm53 and others added 3 commits June 22, 2021 12:04

changed if else statements to return number value

48b362e

changed math

f813499

Formatting Fix

f57c70e

dopey reviewed Jul 2, 2021

View reviewed changes

dopey requested changes Jul 2, 2021

View reviewed changes

Update needsRenewal.go

4d67f07

All commented changes requested from Max have been Implemented here

dopey reviewed Jul 7, 2021

View reviewed changes

dopey and others added 4 commits July 7, 2021 15:33

cleanup

40fb340

Tested and builds without error

de3c152

Put the flags and the Descriptions back

0c36611

--roots and --servername are in with their descriptions from inspect.go. Examples for --roots were copied and adjusted for needs-renewal purposes

Use flag errors for expires-in parse errors

ac40eae

- change a bit of grammar in docs/examples

tommy-56 marked this pull request as ready for review July 8, 2021 20:15

tommy-56 merged commit 74aef56 into master Jul 8, 2021

tommy-56 deleted the megan/expiration branch July 8, 2021 20:17

tommy-56 linked an issue Jul 8, 2021 that may be closed by this pull request

Testing certificates for verdancy #398

Closed

tommy-56 mentioned this pull request Jul 8, 2021

Testing certificates for verdancy #398

Closed

dopey removed the needs triage Waiting for discussion / prioritization by team label Aug 31, 2021


		## EXIT CODES

		This command returns 0 if needing renewal, or returns 1 if the certificate does not need renewal. It will return 255 if any errors occurred


		This command returns 0 if needing renewal, or returns 1 if the certificate does not need renewal. It will return 255 if any errors occurred

		## EXAMPLES

Megan/expiration #495

Megan/expiration #495

Conversation

meganm53 commented Jun 21, 2021

CLAassistant commented Jun 21, 2021 • edited

Choose a reason for hiding this comment

codecov bot commented Jun 21, 2021 • edited

Codecov Report

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

dopey Jun 21, 2021 • edited

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

tashian Jun 22, 2021 • edited

Choose a reason for hiding this comment

dopey Jun 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tashian commented Jun 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dopey Jul 2, 2021 • edited

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

dopey left a 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

tashian commented Jul 19, 2021

CLAassistant commented Jun 21, 2021 •

edited

codecov bot commented Jun 21, 2021 •

edited

dopey Jun 21, 2021 •

edited

tashian Jun 22, 2021 •

edited

dopey Jun 22, 2021 •

edited

tashian commented Jun 22, 2021 •

edited

dopey Jul 2, 2021 •

edited