Allow tolerance for pkcs7 signing time during device enrolment

[alwatts]

An iPad failed to enrol due to a clock issue, most likely due to the iPad's clock not having properly synced yet (server clock confirmed correct and NTP synchronised).

level=info handler=cert-extract trace_id=7dc2a24bd394756e msg=verifying Mdm-Signature header err=pkcs7: signing time "2023-04-07T00:16:58Z" is outside of certificate validity "2023-04-07T00:16:59Z" to "2033-04-04T00:16:59Z" caller=mdm_cert.go: 92

As the iPad was enrolled in DEP and management was mandatory, this left the following consequences:

• The iPad retried enrolment using the enrolment profile it had already obtained (it doesn't try and get another one)
• The SCEP one-time challenge had therefore already been used, so certificate enrolment now failed
• The iPad required erasure by connecting to a Mac and issuing the command with Configurator 2

While chances of this happening are small, it can leave users in a tricky situation with regards to recovering the device.

A tolerance on the signing time when checking against NotBefore of around 2-5 seconds is proposed to mitigate this event.

[jessepeterson]

It's funny you bring this up. This was a topic in the #micromdm channel with @korylprince a few days back and resulted in micromdm/micromdm#871. Part of the discussion was that the code has been like this for however many years the MicroMDM project has been a thing and we hadn't heard of it (though, admittedly, because the errors were effectively hidden it's hard to rule out that people just weren't seeing the error).

Anyway a couple notes/questions:

of around 2-5 seconds is proposed to mitigate this event.

Can you give a basis for your recommendation of 2-5 seconds here? Kory suggested five minutes (in the issue). I'm curious why you couldn't just wait for NTP to sync and re-attempting enrollment?

Secondly since this is NanoMDM and because of our previous Slack conversations I'd highly suggest taking a look at using mTLS (instead of relying on the Mdm-Signature header). This is enabled in NanoMDM by using the -cert-header flag and requires your upstream proxy to hand over the header containing the certificate. I think mTLS is a much better option than the Mdm-Signature header if you can afford it in your environment.

[jessepeterson]

See relevant discussion in smallstep/pkcs7#21.

[roperzh]

hey @jessepeterson ! I see some recent commits working towards a solution for this issue. Would you be open to contributions? I would love to take this on and fix it in a way that aligns what do you have in mind

[jessepeterson]

@roperzh yeah sounds great. Basically I envision a new Mdm-Signature verifier service (struct) that takes in a skew configuration from the CLI and uses that to evaluate against the new typed error in the upstream PKCS#7 package. Very similar to what @korylprince already added in micromdm/micromdm#887.

Note that there are two types of skew checking. There's the above and the there's also the VerifyWithChainAtTime which MicroMDM already has code for (but which doesn't catch the issuance issue, above):

https://github.com/micromdm/micromdm/blob/4bbce9a67895214c5e5518fbc44d4b6973d91e7a/pkg/crypto/helpers.go#L198-L214

I envision this service would have two separate adjustments for skew that do those two verifications differently (even if for simplicity there's just one CLI flag that sets them both the same).

[roperzh]

hey @jessepeterson I finally had time to sit down and have fun with this. Doing the final review before submitting a PR and I think I found VerifyWithChainAtTime is not helpful in our case, I would appreciate if you sanity check my logic here.

The time.Time parameter VerifyWithChainAtTime receives and ends up passing to verifySignatureAtTime it's only used for the certificate verification (which we're skipping anyways by passing a nil truststore)

https://github.com/smallstep/pkcs7/blob/84613357c7f2c2e116a62f93034417fd40ef9265/verify.go#L120-L125

The code we're interested in will still fail regardless:

https://github.com/smallstep/pkcs7/blob/84613357c7f2c2e116a62f93034417fd40ef9265/verify.go#L108-L119

In smallstep/pkcs7#21 I see it was discussed at some point to have both, the custom error, and a way to skew the time:

I think the combination of a typed error, with a clear error message, and being able to provide a time, is a good compromise over entirely disabling the check.
(source)

If you agree I can stir up the discussion on that issue again and see if we can agree on an implementation for the last bit: the ability to skew the attribute verification time.

---

edit: hm, re-reading your original message maybe the idea is to do the skew check for the first case ourselves