From b490c842dd33c67eb7ea71343ff80300a3c1e2f8 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Sat, 2 Mar 2024 12:58:15 +0200 Subject: [PATCH 1/2] signer: Refactor pushing and get_repo_name This is shared code between sign and delegate Also refactor get_repo_name(): * move to _common: It will be needed there soon * Add sanity check, comments --- signer/tuf_on_ci_sign/_common.py | 36 +++++++++++++++++++++++++++++++ signer/tuf_on_ci_sign/delegate.py | 28 ++++-------------------- signer/tuf_on_ci_sign/sign.py | 15 ++----------- 3 files changed, 42 insertions(+), 37 deletions(-) diff --git a/signer/tuf_on_ci_sign/_common.py b/signer/tuf_on_ci_sign/_common.py index 0c94e450..484af268 100644 --- a/signer/tuf_on_ci_sign/_common.py +++ b/signer/tuf_on_ci_sign/_common.py @@ -10,6 +10,7 @@ from contextlib import contextmanager from datetime import datetime, timedelta from tempfile import TemporaryDirectory +from urllib import parse from urllib.request import Request, urlopen import click @@ -154,3 +155,38 @@ def application_update_reminder() -> None: except Exception as e: # noqa: BLE001 logger.warning(f"Failed to check current tuf-on-ci-sign version: {e}") + + +def push_changes(user: User, event_name: str) -> None: + """Push the event branch to users push remote""" + branch = f"{user.push_remote}/{event_name}" + msg = f"Press enter to push changes to {branch}" + click.prompt(bold(msg), default=True, show_default=False) + git_echo( + [ + "push", + "--progress", + user.push_remote, + f"HEAD:refs/heads/{event_name}", + ] + ) + + +def get_repo_name(remote: str) -> str: + """Return 'owner/repo' string for given GitHub remote""" + url = parse.urlparse(git_expect(["config", "--get", f"remote.{remote}.url"])) + owner_repo = url.path[: -len(".git")] + # ssh-urls are relative URLs according to urllib: host is actually part of + # path. We don't want the host part: + _, _, owner_repo = owner_repo.rpartition(":") + # http urls on the other hand are not relative: remove the leading / + owner_repo = owner_repo.lstrip("/") + + # sanity check + owner, slash, repo = owner_repo.partition("/") + if not owner or slash != "/" or not repo: + raise RuntimeError( + "Failed to parse GitHub repository from git URL {url} for remote {remote}" + ) + + return owner_repo diff --git a/signer/tuf_on_ci_sign/delegate.py b/signer/tuf_on_ci_sign/delegate.py index 4e863d1b..ebebcd15 100755 --- a/signer/tuf_on_ci_sign/delegate.py +++ b/signer/tuf_on_ci_sign/delegate.py @@ -7,7 +7,6 @@ import os import re from copy import deepcopy -from urllib import parse import click from securesystemslib.signer import ( @@ -23,9 +22,10 @@ from tuf_on_ci_sign._common import ( application_update_reminder, bold, + get_repo_name, get_signing_key_input, - git_echo, git_expect, + push_changes, signing_event, ) from tuf_on_ci_sign._signer_repository import ( @@ -118,20 +118,10 @@ def verify_signers(response: str) -> list[str]: return config -def _get_repo_name(remote: str): - url = parse.urlparse(git_expect(["config", "--get", f"remote.{remote}.url"])) - repo = url.path[: -len(".git")] - # ssh-urls are relative URLs according to urllib: host is actually part of - # path. We don't want the host part: - _, _, repo = repo.rpartition(":") - # http urls on the other hand are not relative: remove the leading / - return repo.lstrip("/") - - def _sigstore_import(pull_remote: str) -> Key: # WORKAROUND: build sigstore key and uri here since there is no import yet issuer = "https://token.actions.githubusercontent.com" - repo = _get_repo_name(pull_remote) + repo = get_repo_name(pull_remote) id = f"https://github.com/{repo}/.github/workflows/online-sign.yml@refs/heads/main" key = SigstoreKey( @@ -392,17 +382,7 @@ def delegate(verbose: int, push: bool, event_name: str, role: str | None): ) if push: - branch = f"{user_config.push_remote}/{event_name}" - msg = f"Press enter to push changes to {branch}" - click.prompt(bold(msg), default=True, show_default=False) - git_echo( - [ - "push", - "--progress", - user_config.push_remote, - f"HEAD:refs/heads/{event_name}", - ] - ) + push_changes(user_config, event_name) else: # TODO: deal with existing branch? click.echo(f"Creating local branch {event_name}") diff --git a/signer/tuf_on_ci_sign/sign.py b/signer/tuf_on_ci_sign/sign.py index 98a7ac52..f9fd2ecf 100755 --- a/signer/tuf_on_ci_sign/sign.py +++ b/signer/tuf_on_ci_sign/sign.py @@ -9,10 +9,9 @@ from tuf_on_ci_sign._common import ( application_update_reminder, - bold, get_signing_key_input, - git_echo, git_expect, + push_changes, signing_event, ) from tuf_on_ci_sign._signer_repository import SignerState @@ -73,17 +72,7 @@ def sign(verbose: int, push: bool, event_name: str): git_expect(["add", "metadata"]) git_expect(["commit", "-m", f"Signed by {user_config.name}", "--signoff"]) if push: - branch = f"{user_config.push_remote}/{event_name}" - msg = f"Press enter to push signature(s) to {branch}" - click.prompt(bold(msg), default=True, show_default=False) - git_echo( - [ - "push", - "--progress", - user_config.push_remote, - f"HEAD:refs/heads/{event_name}", - ] - ) + push_changes(user_config, event_name) else: # TODO: maybe deal with existing branch? click.echo(f"Creating local branch {event_name}") From 34717e804148dd2bf4ecaadb900a828f2a1530a9 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Sat, 2 Mar 2024 13:03:19 +0200 Subject: [PATCH 2/2] signer: Open PR in browser when using forks * use webbrowser module to open PR in browser when fork is used * Allow use of a specific PR template (e.g. ".github/PULL_REQUEST_TEMPLATE/signing_event.md") * Tweak the commit and PR messages: * "Signature from " * " accepted invitation" * " role/delegation change" * "Initial root and targets" * Remove "--progress" from push: This should remove most git output * Use --force when using a fork: * This is useful because a signer may make multiple PRs into a signing event (this is not very common but can happen e.g. if multiple signers are added in one event). * This is safe since any existing, unmerged PRs should be obsoleted by this new push. --- signer/tuf_on_ci_sign/_common.py | 47 +++++++++++++++++++++++++------ signer/tuf_on_ci_sign/delegate.py | 2 +- signer/tuf_on_ci_sign/sign.py | 14 ++++----- 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/signer/tuf_on_ci_sign/_common.py b/signer/tuf_on_ci_sign/_common.py index 484af268..8427a1c0 100644 --- a/signer/tuf_on_ci_sign/_common.py +++ b/signer/tuf_on_ci_sign/_common.py @@ -6,6 +6,7 @@ import logging import os import subprocess +import webbrowser from collections.abc import Generator from contextlib import contextmanager from datetime import datetime, timedelta @@ -157,19 +158,47 @@ def application_update_reminder() -> None: logger.warning(f"Failed to check current tuf-on-ci-sign version: {e}") -def push_changes(user: User, event_name: str) -> None: +def push_changes(user: User, event_name: str, title: str) -> None: """Push the event branch to users push remote""" branch = f"{user.push_remote}/{event_name}" msg = f"Press enter to push changes to {branch}" click.prompt(bold(msg), default=True, show_default=False) - git_echo( - [ - "push", - "--progress", - user.push_remote, - f"HEAD:refs/heads/{event_name}", - ] - ) + if user.push_remote == user.pull_remote: + # maintainer flow: just push to signing event branch + git_echo( + [ + "push", + user.push_remote, + f"HEAD:refs/heads/{event_name}", + ] + ) + else: + # non-maintainer flow: push to fork, make a PR. + # NOTE: we force push: this is safe since any existing fork branches + # have either been merged or are obsoleted by this push + git_echo( + [ + "push", + "--force", + user.push_remote, + f"HEAD:refs/heads/{event_name}", + ] + ) + # Create PR from fork (push remote) to upstream (pull remote) + upstream = get_repo_name(user.pull_remote) + fork = get_repo_name(user.push_remote).replace("/", ":") + query = parse.urlencode( + { + "quick_pull": 1, + "title": title, + "template": "signing_event.md", + } + ) + pr_url = f"https://github.com/{upstream}/compare/{event_name}...{fork}:{event_name}?{query}" + if webbrowser.open(pr_url): + click.echo(bold("Please submit the pull request in your browser.")) + else: + click.echo(bold(f"Please submit the pull request:\n {pr_url}")) def get_repo_name(remote: str) -> str: diff --git a/signer/tuf_on_ci_sign/delegate.py b/signer/tuf_on_ci_sign/delegate.py index ebebcd15..0b4f3d98 100755 --- a/signer/tuf_on_ci_sign/delegate.py +++ b/signer/tuf_on_ci_sign/delegate.py @@ -382,7 +382,7 @@ def delegate(verbose: int, push: bool, event_name: str, role: str | None): ) if push: - push_changes(user_config, event_name) + push_changes(user_config, event_name, msg) else: # TODO: deal with existing branch? click.echo(f"Creating local branch {event_name}") diff --git a/signer/tuf_on_ci_sign/sign.py b/signer/tuf_on_ci_sign/sign.py index f9fd2ecf..23f41118 100755 --- a/signer/tuf_on_ci_sign/sign.py +++ b/signer/tuf_on_ci_sign/sign.py @@ -38,7 +38,7 @@ def sign(verbose: int, push: bool, event_name: str): with signing_event(event_name, user_config) as repo: if repo.state == SignerState.UNINITIALIZED: click.echo("No metadata repository found") - changed = False + change_status = None elif repo.state == SignerState.INVITED: click.echo( f"You have been invited to become a signer for role(s) {repo.invites}." @@ -56,23 +56,23 @@ def sign(verbose: int, push: bool, event_name: str): for rolename in repo.unsigned: click.echo(repo.status(rolename)) repo.sign(rolename) - changed = True + change_status = f"{user_config.name} accepted invitation" elif repo.state == SignerState.SIGNATURE_NEEDED: click.echo(f"Your signature is requested for role(s) {repo.unsigned}.") for rolename in repo.unsigned: click.echo(repo.status(rolename)) repo.sign(rolename) - changed = True + change_status = f"Signature from {user_config.name}" elif repo.state == SignerState.NO_ACTION: - changed = False + change_status = None else: raise NotImplementedError - if changed: + if change_status: git_expect(["add", "metadata"]) - git_expect(["commit", "-m", f"Signed by {user_config.name}", "--signoff"]) + git_expect(["commit", "-m", change_status, "--signoff"]) if push: - push_changes(user_config, event_name) + push_changes(user_config, event_name, change_status) else: # TODO: maybe deal with existing branch? click.echo(f"Creating local branch {event_name}")