Implement secure_join for path

[Ian-Yy]

This PR serve as a prototype for issue #346

[codecov-commenter]

Codecov Report

Merging #354 (9abbf0d) into main (d209d75) will increase coverage by 1.48%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##             main     #354      +/-   ##
==========================================
+ Coverage   69.08%   70.56%   +1.48%     
==========================================
  Files          47       48       +1     
  Lines        7033     7533     +500     
==========================================
+ Hits         4859     5316     +457     
- Misses       2174     2217      +43     

[yihuaf]

Looks good overall.

[yihuaf]

You may have to run cargo fmt --all -- --check and cargo clippy --all.

[Ofenhed]

This is overly complex. Did you check out fs::canonicalize?

[utam0k]

This is overly complex. Did you check out fs::canonicalize?

I was also wondering if it could be implemented more simply with canonicalize. If you have tried to implement using it and found it difficult, please let me know.

[yihuaf]

It is not the same thing unfortunately. First, fs::canonicalize requires the path to first exist, which is not true when we have to evaluate path inside the container root. For example, device path may not be created or mounted, but we have to resolve the path first. Second, the requirement for evaluate symlink to be relative to the rootfs, not relative to the system host root. For example, this is functionally equivalent of:

chroot rootfs
readlink --canonicalize-missing --no-newline unsafe_path

See https://github.com/cyphar/filepath-securejoin for details and this is the same function used in the runc implementation.

[utam0k]

It is not the same thing unfortunately. First, fs::canonicalize requires the path to first exist, which is not true when we have to evaluate path inside the container root. For example, device path may not be created or mounted, but we have to resolve the path first. Second, the requirement for evaluate symlink to be relative to the rootfs, not relative to the system host root. For example, this is functionally equivalent of:

chroot rootfs
readlink --canonicalize-missing --no-newline unsafe_path

See https://github.com/cyphar/filepath-securejoin for details and this is the same function used in the runc implementation.

I understood. Thanks.

[Ian-Yy]

Hi, I've updated the code according to @yihuaf reviews and checked both cargo fmt --all -- --check and cargo clippy --all too this time. Please let me know if there's anything to add.

[yihuaf]

cargo clippy --all-targets --all-features -- -D warnings to make CI happy

[Ian-Yy]

cargo clippy --all-targets --all-features -- -D warnings to make CI happy

Sorry. I thought running cargo clippy --all is enough. Can we perhaps put this and cargo fmt somewhere in the documentation for newcomers?

Meanwhile, I will be making the changes.