-
Notifications
You must be signed in to change notification settings - Fork 252
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
Megan/expiration #495
Megan/expiration #495
Conversation
command/certificate/certificate.go
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking certificate expiration
The tense on the other verbs all seem to end with ing
, so we should be consistent.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
command/certificate/verify.go
Outdated
} else if percentIntoLifeTime < 1 { | ||
fmt.Println("\033[32m", "Leaf is less than 1% through its lifetime.", "\033[0m") | ||
} else { | ||
fmt.Println("Error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return an errors.Errorf
error here that described the issue. See errors returned in above code for examples.
command/certificate/verify.go
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 offmt.Println
if possible. - parameterize commonly used strings and colors so you can reuse them. e.g
var colorExpired = "31m"
.
command/certificate/verify.go
Outdated
@@ -164,6 +177,30 @@ func verifyAction(ctx *cli.Context) error { | |||
} | |||
} | |||
|
|||
if expire { | |||
|
|||
NowTillEndOfCert := time.Until(cert.NotAfter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/verify.go
Outdated
NowTillEndOfCert := time.Until(cert.NotAfter) | ||
totalLifeTimeOfCert := cert.NotAfter.Sub(cert.NotBefore) | ||
|
||
percentIntoLifeTime := ((totalLifeTimeOfCert.Hours() - NowTillEndOfCert.Hours()) / totalLifeTimeOfCert.Hours()) * 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we specifying .Hours()
here rather than using the complete duration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/verify.go
Outdated
if expire { | ||
|
||
NowTillEndOfCert := time.Until(cert.NotAfter) | ||
totalLifeTimeOfCert := cert.NotAfter.Sub(cert.NotBefore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/verify.go
Outdated
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify the remaining validity of a certificate ...
command/certificate/verify.go
Outdated
`, | ||
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`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checks the remaining certificate validity till expiration
command/certificate/verify.go
Outdated
|
||
return nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/verify.go
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
command/certificate/verify.go
Outdated
reset := "\033[0m" | ||
|
||
if percentUsed >= 100 { | ||
fmt.Println(red, "This certificate has already expired.", reset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
command/certificate/verify.go
Outdated
reset := "\033[0m" | ||
|
||
if percentUsed >= 100 { | ||
fmt.Println(red, "This certificate has already expired.", reset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the exit codes are described in #398
command/certificate/verify.go
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verdancy
hi @cookie-annihilator @tommy-56 I'm excited to see this PR! The use cases are:
Happy to answer any other questions y'all have about this feature... |
command/certificate/needsRenewal.go
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/needsRenewal.go
Outdated
this command will produce a non-zero return value. | ||
## POSITIONAL ARGUMENTS | ||
|
||
<crt_file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/needsRenewal.go
Outdated
|
||
## 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command returns '0' if the certificate needs renewal, '1' if the certificate does not need renewal, and '255' for any other error.
command/certificate/needsRenewal.go
Outdated
''' | ||
$ step certificate needs-renewal ./certificate.crt | ||
''' | ||
Check certificate for renewal using a host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using a hostname:
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a colon :
after each sentence prefacing the example command, like so: https://github.com/smallstep/cli/blob/master/command/certificate/verify.go#L38
command/certificate/needsRenewal.go
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With -> For
command/certificate/needsRenewal.go
Outdated
) | ||
|
||
if addr, isURL, err := trimURL(crtFile); err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should return 255 as well
command/certificate/needsRenewal.go
Outdated
if addr, isURL, err := trimURL(crtFile); err != nil { | ||
return err | ||
} else if isURL { | ||
peerCertificates, err := getPeerCertificates(addr, serverName, roots, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/needsRenewal.go
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/needsRenewal.go
Outdated
} | ||
|
||
} | ||
var remainingValidity = time.Until(cert.NotAfter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will happen in a for loop over all the certificates in the list
command/certificate/needsRenewal.go
Outdated
|
||
if expiresIn != "" { | ||
if strings.Contains(expiresIn, "%") { | ||
percentageInput, err := strconv.Atoi(strings.ReplaceAll(expiresIn, "%", "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say just trimSuffix instead of replace all. We want people entering 50% instead of %3%5
command/certificate/needsRenewal.go
Outdated
os.Exit(1) | ||
} | ||
} else { | ||
if percentUsed >= 66 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's parameterize this value, meaning let's make it a constant defined at the top of the file.
const defaultPercentUsedThreshold = 66
} | ||
|
||
if percentageInput > int(percentUsed) { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the case where we want to return 1.
command/certificate/needsRenewal.go
Outdated
} | ||
} else { | ||
if percentUsed >= 66 { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a bunch of comments.
All commented changes requested from Max have been Implemented here
command/certificate/needsRenewal.go
Outdated
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>]`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
command/certificate/needsRenewal.go
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the if
in this line.
command/certificate/needsRenewal.go
Outdated
|
||
<host_name> | ||
: Address of remote host | ||
<cert_file or hostname> The path to a certificate to validate OR a hostname with protocol prefix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Thank you @tommy-56 @cookie-annihilator for this! I've updated our systemd stuff to use |
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.