Просмотр исходного кода

Improve signature vendor API and exception handling

Add vendor-agnostic exceptions (BadSignature, UntrustedSignature) that
preserve metadata like signing and trusted keys. Update all signature
vendors to raise these exceptions instead of implementation-specific ones.

Fix SSHCliSignatureVendor to reject keyids parameter with clear error
message, since it only supports allowedSignersFile configuration.
Jelmer Vernooij 2 недель назад
Родитель
Сommit
44999ea67f
4 измененных файлов с 440 добавлено и 146 удалено
  1. 224 73
      dulwich/signature.py
  2. 1 0
      tests/__init__.py
  3. 133 33
      tests/porcelain/__init__.py
  4. 82 40
      tests/test_signature.py

+ 224 - 73
dulwich/signature.py

@@ -1,8 +1,9 @@
 # signature.py -- Signature vendors for signing and verifying Git objects
 # Copyright (C) 2025 Jelmer Vernooij <jelmer@jelmer.uk>
 #
+# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
 # Dulwich is dual-licensed under the Apache License, Version 2.0 and the GNU
-# General Public License as public by the Free Software Foundation; version 2.0
+# General Public License as published by the Free Software Foundation; version 2.0
 # or (at your option) any later version. You can redistribute it and/or
 # modify it under the terms of either of these two licenses.
 #
@@ -26,6 +27,70 @@ from typing import TYPE_CHECKING
 if TYPE_CHECKING:
     from dulwich.config import Config
 
+__all__ = [
+    "SIGNATURE_FORMAT_OPENPGP",
+    "SIGNATURE_FORMAT_SSH",
+    "SIGNATURE_FORMAT_X509",
+    "BadSignature",
+    "SignatureVendor",
+    "SignatureVerificationError",
+    "UntrustedSignature",
+    "detect_signature_format",
+    "get_available_vendors",
+    "get_signature_vendor",
+    "get_signature_vendor_for_signature",
+]
+
+
+# Signature verification exceptions
+class SignatureVerificationError(Exception):
+    """Base exception for signature verification failures."""
+
+
+class BadSignature(SignatureVerificationError):
+    """Exception raised when a signature is invalid or cannot be verified.
+
+    Attributes:
+      detail: Optional additional detail about the failure
+    """
+
+    def __init__(self, message: str, detail: str | None = None):
+        """Initialize BadSignature exception.
+
+        Args:
+          message: Error message
+          detail: Optional additional detail about the failure
+        """
+        super().__init__(message)
+        self.detail = detail
+
+
+class UntrustedSignature(SignatureVerificationError):
+    """Exception raised when a signature is not from a trusted key.
+
+    Attributes:
+      signing_keys: List of key IDs that signed the data (if determinable)
+      trusted_keys: List of key IDs that were trusted (if applicable)
+    """
+
+    def __init__(
+        self,
+        message: str,
+        signing_keys: list[str] | None = None,
+        trusted_keys: list[str] | None = None,
+    ):
+        """Initialize UntrustedSignature exception.
+
+        Args:
+          message: Error message
+          signing_keys: List of key IDs that signed the data (if determinable)
+          trusted_keys: List of key IDs that were trusted (if applicable)
+        """
+        super().__init__(message)
+        self.signing_keys = signing_keys or []
+        self.trusted_keys = trusted_keys or []
+
+
 # Git signature format constants
 SIGNATURE_FORMAT_OPENPGP = "openpgp"
 SIGNATURE_FORMAT_X509 = "x509"
@@ -79,7 +144,8 @@ class SignatureVendor:
             is valid.
 
         Raises:
-          Exception on verification failure (implementation-specific)
+          BadSignature: if signature verification fails
+          UntrustedSignature: if signature was not created by a trusted key
         """
         raise NotImplementedError(self.verify)
 
@@ -162,10 +228,9 @@ class GPGSignatureVendor(SignatureVendor):
             is valid.
 
         Raises:
-          gpg.errors.BadSignatures: if GPG signature verification fails
-          gpg.errors.MissingSignatures: if the signature was not created by a key
-            specified in keyids
-          ValueError: if signature trust level is below minimum configured level
+          BadSignature: if GPG signature verification fails
+          UntrustedSignature: if the signature was not created by a key
+            specified in keyids or trust level is below minimum
         """
         import gpg
 
@@ -178,33 +243,40 @@ class GPGSignatureVendor(SignatureVendor):
             "ultimate": gpg.constants.validity.ULTIMATE,
         }
 
