Add expires option

[herbygillot]

This PR adds the option to specify when the issued client certificate should expire (Not After field). It's specified in the style of <number>t, where t can be H (hour), m (month), d (days), etc...

While adding this feature, I also found that the client and server certs were being written to the wrong paths. An included commit resolves this.

[codecov]

Codecov Report

Merging #291 (1d5445a) into master (303c504) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            master      #291    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           10        10            
  Lines         1045      1181   +136     
  Branches        45        55    +10     
==========================================
+ Hits          1045      1181   +136     

Impacted Files	Coverage Δ
trustme/__init__.py	100.00% <100.00%> (ø)
trustme/_cli.py	100.00% <100.00%> (ø)
trustme/tests/test_cli.py	100.00% <0.00%> (ø)
trustme/tests/test_trustme.py	100.00% <0.00%> (ø)
a/trustme/trustme/tests/test_cli.py	100.00% <0.00%> (ø)
a/trustme/trustme/tests/test_trustme.py	100.00% <0.00%> (ø)
...sers/runner/work/trustme/trustme/tests/test_cli.py	100.00% <0.00%> (ø)
.../runner/work/trustme/trustme/tests/test_trustme.py	100.00% <0.00%> (ø)

[pquentin]

Hi @herbygillot, thanks a lot for your polished contribution!

Can you please clarify what your use case is for doing this? Are you interested in certs that expire sooner? Are you using trustme through the cli? We have a pretty high bar for allowing options like this, as it's easy to shoot yourself in the foot by using a wrong combination of options.

For example, since November 2019 and macOS 10.15, TLS server certificates issued after July 1, 2019 must have a validity period of 825 days or fewer. We're currently not affected because we issue certs starting from January 1, 2000. But if I merge your PR, what should I do when someone asks to change the notBefore date? Would we need a check for this 825 days rule? What other rules are we not aware of?

There are so many ways to shoot yourself in the foot with TLS certificates and the errors are so unclear when it happens that we're really reluctant to add options like this, which is why I'm asking about your use case. It would have to benefit many trustme users to be included.

While adding this feature, I also found that the client and server certs were being written to the wrong paths. An included commit resolves this.

(I would have appreciated a separate pull request for this change, as it needs to be discussed separately.)

Even we're not testing it, the paths aren't wrong, but the naming is unfortunate.

• When you launch a server, you need to give it:
  1. the server certificate (server.pem), its "public key"
  2. the server private key (server.key), allowing the server to prove that a given certificate authority issued that certificate
• When you launch a client, you need to give it the certificate authority public key (client.pem) so that it can check that the server certificate was issued by this authority

Does that make sense? Do you think renaming client.pem to ca.pem would have helped avoid the confusion?

[herbygillot]

Thank you for reviewing @pquentin. I am using trustme to help test some scenarios relating to certificate expiry, and found trustme's accessibility and ease-of-use in generating certs to be valuable. However I did need to control over the certificate's expiration date in order to do the above, hence this new option.

Would we need a check for this 825 days rule? What other rules are we not aware of?

I think if trustme wants emphasis on the generation of valid and "well-behaved" certs, perhaps the rules should be represented in code and the cert validated against them during/post-generation.

Then we should give users who specifically want to do something outside of these rules an option to do so, like --allow-invalid, or --enable-advanced. The flag could be a long-form flag only (--<flag>) so that it is always clear and the user isn't turning it on by mistake.

Another possibility that could keep the CLI simple is to add advanced options as an argparse subcommand. So one would need to do trustme advanced --set-not-after or something. Again, the user is specifically invoking functionality that may produce broken or invalid certificates, and we assume that they know why they need to do this.

In this way the default CLI is kept as clean and simple as it is today, while at the same time allowing power users access to options they may need. advanced mode should print a message warning the user that there is the possibility of generating possibly invalid certs.

Does that make sense? Do you think renaming client.pem to ca.pem would have helped avoid the confusion?

This does make sense, my fault for misunderstanding, and yes I think renaming could help here.

[pquentin]

Would you be happy with a --expired flag?

[herbygillot]

Well that's certainly up to you, but since we're currently specifying some span of time towards the future, something like --expires-in (--expires-in 1d, --expires-in 2w) could be more fitting.

Alternatively if it's to be specified as an actual datestamp, then --expires-on may work: --expires-on 20211201

[pquentin]

Can you please clarify why it's helpful to specify the precise expiry date?

This does make sense, my fault for misunderstanding, and yes I think renaming could help here.

Would you like to open a PR that changes client.pem to ca.pem?

[herbygillot]

Can you please clarify why it's helpful to specify the precise expiry date?

I only mentioning setting the precise expiry date as an alternative way of setting the expiry... a possible alternative to the method I'm currently proposing in this PR, which are time spans. I'm personally more partial to the method in the PR (time spans), rather than precise date.

Would you like to open a PR that changes client.pem to ca.pem?

I'll remove that commit, and allow you and team to change at your leisure.

[pquentin]

Sorry for the frustration I'm causing here. trustme is a small project that I'd like to keep small¹ and high-level, and this pull request adds 10% more code, which is why I'm reluctant to include this as is. My order of preference is:

• use a simple --expired flag/option, without allowing to set an actual date
• allow a ISO 8601 date, but only support YYYY-MM-DD
• allow a full humanize-like timedelta as you've done in this PR

Why is a simple --expired flag not fitting your needs?

¹ This is also why I don't really want to encode all the possibles rules we want to adhere to in code: it would be nice but it's more code than what I'm willing to maintain.

[herbygillot]

Why is a simple --expired flag not fitting your needs?

I was using trustme to assist in testing automation meant to monitor certificate expiration. Having the ability to set when the cert would expire allowed for verification through the transition from default state (certificate valid) to target state (certificate expired). An --expired flag that produces an already expired certificate is of no use in this scenario. If the concern is about keeping the code small, then perhaps we can see how to shrink the surface area while still making a "humanize" style possible.

[herbygillot]

(Removed cert renaming commit.)

[pquentin]

I'd be happy to hear what other maintainers think, but my opinion in that case is: let's only support --expires-on=2021-12-01, as you suggested above.

I'd be happy to merge such a pull request! But let's open another one to do so.

Thank you for your patience, and sorry for rejecting this.