asf-tooling commented on issue #180:
URL:
https://github.com/apache/tooling-trusted-releases/issues/180#issuecomment-4410440121
<!-- gofannon-issue-triage-bot v2 -->
**Automated triage** — analyzed at `main@2da7807a`
**Type:** `new_feature` • **Classification:** `actionable` •
**Confidence:** `medium`
**Application domain(s):** `cryptographic_keys`, `release_lifecycle`
### Summary
The issue requests preventing deletion of OpenPGP keys that have been used
to sign releases. Currently, `FoundationCommitter.delete_key` in
`atr/storage/writers/keys.py` deletes keys unconditionally without checking
whether they've signed any releases. @dave2wave's most recent comment
(2026-03-18) indicates that issue #911 introduces a table that will be used to
determine if a particular signature is in use. The core fix is to add a guard
in `delete_key` that queries for signature check results referencing the key's
fingerprint before allowing deletion.
### Where this lives in the code today
#### `atr/storage/writers/keys.py` — `FoundationCommitter.delete_key` (lines
210-230)
_needs modification_
This method deletes keys unconditionally; it needs a guard to check if the
key has been used to sign any release.
```python
async def delete_key(self, fingerprint: str) ->
outcome.Outcome[sql.PublicSigningKey]:
try:
key = await self.__data.public_signing_key(
fingerprint=fingerprint,
apache_uid=self.__asf_uid,
_committees=True,
).demand(storage.AccessError(f"Key not found: {fingerprint}",
status=404))
affected_committee_keys = {committee.key for committee in
key.committees}
await self.__data.delete(key)
await self.__data.commit()
self.__write_as.append_to_audit_log(
action="key_delete",
asf_uid=self.__asf_uid,
fingerprint=key.fingerprint,
committee_keys=[k for k in sorted(affected_committee_keys)],
)
for committee_key in sorted(affected_committee_keys):
await self._sync_committee_keys_file(committee_key)
return outcome.Result(key)
except Exception as e:
return outcome.Error(e)
```
#### `atr/post/keys.py` — `_delete_openpgp_key` (lines 275-289)
_currently does this_
POST handler that invokes delete_key; will automatically surface the new
error through the existing outcome.Error path.
```python
async def _delete_openpgp_key(
session: web.Committer, delete_form: shared.keys.DeleteOpenPGPKeyForm
) -> web.WerkzeugResponse:
"""Delete an OpenPGP key from the user's account."""
fingerprint = delete_form.fingerprint
async with storage.write() as write:
wafc = write.as_foundation_committer()
oc: outcome.Outcome[sql.PublicSigningKey] = await
wafc.keys.delete_key(fingerprint)
match oc:
case outcome.Result():
return await session.redirect(get.keys.keys, success="OpenPGP
key deleted successfully")
case outcome.Error(error):
return await session.redirect(get.keys.keys, error=f"Error
deleting OpenPGP key: {error}")
```
### Where new code would go
- `atr/storage/writers/keys.py` — before delete_key method body
Add a private method to check whether a key fingerprint has been used in
any successful signature verification, then call it as a guard at the start of
delete_key.
### Proposed approach
@dave2wave noted (2026-03-18) that issue #911 introduces a table to
determine if a particular signature is in use. The ideal implementation would
use that table. However, the current codebase already stores signature
fingerprints in CheckResult.data['fingerprint'] for successful signature
checks. The proposed approach adds a guard in `FoundationCommitter.delete_key`
that queries CheckResult for any successful signature verification referencing
the key's fingerprint. If found, deletion is refused with a descriptive error.
This can later be updated to use the #911 table when available.
The UI in `atr/get/keys.py` could also be enhanced to disable/hide the
delete button for keys with associated signatures, but the server-side guard is
the critical piece for data integrity.
### Suggested patches
#### `atr/storage/writers/keys.py`
Add a check in delete_key to refuse deletion if the key fingerprint appears
in any successful signature check result.
````diff
--- a/atr/storage/writers/keys.py
+++ b/atr/storage/writers/keys.py
@@ -149,6 +149,8 @@
# Specific to this module
self.__key_block_models_cache = {}
+ _SIGNATURE_CHECKER_KEY: str = "atr.tasks.checks.signature:check" #
TODO: confirm matches checks.function_key(signature.check)
+
async def delete_key(self, fingerprint: str) ->
outcome.Outcome[sql.PublicSigningKey]:
try:
key = await self.__data.public_signing_key(
@@ -156,6 +158,12 @@
apache_uid=self.__asf_uid,
_committees=True,
).demand(storage.AccessError(f"Key not found: {fingerprint}",
status=404))
+
+ # Refuse deletion if this key has been used to sign any release
+ if await self.__key_has_signatures(fingerprint):
+ raise storage.AccessError(
+ f"Key {fingerprint.upper()} cannot be deleted because
it has been used to sign release artifacts.",
+ status=409,
+ )
affected_committee_keys = {committee.key for committee in
key.committees}
await self.__data.delete(key)
await self.__data.commit()
@@ -170,6 +178,24 @@
except Exception as e:
return outcome.Error(e)
+ async def __key_has_signatures(self, fingerprint: str) -> bool:
+ """Check if a key fingerprint appears in any successful signature
check result."""
+ # TODO: When #911 table is available, query that instead for a
cleaner lookup
+ via = sql.validate_instrumented_attribute
+ fingerprint_lower = fingerprint.lower()
+ # Query CheckResult for successful signature checks that reference
this fingerprint
+ # The fingerprint is stored in CheckResult.data JSON field as
data['fingerprint']
+ stmt = (
+ sqlmodel.select(via(sql.CheckResult.id))
+ .where(
+ via(sql.CheckResult.checker) == self._SIGNATURE_CHECKER_KEY,
+ via(sql.CheckResult.status) ==
sql.CheckResultStatus.SUCCESS,
+ sqlalchemy.func.json_extract(via(sql.CheckResult.data),
"$.fingerprint") == fingerprint_lower,
+ )
+ .limit(1)
+ )
+ result = await self.__data.execute(stmt)
+ return result.first() is not None
+
async def ensure_stored_one(self, key_file_text: str) ->
outcome.Outcome[types.Key]:
return await self.__ensure_one(key_file_text, associate=False)
````
### Open questions
- Issue #911 introduces a dedicated table for tracking key-to-signature
relationships. Should this implementation wait for that table, or use the
CheckResult JSON query as a stopgap?
- The exact value of the signature checker key string needs verification -
it's constructed via checks.function_key(signature.check) which I cannot see
the implementation of.
- Should the fingerprint comparison be case-insensitive? The code normalizes
to lowercase in some places but the check result data may store it differently.
- Should there be a way to force-delete a key (e.g., for admins) even if it
has signatures, perhaps with additional audit logging?
- The CheckResult.data field type and whether SQLite json_extract works on
it needs verification - it may be stored as TEXT or JSON depending on the
SQLModel column definition.
- Per @sebbASF's comment, should keys also be prevented from being removed
from committee associations (not just deleted entirely) if they've signed
releases for that committee?
### Files examined
- `atr/storage/writers/keys.py`
- `atr/post/keys.py`
- `atr/shared/keys.py`
- `atr/get/keys.py`
- `atr/storage/writers/release.py`
- `atr/storage/readers/releases.py`
---
*Draft from a triage agent. A human reviewer should validate before merging
any change. The agent did not run tests or verify diffs apply.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
