Skip to content
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

Dealing with unsoundness in chrono #65

Closed
djc opened this issue Oct 25, 2021 · 12 comments · Fixed by #66
Closed

Dealing with unsoundness in chrono #65

djc opened this issue Oct 25, 2021 · 12 comments · Fixed by #66

Comments

@djc
Copy link
Member

djc commented Oct 25, 2021

Hey, have you decided what you want to do with the chrono issues?

https://rustsec.org/advisories/RUSTSEC-2020-0159

Maybe it makes sense for rcgen to just adopt time 0.3 instead?

@est31
Copy link
Member

est31 commented Oct 25, 2021

Thanks for bringing this to my attention. And omg is the chrono thread spammed with links to bug reports. The time crate has less dependencies than chrono, so maybe switching to time is a good idea. It also seems to be more popular at least according to the recent download counts.

@est31
Copy link
Member

est31 commented Oct 25, 2021

Another proof that adding dependencies doesn't absolve you from maintainer duties, in fact they get expanded. You suddenly are responsible for bugs that are in a part of a library that you don't even use (you only use the library).

@djc
Copy link
Member Author

djc commented Oct 25, 2021

chrono depends on time 0.1 IIRC, so I'm not sure the difference in recent download counts is meaningful.

@est31
Copy link
Member

est31 commented Oct 25, 2021

@djc I use download counts as proxy for ecosystem compatibility. The main reason I'm using a date library is because I don't want to burden users with converting back and forth but be able to just put in dates that their library supports.

@est31
Copy link
Member

est31 commented Oct 25, 2021

There is also the issue of asn.rs using chrono, so it has to switch as well I guess?

@makr11st
Copy link

makr11st commented Dec 8, 2021

@est31 Hi, is there an update on #66 it seems the PRs in deps are now merged.

@est31
Copy link
Member

est31 commented Dec 8, 2021

The time crate does not support leap seconds, see time-rs/time#193 . This can be an issue for certificate generation libraries as single seconds can become important in the security domain. So I'm a bit reluctant, because rcgen does not use the offending function that makes chrono unsafe in the database, but it does rely on leap second support. The PR #66 has to change some of the unit tests that check for leap seconds.

Ultimately, if there is no movement on the time crate, I think I'll just merge it as is and accept the regression.

@est31
Copy link
Member

est31 commented Dec 9, 2021

Alright, when the new ring version will be released, I'll switch to time: briansmith/ring#1416 (comment)

@makr11st
Copy link

makr11st commented Dec 9, 2021

No worries and no pushing, just was wondering if there were additional dependencies you were waiting for, it makes sense to implement proper leap seconds handling. I saw the leap second discussion in the other repos, but it doesn't seem like a straightforward problem to solve.
Thanks for replying.

@est31
Copy link
Member

est31 commented Jan 13, 2022

Hmmm no changes for ring. There seems to be progress on the chrono front though: chronotope/chrono#632 (comment)

@est31
Copy link
Member

est31 commented Jan 13, 2022

Also: chronotope/chrono#639

@makr11st
Copy link

Thanks for the merge and release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants