[certificate] Add certificate expiration badge

[fileformat]

Hi! I have this, and it is basically working (i.e. tests pass), but there is some caching going on that is causing problems.

If you try to get the same hostname within 60ish seconds, it will not call checkServerIdentity and you'll get an error. It works again after a bit, and other hostnames will also work (but just once) during the interval.

I think it is something being cached or re-used in guts of got.js, but couldn't find a workaround. I'm happy to bypass got.js and call node.js https.request directly, but wanted to check if this would be acceptable first.

Separate question: I have a website with a page to check certificates example. Is it okay to use it for the link?

[github-actions]

	Messages
📖	✨ Thanks for your contribution to Shields, @fileformat!

Generated by 🚫 dangerJS against 8ef675e

[chris48s]

I haven't really reviewed this PR in any detail as I think it is not really finished yet. I have had a quick look at your core issue though.

Fundamentally, it seems like this is the issue you're hitting: sindresorhus/got#2153

Anecdotally, this seems to work:

const resp = await this._request({
  // stuff
})

const cert = resp.res.req.socket.getPeerCertificate()

// do more stuff with cert - get cert.valid_to, or whatever

[fileformat]

@chris48s Clarifying things:

• The PR code works: the tests are real, and they pass.
• I have worked with certificates in node.js before (successfully).
• Something in the internals of shields.io code and/or got.js is caching/re-using something, that is making the 2nd attempt to verify the same cert fail for a period of time.

Example behavior:

1. check cert for shields.io (time t): gets date, passes
2. re-check cert for shields.io (time t+1 seconds): does not try to get date (i.e. checkServerIdentity is not called, though I can see that fetch does hit the webserver), throws
3. re-check cert for shields.io (time t+90 seconds): gets date, passes

I have already spent a fair bit of time without making much progress. If you want to try to get this in:

• I can rewrite it to use the native node.js https.request instead of got.js
• I am willing to work with someone who understands the guts of shields.io's got.js usage to fix the issue.

Sorry if I'm being snippy: I really do like shields.io & would like to contribute.
Andrew

[chris48s]

I'm aware of the issue you're trying to solve, and I'm trying to help you. In my previous post, I've linked you to a relevant upstream issue with more detail and proposed a solution.

The behaviour you're seeing is a behaviour of got, not shields. Lets take shields completely out of the equation. Here's a minimal script that reproduces the behaviour you're describing just using got directly:

import got from 'got'

for (let i = 0; i < 2; i++) {
  const resp = await got(
    'https://shields.io/',
    {
      https: {
        checkServerIdentity: (innerhost, certificate) => {
          console.log(certificate.valid_to)
        }
      }
    }
  )
}

Note we are running this twice in a loop, but you'll only get one line of output.

As noted in sindresorhus/got#2153 (comment) A got maintainer says this is not the advised way to do this (although admittedly there is not much detail on why).

Here's another minimal example using the approach I've suggested in my post above:

import got from 'got'

for (let i = 0; i < 2; i++) {
  const resp = await got('https://shields.io/')
  console.log(resp.req.socket.getPeerCertificate().valid_to)
}

Again, we're running this twice in a loop. Notice this time you should get 2 lines of output.

You should be able to use that approach to get yourself unstuck.

[fileformat]

Your test program works only because the connection is still open. If you read the response (which _request does), it fails. I can certainly change it to call got directly and bypass using all the _request infrastructure, but then I might as well call https.request.

Failing example (just added a call to .text() to force reading the response):

import got from 'got'

for (let i = 0; i < 2; i++) {
  const resp = await got('https://shields.io/').text()
  console.log(resp.req.socket.getPeerCertificate().valid_to)
}

[chris48s]

BaseService._request() doesn't call .text() (or .json() or .buffer()).

We do return the value of .body, so what we're doing is more like:

import got from 'got'

for (let i = 0; i < 2; i++) {
  const resp = await got('https://shields.io/')
  const body = resp.body
  console.log(resp.req.socket.getPeerCertificate().valid_to)
}

so you should be able to call socket.getPeerCertificate().

This:

const resp = await this._request({
  // stuff
})

const cert = resp.res.req.socket.getPeerCertificate()

does work for me (across multiple calls).

Is your concern with this that you think it doesn't work at all, or that it is in some way unreliable? If its the second one, can you explain further. If we need to, I have a couple of ideas about how we can pass the cert details back to the caller, but I don't think we need to.

In general, if we're going to take on the maintenance of this, we do need to do this via our usual process for calling got as this allows us to manage certain settings globally like making the request with our standard user agent, ensuring there is a limit on the size of response we will accept, etc. I don't want there to be one service which is bypassing those global settings by directly invoking got, https.request, or some other http client.

[fileformat]

I updated the PR as you suggested. The happy path is working, but the error which happens on redirects shows as "error" instead of getting the date. A good example is cloudflare.com, which has a different certificate from www.cloudflare.com. (2024-12-31 vs 2024-05-23)

Is there anything I should be doing differently in the error handler or passing in options?

I would like to handle this case, since certificates on hosts that redirect are easy to overlook, but I suppose it isn't a hard requirement. I am guessing (but haven't tested) that it would fail on sites that have basic auth as well.