-        with gpg.Context() as ctx:
-            verified_data, result = ctx.verify(
-                data,
-                signature=signature,
-            )
-
-            # Check minimum trust level if configured
-            if self.min_trust_level is not None:
-                min_validity = trust_level_map.get(self.min_trust_level)
-                if min_validity is not None:
-                    for sig in result.signatures:
-                        if sig.validity < min_validity:
-                            raise ValueError(
-                                f"Signature trust level {sig.validity} is below "
-                                f"minimum required level {self.min_trust_level}"
-                            )
+        try:
+            with gpg.Context() as ctx:
+                _verified_data, result = ctx.verify(
+                    data,
+                    signature=signature,
+                )
 
-            if keyids:
-                keys = [ctx.get_key(key) for key in keyids]
-                for key in keys:
-                    for subkey in key.subkeys:
+                # Check minimum trust level if configured
+                if self.min_trust_level is not None:
+                    min_validity = trust_level_map.get(self.min_trust_level)
+                    if min_validity is not None:
                         for sig in result.signatures:
-                            if subkey.can_sign and subkey.fpr == sig.fpr:
-                                return
-                raise gpg.errors.MissingSignatures(
-                    result, keys, results=(verified_data, result)
-                )
+                            if sig.validity < min_validity:
+                                raise UntrustedSignature(
+                                    f"Signature trust level {sig.validity} is below "
+                                    f"minimum required level {self.min_trust_level}"
+                                )
+
+                if keyids:
+                    keys = [ctx.get_key(key) for key in keyids]
+                    for key in keys:
+                        for subkey in key.subkeys:
+                            for sig in result.signatures:
+                                if subkey.can_sign and subkey.fpr == sig.fpr:
+                                    return
+                    # Extract signing key fingerprints from the signatures
+                    signing_fprs = [sig.fpr for sig in result.signatures]
+                    raise UntrustedSignature(
+                        f"Signature not created by any of the trusted keys: {keyids}",
+                        signing_keys=signing_fprs,
+                        trusted_keys=list(keyids),
+                    )
+        except gpg.errors.BadSignatures as e:
+            raise BadSignature(f"GPG signature verification failed: {e}") from e
 
 
 class GPGCliSignatureVendor(SignatureVendor):
@@ -286,8 +358,8 @@ class GPGCliSignatureVendor(SignatureVendor):
             is valid.
 
         Raises:
-          subprocess.CalledProcessError: if GPG signature verification fails
-          ValueError: if signature was not created by a trusted key
+          BadSignature: if GPG signature verification fails
+          UntrustedSignature: if signature was not created by a trusted key
         """
         import subprocess
         import tempfile
@@ -305,11 +377,16 @@ class GPGCliSignatureVendor(SignatureVendor):
 
             args = [self.gpg_command, "--verify", sig_file.name, data_file.name]
 
-            result = subprocess.run(
-                args,
-                capture_output=True,
-                check=True,
-            )
+            try:
+                result = subprocess.run(
+                    args,
+                    capture_output=True,
+                    check=True,
+                )
+            except subprocess.CalledProcessError as e:
+                raise BadSignature(
+                    f"GPG signature verification failed: {e.stderr.decode('utf-8', errors='replace')}"
+                ) from e
 
             # If keyids are specified, check that the signature was made by one of them
             if keyids:
@@ -338,7 +415,9 @@ class GPGCliSignatureVendor(SignatureVendor):
                         signing_keys.append(fpr)
 
                 if not signing_keys:
-                    raise ValueError("Could not determine signing key from GPG output")
+                    raise UntrustedSignature(
+                        "Could not determine signing key from GPG output"
+                    )
 
                 # Check if any of the signing keys (subkey or primary) match the trusted keyids
                 keyids_normalized = [k.replace(" ", "").upper() for k in keyids]
@@ -355,9 +434,11 @@ class GPGCliSignatureVendor(SignatureVendor):
                         return
 
                 # None of the signing keys matched
-                raise ValueError(
+                raise UntrustedSignature(
                     f"Signature not created by a trusted key. "
-                    f"Signed by: {signing_keys}, trusted keys: {list(keyids)}"
+                    f"Signed by: {signing_keys}, trusted keys: {list(keyids)}",
+                    signing_keys=signing_keys,
+                    trusted_keys=list(keyids),
                 )
 
 
@@ -443,8 +524,8 @@ class X509SignatureVendor(SignatureVendor):
             fail. If not specified, this function only verifies that the signature is valid.
 
         Raises:
-          subprocess.CalledProcessError: if gpgsm signature verification fails
-          ValueError: if signature was not created by a trusted certificate
+          BadSignature: if gpgsm signature verification fails
+          UntrustedSignature: if signature was not created by a trusted certificate
         """
         import subprocess
         import tempfile
