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.

Sign up for GitHub

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 signature checking to install-debs.py #15374

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+82 −19
Open

Add signature checking to install-debs.py #15374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e74cb5a
Add signature checking to install-debs.py
akoeplinger Jan 5, 2025
e9d0232
Fix unassigned variable error
akoeplinger Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions eng/common/cross/build-rootfs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,14 @@ elif [[ "$__CodeName" == "haiku" ]]; then
popd
rm -rf "$__RootfsDir/tmp"
elif [[ -n "$__CodeName" ]]; then
__UpdateOptions=
if [[ "$__SkipSigCheck" == "0" ]]; then
__Keyring="$__Keyring --force-check-gpg"
else
__Keyring=
__UpdateOptions="--allow-unauthenticated --allow-insecure-repositories"
fi

if [[ "$__SkipEmulation" == "1" ]]; then
if [[ -z "$AR" ]]; then
if command -v ar &>/dev/null; then
Expand All @@ -800,26 +808,18 @@ elif [[ -n "$__CodeName" ]]; then
PYTHON=${PYTHON_EXECUTABLE:-python3}

# shellcheck disable=SC2086,SC2046
echo running "$PYTHON" "$__CrossDir/install-debs.py" --arch "$__UbuntuArch" --mirror "$__UbuntuRepo" --rootfsdir "$__RootfsDir" --artool "$AR" \
echo running "$PYTHON" "$__CrossDir/install-debs.py" $__Keyring --arch "$__UbuntuArch" --mirror "$__UbuntuRepo" --rootfsdir "$__RootfsDir" --artool "$AR" \
$(echo $suites | xargs -n 1 | xargs -I {} echo -n "--suite {} ") \
$__UbuntuPackages

# shellcheck disable=SC2086,SC2046
"$PYTHON" "$__CrossDir/install-debs.py" --arch "$__UbuntuArch" --mirror "$__UbuntuRepo" --rootfsdir "$__RootfsDir" --artool "$AR" \
"$PYTHON" "$__CrossDir/install-debs.py" $__Keyring --arch "$__UbuntuArch" --mirror "$__UbuntuRepo" --rootfsdir "$__RootfsDir" --artool "$AR" \
$(echo $suites | xargs -n 1 | xargs -I {} echo -n "--suite {} ") \
$__UbuntuPackages

exit 0
fi

__UpdateOptions=
if [[ "$__SkipSigCheck" == "0" ]]; then
__Keyring="$__Keyring --force-check-gpg"
else
__Keyring=
__UpdateOptions="--allow-unauthenticated --allow-insecure-repositories"
fi

# shellcheck disable=SC2086
echo running debootstrap "--variant=minbase" $__Keyring --arch "$__UbuntuArch" "$__CodeName" "$__RootfsDir" "$__UbuntuRepo"

Expand Down
81 changes: 72 additions & 9 deletions eng/common/cross/install-debs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import asyncio
import aiohttp
import gzip
import hashlib
import os
import re
import shutil
Expand All @@ -16,7 +17,7 @@
from collections import deque
from functools import cmp_to_key

