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

Add a filter in install-debs.py #15372

Merged
merged 4 commits into from
Jan 3, 2025
Merged

Add a filter in install-debs.py #15372

merged 4 commits into from
Jan 3, 2025

Conversation

am11
Copy link
Member

@am11 am11 commented Jan 3, 2025

@akoeplinger
Copy link
Member

akoeplinger commented Jan 3, 2025

Are you sure we couldn't use the tar or data filter? From what I can see the files inside of data.tar.xz are not using absolute paths.

@am11
Copy link
Member Author

am11 commented Jan 3, 2025

I build rootfs with and without it and diff the outputs of find /crossrootfs/loongarch64, there is no difference.

$ python3 installdebs.py --arch loong64 --rootfsdir /crossrootfs/loongarch64 \
    --artool /usr/bin/ar --suite sid --suite unreleased --mirror http://ftp.ports.debian.org/debian-ports/ \
    build-essential symlinks libicu-dev liblttng-ust-dev libunwind8-dev libcurl4-openssl-dev libkrb5-dev \
    libssl-dev zlib1g-dev libbrotli-dev libomp5 libomp-dev liblldb-19-dev

@akoeplinger
Copy link
Member

Yes which means we can use the tar filter which is safer.

I also tried data but it complains when extracting the tar package because it contains this symlink which points outside the destination:

lrwxrwxrwx  0 root   root        0 Dec 18 15:37 ./etc/rmt -> /usr/sbin/rmt

Btw. we don't check the signatures of these packages right? We're downloading them over http

@am11
Copy link
Member Author

am11 commented Jan 3, 2025

AFAIK, the symlinks are working fine. I had to change my entire approach to get the symlinks working otherwise https://github.com/dotnet/dotnet-buildtools-prereqs-docker/blob/7859d6aa101d4bbb769a39d20358cc9e77f29292/src/azurelinux/3.0/net10.0/cross/loongarch64/Dockerfile#L13 was failing to build. In the finalize_setup, I had to streamline /lib->/usr/lib because it was ordered dependent.

Btw. we don't check the signatures of these packages right? We're downloading them over http

We can add the sig check which should be straightforward. Feel free to enhance. ;)

@akoeplinger
Copy link
Member

Did you try with the filter='tar'? that one should allow symlinks etc.
I'm mainly asking because if we can't use that one then we should probably just pass the fully_trusted filter: https://docs.python.org/3.12/library/tarfile.html#extraction-filters

@am11
Copy link
Member Author

am11 commented Jan 3, 2025

Not sure what is the issue we are trying to solve here. Can you explain?

@akoeplinger
Copy link
Member

akoeplinger commented Jan 3, 2025

Tar files can have unexpected or potentially dangerous file path constructs and that's why Python is changing the defaults to prevent these. If possible we should leverage those protections instead of turning them off.

If that's not possible then using filter='fully_trusted' to explicitly opt in to the earlier behavior is IMO better than the pass-through lambda so it's more clear what the code does.

@am11
Copy link
Member Author

am11 commented Jan 3, 2025

Yes, as I explained earlier, debs extract in various system directories so we need out of tree extraction. I have changed it to idiomatic filter.

@akoeplinger
Copy link
Member

debs extract in various system directories

That's normally not a problem, the paths in the tar files are relative and are extracted in a given destination directory (which is / for root installs). I tried using filter='tar' and the command you pasted in #15372 (comment) and it didn't complain, but I haven't done a full end-to-end test so that's why I was wondering.

akoeplinger
akoeplinger previously approved these changes Jan 3, 2025
@am11
Copy link
Member Author

am11 commented Jan 3, 2025

Ah right, so if our desitnation is /crossrootfs/loongarch64, then everything is contained within that root. 'tar' should be fine as well.

@akoeplinger
Copy link
Member

Ok, let's use that then :)

I'll take a look at adding the signature validation.

@akoeplinger akoeplinger changed the title Add a passthru filter in install-debs.py Add a filter in install-debs.py Jan 3, 2025
@akoeplinger akoeplinger merged commit 2f07a67 into dotnet:main Jan 3, 2025
11 checks passed
@am11
Copy link
Member Author

am11 commented Jan 3, 2025

I'll take a look at adding the signature validation.

Note it won't work for new arch, or more specificlaly on Debian sid. We don't have those in Azure Linux. The first time we enabled signature checking on riscv was when we moved it to Ubuntu 24.04. Before that, it was using --skipsigcheck https://github.com/dotnet/dotnet-buildtools-prereqs-docker/pull/1151/files. So we will need it to honor --skipsigcheck and/or add debian sid key (which frequently changes) in crossdeps-builder. I suggest not to complicate it for sid (~ experimental) stuff.

@am11 am11 deleted the patch-15 branch January 3, 2025 20:40
@akoeplinger
Copy link
Member

@am11 ugh, looks like tar doesn't work after all, I just updated to python 3.13 on macOS and it starts to complain about the /usr/sbin/rmt symlink which worked in 3.12...

would you mind changing it back to fully_trusted? sorry about that

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.

2 participants