@@ -462,11 +543,16 @@ class X509SignatureVendor(SignatureVendor):
 
             args = [self.gpgsm_command, "--verify", sig_file.name, data_file.name]
 
-            result = subprocess.run(
-                args,
-                capture_output=True,
-                check=True,
-            )
+            try:
+                result = subprocess.run(
+                    args,
+                    capture_output=True,
+                    check=True,
+                )
+            except subprocess.CalledProcessError as e:
+                raise BadSignature(
+                    f"X.509 signature verification failed: {e.stderr.decode('utf-8', errors='replace')}"
+                ) from e
 
             # If keyids are specified, check that the signature was made by one of them
             if keyids:
@@ -488,7 +574,7 @@ class X509SignatureVendor(SignatureVendor):
                                 signing_certs.append(part)
 
                 if not signing_certs:
-                    raise ValueError(
+                    raise UntrustedSignature(
                         "Could not determine signing certificate from gpgsm output"
                     )
 
@@ -504,9 +590,11 @@ class X509SignatureVendor(SignatureVendor):
                         return
 
                 # None of the signing certs matched
-                raise ValueError(
+                raise UntrustedSignature(
                     f"Signature not created by a trusted certificate. "
-                    f"Signed by: {signing_certs}, trusted certs: {list(keyids)}"
+                    f"Signed by: {signing_certs}, trusted certs: {list(keyids)}",
+                    signing_keys=signing_certs,
+                    trusted_keys=list(keyids),
                 )
 
 
@@ -600,8 +688,8 @@ class SSHSigSignatureVendor(SignatureVendor):
                  If not provided, uses gpg.ssh.allowedSignersFile from config.
 
         Raises:
-          ValueError: if no allowed signers are configured or provided
-          sshsig.sshsig.InvalidSignature: if signature verification fails
+          UntrustedSignature: if no allowed signers are configured or provided
+          BadSignature: if signature verification fails
         """
         from typing import Any
 
@@ -643,17 +731,17 @@ class SSHSigSignatureVendor(SignatureVendor):
                     )
                 )
             except FileNotFoundError:
-                raise ValueError(
+                raise UntrustedSignature(
                     f"Allowed signers file not found: {self.allowed_signers_file}"
                 )
         else:
-            raise ValueError(
+            raise UntrustedSignature(
                 "SSH signature verification requires either keyids or "
                 "gpg.ssh.allowedSignersFile to be configured"
             )
 
         if not allowed_keys:
-            raise ValueError("No valid allowed signers found")
+            raise UntrustedSignature("No valid allowed signers found")
 
         # Verify the signature
         # sshsig.verify expects armored signature as string
@@ -662,12 +750,15 @@ class SSHSigSignatureVendor(SignatureVendor):
         )
 
         # Verify with namespace "git" (Git's default)
-        sshsig.sshsig.verify(
-            msg_in=data,
-            armored_signature=sig_str,
-            allowed_signers=allowed_keys,
-            namespace="git",
-        )
+        try:
+            sshsig.sshsig.verify(
+                msg_in=data,
+                armored_signature=sig_str,
+                allowed_signers=allowed_keys,
+                namespace="git",
+            )
+        except Exception as e:
+            raise BadSignature(f"SSH signature verification failed: {e}") from e
 
 
 class SSHCliSignatureVendor(SignatureVendor):
@@ -818,19 +909,25 @@ class SSHCliSignatureVendor(SignatureVendor):
         Args:
           data: The data that was signed
           signature: The signature to verify
-          keyids: Not used for SSH verification. Instead, allowed signers
-                 are read from gpg.ssh.allowedSignersFile config
+          keyids: Not supported for SSH CLI verification. If provided, an error
+                 will be raised. Use gpg.ssh.allowedSignersFile config instead.
 
         Raises:
-          subprocess.CalledProcessError: if signature verification fails
-          ValueError: if allowedSignersFile is not configured
+          BadSignature: if signature verification fails
+          UntrustedSignature: if allowedSignersFile is not configured or keyids is provided
         """
         import os
         import subprocess
         import tempfile
 
+        if keyids is not None:
+            raise UntrustedSignature(
+                "SSHCliSignatureVendor does not support keyids parameter. "
+                "Use gpg.ssh.allowedSignersFile configuration instead."
+            )
+
         if self.allowed_signers_file is None:
-            raise ValueError(
+            raise UntrustedSignature(
                 "SSH signature verification requires gpg.ssh.allowedSignersFile "
                 "to be configured"
             )
@@ -867,12 +964,17 @@ class SSHCliSignatureVendor(SignatureVendor):
             if self.revocation_file:
                 args.extend(["-r", self.revocation_file])
 
-            subprocess.run(
-                args,
-                stdin=open(data_filename, "rb"),
-                capture_output=True,
-                check=True,
-            )
+            try:
+                subprocess.run(
+                    args,
+                    stdin=open(data_filename, "rb"),
+                    capture_output=True,
+                    check=True,
+                )
+            except subprocess.CalledProcessError as e:
+                raise BadSignature(
+                    f"SSH signature verification failed: {e.stderr.decode('utf-8', errors='replace')}"
+                ) from e
 
 
 def get_signature_vendor(
@@ -993,5 +1095,54 @@ def get_signature_vendor_for_signature(
     return get_signature_vendor(format=format, config=config)
 
 
+def get_available_vendors() -> dict[str, list[type[SignatureVendor]]]:
+    """Get all available signature vendors on this system.
+
+    Returns a dictionary mapping signature format names to lists of available
+    vendor classes for each format. Only vendors whose dependencies are available
+    are included.
+
+    Returns:
+      Dictionary mapping format names (e.g. "openpgp", "ssh", "x509") to lists
+      of available vendor classes. If no vendors are available for a format,
+      that format will not be present in the dictionary.
+
+    Example:
+      >>> vendors = get_available_vendors()
+      >>> if "openpgp" in vendors:
+      ...     print(f"GPG vendors: {vendors['openpgp']}")
+      >>> if "ssh" in vendors:
+      ...     print(f"SSH vendors: {vendors['ssh']}")
+    """
+    available: dict[str, list[type[SignatureVendor]]] = {}
+
+    # Check OpenPGP/GPG vendors
+    openpgp_vendors: list[type[SignatureVendor]] = []
+    if GPGSignatureVendor.available():
+        openpgp_vendors.append(GPGSignatureVendor)
+    if GPGCliSignatureVendor.available():
+        openpgp_vendors.append(GPGCliSignatureVendor)
+    if openpgp_vendors:
+        available[SIGNATURE_FORMAT_OPENPGP] = openpgp_vendors
+
+    # Check X.509 vendors
+    x509_vendors: list[type[SignatureVendor]] = []
+    if X509SignatureVendor.available():
+        x509_vendors.append(X509SignatureVendor)
+    if x509_vendors:
+        available[SIGNATURE_FORMAT_X509] = x509_vendors
+
+    # Check SSH vendors
+    ssh_vendors: list[type[SignatureVendor]] = []
+    if SSHSigSignatureVendor.available():
+        ssh_vendors.append(SSHSigSignatureVendor)
+    if SSHCliSignatureVendor.available():
+        ssh_vendors.append(SSHCliSignatureVendor)
+    if ssh_vendors:
+        available[SIGNATURE_FORMAT_SSH] = ssh_vendors
+
+    return available
+
+
 # Default GPG vendor instance
 gpg_vendor = GPGSignatureVendor()

+ 1 - 0
tests/__init__.py

@@ -183,6 +183,7 @@ def self_test_suite() -> unittest.TestSuite:
         "server",
         "sha256",
         "sha256_pack",
+        "signature",
         "source",
         "sparse_patterns",
         "stash",

+ 133 - 33
tests/porcelain/__init__.py

@@ -633,21 +633,37 @@ class CommitSignTests(PorcelainGpgTestCase):
         commit = self.repo.get_object(sha)
         assert isinstance(commit, Commit)
         # GPG Signatures aren't deterministic, so we can't do a static assertion.
-        commit.verify()
-        commit.verify(keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID])
+        from dulwich.signature import (
+            BadSignature,
+            UntrustedSignature,
+            get_signature_vendor_for_signature,
+        )
+
+        self.assertIsNotNone(commit.gpgsig)
+        vendor = get_signature_vendor_for_signature(commit.gpgsig)
+        vendor.verify(commit.raw_without_sig(), commit.gpgsig)
+        vendor.verify(
+            commit.raw_without_sig(),
+            commit.gpgsig,
+            keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID],
+        )
 
         self.import_non_default_key()
         self.assertRaises(
-            gpg.errors.MissingSignatures,
-            commit.verify,
+            UntrustedSignature,
+            vendor.verify,
+            commit.raw_without_sig(),
+            commit.gpgsig,
             keyids=[PorcelainGpgTestCase.NON_DEFAULT_KEY_ID],
         )
 
         assert isinstance(commit, Commit)
         commit.committer = b"Alice <alice@example.com>"
         self.assertRaises(
-            gpg.errors.BadSignatures,
-            commit.verify,
+            BadSignature,
+            vendor.verify,
+            commit.raw_without_sig(),
+            commit.gpgsig,
         )
 
     def test_non_default_key(self) -> None:
@@ -664,7 +680,7 @@ class CommitSignTests(PorcelainGpgTestCase):
             message="Some message",
             author="Joe <joe@example.com>",
             committer="Bob <bob@example.com>",
-            signoff=PorcelainGpgTestCase.NON_DEFAULT_KEY_ID,
+            sign=PorcelainGpgTestCase.NON_DEFAULT_KEY_ID,
         )
         self.assertIsInstance(sha, bytes)
         self.assertEqual(len(sha), 40)
@@ -672,7 +688,11 @@ class CommitSignTests(PorcelainGpgTestCase):
         commit = self.repo.get_object(sha)
         assert isinstance(commit, Commit)
         # GPG Signatures aren't deterministic, so we can't do a static assertion.
-        commit.verify()
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(commit.gpgsig)
+        vendor = get_signature_vendor_for_signature(commit.gpgsig)
+        vendor.verify(commit.raw_without_sig(), commit.gpgsig)
 
     def test_sign_uses_config_signingkey(self) -> None:
         """Test that sign=True uses user.signingKey from config."""
@@ -703,8 +723,16 @@ class CommitSignTests(PorcelainGpgTestCase):
         commit = self.repo.get_object(sha)
         assert isinstance(commit, Commit)
         # Verify the commit is signed with the configured key
-        commit.verify()
-        commit.verify(keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID])
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(commit.gpgsig)
+        vendor = get_signature_vendor_for_signature(commit.gpgsig)
+        vendor.verify(commit.raw_without_sig(), commit.gpgsig)
+        vendor.verify(
+            commit.raw_without_sig(),
+            commit.gpgsig,
+            keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID],
+        )
 
     def test_commit_gpg_sign_config_enabled(self) -> None:
         """Test that commit.gpgSign=true automatically signs commits."""
