check if any certificate needs to be renewed by leipert · Pull Request #38 · rolodato/gitlab-letsencrypt

leipert · 2018-01-15T23:14:04Z

@travismiller was faster on automating the pages workflow, thank you! 🎉

On my local branch, I added a quick check, if certificate renewal is even needed at all.
High level workflow is like this

Only get a new certificate, if at least one of the pages given in the arguments:

is not registered yet
does not have a certificate
has an expired certificate
has an certificate expiring within X days (default 15)

$ ./index.js --domain xyz.leipert.io --domain leipert.io \
    --email xxx@yyy.io \
    --repository https://gitlab.com/leipert/leipert.gitlab.io \
    --path /public/.well-known/acme-challenge
All domains xyz.leipert.io, leipert.io have a valid certificate (expiration in more than 15 days)

tmaier · 2018-01-16T10:23:56Z

Great idea. Consider to increase the default to 31 days, as this fits better to the Pipeline Scheduling feature supported if #36 gets merged.

The updated README suggests to run the pipeline once per month. With a default of 15, the user would miss the renewal date.

rolodato · 2018-01-16T20:07:32Z

Thanks for the PR! I would make the following changes for consistency with certbot renew:

Default expiration should be 30 days
No option to configure a default expiration
Add a --force-renewal option that bypasses the expiration check and renews anyway

tmaier · 2018-01-16T23:52:13Z

The reasons why I proposed 31 instead of 30 are

some months have 31 days (with 30, they would not request a new certificate)
you could come into a race condition where the previous build and validation step took longer than the current one (can be just a few minutes). In this case the the delta would be higher than 30 days and the renewal would be skipped

leipert · 2018-01-17T08:49:03Z

I have to say, that I agree with @rolodato on that one:

Consistency with certbot is a good idea.
if you run gitlab-le with a cronjob each day, it doesn't matter whether it's 30 or 31 days.

leipert · 2018-01-17T10:10:30Z

I updated the behavior as suggested. Additionally I made sure that the exit code is 0 if all certificates are valid

leipert · 2018-01-22T13:08:50Z

/ping @rolodato

rolodato · 2018-01-22T22:02:12Z

args.js

        type: 'boolean',
        default: false
+    }).option('force-renewal', {
+        describe: 'Force renewal of certificate, even if it expires in more than 30 days',


This should be phrased with plural instead:

Force renewal of certificates, even if they expire in more than 30 days.

Two issues with that:

--production also is in singular:

Obtain a real certificate instead of a dummy one

Technically you only obtain one certificate per execution, right?

Should I still change the text?

rolodato · 2018-01-22T22:05:19Z

package.json

  "dependencies": {
    "bluebird": "3.5.0",
    "le-acme-core": "https://git.daplie.com/rolodato/le-acme-core/repository/archive.tar.gz",
+    "moment": "^2.20.1",


I don't think it's necessary to introduce this dependency, we can do a little date arithmetic instead :)

removed the dependency

rolodato · 2018-01-22T22:07:02Z

lib.js

 const path = require('path');
 const { URL } = require('url');

+const EXPIRATION_IN_DAYS = 30;


Can be repliaced by const EXPIRATION = ms('30 days') if we get rid of the moment dependency.

removed the dependency

rolodato · 2018-01-22T22:12:25Z

lib.js

+
+            if (needsNoRenewal) {
+                console.log(`All domains (${options.domain.join(', ')}) have a valid certificate (expiration in more than ${EXPIRATION_IN_DAYS} days)`);
+                process.exit(0);


I would try to avoid this process.exit here - this file should only be used as a library for the main program (main.js). I think the better approach would be to refactor this file so the exported function (getCertificates) resolves successfully but with an empty array as a result, which means that everything went OK but no new certificates needed to be generated.

rolodato · 2018-01-22T22:23:44Z

lib.js

    };

+    const hasValidCertificate = (pagesDomain) => {
+        if (options.domain.includes(pagesDomain.domain)) {


This function should not care about option.domain, it should only care about the domain passed as a parameter. If we need to filter other domains it should be done at the call site instead of here.

rolodato · 2018-01-22T22:24:32Z

lib.js


+    const hasValidCertificate = (pagesDomain) => {
+        if (options.domain.includes(pagesDomain.domain)) {
+            if (pagesDomain.certificate) {


Can we avoid nesting these ifs so deeply? :)

leipert · 2018-01-30T23:08:02Z

@rolodato, I fixed all the issues you described in your review (except the singular/plural one).
For the process.exit change, I had to refactor a bit:

I moved registering of ACME account, certificate retrieval and certificate upload into a new function runACMEWorkflow.

leipert · 2018-02-14T08:45:10Z

Hey @rolodato,

I thought, I'd ping again ;)

leipert · 2018-04-14T11:15:43Z

@rolodato Ping ping :P

leipert mentioned this pull request Jan 17, 2018

Be more verbose #41

Open

leipert force-pushed the feature/checkIfCertificatesExpiring branch from 6cde205 to 1be80c8 Compare January 17, 2018 10:16

rolodato suggested changes Jan 22, 2018

View reviewed changes

leipert and others added 7 commits January 30, 2018 22:44

check if any certificate needs to be renewed

109e703

Change default expiration to 31 days from 15

bc2e2e0

change minimum valid days to 30 days

013bcf0

implement parameter force-renewal

408cc8d

ensure that the script exist with 0 if all certificates are valid

ae4b809

remove dependency to moment

5fe13f0

reduce complexity of hasValidCertificate

b7cf54c

leipert force-pushed the feature/checkIfCertificatesExpiring branch from c538f91 to b7cf54c Compare January 30, 2018 21:45

refactor renewal check to not exit process, if renewal is not necessary

34fd4db

use easier return instead of then(() =>

31a1b7f

Conversation

leipert commented Jan 15, 2018

Uh oh!

tmaier commented Jan 16, 2018

Uh oh!

rolodato commented Jan 16, 2018

Uh oh!

tmaier commented Jan 16, 2018

Uh oh!

leipert commented Jan 17, 2018

Uh oh!

leipert commented Jan 17, 2018

Uh oh!

leipert commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leipert commented Jan 30, 2018

Uh oh!

leipert commented Feb 14, 2018

Uh oh!

leipert commented Apr 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants