This is an automated email from the ASF dual-hosted git repository.
sbp pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tooling-trusted-release.git
The following commit(s) were added to refs/heads/main by this push:
new ab6a518 Convert signature checks to check result style
ab6a518 is described below
commit ab6a518ba4839992c040f5e90398fb72f9219004
Author: Sean B. Palmer <[email protected]>
AuthorDate: Mon Mar 31 16:44:51 2025 +0100
Convert signature checks to check result style
---
atr/routes/keys.py | 73 ++++++++++++++++---------
atr/tasks/__init__.py | 30 ++++++-----
atr/tasks/{ => checks}/signature.py | 101 ++++++++++++++++++++++-------------
atr/templates/draft-review-path.html | 21 ++++++++
atr/worker.py | 4 +-
5 files changed, 151 insertions(+), 78 deletions(-)
diff --git a/atr/routes/keys.py b/atr/routes/keys.py
index 6f0fea1..127b163 100644
--- a/atr/routes/keys.py
+++ b/atr/routes/keys.py
@@ -113,38 +113,56 @@ def key_ssh_fingerprint(ssh_key_string: str) -> str:
raise ValueError("Invalid SSH key format")
-async def key_user_add(asf_uid: str | None, public_key: str,
selected_committees: list[str]) -> dict | None:
- if not public_key:
- raise routes.FlashError("Public key is required")
-
- # Import the key into GPG to validate and extract info
- # TODO: We'll just assume for now that gnupg.GPG() doesn't need to be async
+async def _key_user_add_validate_key_properties(public_key: str) ->
tuple[dict, str]:
+ """Validate GPG key string, import it, and return its properties and
fingerprint."""
async with ephemeral_gpg_home() as gpg_home:
gpg = gnupg.GPG(gnupghome=gpg_home)
import_result = await asyncio.to_thread(gpg.import_keys, public_key)
if not import_result.fingerprints:
- raise routes.FlashError("Invalid public key format")
+ raise routes.FlashError("Invalid public key format or failed
import")
fingerprint = import_result.fingerprints[0]
- if fingerprint is not None:
- fingerprint = fingerprint.lower()
- # APP.logger.info("Import result: %s", vars(import_result))
- # Get key details
- # We could probably use import_result instead
- # But this way it shows that they've really been imported
+ if fingerprint is None:
+ # Should be unreachable given the previous check, but satisfy type
checker
+ raise routes.FlashError("Failed to get fingerprint after import")
+ fingerprint_lower = fingerprint.lower()
+
+ # List keys to get details
keys = await asyncio.to_thread(gpg.list_keys)
- # Then we have the properties listed here:
- # https://gnupg.readthedocs.io/en/latest/#listing-keys
- # Note that "fingerprint" is not listed there, but we have it anyway...
- key = next((k for k in keys if (k["fingerprint"] is not None) and
(k["fingerprint"].lower() == fingerprint)), None)
- if not key:
- raise routes.FlashError("Failed to import key")
- if (key.get("algo") == "1") and (int(key.get("length", "0")) < 2048):
- # https://infra.apache.org/release-signing.html#note
- # Says that keys must be at least 2048 bits
- raise routes.FlashError("Key is not long enough; must be at least 2048
bits")
+ # Find the specific key details from the list using the fingerprint
+ key_details = None
+ for k in keys:
+ if k.get("fingerprint") is not None and k["fingerprint"].lower() ==
fingerprint_lower:
+ key_details = k
+ break
+
+ if not key_details:
+ # This might indicate an issue with gpg.list_keys or the environment
+ logging.error(
+ f"Could not find key details for fingerprint {fingerprint_lower}"
+ f" after successful import. Keys listed: {keys}"
+ )
+ raise routes.FlashError("Failed to retrieve key details after import")
+
+ # Validate key algorithm and length
+ # https://infra.apache.org/release-signing.html#note
+ # Says that keys must be at least 2048 bits
+ if (key_details.get("algo") == "1") and (int(key_details.get("length",
"0")) < 2048):
+ raise routes.FlashError("RSA Key is not long enough; must be at least
2048 bits")
+
+ return key_details, fingerprint_lower
+
+
+async def key_user_add(asf_uid: str | None, public_key: str,
selected_committees: list[str]) -> dict | None:
+ if not public_key:
+ raise routes.FlashError("Public key is required")
+
+ # Validate the key using GPG and get its properties
+ key, _fingerprint = await _key_user_add_validate_key_properties(public_key)
+
+ # Determine ASF UID if not provided
if asf_uid is None:
for uid in key["uids"]:
match = re.search(r"([A-Za-z0-9]+)@apache.org", uid)
@@ -152,7 +170,7 @@ async def key_user_add(asf_uid: str | None, public_key:
str, selected_committees
asf_uid = match.group(1).lower()
break
else:
- logging.warning(f"key_user_add called with no ASF UID: {keys}")
+ logging.warning(f"key_user_add called with no ASF UID found in key
UIDs: {key.get('uids')}")
if asf_uid is None:
# We place this here to make it easier on the type checkers
raise routes.FlashError("No Apache UID found in the key UIDs")
@@ -178,6 +196,13 @@ async def key_user_session_add(
# return ("You already have a key registered", None)
fingerprint = key.get("fingerprint")
+ # for subkey in key.get("subkeys", []):
+ # if subkey[1] == "s":
+ # # It's a signing key, so use its fingerprint instead
+ # # TODO: Not sure that we should do this
+ # # TODO: Check for multiple signing subkeys
+ # fingerprint = subkey[2]
+ # break
if not isinstance(fingerprint, str):
raise routes.FlashError("Invalid key fingerprint")
fingerprint = fingerprint.lower()
diff --git a/atr/tasks/__init__.py b/atr/tasks/__init__.py
index 6cb5d78..45eb28d 100644
--- a/atr/tasks/__init__.py
+++ b/atr/tasks/__init__.py
@@ -23,6 +23,7 @@ import atr.tasks.checks.archive as archive
import atr.tasks.checks.hashing as hashing
import atr.tasks.checks.license as license
import atr.tasks.checks.rat as rat
+import atr.tasks.checks.signature as signature
import atr.util as util
@@ -42,12 +43,13 @@ async def asc_checks(release: models.Release,
signature_path: str) -> list[model
tasks.append(
models.Task(
status=models.TaskStatus.QUEUED,
- task_type="verify_signature",
- task_args=[
- release.committee.name,
- full_artifact_path,
- full_signature_path,
- ],
+ task_type=checks.function_key(signature.check),
+ task_args=signature.Check(
+ release_name=release.name,
+ committee_name=release.committee.name,
+ abs_artifact_path=full_artifact_path,
+ abs_signature_path=full_signature_path,
+ ).model_dump(),
release_name=release.name,
path=signature_path,
modified=modified,
@@ -139,14 +141,14 @@ async def tar_gz_checks(release: models.Release, path:
str) -> list[models.Task]
path=path,
modified=modified,
),
- models.Task(
- status=models.TaskStatus.QUEUED,
- task_type="generate_cyclonedx_sbom",
- task_args=[full_path],
- release_name=release.name,
- path=path,
- modified=modified,
- ),
+ # models.Task(
+ # status=models.TaskStatus.QUEUED,
+ # task_type="generate_cyclonedx_sbom",
+ # task_args=[full_path],
+ # release_name=release.name,
+ # path=path,
+ # modified=modified,
+ # ),
]
return tasks
diff --git a/atr/tasks/signature.py b/atr/tasks/checks/signature.py
similarity index 51%
rename from atr/tasks/signature.py
rename to atr/tasks/checks/signature.py
index f80406d..7b9595b 100644
--- a/atr/tasks/signature.py
+++ b/atr/tasks/checks/signature.py
@@ -15,68 +15,89 @@
# specific language governing permissions and limitations
# under the License.
-import contextlib
+import asyncio
import logging
-import shutil
import tempfile
-from collections.abc import Generator
-from typing import Any, BinaryIO, Final
+from typing import Any, Final
import gnupg
-import sqlalchemy.sql as sql
+import pydantic
+import sqlmodel
import atr.db as db
import atr.db.models as models
-import atr.tasks.task as task
+import atr.tasks.checks as checks
_LOGGER = logging.getLogger(__name__)
-def check(args: list[str]) -> tuple[models.TaskStatus, str | None, tuple[Any,
...]]:
+class Check(pydantic.BaseModel):
+ """Parameters for signature checking."""
+
+ release_name: str = pydantic.Field(..., description="Release name")
+ committee_name: str = pydantic.Field(..., description="Name of the
committee whose keys should be used")
+ abs_artifact_path: str = pydantic.Field(..., description="Absolute path to
the artifact file")
+ abs_signature_path: str = pydantic.Field(..., description="Absolute path
to the signature file (.asc)")
+
+
[email protected]_model(Check)
+async def check(args: Check) -> str | None:
"""Check a signature file."""
- task_results = task.results_as_tuple(_check_core(*args))
- _LOGGER.info(f"Verified {args} with result {task_results[0]}")
- status = task.FAILED if task_results[0].get("error") else task.COMPLETED
- error = task_results[0].get("error")
- return status, error, task_results
+ rel_path = checks.rel_path(args.abs_signature_path)
+ check_instance = await checks.Check.create(checker=check,
release_name=args.release_name, path=rel_path)
+ _LOGGER.info(
+ f"Checking signature {args.abs_signature_path} for
{args.abs_artifact_path}"
+ f" using {args.committee_name} keys (rel: {rel_path})"
+ )
+
+ try:
+ result_data = await _check_core_logic(
+ committee_name=args.committee_name,
+ artifact_path=args.abs_artifact_path,
+ signature_path=args.abs_signature_path,
+ )
+ if result_data.get("error"):
+ await check_instance.failure(result_data["error"], result_data)
+ elif result_data.get("verified"):
+ await check_instance.success("Signature verified successfully",
result_data)
+ else:
+ # Shouldn't happen
+ await check_instance.exception("Signature verification failed for
unknown reasons", result_data)
+ except Exception as e:
+ await check_instance.exception("Error during signature check
execution", {"error": str(e)})
-def _check_core(committee_name: str, artifact_path: str, signature_path: str)
-> dict[str, Any]:
+ return None
+
+
+async def _check_core_logic(committee_name: str, artifact_path: str,
signature_path: str) -> dict[str, Any]:
"""Verify a signature file using the committee's public signing keys."""
- # Query only the signing keys associated with this committee
- # TODO: Rename create_sync_db_session to create_session_sync
- # Using isinstance does not work here, with pyright
+ _LOGGER.info(f"Attempting to fetch keys for committee_name:
'{committee_name}'")
name = db.validate_instrumented_attribute(models.Committee.name)
- with db.create_sync_db_session() as session:
- # TODO: This is our only remaining use of select
+ async with db.session() as session:
statement = (
- sql.select(models.PublicSigningKey)
+ sqlmodel.select(models.PublicSigningKey)
.join(models.KeyLink)
.join(models.Committee)
.where(name == committee_name)
)
- result = session.execute(statement)
+ result = await session.execute(statement)
public_keys = [key.ascii_armored_key for key in result.scalars().all()]
- with open(signature_path, "rb") as sig_file:
- return _signature_gpg_file(sig_file, artifact_path, public_keys)
-
-
[email protected]
-def _ephemeral_gpg_home() -> Generator[str]:
- # TODO: Deduplicate, and move somewhere more appropriate
- """Create a temporary directory for an isolated GPG home, and clean it up
on exit."""
- temp_dir = tempfile.mkdtemp(prefix="gpg-")
- try:
- yield temp_dir
- finally:
- shutil.rmtree(temp_dir)
+ return await asyncio.to_thread(
+ _check_core_logic_verify_signature,
+ signature_path=signature_path,
+ artifact_path=artifact_path,
+ ascii_armored_keys=public_keys,
+ )
-def _signature_gpg_file(sig_file: BinaryIO, artifact_path: str,
ascii_armored_keys: list[str]) -> dict[str, Any]:
+def _check_core_logic_verify_signature(
+ signature_path: str, artifact_path: str, ascii_armored_keys: list[str]
+) -> dict[str, Any]:
"""Verify a GPG signature for a file."""
- with _ephemeral_gpg_home() as gpg_home:
- gpg: Final[gnupg.GPG] = gnupg.GPG(gnupghome=gpg_home)
+ with tempfile.TemporaryDirectory(prefix="gpg-") as gpg_dir,
open(signature_path, "rb") as sig_file:
+ gpg: Final[gnupg.GPG] = gnupg.GPG(gnupghome=gpg_dir)
# Import all PMC public signing keys
for key in ascii_armored_keys:
@@ -103,14 +124,18 @@ def _signature_gpg_file(sig_file: BinaryIO,
artifact_path: str, ascii_armored_ke
}
if not verified:
- raise task.Error("No valid signature found", debug_info)
+ return {
+ "verified": False,
+ "error": "No valid signature found",
+ "debug_info": debug_info,
+ }
return {
"verified": True,
"key_id": verified.key_id,
"timestamp": verified.timestamp,
"username": verified.username or "Unknown",
- "email": verified.pubkey_fingerprint.lower() or "Unknown",
+ "fingerprint": verified.pubkey_fingerprint.lower() or "Unknown",
"status": "Valid signature",
"debug_info": debug_info,
}
diff --git a/atr/templates/draft-review-path.html
b/atr/templates/draft-review-path.html
index 43e7883..967fa37 100644
--- a/atr/templates/draft-review-path.html
+++ b/atr/templates/draft-review-path.html
@@ -176,6 +176,27 @@
</tr>
{% endif %}
{% endfor %}
+
+ {% if check_result.data.get("debug_info") is mapping %}
+ {% for debug_key, debug_value in
check_result.data.debug_info.items() %}
+ <tr>
+ <th class="bg-light fw-bold align-top">
+ <span class="text-muted">(Debug)</span> {{
debug_key|replace('_', ' ') |title }}
+ </th>
+ <td>
+ {% if debug_key == 'stderr' %}
+ <pre class="mb-0 small"><code class="small">{{
debug_value }}</code></pre>
+ {% elif debug_value is boolean %}
+ {{ "Yes" if debug_value else "No" }}
+ {% elif debug_value is none %}
+ <span class="text-muted">(None)</span>
+ {% else %}
+ {{ debug_value }}
+ {% endif %}
+ </td>
+ </tr>
+ {% endfor %}
+ {% endif %}
</tbody>
</table>
</details>
diff --git a/atr/worker.py b/atr/worker.py
index 1e0bd0f..0ee92cf 100644
--- a/atr/worker.py
+++ b/atr/worker.py
@@ -43,10 +43,10 @@ import atr.tasks.checks.archive as archive
import atr.tasks.checks.hashing as hashing
import atr.tasks.checks.license as license
import atr.tasks.checks.rat as rat
+import atr.tasks.checks.signature as signature
import atr.tasks.mailtest as mailtest
import atr.tasks.rsync as rsync
import atr.tasks.sbom as sbom
-import atr.tasks.signature as signature
import atr.tasks.task as task
import atr.tasks.vote as vote
@@ -193,6 +193,7 @@ async def _task_process(task_id: int, task_type: str,
task_args: list[str] | dic
checks.function_key(license.files): license.files,
checks.function_key(license.headers): license.headers,
checks.function_key(rat.check): rat.check,
+ checks.function_key(signature.check): signature.check,
}
# TODO: We should use a decorator to register these automatically
dict_task_handlers = {
@@ -202,7 +203,6 @@ async def _task_process(task_id: int, task_type: str,
task_args: list[str] | dic
# TODO: These are synchronous
# We plan to convert these to async dict handlers
list_task_handlers = {
- "verify_signature": signature.check,
"generate_cyclonedx_sbom": sbom.generate_cyclonedx,
"mailtest_send": mailtest.send,
"vote_initiate": vote.initiate,
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
