Needs-Renewal #496

tommy-56 · 2021-06-24T19:32:57Z

Referencing issue #398
step certificate needs-renewal subcommand using exit codes
And having optional flag --expires-in
Example:
step certificate needs-renewal foo.crt
step certificate needs-renewal foo.crt --expires-in 15m

step certificate needs-renewal https://smallstep.com
step certificate needs-renewal https://smallstep.com --expires-in 15m

Fixes smallstep/certificates#607

Add support for Google CAS v1

needs-renewal is in the commands base now

Works standalone and with flag. Flag currently only check if the given time is > than the time left on the cert

Formatted right

removed comments

codecov · 2021-06-24T19:34:20Z

Codecov Report

Merging #496 (4868354) into megan/expiration (0da3b28) will decrease coverage by 0.48%.
The diff coverage is 37.20%.

@@                 Coverage Diff                  @@
##           megan/expiration     #496      +/-   ##
====================================================
- Coverage             63.29%   62.80%   -0.49%     
====================================================
  Files                    57       58       +1     
  Lines                  6865     6980     +115     
====================================================
+ Hits                   4345     4384      +39     
- Misses                 2281     2357      +76     
  Partials                239      239

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

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...4868354. Read the comment docs.

Was left behind on accident when testing

dopey · 2021-06-25T21:09:00Z

command/certificate/needsRenewal.go

+'''
+$ step certificate needs-renewal ./certificate.crt 
+'''
+Check if certificate will expire within a given time 


We should add an example using a host and one using percentage (rather than duration)

dopey · 2021-06-25T21:10:22Z

command/certificate/needsRenewal.go

