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

bump time #639

Closed
wants to merge 30 commits into from
Closed

bump time #639

wants to merge 30 commits into from

Conversation

Milo123459
Copy link
Member

  • bump time
  • bump changelog

Thanks for contributing to chrono!

  • Have you added yourself and the change to the changelog? (Don't worry
    about adding the PR number)
  • If this pull request fixes a bug, does it add a test that verifies that
    we can't reintroduce it?

@est31
Copy link

est31 commented Jan 18, 2022

Hi, it would be great if this could be merged. What is blocking this PR?

@kolbma
Copy link

kolbma commented Jan 20, 2022

Unmaintenance is blocking it.

@est31
Copy link

est31 commented Jan 20, 2022

@Milo123459 is a maintainer though, no?

@Milo123459
Copy link
Member Author

Unmaintenance is blocking it.

I'm a maintainer, I made this PR.

This change is rather large, and CI failing with un-helpful errors due to the way this was set up for Rust 2015 doesn't make it easier. I have emailed the owner of the project, asking him to review and find changes, and he hasn't replied yet. There is simply not much I can do other than fixing CI (which I plan on doing soon).

@est31
Copy link

est31 commented Jan 20, 2022

Thanks for the reply. I'm looking forward to seeing this merged.

@ghost
Copy link

ghost commented Jan 29, 2022

can plz update to newest not just min to fix vuln? time at 0.3.7 now https://github.com/time-rs/time

these vulns block for me

@Milo123459
Copy link
Member Author

This creates API changes that the owner (Brandon) doesn't want.

@djc
Copy link
Contributor

djc commented Jan 29, 2022

Is that discussion public somewhere? I'd be happy to help think about the required API changes.

@Milo123459
Copy link
Member Author

Is that discussion public somewhere? I'd be happy to help think about the required API changes.

No, it isn't. Sorry. However, I appreciate the offer.

@est31
Copy link

est31 commented Jan 29, 2022

If I add ?w=1 to the URL of this PR, it seems that there are no public API changes done by this PR. Is the objection towards API changes of the time crate? Is the opinion that chrono shall remain on an outdated and insecure time version forever? I hope not?

@Milo123459
Copy link
Member Author

Brandon doesn't want to change the API AFAIK.
Maintaining Chrono is so hard due to the code being based on 2015 Rust, the fact that there are vulnerabilities and that I can't do much without contacting the owner. I emailed him a while ago but I'm still awaiting a response.

The main idea of this is to not change the API, instead to make it so that it patches the vulnerability. That's it.

@ghost
Copy link

ghost commented Jan 29, 2022

if chrono refuse to keep deps update and secure what is alt lib that can be use in place? so many open pr for security issue, its not good look

look like exploits in wild rustsec/advisory-db#1082 (comment)

@Milo123459
Copy link
Member Author

Milo123459 commented Jan 29, 2022

if chrono refuse to keep deps update and secure what is alt lib that can be use in place? so many open pr for security issue, its not good look

look like exploits in wild rustsec/advisory-db#1082 (comment)

We aren't refusing. We are literally trying to update this dependency to fix the vulnerability.

Please re-read what I said, I simply said that I'm trying to fix the security vulnerability; not change the API.

EDIT: Sorry about sending it 4 times, GitHub wasn't working so I tried it again, to which it apparently worked.

@ghost
Copy link

ghost commented Jan 29, 2022

rise major vers if api is issue to not break others. not good look to still use old vers of dep that may have other issue still. if owner awol/not care then maintainer should just take over and do what need done for good of community and security

@ghost
Copy link

ghost commented Jan 29, 2022

for record: if api need change to fix vuln will it or users left insecure when use chrono?

@DanielJoyce
Copy link

Maybe it's time to fork... It's been months now, and this crate is used everywhere.

@Milo123459
Copy link
Member Author

Maybe it's time to fork... It's been months now, and this crate is used everywhere.

If you want to, fork chrono and make a pull request. It is the lack of communication from the higher ups that is causing so many road blocks.

bors bot added a commit to meilisearch/milli that referenced this pull request Feb 15, 2022
450: Get rid of chrono in favor of time r=Kerollmops a=irevoire

We only use `chrono` as a wrapper around `time`, and since there has been an [open CVE on `chrono` for at least 3 months now](chronotope/chrono#632) and the repo seems to be [struggling with maintenance](chronotope/chrono#639), I think we should use `time` directly which is way more active and sufficient for our use case.

EDIT: Actually the CVE status has been known for more than 6 months: chronotope/chrono#602

Co-authored-by: Irevoire <tamo@meilisearch.com>
bors bot added a commit to massalabs/massa that referenced this pull request Mar 8, 2022
2376: Fix security audit - get rid of chrono and use time directly r=AurelienFT a=AurelienFT

Chrono still use a very old version of time (0.1 now it's 0.3). They have a PR running since months for updating but it seems that there is communication problems that lead to long time development. The PR : chronotope/chrono#639

This break our CI like a lot of others projects that use `cargo audit`. A lot of projects that use chrono to do things that are now implemented in the new version of `time` has switched to use `time` directly instead of using tokio. Some examples : 
- brave/brave-browser#20568
- meilisearch/milli#450

So as we also only use chrono to make things that now possible in `time` which is more maintained I suggest in this PR a change to use `time` instead of `chrono`. So that it will fix our CI and make us use a more maintained dependency.

Fix #2374 

Co-authored-by: AurelienFT <aurelien.foucault@epitech.eu>
This was referenced Mar 14, 2022
@djc
Copy link
Contributor

djc commented Mar 23, 2022

I don't think we'll be updating the version of time used in chrono. Since #478, chrono has a minimal dependency on time, and in the next semver-incompatible version we'll remove the dependency entirely.

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 this pull request may close these issues.

None yet

6 participants