@@ -736,8 +764,16 @@ class CommitSignTests(PorcelainGpgTestCase):
         commit = self.repo.get_object(sha)
         assert isinstance(commit, Commit)
         # Verify the commit is signed due to config
-        commit.verify()
-        commit.verify(keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID])
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(commit.gpgsig)
+        vendor = get_signature_vendor_for_signature(commit.gpgsig)
+        vendor.verify(commit.raw_without_sig(), commit.gpgsig)
+        vendor.verify(
+            commit.raw_without_sig(),
+            commit.gpgsig,
+            keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID],
+        )
 
     def test_commit_gpg_sign_config_disabled(self) -> None:
         """Test that commit.gpgSign=false does not sign commits."""
@@ -800,7 +836,11 @@ class CommitSignTests(PorcelainGpgTestCase):
         commit = self.repo.get_object(sha)
         assert isinstance(commit, Commit)
         # Verify the commit is signed with default key
-        commit.verify()
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(commit.gpgsig)
+        vendor = get_signature_vendor_for_signature(commit.gpgsig)
+        vendor.verify(commit.raw_without_sig(), commit.gpgsig)
 
     def test_explicit_signoff_overrides_config(self) -> None:
         """Test that explicit signoff parameter overrides commit.gpgSign config."""