[chris48s]

We're throwing here

shields/core/base-service/check-error-response.js

Lines 16 to 18 in 84e2944

	const underlying = Error(
	`Got status code ${res.statusCode} (expected 200)`,
	)

I would say if we're using the option followRedirect: false, we shouldn't be throwing there. If that option is set to false, a 301 or 302 is valid/expected. This has just never come up before because there are zero other badges where we use followRedirect: false.

I think the solution here is to modify BaseService._request() to pass options.followRedirect to checkErrorResponse() and modify checkErrorResponse() so it does not throw if followRedirect === false and res.statusCode === 301 or res.statusCode === 302.

[chris48s]

You are right about basic auth because it will respond with a 401. Tbh, any site that responds with a 404, 500, whatever will fail even if you could in theory still inspect the cert.

We do attach the response explicitly to the exception object before we throw it, so you could see if that works any better than what you are doing:

shields/core/base-service/check-error-response.js

Lines 34 to 37 in 84e2944

	if (error) {
	error.response = res
	error.buffer = buffer
	throw error

(untested)

The other thing we could do here (which I don't love) is have an option to _request() to completely bypass error checking and just return { buffer, res }

[chris48s]

Just having chewed this over slightly more, I reckon using error.response is probably the best option to look at first.

[fileformat]

Update per your suggestion and it seems to work. Once it is live, it is worth seeing if there are any of the caught errors without an expireStr.

[chris48s]

OK cool. Now that you've got this working, I've done a first pass of review comments over the code. Ta

[chris48s]

In general, we don't provide this sort of customisation via URL params.
I think we should make a decision on some thresholds for colours and just apply it.
There's some more comments on formatting further down.

[fileformat]

I disagree about this:

1. Something that makes sense for LetsEncrypt (3 months) will not make sense for a manually renewed cert that lasts 12 months.
2. It would make testing the colors very unreliable (or at least very tricky)

[chris48s]

I think we can ignore "it would be hard to test" as a consideration. It is fine for the live tests to omit the colour. If we switch to using colorScale(), that function is already well tested. If we implement custom formatting logic that we want to test, we can either pull that out to a method and unit test the method in a .spec.js, or you could do integration tests with mocks. There are ways we can handle that. We probably don't want to be leaning too hard on live tests with a service like this that doesn't depend on a single upstream service anyway.

If what is "good" or "bad" is so context dependent that we can't possibly define it, I think my suggestion is:

• The expiry date is already expired: red - that is unambiguously bad in 100% of cases
• The expiry date is in the future: informational blue

[chris48s]

Can we add a description property here explaining what this badge does.

[fileformat]

Done

[chris48s]

What exceptions are we trying to catch here?
In general, if we're actively expecting some error might happen in some cirtcumstances we should explictly check for it and throw/wrap an appropriate ShieldsRuntimeError. If we're not expecting a problem, any error should just be thrown and we'll catch it in Sentry. What you're doing here looks "defensive", but really this just silences errors and means we won't get a stacktrace in Sentry if something we haven't anticipated happens.

I think given we're not applying a response schema, I'd suggest we start by parsing the date to a dayjs object (more on this in a sec) with a format string, and throw an InvalidResponse error if it is bad.

so that would look something like:

const expires = dayjs(expiresStr, 'MMM DD HH:mm:ss YYYY [GMT]')
if (!expires.isValid()) {
  // throw an InvalidResponse error
}

Then if we're not anticipating any other call to fail, bin the rest of this big ol' try/catch.

[chris48s]

Again, there's a few things here:

• "success" colour should be brightgreen here

• We are doing some quite low-level stuff with date objects and numbers of seconds here. I'm not saying your code is wrong but I don't find it very readable. We use dayjs on this project as our higher level abstraction for datetimes. That allows us to do stuff like

import dayjs from 'dayjs'

dayjs().add(1, 'week') // 1 week from now
dayjs().add(1, 'month') // 1 month from now

Tbh though, I think we can probably cut through a lot of this and use the colorScale() helper in a similar way we do in the age() formatter but with a 3 colour palette:

• shields/services/color-formatters.js

  Lines 175 to 189 in c6be456

  	/**
  	* Determines the color used for a badge according to the age.
  	* Age is calculated as days elapsed till current date.
  	* The color varies from bright green to red as the age increases
  	* or the other way around if `reverse` is given `true`.
  	*
  	* @param {string} date Date string
  	* @param {boolean} reversed Reverse the color scale a.k.a. the older, the better
  	* @returns {string} Badge color
  	*/
  	function age(date, reversed = false) {
  	const colorByAge = colorScale([7, 30, 180, 365, 730], undefined, !reversed)
  	const daysElapsed = dayjs().diff(dayjs(date), 'days')
  	return colorByAge(daysElapsed)
  	}

• shields/services/color-formatters.js

  Line 142 in c6be456

  2: ['red', 'yellow', 'brightgreen'],

[chris48s]

Can we use formatRelativeDate() https://github.com/badges/shields/blob/master/services/text-formatters.js#L185-L198 to display the date here.

[chris48s]

(and isRelativeFormattedDate for the tests assertions https://github.com/badges/shields/blob/master/services/test-validators.js#L119-L123 )