async def download_file(session, url, dest_path, max_retries=3, retry_delay=2, timeout=60):
async def download_file(session, url, dest_path, max_retries=3, retry_delay=2, timeout=60, checksum=None):
"""Asynchronous file download with retries."""
attempt = 0
while attempt < max_retries:
Expand All @@ -25,6 +26,13 @@ async def download_file(session, url, dest_path, max_retries=3, retry_delay=2, t
if response.status == 200:
with open(dest_path, "wb") as f:
content = await response.read()

# verify checksum if provided
if checksum:
sha256 = hashlib.sha256(content).hexdigest()
if sha256 != checksum:
raise Exception(f"SHA256 mismatch for {url}: expected {checksum}, got {sha256}")

f.write(content)
print(f"Downloaded {url} at {dest_path}")
return
Expand All @@ -51,22 +59,21 @@ async def download_deb_files_parallel(mirror, packages, tmp_dir):
if filename:
url = f"{mirror}/{filename}"
dest_path = os.path.join(tmp_dir, os.path.basename(filename))
tasks.append(asyncio.create_task(download_file(session, url, dest_path)))
tasks.append(asyncio.create_task(download_file(session, url, dest_path, checksum=info.get("SHA256"))))

await asyncio.gather(*tasks)

async def download_package_index_parallel(mirror, arch, suites):
async def download_package_index_parallel(mirror, arch, suites, check_sig, keyring):
"""Download package index files for specified suites and components entirely in memory."""
tasks = []
timeout = aiohttp.ClientTimeout(total=60)

async with aiohttp.ClientSession(timeout=timeout) as session:
for suite in suites:
for component in ["main", "universe"]:
url = f"{mirror}/dists/{suite}/{component}/binary-{arch}/Packages.gz"
tasks.append(fetch_and_decompress(session, url))
tasks.append(fetch_and_decompress(session, mirror, arch, suite, component, check_sig, keyring))

results = await asyncio.gather(*tasks, return_exceptions=True)
results = await asyncio.gather(*tasks)
akoeplinger marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@akoeplinger akoeplinger Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@am11 am11 Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

it will resolve both issues.


merged_content = ""
for result in results:
Expand All @@ -77,21 +84,73 @@ async def download_package_index_parallel(mirror, arch, suites):

return merged_content

async def fetch_and_decompress(session, url):
async def fetch_and_decompress(session, mirror, arch, suite, component, check_sig, keyring):
"""Fetch and decompress the Packages.gz file."""

path = f"{component}/binary-{arch}/Packages.gz"
url = f"{mirror}/dists/{suite}/{path}"

try:
async with session.get(url) as response:
if response.status == 200:
compressed_data = await response.read()
decompressed_data = gzip.decompress(compressed_data).decode('utf-8')
print(f"Downloaded index: {url}")

if check_sig:
# Verify the package index against the sha256 recorded in the Release file
release_file_content = await fetch_release_file(session, mirror, suite, keyring)
packages_sha = parse_release_file(release_file_content, path)

sha256 = hashlib.sha256(compressed_data).hexdigest()
if sha256 != packages_sha:
raise Exception(f"SHA256 mismatch for {path}: expected {packages_sha}, got {sha256}")
print(f"Checksum verified for {path}")

return decompressed_data
else:
print(f"Skipped index: {url} (doesn't exist)")
return None
except Exception as e:
print(f"Error fetching {url}: {e}")

async def fetch_release_file(session, mirror, suite, keyring):
"""Fetch Release and Release.gpg files and verify the signature."""

release_url = f"{mirror}/dists/{suite}/Release"
release_gpg_url = f"{mirror}/dists/{suite}/Release.gpg"

with tempfile.NamedTemporaryFile() as release_file, tempfile.NamedTemporaryFile() as release_gpg_file:
await download_file(session, release_url, release_file.name)
await download_file(session, release_gpg_url, release_gpg_file.name)

keyring_arg = f"--keyring {keyring}" if keyring != '' else ''

print("Verifying signature of Release with Release.gpg.")
verify_command = f"gpg {keyring_arg} --verify {release_gpg_file.name} {release_file.name}"
result = subprocess.run(verify_command, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)

if result.returncode != 0:
raise Exception(f"Signature verification failed: {result.stderr.decode('utf-8')}")

print("Signature verified successfully.")

with open(release_file.name) as f: return f.read()

def parse_release_file(content, path):
"""Parses the Release file and returns sha256 checksum of the specified path."""

# data looks like this:
# <checksum> <size> <path>
matches = re.findall(r'^ (\S*) +(\S*) +(\S*)$', content, re.MULTILINE)

for entry in matches:
# the file has both md5 and sha256 checksums, we want sha256 which has a length of 64
if entry[2] == path and len(entry[0]) == 64:
return entry[0]

raise Exception(f"Could not find checksum for {path} in Release file.")

def parse_debian_version(version):
"""Parse a Debian package version into epoch, upstream version, and revision."""
match = re.match(r'^(?:(\d+):)?([^-]+)(?:-(.+))?$', version)
Expand Down Expand Up @@ -171,13 +230,15 @@ def parse_package_index(content):
filename = fields.get("Filename")
depends = fields.get("Depends")
provides = fields.get("Provides", None)
sha256 = fields.get("SHA256")

# Only update if package_name is not in packages or if the new version is higher
if package_name not in packages or compare_debian_versions(version, packages[package_name]["Version"]) > 0:
packages[package_name] = {
"Version": version,
"Filename": filename,
"Depends": depends
"Depends": depends,
"SHA256": sha256
}

# Update aliases if package provides any alternatives
Expand Down Expand Up @@ -301,6 +362,8 @@ def finalize_setup(rootfsdir):
parser.add_argument('--suite', required=True, action='append', help='Specify one or more repository suites to collect index data.')
parser.add_argument("--mirror", required=False, help="Mirror (e.g., http://ftp.debian.org/debian-ports etc.)")
parser.add_argument("--artool", required=False, default="ar", help="ar tool to extract debs (e.g., ar, llvm-ar etc.)")
parser.add_argument("--force-check-gpg", required=False, action='store_true', help="Verify the packages against signatures in Release file.")
parser.add_argument("--keyring", required=False, default='', help="Keyring file to check signature of Release file.")
parser.add_argument("packages", nargs="+", help="List of package names to be installed.")

args = parser.parse_args()
Expand All @@ -324,7 +387,7 @@ def finalize_setup(rootfsdir):

print(f"Creating rootfs. rootfsdir: {args.rootfsdir}, distro: {args.distro}, arch: {args.arch}, suites: {args.suite}, mirror: {args.mirror}")

package_index_content = asyncio.run(download_package_index_parallel(args.mirror, args.arch, args.suite))
package_index_content = asyncio.run(download_package_index_parallel(args.mirror, args.arch, args.suite, args.force_check_gpg, args.keyring))

packages_info, aliases = parse_package_index(package_index_content)

Expand Down
Loading