@@ -832,8 +872,16 @@ class CommitSignTests(PorcelainGpgTestCase):
         commit = self.repo.get_object(sha)
         assert isinstance(commit, Commit)
         # Verify the commit is signed despite config=false
-        commit.verify()
-        commit.verify(keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID])
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(commit.gpgsig)
+        vendor = get_signature_vendor_for_signature(commit.gpgsig)
+        vendor.verify(commit.raw_without_sig(), commit.gpgsig)
+        vendor.verify(
+            commit.raw_without_sig(),
+            commit.gpgsig,
+            keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID],
+        )
 
     def test_explicit_false_disables_signing(self) -> None:
         """Test that explicit signoff=False disables signing even with config=true."""
@@ -900,7 +948,9 @@ class VerifyCommitTests(PorcelainGpgTestCase):
         )
 
     def test_verify_commit_with_wrong_key(self) -> None:
-        """Test that verifying with wrong keyid raises MissingSignatures."""
+        """Test that verifying with wrong keyid raises UntrustedSignature."""
+        from dulwich.signature import UntrustedSignature
+
         _c1, _c2, c3 = build_commit_graph(
             self.repo.object_store, [[1], [2, 1], [3, 1, 2]]
         )
@@ -920,7 +970,7 @@ class VerifyCommitTests(PorcelainGpgTestCase):
 
         self.import_non_default_key()
         self.assertRaises(
-            gpg.errors.MissingSignatures,
+            UntrustedSignature,
             porcelain.verify_commit,
             self.repo.path,
             sha,
@@ -979,7 +1029,9 @@ class VerifyTagTests(PorcelainGpgTestCase):
         )
 
     def test_verify_tag_with_wrong_key(self) -> None:
-        """Test that verifying with wrong keyid raises MissingSignatures."""
+        """Test that verifying with wrong keyid raises UntrustedSignature."""
+        from dulwich.signature import UntrustedSignature
+
         _c1, _c2, c3 = build_commit_graph(
             self.repo.object_store, [[1], [2, 1], [3, 1, 2]]
         )
@@ -1000,7 +1052,7 @@ class VerifyTagTests(PorcelainGpgTestCase):
 
         self.import_non_default_key()
         self.assertRaises(
-            gpg.errors.MissingSignatures,
+            UntrustedSignature,
             porcelain.verify_tag,
             self.repo.path,
             b"signed-tag",
@@ -3246,21 +3298,37 @@ class TagCreateSignTests(PorcelainGpgTestCase):
         tag = self.repo[b"refs/tags/tryme"]
         assert isinstance(tag, Tag)
         # GPG Signatures aren't deterministic, so we can't do a static assertion.
-        tag.verify()
-        tag.verify(keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID])
+        from dulwich.signature import (
+            BadSignature,
+            UntrustedSignature,
+            get_signature_vendor_for_signature,
+        )
+
+        self.assertIsNotNone(tag.signature)
+        vendor = get_signature_vendor_for_signature(tag.signature)
+        vendor.verify(tag.raw_without_sig(), tag.signature)
+        vendor.verify(
+            tag.raw_without_sig(),
+            tag.signature,
+            keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID],
+        )
 
         self.import_non_default_key()
         self.assertRaises(
-            gpg.errors.MissingSignatures,
-            tag.verify,
+            UntrustedSignature,
+            vendor.verify,
+            tag.raw_without_sig(),
+            tag.signature,
             keyids=[PorcelainGpgTestCase.NON_DEFAULT_KEY_ID],
         )
 
         assert tag.signature is not None
         tag._chunked_text = [b"bad data", tag.signature]
         self.assertRaises(
-            gpg.errors.BadSignatures,
-            tag.verify,
+            BadSignature,
+            vendor.verify,
+            tag.raw_without_sig(),
+            tag.signature,
         )
 
     def test_non_default_key(self) -> None:
@@ -3291,7 +3359,11 @@ class TagCreateSignTests(PorcelainGpgTestCase):
         tag = self.repo[b"refs/tags/tryme"]
         assert isinstance(tag, Tag)
         # GPG Signatures aren't deterministic, so we can't do a static assertion.
-        tag.verify()
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(tag.signature)
+        vendor = get_signature_vendor_for_signature(tag.signature)
+        vendor.verify(tag.raw_without_sig(), tag.signature)
 
     def test_sign_uses_config_signingkey(self) -> None:
         """Test that sign=True uses user.signingKey from config."""
@@ -3323,8 +3395,16 @@ class TagCreateSignTests(PorcelainGpgTestCase):
         self.assertIsInstance(tag, Tag)
 
         # Verify the tag is signed with the configured key
-        tag.verify()
-        tag.verify(keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID])
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(tag.signature)
+        vendor = get_signature_vendor_for_signature(tag.signature)
+        vendor.verify(tag.raw_without_sig(), tag.signature)
+        vendor.verify(
+            tag.raw_without_sig(),
+            tag.signature,
+            keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID],
+        )
 
     def test_tag_gpg_sign_config_enabled(self) -> None:
         """Test that tag.gpgSign=true automatically signs tags."""
