Add signature checking to install-debs.py #15374

akoeplinger · 2025-01-03T21:04:12Z

eng/common/cross/install-debs.py

eng/common/cross/build-rootfs.sh

am11 · 2025-01-03T21:15:32Z

Could you apply fully_trusted change here as well? No need for another PR since Azure Linux has 3.12 atm and won't be updating anytime soon given their pace of updates. 😅

akoeplinger · 2025-01-03T21:15:57Z

Could you apply fully_trusted change here as well?

I could but then I can't approve it since it's my own PR :)

am11 · 2025-01-03T21:18:20Z

Yea no rush. Images are building fine (with python 3.12) for now.

$ docker run --rm mcr.microsoft.com/dotnet-buildtools/prereqs:azurelinux-3.0-net10.0-cross-loongarch64 \
    python3 --version

Python 3.12.3

eng/common/cross/install-debs.py

am11 · 2025-01-03T21:30:38Z

eng/common/cross/install-debs.py

+        await download_file(session, release_gpg_url, release_gpg_file.name)
+
+        if args.keyring != '':
+            keyring_arg = f"--keyring {args.keyring}"


When __Keyring is set in bash script, it has this prefix.

arcade/eng/common/cross/build-rootfs.sh

Line 183 in 2f07a67

__Keyring="--keyring $__KeyringFile"

that's intentional, we pass --keyring <file> to install-debs.py which in turn parses that arg and sets the file and we pass the value on to gpg

am11 · 2025-01-04T00:10:09Z

@akoeplinger I think 43f2fa5 is not optional if we want to keep arguments order independent. I'm a bit busy with another build but please double check before merging. build-rootfs sid loongarch64 vs. build-rootfs loongarch64 sid or just "trust me bro ™️" and apply the patch. 😅

akoeplinger · 2025-01-04T00:28:27Z

@am11 I looked at the script but I don't see where it'd make a difference. That said I think we can remove the global __Keyring and move the check further down

am11 · 2025-01-04T00:39:16Z

@akoeplinger have you tested it with --skipsigcheck with different orders of argument? It matters when we are running it locally vs. in prereqs.

akoeplinger · 2025-01-04T00:44:24Z

hm I think I see what you mean. I don't think we can rely on __UbuntuArch when parsing the sid) argument or setting __UbuntuRepo in loongarch64) since one or the other isn't defined depending on the order

am11 · 2025-01-04T00:51:10Z

It will be so that build-rootfs sid <arch> --skipsigcheck and build-rootfs <arch> --skipsigcheck will work in same way with or without --skipemulation. We don't need larger refactoring for it, the patch should cover the case.

am11 · 2025-01-04T15:11:44Z

eng/common/cross/install-debs.py


-        results = await asyncio.gather(*tasks, return_exceptions=True)
+        results = await asyncio.gather(*tasks)


This is intentional. We need to continue on error here otherwise it fails the build. Some index files are optional and may not be available for certain distro.

that's already handled by fetch_and_decompress returning None when response.status is not 200

It fails from this script when it doesn't find some index. That's why I added success and error logging here. apt behavior is the same, it warns and continue for the missing index.

Lets see if it fails dotnet/dotnet-buildtools-prereqs-docker#1310. Local build was failing when I applied your patch. I suggest you open a similar PR to test variations. You can update loongarch with --skipemulation --skipsigcheck (we don't have keys for Debian sid) and riscv with --skipemulation without skipping the check (we have keys for Ubuntu noble) to showcase that it is working.

There are other issues:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=906926&view=logs&s=6884a131-87da-5381-61f3-d7acc3b91d76&j=fc59f0f2-c1bd-58ae-b870-833d1e8a924c

> [builder 3/4] RUN rootfsEnv="/usr/local/share/rootfs" && python3 -m venv "$rootfsEnv" && "$rootfsEnv/bin/python" -m pip install aiohttp zstandard && PYTHON_EXECUTABLE="$rootfsEnv/bin/python" /scripts/eng/common/cross/build-rootfs.sh loongarch64 sid --skipunmount --skipemulation: 12.19 Downloaded http://ftp.ports.debian.org/debian-ports//dists/unreleased/Release at /tmp/tmp9kl7odro 12.19 Downloaded http://ftp.ports.debian.org/debian-ports//dists/unreleased/Release.gpg at /tmp/tmpd_jy2s9z 12.19 Verifying signature of Release with Release.gpg. 12.19 Error fetching http://ftp.ports.debian.org/debian-ports//dists/unreleased/main/binary-loong64/Packages.gz: cannot access local variable 'keyring_arg' where it is not associated with a value 12.19 Downloaded index: http://ftp.ports.debian.org/debian-ports//dists/sid/main/binary-loong64/Packages.gz 12.19 Downloaded http://ftp.ports.debian.org/debian-ports//dists/sid/Release at /tmp/tmphulowxes 12.19 Downloaded http://ftp.ports.debian.org/debian-ports//dists/sid/Release.gpg at /tmp/tmp5h7tvol5 12.19 Verifying signature of Release with Release.gpg. 12.19 Error fetching http://ftp.ports.debian.org/debian-ports//dists/sid/main/binary-loong64/Packages.gz: cannot access local variable 'keyring_arg' where it is not associated with a value 12.19 Error: Package 'build-essential' was not found in the available packages. ------ ERROR: failed to solve: executor failed running [/bin/sh -c rootfsEnv="/usr/local/share/rootfs" && python3 -m venv "$rootfsEnv" && "$rootfsEnv/bin/python" -m pip install aiohttp zstandard && PYTHON_EXECUTABLE="$rootfsEnv/bin/python" /scripts/eng/common/cross/build-rootfs.sh loongarch64 sid --skipunmount --skipemulation]: exit code: 1 Retry 4/5, retrying in 125 seconds... #0 building with "default" instance using docker driver