+		Name:      "needs-renewal",
+		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>]`,


We don't really do this <crt_file> or <host_name> in other places. Which is not to say that we shouldn't be doing that.

But I think instead let's try <crt_file or host_name> to make it clear that's it’s one argument.

@dopey are we naming this needs-renewal instead of verdancy?

dopey · 2021-06-25T21:16:25Z

command/certificate/needsRenewal.go

+`,
+		Flags: []cli.Flag{
+			cli.StringFlag{
+				Name:  "expires-in",


See the description of the not-before flag in command/certificate/create.go. Use the second part of that description that talks about duration for this flag as well. And additionally, I think we want the option to use a percent for this flag. So if I pass 70% it should be parsed as a percent. Let's try to make that clear in the description.

So for example -- the beginning of not-before description starts with <time|duration>. This description could start with <percent|duration>.

dopey · 2021-06-25T21:22:17Z

command/certificate/needsRenewal.go

+	} else if isURL {
+		peerCertificates, err := getPeerCertificates(addr, serverName, roots, false)
+		if err != nil {
+			os.Exit(255)


Returning 255 is technically correct. But we want a bit more than that. What we really want is to return an error type that has an attribute where we can store what the return code should be. And then we need to be able to parse that error type wherever this error is handled.

Think of yourself as the user of the program. You run it and all you see is exit code 255. Well, a bunch of things probably cause that error code, so how do you figure out what went wrong. That's why we return error types. So there's some context for the user as to what went wrong.

Unfortunately, I'm not sure if we have an error type that you could use for this. We may have to write a new one. @maraino do we return non zero or 1 exit status from anywhere else? If not, should we create a new error type and then check for it in main.go?

We usually just return a wrapped error in these cases. Commands returning an error will print the error and exit with 1. And we can get the trace using STEPDEBUG=1

We were going for this functionality I believe:

return 0 if it's a certificate that needs to be renewed

return 0 if the certificate is expired

return 1 if it's a certificate that is valid (timewise) but doesn't need to be renewed

return 255 for any error

Which is why we needed a different error type that would give 255 as an exit code
os.Exit(255) was there in the mean time to make sure the logic worked right until we knew if there was an error type built like that.
For cases where exit code 1 is needed should we continue to use os.Exit(1) or should we change it to some kind of error type which doesn't seem right if its not actually an error?

See #495 (comment), I think we should be consistent and keep 1 as exit code for an error.

In bash scripts is very easy to differentiate exit errors. Unless systemd is an issue we should keep 1 as an error and use 2 for example to say that the certificate is valid.

dopey · 2021-06-25T21:51:06Z

command/certificate/needsRenewal.go

+	var totalValidity = cert.NotAfter.Sub(cert.NotBefore)
+	var percentUsed = (1 - remainingValidity.Minutes()/totalValidity.Minutes()) * 100
+
+	if expiresIn != "" {


I believe we want --expires-in to be either a percent or a duration (#495 (comment)). In order to do that we can try the following logic:

Check if input string has a % sign as it's last index. If it does, then treat the input as a percent. The prefix of the input should be a number in a string. Golang has utilites to parse a int from a string. Parse it and then verify that it is between 0 and 100. If not we'll return an error.

The input does not have a % sign and therefore is a duration. Your code already looks correct for this case.

Question for @mmalone: How exactly is the % option supposed to work (it's not as easy to understand as the duration option)? E.g.

step certificate needs-renewal --expires-in 15m

Answers the question: Will this certificate be expired in 15 minutes?

step certificate needs-renewal --expires-in 75%

Do we want to know if the certificate will expire if 75% of it's lifetime passes? If it's already lived past 75% of it's lifetime. It's not very clear.

Also updated examples and usage text

maraino · 2021-06-28T19:14:48Z

command/certificate/needsRenewal.go

+	} else {
+		crtBytes, err := ioutil.ReadFile(crtFile)
+		if err != nil {
+			os.Exit(255)


We should return an error here too

We should return wrapped errors here and in all the exit conditions. For example this should be like:

crtBytes, err := ioutil.ReadFile(crtFile) if err != nil { return errors.Wrapf(err, "error reading %s", crtFile) }

Or use the helper errs.FileError(err, crtFile)

maraino · 2021-06-28T19:18:39Z

command/certificate/needsRenewal.go

+		}
+
+		var (
+			block *pem.Block


To define one variable use just var block *pem.Block. It looks cleaner to me.

maraino

Make sure to return errors always instead of doing os.Exit(1)

They are now returning custom errors with exit code 255 while also giving a short print out of what went wrong

Changed the os.Exit(0) to return nil. os.Exit(255) is now replaced returning error types with the exit code of 255 Made edits to the examples and usage instructions

maraino and others added 13 commits June 8, 2021 18:04

Add support for privateca v1.

1c76ad6

Fixes smallstep/certificates#607

Upgrade dependencies.

8e48d17

Upgrade certificates.

d7b542b

Merge pull request smallstep#489 from smallstep/casv1

48a5747

Add support for Google CAS v1

Added new command in

35fb392

needs-renewal is in the commands base now

Templated With No Actions

fd845d7

Update needsRenewal.go

117607d

Works standalone and with flag. Flag currently only check if the given time is > than the time left on the cert

Create trash.go

f7d6479

Delete trash.go

29172dc

needsRenewal Update Description

5d215ab

update description

83b12e3

Update needsRenewal.go

4e591a8

Formatted right

Update needsRenewal.go

20da982

removed comments

tommy-56 requested a review from dopey June 24, 2021 19:33

Removed Un-needed flag

4868354

Was left behind on accident when testing

dopey reviewed Jun 25, 2021

View reviewed changes

Now takes duration and Percentages

e24517d

Also updated examples and usage text

maraino reviewed Jun 28, 2021

View reviewed changes

maraino requested changes Jun 28, 2021

View reviewed changes

tommy-56 and others added 5 commits June 28, 2021 14:36

Update needsRenewal.go

ddc6cc7

Error Fixes and minor clean ups

4b489e0

update flag usage

0ef63c6

Updated the Errors to be return types

2675a8e

They are now returning custom errors with exit code 255 while also giving a short print out of what went wrong

Format Fix

2cc83e9

tommy-56 added 3 commits June 29, 2021 13:01

Changes to exit codes and returning error types

c743f46

Changed the os.Exit(0) to return nil. os.Exit(255) is now replaced returning error types with the exit code of 255 Made edits to the examples and usage instructions

fixed lint errors

61cfbba

Merge branch 'needs-Renewal' into verdancy

742bfc9

tommy-56 requested a review from dopey June 30, 2021 20:43

dopey closed this Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Needs-Renewal #496

Needs-Renewal #496

tommy-56 commented Jun 24, 2021 •

edited

Loading

codecov bot commented Jun 24, 2021 •

edited

Loading

dopey Jun 25, 2021

dopey Jun 25, 2021

maraino Jun 28, 2021

dopey Jun 25, 2021

dopey Jun 25, 2021 •

edited

Loading

maraino Jun 28, 2021 •

edited

Loading

tommy-56 Jun 28, 2021

maraino Jun 28, 2021

dopey Jun 25, 2021

maraino Jun 28, 2021

maraino Jun 28, 2021

maraino Jun 28, 2021 •

edited

Loading

maraino left a comment

Needs-Renewal #496

Needs-Renewal #496

Conversation

tommy-56 commented Jun 24, 2021 • edited Loading

codecov bot commented Jun 24, 2021 • edited Loading

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

dopey Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

maraino Jun 28, 2021 • 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

maraino Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

maraino left a comment

Choose a reason for hiding this comment

tommy-56 commented Jun 24, 2021 •

edited

Loading

codecov bot commented Jun 24, 2021 •

edited

Loading

dopey Jun 25, 2021 •

edited

Loading

maraino Jun 28, 2021 •

edited

Loading

maraino Jun 28, 2021 •

edited

Loading