@@ -3357,8 +3437,16 @@ class TagCreateSignTests(PorcelainGpgTestCase):
         self.assertIsInstance(tag, Tag)
 
         # Verify the tag is signed due to config
-        tag.verify()
-        tag.verify(keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID])
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(tag.signature)
+        vendor = get_signature_vendor_for_signature(tag.signature)
+        vendor.verify(tag.raw_without_sig(), tag.signature)
+        vendor.verify(
+            tag.raw_without_sig(),
+            tag.signature,
+            keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID],
+        )
 
     def test_tag_gpg_sign_config_disabled(self) -> None:
         """Test that tag.gpgSign=false does not sign tags."""
@@ -3423,7 +3511,11 @@ class TagCreateSignTests(PorcelainGpgTestCase):
         self.assertIsInstance(tag, Tag)
 
         # Verify the tag is signed with default key
-        tag.verify()
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(tag.signature)
+        vendor = get_signature_vendor_for_signature(tag.signature)
+        vendor.verify(tag.raw_without_sig(), tag.signature)
 
     def test_explicit_sign_overrides_config(self) -> None:
         """Test that explicit sign parameter overrides tag.gpgSign config."""
@@ -3456,8 +3548,16 @@ class TagCreateSignTests(PorcelainGpgTestCase):
         self.assertIsInstance(tag, Tag)
 
         # Verify the tag is signed despite config=false
-        tag.verify()
-        tag.verify(keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID])
+        from dulwich.signature import get_signature_vendor_for_signature
+
+        self.assertIsNotNone(tag.signature)
+        vendor = get_signature_vendor_for_signature(tag.signature)
+        vendor.verify(tag.raw_without_sig(), tag.signature)
+        vendor.verify(
+            tag.raw_without_sig(),
+            tag.signature,
+            keyids=[PorcelainGpgTestCase.DEFAULT_KEY_ID],
+        )
 
     def test_explicit_false_disables_tag_signing(self) -> None:
         """Test that explicit sign=False disables signing even with config=true."""

+ 82 - 40
tests/test_signature.py

@@ -47,6 +47,57 @@ except ImportError:
     gpg = None
 
 
+def get_valid_gpg_key() -> str | None:
+    """Get a valid (non-revoked, non-expired, can-sign) GPG key from the keyring.
+
+    Returns:
+      A key fingerprint string that can be used for signing, or None if no valid key found.
+
+    Raises:
+      unittest.SkipTest: if gpg module is not available
+    """
+    if gpg is None:
+        raise unittest.SkipTest("gpg module not available")
+
+    with gpg.Context() as ctx:
+        keys = list(ctx.keylist(secret=True))
+        if not keys:
+            return None
+
+        # Find a non-revoked, non-expired key that can sign
+        for key in keys:
+            if not key.revoked and not key.expired and key.can_sign:
+                return str(key.fpr)
+
+    return None
+
+
+def get_valid_gpg_key_cli() -> str | None:
+    """Get a valid (non-revoked, non-expired) GPG key fingerprint using CLI.
+
+    Returns:
+      A key fingerprint string, or None if no valid key found.
+    """
+    result = subprocess.run(
+        ["gpg", "--list-secret-keys", "--with-colons"],
+        capture_output=True,
+        check=True,
+        text=True,
+    )
+
+    # Find a valid key (field 2 should be '-' for valid, 'e' for expired, 'r' for revoked)
+    current_key_valid = False
+    for line in result.stdout.split("\n"):
+        if line.startswith("sec:"):
+            fields = line.split(":")
+            # Only accept valid keys (field 2 is '-')
+            current_key_valid = fields[1] == "-"
+        elif line.startswith("fpr:") and current_key_valid:
+            return line.split(":")[9]
+
+    return None
+
+
 class SignatureVendorTests(unittest.TestCase):
     """Tests for SignatureVendor base class."""
 
@@ -125,19 +176,16 @@ class GPGSignatureVendorTests(unittest.TestCase):
         test_data = b"test data to sign"
 
         try:
