feather-rs / feather

A Minecraft server implementation in Rust
Apache License 2.0
2.58k stars 143 forks source link

Change from `chrono` implementation to impl based on `time` crate #506

Closed noahcoetsee closed 2 years ago

noahcoetsee commented 2 years ago

Change from chrono implementation to impl based on time crate

Status

Description

Change from chrono implementation to impl based on time crate, as it has addressed the issues present in RUSTSEC-2020-0159 in its own security advisory: RUSTSEC-2020-071. This is due to chrono no longer being actively maintained.

The new implementation still logs in the exact same time format as the previous chrono implementation.

This is necessary to prevent some cargo audit issues in the CI/CL Action cycle. There are other issues (involving the same RUSTSEC advisory) currently present due to this issue in crates used by feather such as rust-simple_logger, and I have made a PR there as well to address the issue. Once that PR is (hopefully) merged, we can update the version in feather-util and minecraft-proxy. The issue is also present in simple_asn1 and I'm currently working on a PR for that repo as well.

Obviously I'm aware that the security end of this issue isn't super critical given that the library isn't exposed by feather and feather isn't modifying environment variables in many threads (at all), but given it fails the cargo audit check in the CI/CL cycle it should be addressed so that future security advisories are not ignored when cargo audit is used.

Here's the dependency tree from the cargo audit: image

Related issues

https://github.com/borntyping/rust-simple_logger/pull/41

Checklist

Note: if you locally don't get any errors, but GitHub Actions fails (especially at clippy) you might want to check your rust toolchain version. You can then feel free to fix these warnings/errors in your PR.

Iaiao commented 2 years ago

You can just remove simple_logger from feather-utils because it's an unused dependency that we somehow forgot to remove from Cargo.toml. Also, I'm not sure we should use simple_logger in minecraft-proxy because we use fern in feather-server, so why not to use the same logging backend in both server and proxy?

simple_asn1 0.6 has already moved to time, but we need to update it to 0.6 by updating it in https://github.com/caelunshun/rsa-der and updating rsa to 0.5.

noahcoetsee commented 2 years ago

Okay. I'll remove the unused deps and switch minecraft_proxy over to fern. I see no reason for it to differ from feather-server's implementation

but we need to update it to 0.6 by updating it in https://github.com/caelunshun/rsa-der and updating rsa to 0.5.

Update: caelunshun has accepted my PR and updated rsa-der to v0.3.0 with the suggested changes.

Defman commented 2 years ago

Huh, seems like we are depending on an old version of the time crate as well... maybe cargo lock?

noahcoetsee commented 2 years ago

Huh, seems like we are depending on an old version of the time crate as well... maybe cargo lock?

Yeah I need to mark this PR as still in progress as I moved on to address the other dependency issues. The old version of time was a dependency of some other dependencies and would generate a similar RUSTSEC segfault error in cargo audit.

Defman commented 2 years ago

Any updates?

noahcoetsee commented 2 years ago

Yeah I've been pretty busy with work and there are some wasm dep. issues that are seemingly irremovable. I'll look into it again in the coming week and see if I can clear it up or not. If not I'll push most of the changes.

dlee13 commented 2 years ago

Superseded by #512. This can be closed.