I will now revert this line from the PR, and highly recommend you open similar PR downstream with that line in LA64 and RA64 net10.0 docksfiles so we have some coverage.

RUN rm -rf /scripts && git clone https://github.com/dotnet/arcade --single-branch --depth 1 -b akoeplinger-patch-1 /scripts

@akoeplinger, this PR is great, my intention with the feedback is we get the local and cloud scenarios right. For local testing, I'm using podman desktop on macOS for loongarch to test without --skipemulation and docker&podman deskotp with --skipemulation as docker desktop is missing the registration docker/roadmap#764. I am sharing the use-cases which I was testing locally. Hope my feedback doesn't come across different than the intended spirit. 👍

@am11 I tried locally and it fails because we don't have /usr/share/keyrings/debian-archive-keyring.gpg nor /usr/share/keyrings/debian-ports-archive-keyring.gpg in the builder image, I suppose we need to add it to https://github.com/dotnet/dotnet-buildtools-prereqs-docker/blob/c77a9f6b01eff67ba53a9363f949dee7ccde38c5/src/azurelinux/3.0/net10.0/crossdeps-builder/amd64/Dockerfile#L46-L51

Was this fetched by debootstrap before?

Hope my feedback doesn't come across different than the intended spirit. 👍

No worries, I'm still on vacation till tomorrow so I can't spend a lot of time on this yet 😄

--skipsigcheck should not require the keyring. That's the purpose of this arg. If you incorporate the feedback:

Add signature checking to install-debs.py #15374 (comment) (apply that patch)

undo results = await asyncio.gather(*tasks, return_exceptions=True)

it will resolve both issues.

akoeplinger commented Jan 3, 2025

View reviewed changes

eng/common/cross/install-debs.py Show resolved Hide resolved

akoeplinger commented Jan 3, 2025

View reviewed changes

eng/common/cross/install-debs.py Outdated Show resolved Hide resolved

am11 reviewed Jan 3, 2025

View reviewed changes

eng/common/cross/build-rootfs.sh Outdated Show resolved Hide resolved

am11 reviewed Jan 3, 2025

View reviewed changes

eng/common/cross/install-debs.py Outdated Show resolved Hide resolved

am11 reviewed Jan 3, 2025

View reviewed changes

akoeplinger requested review from jkoritzinsky and janvorli January 3, 2025 23:39

jkoritzinsky previously approved these changes Jan 4, 2025

View reviewed changes

am11 reviewed Jan 4, 2025

View reviewed changes

akoeplinger dismissed jkoritzinsky’s stale review via aa168ef January 5, 2025 10:58

akoeplinger requested a review from jkoritzinsky January 5, 2025 10:59

Add signature checking to install-debs.py

e74cb5a

akoeplinger force-pushed the akoeplinger-patch-1 branch from aa168ef to e74cb5a Compare January 5, 2025 11:02

Fix unassigned variable error

e9d0232

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add signature checking to install-debs.py #15374

Add signature checking to install-debs.py #15374

akoeplinger commented Jan 3, 2025

am11 commented Jan 3, 2025

akoeplinger commented Jan 3, 2025 •

edited

Loading

am11 commented Jan 3, 2025 •

edited

Loading

am11 Jan 3, 2025 •

edited

Loading

akoeplinger Jan 3, 2025 •

edited

Loading

am11 commented Jan 4, 2025

akoeplinger commented Jan 4, 2025 •

edited

Loading

am11 commented Jan 4, 2025

akoeplinger commented Jan 4, 2025

am11 commented Jan 4, 2025

am11 Jan 4, 2025

akoeplinger Jan 5, 2025 •

edited

Loading

am11 Jan 5, 2025 •

edited

Loading

am11 Jan 5, 2025

akoeplinger Jan 6, 2025

am11 Jan 6, 2025


		results = await asyncio.gather(*tasks, return_exceptions=True)
		results = await asyncio.gather(*tasks)

Add signature checking to install-debs.py #15374

Are you sure you want to change the base?

Add signature checking to install-debs.py #15374

Conversation

akoeplinger commented Jan 3, 2025

am11 commented Jan 3, 2025

akoeplinger commented Jan 3, 2025 • edited Loading

am11 commented Jan 3, 2025 • edited Loading

am11 Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

akoeplinger Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

am11 commented Jan 4, 2025

akoeplinger commented Jan 4, 2025 • edited Loading

am11 commented Jan 4, 2025

akoeplinger commented Jan 4, 2025

am11 commented Jan 4, 2025

am11 Jan 4, 2025

Choose a reason for hiding this comment

akoeplinger Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

am11 Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

am11 Jan 5, 2025

Choose a reason for hiding this comment

akoeplinger Jan 6, 2025

Choose a reason for hiding this comment

am11 Jan 6, 2025

Choose a reason for hiding this comment

akoeplinger commented Jan 3, 2025 •

edited

Loading

am11 commented Jan 3, 2025 •

edited

Loading

am11 Jan 3, 2025 •

edited

Loading

akoeplinger Jan 3, 2025 •

edited

Loading

akoeplinger commented Jan 4, 2025 •

edited

Loading

akoeplinger Jan 5, 2025 •

edited

Loading

am11 Jan 5, 2025 •

edited

Loading