-            # Try to get a key from the keyring
-            with gpg.Context() as ctx:
-                keys = list(ctx.keylist(secret=True))
-                if not keys:
-                    self.skipTest("No GPG keys available for testing")
-
-                key = keys[0]
-                signature = vendor.sign(test_data, keyid=key.fpr)
-                self.assertIsInstance(signature, bytes)
-                self.assertGreater(len(signature), 0)
-
-                # Verify the signature
-                vendor.verify(test_data, signature)
+            key = get_valid_gpg_key()
+            if not key:
+                self.skipTest("No valid GPG keys available for testing")
+
+            signature = vendor.sign(test_data, keyid=key)
+            self.assertIsInstance(signature, bytes)
+            self.assertGreater(len(signature), 0)
+
+            # Verify the signature
+            vendor.verify(test_data, signature)
         except gpg.errors.GPGMEError as e:
             self.skipTest(f"GPG key not available: {e}")
 
@@ -170,11 +218,13 @@ class GPGCliSignatureVendorTests(unittest.TestCase):
 
     def test_verify_invalid_signature(self) -> None:
         """Test that verify raises an error for invalid signatures."""
+        from dulwich.signature import BadSignature
+
         vendor = GPGCliSignatureVendor()
         test_data = b"test data"
         invalid_signature = b"this is not a valid signature"
 
-        with self.assertRaises(subprocess.CalledProcessError):
+        with self.assertRaises(BadSignature):
             vendor.verify(test_data, invalid_signature)
 
     def test_sign_with_keyid(self) -> None:
@@ -212,37 +262,25 @@ class GPGCliSignatureVendorTests(unittest.TestCase):
 
     def test_verify_with_keyids(self) -> None:
         """Test verifying with specific trusted key IDs."""
+        from dulwich.signature import UntrustedSignature
+
         vendor = GPGCliSignatureVendor()
         test_data = b"test data to sign"
 
         try:
-            # Sign without specifying a key (use default)
-            signature = vendor.sign(test_data)
+            valid_keyid = get_valid_gpg_key_cli()
+            if not valid_keyid:
+                self.skipTest("No valid GPG keys available for testing")
 
-            # Get the primary key fingerprint from the keyring
-            result = subprocess.run(
-                ["gpg", "--list-secret-keys", "--with-colons"],
-                capture_output=True,
-                check=True,
-                text=True,
-            )
+            # Sign with the specific key
+            signature = vendor.sign(test_data, keyid=valid_keyid)
 
-            primary_keyid = None
-            for line in result.stdout.split("\n"):
-                if line.startswith("fpr:"):
-                    primary_keyid = line.split(":")[9]
-                    break
-
-            if not primary_keyid:
-                self.skipTest("No GPG keys available for testing")
-
-            # Verify with the correct primary keyid - should succeed
-            # (GPG shows primary key fingerprint even if signed by subkey)
-            vendor.verify(test_data, signature, keyids=[primary_keyid])
+            # Verify with the correct keyid - should succeed
+            vendor.verify(test_data, signature, keyids=[valid_keyid])
 
             # Verify with a different keyid - should fail
             fake_keyid = "0" * 40  # Fake 40-character fingerprint
-            with self.assertRaises(ValueError):
+            with self.assertRaises(UntrustedSignature):
                 vendor.verify(test_data, signature, keyids=[fake_keyid])
 
         except subprocess.CalledProcessError as e:
@@ -429,9 +467,11 @@ class SSHSigSignatureVendorTests(unittest.TestCase):
         self.assertIn("SSHCliSignatureVendor", str(cm.exception))
 
     def test_verify_without_config_raises(self) -> None:
-        """Test that verify without config or keyids raises ValueError."""
+        """Test that verify without config or keyids raises UntrustedSignature."""
+        from dulwich.signature import UntrustedSignature
+
         vendor = SSHSigSignatureVendor()
-        with self.assertRaises(ValueError) as cm:
+        with self.assertRaises(UntrustedSignature) as cm:
             vendor.verify(b"test data", b"fake signature")
         self.assertIn("allowedSignersFile", str(cm.exception))
 
@@ -553,9 +593,11 @@ class SSHCliSignatureVendorTests(unittest.TestCase):
         self.assertIn("key", str(cm.exception).lower())
 
     def test_verify_without_allowed_signers_raises(self) -> None:
-        """Test that verify without allowedSignersFile raises ValueError."""
+        """Test that verify without allowedSignersFile raises UntrustedSignature."""
+        from dulwich.signature import UntrustedSignature
+
         vendor = SSHCliSignatureVendor()
-        with self.assertRaises(ValueError) as cm:
+        with self.assertRaises(UntrustedSignature) as cm:
             vendor.verify(b"test data", b"fake signature")
         self.assertIn("allowedSignersFile", str(cm.exception))