diff --git a/.github/workflows/modern-distributions.yml b/.github/workflows/modern-distributions.yml index 9bdd09d..7d6121c 100644 --- a/.github/workflows/modern-distributions.yml +++ b/.github/workflows/modern-distributions.yml @@ -8,8 +8,6 @@ jobs: check: name: Validate tar based distributions changes runs-on: ubuntu-latest - permissions: - pull-requests: write steps: - name: Checkout repo uses: actions/checkout@v4 @@ -26,7 +24,5 @@ jobs: --repo-path . \ --compare-with-branch 'origin/${{ github.base_ref }}' \ --manifest distributions/DistributionInfo.json \ - --github-token '${{ secrets.GITHUB_TOKEN }}' \ - --github-pr '${{ github.event.pull_request.number }}' \ - --github-commit '${{ github.sha }}' + shell: bash diff --git a/distributions/requirements.txt b/distributions/requirements.txt index 26c6187..f4914ce 100644 --- a/distributions/requirements.txt +++ b/distributions/requirements.txt @@ -1,4 +1,5 @@ python-magic==0.4.27 click==8.1.3 GitPython==3.1.41 -PyGithub==2.5.0 \ No newline at end of file +PyGithub==2.5.0 +json-cfg==0.4.2 \ No newline at end of file diff --git a/distributions/validate-modern.py b/distributions/validate-modern.py index a40fd51..f8388cb 100644 --- a/distributions/validate-modern.py +++ b/distributions/validate-modern.py @@ -1,5 +1,6 @@ import click -import json +import jsoncfg +from jsoncfg.config_classes import ConfigJSONObject, ConfigJSONArray, ConfigJSONScalar import requests import tempfile import hashlib @@ -31,9 +32,27 @@ DISCOURAGED_SYSTEM_UNITS = ['systemd-resolved.service', 'NetworkManager.service', 'networking.service'] -errors = [] -warnings = [] +errors = {} +warnings = {} +def subset(inner, outer) -> bool: + for key, value in inner: + if key not in outer: + return True + + if not node_equals(value, outer[key]): + return False + + return True + +def node_equals(left, right): + if isinstance(left, ConfigJSONScalar): + return left() == right() + + elif isinstance(left, ConfigJSONArray): + return len(left) == len(right) and all(node_equals(l, r) for l, r in zip(left, right)) + else: + return subset(left, right) and subset(right, left) @click.command() @click.option('--manifest', default=None) @@ -41,11 +60,8 @@ warnings = [] @click.option('--compare-with-branch') @click.option('--repo-path', '..') @click.option('--arm64', is_flag=True) -@click.option('--github-token', default=None) -@click.option('--github-pr', default=None, type=int) -@click.option('--github-commit', default=None) @click.option('--debug', is_flag=True) -def main(manifest: str, tar: str, compare_with_branch: str, repo_path: str, arm64: bool, github_token: str, github_pr: str, github_commit: str, debug: bool): +def main(manifest: str, tar: str, compare_with_branch: str, repo_path: str, arm64: bool, debug: bool): try: if tar is not None: with open(tar, 'rb') as fd: @@ -54,69 +70,64 @@ def main(manifest: str, tar: str, compare_with_branch: str, repo_path: str, arm6 if manifest is None: raise RuntimeError('Either --tar or --manifest is required') - with open(manifest) as fd: - manifest_content = json.loads(fd.read()) + manifest_content = jsoncfg.load_config(manifest) baseline_manifest = None if compare_with_branch is not None: repo = git.Repo(repo_path) baseline_json = repo.commit(compare_with_branch).tree / 'distributions/DistributionInfo.json' - baseline_manifest = json.load(baseline_json.data_stream).get('ModernDistributions', {}) + baseline_manifest = jsoncfg.loads_config(baseline_json.data_stream.read().decode())['ModernDistributions'] - for flavor, versions in manifest_content["ModernDistributions"].items(): - baseline_flavor = baseline_manifest.get(flavor, None) if baseline_manifest else None + for flavor, versions in manifest_content["ModernDistributions"]: + baseline_flavor = baseline_manifest[flavor] if baseline_manifest and flavor in baseline_manifest else None for e in versions: - name = e.get('Name', None) + name = e['Name']() if 'Name' in e else None if name is None: - error(flavor, None, 'Found nameless distribution') + error(flavor, 'Found nameless distribution') continue if baseline_flavor is not None: - baseline_version = next((entry for entry in baseline_flavor if entry['Name'] == name), None) + baseline_version = next((entry for entry in baseline_flavor if entry['Name']() == name), None) if baseline_version is None: click.secho(f'Found new entry for flavor "{flavor}": {name}', fg='green', bold=True) - elif baseline_version != e: + elif not node_equals(baseline_version, e): click.secho(f'Found changed entry for flavor "{flavor}": {name}', fg='green', bold=True) else: click.secho(f'Distribution entry "{flavor}/{name}" is unchanged, skipping') continue - click.secho(f'Reading information for distribution: {e["Name"]}', bold=True) + click.secho(f'Reading information for distribution: {name}', bold=True) if 'FriendlyName' not in e: - error(flavor, name, 'Manifest entry is missing a "FriendlyName" entry') + error(e, 'Manifest entry is missing a "FriendlyName" entry') if not name.startswith(flavor): - error(flavor, name, f'Name should start with "{flavor}"') + error(e, f'Name should start with "{flavor}"') url_found = False if 'Amd64Url' in e: - read_url(flavor, name, e['Amd64Url'], X64_ELF_MAGIC) + read_url(e['Amd64Url'], X64_ELF_MAGIC) url_found = True if 'Arm64Url' in e: - read_url(flavor, name, e['Arm64Url'], ARM64_ELF_MAGIC) + read_url(e['Arm64Url'], ARM64_ELF_MAGIC) url_found = True if not url_found: - error(flavor, name, 'No URL found') + error(flavor, 'No URL found') expectedKeys = ['Name', 'FriendlyName', 'Default', 'Amd64Url', 'Arm64Url'] - for key in e.keys(): + for key, value in e: if key not in expectedKeys: - error(flavor, name, 'Unexpected key: "{key}"') + error(e, f'Unexpected key: "{key}"') - - default_entries = sum(1 for e in versions if e.get('Default', False)) + default_entries = sum(1 for e in versions if 'Default' in e and e['Default']()) if default_entries != 1: - error(flavor, None, 'Found no default distribution' if default_entries == 0 else 'Found multiple default distributions') + error(e, 'Found no default distribution' if default_entries == 0 else 'Found multiple default distributions') - if github_pr is not None: - assert github_token is not None and github_commit is not None and manifest is not None - - report_status_on_pr(github_pr, github_token, github_commit, manifest) + report_status_on_pr(manifest) except: if debug: @@ -127,30 +138,22 @@ def main(manifest: str, tar: str, compare_with_branch: str, repo_path: str, arm6 else: raise -def report_status_on_pr(pr: int, github_token: str, github_commit: str, manifest: str): - github = Github(github_token) - repo = github.get_repo('microsoft/WSL') - +def report_status_on_pr(manifest: str): def format_list(entries: list) -> str: - output = '\n' + if len(entries) == 1: + return entries[0] + output = '' for e in entries: output += f'\n* {e}' - return output + '\n' + return output - body = 'Thank you for your contribution to WSL.\n' - if errors: - body += f'**The following fatal errors have been found in this pull request:** {format_list(errors)}\n' - else: - body += 'No fatal errors have been found.\n' + for line, text in errors.items(): + print(f'::error file={manifest},line={line}::Error: {format_list(text).replace('\n', '%0A')}') - if warnings: - body += f'**The following suggestions have been found in this pull request:** {format_list(warnings)}\n' - else: - body += 'No suggestions have been found.\n' - - repo.get_pull(pr).create_review(body=body, commit=repo.get_commit(github_commit)) + for line, text in warnings.items(): + print(f'::warning file={manifest},line={line}::Warning: {format_list(text).replace('\n', '%0A')}') def read_config_keys(config: configparser.ConfigParser) -> dict: @@ -162,17 +165,17 @@ def read_config_keys(config: configparser.ConfigParser) -> dict: return keys -def read_passwd(flavor: str, name: str, default_uid: int, fd): +def read_passwd(node, default_uid: int, fd): def read_passwd_line(line: str): fields = line.split(':') if len(fields) != 7: - error(flavor, name, f'Invalid passwd entry: {line}') + error(node, f'Invalid passwd entry: {line}') return None, None try: uid = int(fields[2]) except ValueError: - error(flavor, name, f'Invalid passwd entry: {line}') + error(node, f'Invalid passwd entry: {line}') return None, None return uid, fields @@ -183,20 +186,20 @@ def read_passwd(flavor: str, name: str, default_uid: int, fd): uid, fields = read_passwd_line(line.decode()) if uid in entries: - error(flavor, name, f'found duplicated uid in /etc/passw: {uid}') + error(node, f'found duplicated uid in /etc/passw: {uid}') else: entries[uid] = fields if 0 not in entries: error(flavor, name, f'No root (uid=0) found in /etc/passwd') elif entries[0][0] != 'root': - error(flavor, name, f'/etc/passwd has a uid=0, but it is not root: {entries[0][0]}') + error(node, f'/etc/passwd has a uid=0, but it is not root: {entries[0][0]}') if default_uid is not None and default_uid in entries: - warning(flavor, name, f'/etc/passwd already has an entry for default uid: {entries[default_uid]}') + warning(node, f'/etc/passwd already has an entry for default uid: {entries[default_uid]}') # This logic isn't perfect at listing all boot units, but parsing all of systemd configuration would be too complex. -def read_systemd_enabled_units(flavor: str, name: str, tar) -> dict: +def read_systemd_enabled_units(node, tar) -> dict: config_dirs = ['/usr/local/lib/systemd/system', '/usr/lib/systemd/system', '/etc/systemd/system'] all_files = tar.getnames() @@ -317,7 +320,7 @@ def get_tar_file(tar, path: str, follow_symlink=False, symlink_depth=10): return None, None -def read_tar(flavor: str, name: str, file, elf_magic: str): +def read_tar(node, file, elf_magic: str): with tarfile.open(fileobj=file) as tar: def validate_mode(path: str, mode, uid, gid, max_size = None, optional = False, follow_symlink = False, magic = None, parse_method = None): @@ -330,16 +333,16 @@ def read_tar(flavor: str, name: str, file, elf_magic: str): permissions = oct(info.mode) if permissions not in mode: - warning(flavor, name, f'file: "{path}" has unexpected mode: {permissions} (expected: {mode})') + warning(node, f'file: "{path}" has unexpected mode: {permissions} (expected: {mode})') if info.uid != uid: - warning(flavor, name, f'file: "{path}" has unexpected uid: {info.uid} (expected: {uid})') + warning(node, f'file: "{path}" has unexpected uid: {info.uid} (expected: {uid})') if gid is not None and info.gid != gid: - warning(flavor, name, f'file: "{path}" has unexpected gid: {info.gid} (expected: {gid})') + warning(node, f'file: "{path}" has unexpected gid: {info.gid} (expected: {gid})') if max_size is not None and info.size > max_size: - error(flavor, name, f'file: "{path}" is too big (info.size), max: {max_size}') + error(node, f'file: "{path}" is too big (info.size), max: {max_size}') if magic is not None or parse_method is not None: content = tar.extractfile(real_path) @@ -352,14 +355,14 @@ def read_tar(flavor: str, name: str, file, elf_magic: str): buffer = content.read(256) file_magic = MAGIC.from_buffer(buffer) if not magic.match(file_magic): - error(flavor, name, f'file: "{path}" has unexpected magic type: {file_magic} (expected: {magic})') + error(node, f'file: "{path}" has unexpected magic type: {file_magic} (expected: {magic})') return True def validate_config(path: str, valid_keys: list): _, path = get_tar_file(tar, path, follow_symlink=True) if path is None: - error(flavor, name, f'File "{file}" not found in tar') + error(node, f'File "{file}" not found in tar') return None content = tar.extractfile(path) @@ -370,7 +373,7 @@ def read_tar(flavor: str, name: str, file, elf_magic: str): unexpected_keys = [e for e in keys if e.lower() not in valid_keys] if unexpected_keys: - error(flavor, name, f'Found unexpected_keys in "{path}": {unexpected_keys}') + error(node, f'Found unexpected_keys in "{path}": {unexpected_keys}') else: click.secho(f'Found valid keys in "{path}": {list(keys.keys())}') @@ -384,11 +387,11 @@ def read_tar(flavor: str, name: str, file, elf_magic: str): validate_mode(oobe_command, [oct(0o775), oct(0o755)], 0, 0) if not oobe_command.startswith(USR_LIB_WSL): - warning(flavor, name, f'value for oobe.command is not under {USR_LIB_WSL}: "{oobe_command}"') + warning(node, f'value for oobe.command is not under {USR_LIB_WSL}: "{oobe_command}"') if defaultUid := config.get('oobe.defaultuid', None): if defaultUid != '1000': - warning(flavor, name, f'Default UID is not 1000. Found: {defaultUid}') + warning(node, f'Default UID is not 1000. Found: {defaultUid}') defaultUid = int(defaultUid) @@ -396,37 +399,39 @@ def read_tar(flavor: str, name: str, file, elf_magic: str): validate_mode(shortcut_icon, [oct(0o664), oct(0o644)], 0, 0, 1024 * 1024) if not shortcut_icon.startswith(USR_LIB_WSL): - warning(flavor, name, f'value for shortcut.icon is not under {USR_LIB_WSL}: "{shortcut_icon}"') + warning(node, f'value for shortcut.icon is not under {USR_LIB_WSL}: "{shortcut_icon}"') if terminal_profile := config.get('windowsterminal.profileTemplate', None): validate_mode(terminal_profile, [oct(0o660), oct(0o640)], 0, 0, 1024 * 1024) if not terminal_profile.startswith(USR_LIB_WSL): - warning(flavor, name, f'value for windowsterminal.profileTemplate is not under {USR_LIB_WSL}: "{terminal_profile}"') + warning(node, f'value for windowsterminal.profileTemplate is not under {USR_LIB_WSL}: "{terminal_profile}"') if validate_mode('/etc/wsl.conf', [oct(0o664), oct(0o644)], 0, 0, optional=True): config = validate_config('/etc/wsl.conf', ['boot.systemd']) if config.get('boot.systemd', False): validate_mode('/sbin/init', [oct(0o775), oct(0o755)], 0, 0, magic=elf_magic, follow_symlink=True) - validate_mode('/etc/passwd', [oct(0o664), oct(0o644)], 0, 0, parse_method = lambda fd: read_passwd(flavor, name, defaultUid, fd)) + validate_mode('/etc/passwd', [oct(0o664), oct(0o644)], 0, 0, parse_method = lambda fd: read_passwd(node, defaultUid, fd)) validate_mode('/etc/shadow', [oct(0o640), oct(0o600)], 0, None) validate_mode('/bin/bash', [oct(0o755), oct(0o775)], 0, 0, magic=elf_magic, follow_symlink=True) validate_mode('/bin/sh', [oct(0o755), oct(0o775)], 0, 0, magic=elf_magic, follow_symlink=True) - enabled_systemd_units = read_systemd_enabled_units(flavor, name, tar) + enabled_systemd_units = read_systemd_enabled_units(node, tar) for unit, path in enabled_systemd_units.items(): if unit in DISCOURAGED_SYSTEM_UNITS: - warning(flavor, name, f'Found discouraged system unit: {path}') + warning(node, f'Found discouraged system unit: {path}') -def read_url(flavor: str, name: str, url: dict, elf_magic): +def read_url(url: dict, elf_magic): hash = hashlib.sha256() - if not url['Url'].endswith('.wsl'): - warning(flavor, name, f'Url does not point to a .wsl file: {url["Url"]}') + address = url['Url']() + + if not address.endswith('.wsl'): + warning(url, f'Url does not point to a .wsl file: {address}') tar_format = None - if url['Url'].startswith('file://'): - with open(url['Url'].replace('file:///', '').replace('file://', ''), 'rb') as fd: + if address.startswith('file://'): + with open(address.replace('file:///', '').replace('file://', ''), 'rb') as fd: while True: e = fd.read(4096 * 4096 * 10) if not e: @@ -438,9 +443,9 @@ def read_url(flavor: str, name: str, url: dict, elf_magic): tar_format = MAGIC.from_buffer(e) fd.seek(0, 0) - read_tar(flavor, name, fd, elf_magic) + read_tar(url, fd, elf_magic) else: - with requests.get(url['Url'], stream=True) as response: + with requests.get(address, stream=True) as response: response.raise_for_status() with tempfile.NamedTemporaryFile() as file: @@ -452,42 +457,43 @@ def read_url(flavor: str, name: str, url: dict, elf_magic): tar_format = MAGIC.from_buffer(e) file.seek(0, 0) - read_tar(flavor, name, file, elf_magic) + read_tar(url, file, elf_magic) - expected_sha = url.get('Sha256', None) + expected_sha = url['Sha256']() if 'Sha256' in url else None if expected_sha is None: - error(flavor, name, 'URL is missing "Sha256"') + error(url, 'URL is missing "Sha256"') else: if expected_sha.startswith('0x'): expected_sha = expected_sha[2:] sha = hash.digest() if bytes.fromhex(expected_sha) != sha: - error(flavor, name, f'URL {url["Url"]} Sha256 does not match. Expected: {expected_sha}, actual: {hash.hexdigest()}') + error(url, f'URL {address} Sha256 does not match. Expected: {expected_sha}, actual: {hash.hexdigest()}') else: - click.secho(f'Hash for {url["Url"]} matches ({expected_sha})', fg='green') + click.secho(f'Hash for {address} matches ({expected_sha})', fg='green') known_format = next((value for key, value in KNOWN_TAR_FORMATS.items() if re.match(key, tar_format)), None) if known_format is None: - error(flavor, name, f'Unknown tar format: {tar_format}') + error(url, f'Unknown tar format: {tar_format}') elif not known_format: - warning(flavor, name, f'Tar format not supported by WSL1: {tar_format}') + warning(url, f'Tar format not supported by WSL1: {tar_format}') -def error(flavor: str, distribution: str, message: str): +def error(node, message: str): global errors - message = f'{flavor}/{distribution}: {message}' - click.secho(f'Error: {message}', fg='red') + line = jsoncfg.node_location(node).line + click.secho(f'Error on line {line}: {message}', fg='red') - errors.append(message) + errors[line] = errors.get(line, []) + [message] -def warning(flavor: str, distribution: str, message: str): +def warning(node, message: str): global warnings - message = f'{flavor}/{distribution}: {message}' - click.secho(f'Warning: {message}', fg='yellow') + line = jsoncfg.node_location(node).line + click.secho(f'Warning on line {line}: {message}', fg='yellow') + + warnings[line] = warnings.get(line, []) + [message] - warnings.append(message) if __name__ == "__main__": main() \ No newline at end of file