Repository: DataDog/guarddog (python)

Issue:
Create a Separate GuardDog Signal for Admin Account with a Domain that can be Hijacked 
Right now, [`class PotentiallyCompromisedEmailDomainDetector(Detector)`](https://github.com/DataDog/guarddog/blob/1433ab0448f1ba9ecde6980a76b0097a5e94eb96/guarddog/analyzer/metadata/potentially_compromised_email_domain.py) reports both packages with a maintainer which email address points to an unclaimed domain name as well as packages with a maintainer which email address points to a domain that was (re)registered more recently than the most recent package version.

Now the first case is a higher-fidelity security signal (you *know* you have a problem) when the second one is a bit less high-fidelity as it can cause false positives (typically, a maintainer changed its domain recently or was added as maintainer recently, etc...).

We should give users a way to clearly distinguish between the two cases, typically with two different [detector names](https://github.com/DataDog/guarddog/blob/1433ab0448f1ba9ecde6980a76b0097a5e94eb96/guarddog/analyzer/metadata/detector.py#L9), so that they can react differently to these two different situations.

I would keep `potentially_compromised_email_domain` for domains that have a recent creation date, and create a new name `maintainer_email_domain_unclaimed`.

If we are afraid that changing the semantics of `potentially_compromised_email_domain` may be a breaking change for some users, we can create two new detector names instead.

I am happy to write the Pull Request for this change.

Pull request description:
Move "unclaimed maintainer email domain" to New Signal
Closes #333 

* including some necessary code factorization
* plus add warning about false positives for NPM
* including unit tests

Patch:
diff --git a/guarddog/analyzer/metadata/npm/__init__.py b/guarddog/analyzer/metadata/npm/__init__.py
index 4b6c485..b01db9d 100644
--- a/guarddog/analyzer/metadata/npm/__init__.py
+++ b/guarddog/analyzer/metadata/npm/__init__.py
@@ -2,6 +2,9 @@ from guarddog.analyzer.metadata.npm.empty_information import NPMEmptyInfoDetecto
 from guarddog.analyzer.metadata.npm.potentially_compromised_email_domain import (
     NPMPotentiallyCompromisedEmailDomainDetector,
 )
+from guarddog.analyzer.metadata.npm.unclaimed_maintainer_email_domain import (
+    NPMUnclaimedMaintainerEmailDomainDetector,
+)
 from guarddog.analyzer.metadata.npm.release_zero import NPMReleaseZeroDetector
 from guarddog.analyzer.metadata.npm.typosquatting import NPMTyposquatDetector
 from guarddog.analyzer.metadata.npm.direct_url_dependency import (
@@ -15,6 +18,7 @@ classes = [
     NPMEmptyInfoDetector,
     NPMReleaseZeroDetector,
     NPMPotentiallyCompromisedEmailDomainDetector,
+    NPMUnclaimedMaintainerEmailDomainDetector,
     NPMTyposquatDetector,
     NPMDirectURLDependencyDetector,
     NPMMetadataMismatch,
diff --git a/guarddog/analyzer/metadata/npm/potentially_compromised_email_domain.py b/guarddog/analyzer/metadata/npm/potentially_compromised_email_domain.py
index 54e8a30..6bebfac 100644
--- a/guarddog/analyzer/metadata/npm/potentially_compromised_email_domain.py
+++ b/guarddog/analyzer/metadata/npm/potentially_compromised_email_domain.py
@@ -10,16 +10,26 @@ from dateutil import parser
 
 from guarddog.analyzer.metadata.potentially_compromised_email_domain import PotentiallyCompromisedEmailDomainDetector
 
+from .utils import NPM_API_MAINTAINER_EMAIL_WARNING, get_email_addresses
+
 
 class NPMPotentiallyCompromisedEmailDomainDetector(PotentiallyCompromisedEmailDomainDetector):
     def __init__(self):
         super().__init__("npm")
 
-    def get_email_addresses(self, package_info: dict) -> list[str]:
-        if package_info.get("maintainers"):
-            return list(map(lambda x: x["email"], package_info["maintainers"]))
-        else:
-            return []
+        self.description += "; " + NPM_API_MAINTAINER_EMAIL_WARNING
+
+    def get_email_addresses(self, package_info: dict):
+        return get_email_addresses(package_info)
+
+    def detect(self, package_info, path: Optional[str] = None, name: Optional[str] = None,
+               version: Optional[str] = None) -> tuple[bool, str]:
+        has_issues, message = super().detect(package_info, path, name, version)
+
+        if has_issues:
+            message += "\n\n" + NPM_API_MAINTAINER_EMAIL_WARNING
+
+        return has_issues, message
 
     def get_project_latest_release_date(self, package_info) -> Optional[datetime]:
         """
diff --git a/guarddog/analyzer/metadata/npm/unclaimed_maintainer_email_domain.py b/guarddog/analyzer/metadata/npm/unclaimed_maintainer_email_domain.py
new file mode 100644
index 0000000..a30ba2f
--- /dev/null
+++ b/guarddog/analyzer/metadata/npm/unclaimed_maintainer_email_domain.py
@@ -0,0 +1,23 @@
+from typing import Optional
+from guarddog.analyzer.metadata.unclaimed_maintainer_email_domain import UnclaimedMaintainerEmailDomainDetector
+
+from .utils import NPM_API_MAINTAINER_EMAIL_WARNING, get_email_addresses
+
+
+class NPMUnclaimedMaintainerEmailDomainDetector(UnclaimedMaintainerEmailDomainDetector):
+    def __init__(self):
+        super().__init__("npm")
+
+        self.description += "; " + NPM_API_MAINTAINER_EMAIL_WARNING
+
+    def get_email_addresses(self, package_info: dict):
+        return get_email_addresses(package_info)
+
+    def detect(self, package_info, path: Optional[str] = None, name: Optional[str] = None,
+               version: Optional[str] = None) -> tuple[bool, str]:
+        has_issues, message = super().detect(package_info, path, name, version)
+
+        if has_issues:
+            message += "\n\n" + NPM_API_MAINTAINER_EMAIL_WARNING
+
+        return has_issues, message
diff --git a/guarddog/analyzer/metadata/npm/utils.py b/guarddog/analyzer/metadata/npm/utils.py
new file mode 100644
index 0000000..499aca1
--- /dev/null
+++ b/guarddog/analyzer/metadata/npm/utils.py
@@ -0,0 +1,13 @@
+NPM_API_MAINTAINER_EMAIL_WARNING = (
+    "note that NPM's API may not provide accurate information regarding the maintainer's email, "
+    "so this detector may cause false positives for NPM packages. "
+    "see https://www.theregister.com/2022/05/10/security_npm_email/"
+)
+
+
+def get_email_addresses(package_info: dict) -> set[str]:
+    return {
+        m["email"]
+        for m in package_info.get("maintainers", [])
+        if "email" in m
+    } - {None, ""}
diff --git a/guarddog/analyzer/metadata/potentially_compromised_email_domain.py b/guarddog/analyzer/metadata/potentially_compromised_email_domain.py
index 66c90c8..0ad65d8 100644
--- a/guarddog/analyzer/metadata/potentially_compromised_email_domain.py
+++ b/guarddog/analyzer/metadata/potentially_compromised_email_domain.py
@@ -1,11 +1,10 @@
 from abc import abstractmethod
-from datetime import datetime
 from typing import Optional
 
-import whois  # type: ignore
-
 from guarddog.analyzer.metadata.detector import Detector
 
+from .utils import extract_email_address_domain, get_domain_creation_date
+
 
 class PotentiallyCompromisedEmailDomainDetector(Detector):
     # The name of the rule is dependent on the ecosystem and is provided by the implementing subclasses
@@ -17,54 +16,18 @@ class PotentiallyCompromisedEmailDomainDetector(Detector):
         )
         self.ecosystem = ecosystem
 
-    def _get_domain_creation_date(self, email_domain) -> tuple[Optional[datetime], bool]:
-        """
-        Gets the creation date of an email address domain
-
-        Args:
-            email_domain (str): domain of email address
-
-        Raises:
-            Exception: "Domain {email_domain} does not exist"
-
-        Returns:
-            datetime: creation date of email_domain
-            bool:     if the domain is currently registered
-        """
-
-        try:
-            domain_information = whois.whois(email_domain)
-        except whois.parser.PywhoisError as e:
-            # The domain doesn't exist at all, if that's the case we consider it vulnerable
-            # since someone could register it
-            return None, (not str(e).lower().startswith('no match for'))
-
-        if domain_information.creation_date is None:
-            # No creation date in whois, so we can't know
-            return None, True
-
-        creation_dates = domain_information.creation_date
-
-        if type(creation_dates) is list:
-            return min(creation_dates), True
-
-        return creation_dates, True
-
     def detect(self, package_info, path: Optional[str] = None, name: Optional[str] = None,
                version: Optional[str] = None) -> tuple[bool, str]:
         """
-        Uses a package's information from PyPI's JSON API to determine
-        if the package's email domain might have been compromised
+        Uses a package's information to determine
+        if the maintainer's email domain might have been compromised
 
         Args:
-            package_info (dict): dictionary representation of PyPI's JSON
-                output
-
-        Raises:
-            Exception: "Email for {package_info['info']['name']} does not exist."
+            package_info (dict): package info from the package repository
 
         Returns:
-            bool: True if email domain is compromised
+            bool: True if email domain may be compromised
+            str: Message explaining the issue
         """
 
         emails = self.get_email_addresses(package_info)
@@ -78,22 +41,25 @@ class PotentiallyCompromisedEmailDomainDetector(Detector):
         has_issues = False
         messages = []
         for email in emails:
-            sanitized_email = email.strip().replace(">", "").replace("<", "")
-            email_domain = sanitized_email.split("@")[-1]
-            domain_creation_date, domain_exists = self._get_domain_creation_date(email_domain)
+            domain = extract_email_address_domain(email)
+            # note: get_domain_creation_date is cached
+            domain_creation_date, domain_exists = get_domain_creation_date(domain)
 
             if not domain_exists:
-                has_issues = True
-                messages.append(f"The maintainer's email ({email}) domain does not exist and can likely be registered "
-                                f"by an attacker to compromise the maintainer's {self.ecosystem} account")
+                # will be caught by the "unclaimed_maintainer_email_domain" detector
+                continue
             if domain_creation_date is None or latest_project_release is None:
                 continue
             if latest_project_release < domain_creation_date:
                 has_issues = True
-                messages.append(f"The domain name of the maintainer's email address ({email}) was"" re-registered after"
-                                " the latest release of this ""package. This can be an indicator that this is a"""
-                                " custom domain that expired, and was leveraged by"" an attacker to compromise the"
-                                f" package owner's {self.ecosystem}"" account.")
+
+                messages.append(
+                    f"The domain name of the maintainer's email address ({email}) was"" re-registered after"
+                    " the latest release of this ""package. This can be an indicator that this is a"""
+                    " custom domain that expired, and was leveraged by"" an attacker to compromise the"
+                    f" package owner's {self.ecosystem}"" account."
+                )
+
         return has_issues, "\n".join(messages)
 
     @abstractmethod
@@ -101,5 +67,5 @@ class PotentiallyCompromisedEmailDomainDetector(Detector):
         pass
 
     @abstractmethod
-    def get_email_addresses(self, package_info):
-        pass
+    def get_email_addresses(self, package_info) -> set[str]:
+        return set()
diff --git a/guarddog/analyzer/metadata/pypi/__init__.py b/guarddog/analyzer/metadata/pypi/__init__.py
index 1992c99..23535b6 100644
--- a/guarddog/analyzer/metadata/pypi/__init__.py
+++ b/guarddog/analyzer/metadata/pypi/__init__.py
@@ -1,6 +1,8 @@
 from guarddog.analyzer.metadata.pypi.empty_information import PypiEmptyInfoDetector
 from guarddog.analyzer.metadata.pypi.potentially_compromised_email_domain import \
     PypiPotentiallyCompromisedEmailDomainDetector
+from guarddog.analyzer.metadata.pypi.unclaimed_maintainer_email_domain import \
+    PypiUnclaimedMaintainerEmailDomainDetector
 from guarddog.analyzer.metadata.pypi.release_zero import PypiReleaseZeroDetector
 from guarddog.analyzer.metadata.pypi.repository_integrity_mismatch import PypiIntegrityMismatchDetector
 from guarddog.analyzer.metadata.pypi.single_python_file import PypiSinglePythonFileDetector
@@ -13,6 +15,7 @@ classes = [
     PypiReleaseZeroDetector,
     PypiTyposquatDetector,
     PypiPotentiallyCompromisedEmailDomainDetector,
+    PypiUnclaimedMaintainerEmailDomainDetector,
     PypiIntegrityMismatchDetector,
     PypiSinglePythonFileDetector
 
diff --git a/guarddog/analyzer/metadata/pypi/potentially_compromised_email_domain.py b/guarddog/analyzer/metadata/pypi/potentially_compromised_email_domain.py
index e11a1fd..b5cc171 100644
--- a/guarddog/analyzer/metadata/pypi/potentially_compromised_email_domain.py
+++ b/guarddog/analyzer/metadata/pypi/potentially_compromised_email_domain.py
@@ -10,19 +10,15 @@ from packaging import version
 
 from guarddog.analyzer.metadata.potentially_compromised_email_domain import PotentiallyCompromisedEmailDomainDetector
 
+from .utils import get_email_addresses
+
 
 class PypiPotentiallyCompromisedEmailDomainDetector(PotentiallyCompromisedEmailDomainDetector):
     def __init__(self):
         super().__init__("pypi")
 
-    def get_email_addresses(self, package_info: dict) -> list[str]:
-        author_email = package_info["info"]["author_email"]
-        maintainer_email = package_info["info"]["maintainer_email"]
-        email = author_email or maintainer_email
-        if email is not None:
-            return [email]
-        else:
-            return []
+    def get_email_addresses(self, package_info: dict):
+        return get_email_addresses(package_info)
 
     def get_project_latest_release_date(self, package_info) -> Optional[datetime]:
         """
diff --git a/guarddog/analyzer/metadata/pypi/unclaimed_maintainer_email_domain.py b/guarddog/analyzer/metadata/pypi/unclaimed_maintainer_email_domain.py
new file mode 100644
index 0000000..2ce0bd9
--- /dev/null
+++ b/guarddog/analyzer/metadata/pypi/unclaimed_maintainer_email_domain.py
@@ -0,0 +1,11 @@
+from guarddog.analyzer.metadata.unclaimed_maintainer_email_domain import UnclaimedMaintainerEmailDomainDetector
+
+from .utils import get_email_addresses
+
+
+class PypiUnclaimedMaintainerEmailDomainDetector(UnclaimedMaintainerEmailDomainDetector):
+    def __init__(self):
+        super().__init__("pypi")
+
+    def get_email_addresses(self, package_info: dict):
+        return get_email_addresses(package_info)
diff --git a/guarddog/analyzer/metadata/pypi/utils.py b/guarddog/analyzer/metadata/pypi/utils.py
new file mode 100644
index 0000000..38a25f8
--- /dev/null
+++ b/guarddog/analyzer/metadata/pypi/utils.py
@@ -0,0 +1,7 @@
+def get_email_addresses(package_info: dict) -> set[str]:
+    info = package_info.get("info", {})
+
+    return {
+        info.get("author_email")
+        or info.get("maintainer_email")
+    } - {None}
diff --git a/guarddog/analyzer/metadata/unclaimed_maintainer_email_domain.py b/guarddog/analyzer/metadata/unclaimed_maintainer_email_domain.py
new file mode 100644
index 0000000..647160a
--- /dev/null
+++ b/guarddog/analyzer/metadata/unclaimed_maintainer_email_domain.py
@@ -0,0 +1,70 @@
+from abc import abstractmethod
+from typing import Optional
+
+from guarddog.analyzer.metadata.detector import Detector
+
+from .utils import extract_email_address_domain, get_domain_creation_date
+
+
+class UnclaimedMaintainerEmailDomainDetector(Detector):
+    # The name of the rule is dependent on the ecosystem and is provided by the implementing subclasses
+    def __init__(self, ecosystem: str):
+        description = (
+            "Identify when a package maintainer e-mail domain (and therefore package manager account) "
+            "is currently unclaimed and could be registered by an attacker"
+        )
+
+        super().__init__(
+            name="unclaimed_maintainer_email_domain",
+            description=description,
+        )
+        self.ecosystem = ecosystem
+
+    def detect(self, package_info, path: Optional[str] = None, name: Optional[str] = None,
+               version: Optional[str] = None) -> tuple[bool, str]:
+        """
+        Uses a package's information to determine
+        if the maintainer's email domain is unclaimed and thus exposed to hijacking
+
+        Args:
+            package_info (dict): package info from the package repository
+
+        Returns:
+            bool: True if email domain is unclaimed
+            str: Message explaining the issue
+        """
+
+        emails = self.get_email_addresses(package_info)
+
+        if len(emails) == 0:
+            # No e-mail is set for this package, hence no risk
+            return False, "No e-mail found for this package"
+
+        has_issues = False
+        messages = []
+        for email in emails:
+            domain = extract_email_address_domain(email)
+            # note: get_domain_creation_date is cached
+            _, domain_exists = get_domain_creation_date(domain)
+
+            if domain_exists:
+                continue
+
+            # domain does not exist
+
+            has_issues = True
+
+            messages.append(
+                f"The maintainer's email ({email}) domain does not exist and can likely be registered "
+                f"by an attacker to compromise the maintainer's {self.ecosystem} account"
+            )
+
+        return has_issues, "\n".join(messages)
+
+    @abstractmethod
+    def get_project_latest_release_date(self, package_info):
+        pass
+
+    @abstractmethod
+    def get_email_addresses(self, package_info) -> set[str]:
+        return set()
diff --git a/guarddog/analyzer/metadata/utils.py b/guarddog/analyzer/metadata/utils.py
new file mode 100644
index 0000000..75818b5
--- /dev/null
+++ b/guarddog/analyzer/metadata/utils.py
@@ -0,0 +1,54 @@
+from datetime import datetime
+from functools import cache
+from typing import Optional
+
+import whois  # type: ignore
+
+NPM_MAINTAINER_EMAIL_WARNING = (
+    "note that NPM's API may not provide accurate information regarding the maintainer's email, "
+    "so this detector may cause false positives for NPM packages. "
+    "see https://www.theregister.com/2022/05/10/security_npm_email/"
+)
+
+
+@cache
+def get_domain_creation_date(domain) -> tuple[Optional[datetime], bool]:
+    """
+    Gets the creation date of an domain name
+
+    Args:
+        domain (str): domain of email address
+
+    Returns:
+        datetime: creation date of domain
+        bool:     if the domain is currently registered
+    """
+
+    try:
+        domain_information = whois.whois(domain)
+    except whois.parser.PywhoisError as e:
+        # The domain doesn't exist at all, if that's the case we consider it vulnerable
+        # since someone could register it
+        return None, (not str(e).lower().startswith('no match for'))
+
+    if domain_information.creation_date is None:
+        # No creation date in whois, so we can't know
+        return None, True
+
+    creation_dates = domain_information.creation_date
+
+    if type(creation_dates) is list:
+        return min(creation_dates), True
+
+    return creation_dates, True
+
+
+def extract_email_address_domain(email_address: str):
+    sanitized_email = email_address.strip().replace(">", "").replace("<", "")
+
+    try:
+        domain = sanitized_email.split("@")[-1]
+        return domain
+
+    except IndexError:
+        raise ValueError(f"Invalid email address: {email_address}")


Test patch:
diff --git a/tests/analyzer/metadata/test_potentially_compromised_email_domain.py b/tests/analyzer/metadata/test_potentially_compromised_email_domain.py
index 01a224f..5187b93 100644
--- a/tests/analyzer/metadata/test_potentially_compromised_email_domain.py
+++ b/tests/analyzer/metadata/test_potentially_compromised_email_domain.py
@@ -7,6 +7,7 @@ from datetime import datetime
 import pytest
 from _pytest.monkeypatch import MonkeyPatch
 
+import guarddog.analyzer.metadata.utils
 from guarddog.analyzer.metadata.npm import NPMPotentiallyCompromisedEmailDomainDetector
 from guarddog.analyzer.metadata.pypi import PypiPotentiallyCompromisedEmailDomainDetector
 from tests.analyzer.metadata.resources.sample_project_info import (
@@ -15,18 +16,20 @@ from tests.analyzer.metadata.resources.sample_project_info import (
     generate_npm_project_info
     )
 
-with open(os.path.join(pathlib.Path(__file__).parent.resolve(), "resources", "npm_data.json"), "r") as file:
-    NPM_PACKAGE_INFO = json.load(file)
-
+from tests.analyzer.metadata.utils import MockWhoIs
 
-class MockWhoIs:
-    def __init__(self, date) -> None:
-        self.creation_date = date
 
+with open(os.path.join(pathlib.Path(__file__).parent.resolve(), "resources", "npm_data.json"), "r") as file:
+    NPM_PACKAGE_INFO = json.load(file)
 
 pypi_detector = PypiPotentiallyCompromisedEmailDomainDetector()
 npm_detector = NPMPotentiallyCompromisedEmailDomainDetector()
 
+# required because mocking in tests will cause get_domain_creation_date()
+# to return different results for a same domain
+@pytest.fixture(autouse=True)
+def clear_caches():
+    guarddog.analyzer.metadata.utils.get_domain_creation_date.cache_clear()
 
 class TestCompromisedEmail:
 
@@ -57,21 +60,21 @@ class TestCompromisedEmail:
 
         MonkeyPatch().setattr("whois.whois", mock_whois)
         compromised, _ = pypi_detector.detect(PYPI_PACKAGE_INFO)
-        assert compromised
+        assert not compromised
 
     empty_author_pypi = generate_pypi_project_info("author_email", None)
     empty_author_npm = generate_npm_project_info("maintainters", [{
         "name": "john doe",
         "email": None
     }])
-    
+
 
     @pytest.mark.parametrize("package_info, detector",
                              [(empty_author_pypi, pypi_detector), (empty_author_npm, npm_detector)])
     def test_email_domain_none(self, package_info, detector):
         def mock_whois(domain):
             return MockWhoIs(datetime(1990, 1, 31))
-        
+
         MonkeyPatch().setattr("whois.whois", mock_whois)
         compromised, _ = detector.detect(package_info)
         assert not compromised
diff --git a/tests/analyzer/metadata/test_unclaimed_maintainer_email_domain.py b/tests/analyzer/metadata/test_unclaimed_maintainer_email_domain.py
new file mode 100644
index 0000000..2f1eedb
--- /dev/null
+++ b/tests/analyzer/metadata/test_unclaimed_maintainer_email_domain.py
@@ -0,0 +1,45 @@
+from datetime import datetime
+import json
+import os
+import pathlib
+
+from _pytest.monkeypatch import MonkeyPatch
+import pytest
+
+from guarddog.analyzer.metadata.pypi import PypiUnclaimedMaintainerEmailDomainDetector
+import guarddog.analyzer.metadata.utils
+from tests.analyzer.metadata.resources.sample_project_info import PYPI_PACKAGE_INFO
+
+from tests.analyzer.metadata.utils import MockWhoIs
+
+
+with open(os.path.join(pathlib.Path(__file__).parent.resolve(), "resources", "npm_data.json"), "r") as file:
+    NPM_PACKAGE_INFO = json.load(file)
+
+pypi_detector = PypiUnclaimedMaintainerEmailDomainDetector()
+
+# required because mocking in tests will cause get_domain_creation_date()
+# to return different results for a same domain
+@pytest.fixture(autouse=True)
+def clear_caches():
+    guarddog.analyzer.metadata.utils.get_domain_creation_date.cache_clear()
+
+class TestUnclaimedMaintainerEmailDomain:
+    def test_email_domain_doesnt_exist(self):
+        def mock_whois(domain):
+            import whois
+            raise whois.parser.PywhoisError('No match for "nope.com".')
+
+        MonkeyPatch().setattr("whois.whois", mock_whois)
+        # should work exactly the same for NPM
+        compromised, _ = pypi_detector.detect(PYPI_PACKAGE_INFO)
+        assert compromised
+
+    def test_email_domain_does_exist(self):
+        def mock_whois(domain):
+            return MockWhoIs(datetime(1990, 1, 31))
+
+        MonkeyPatch().setattr("whois.whois", mock_whois)
+        # should work exactly the same for NPM
+        compromised, _ = pypi_detector.detect(PYPI_PACKAGE_INFO)
+        assert not compromised
diff --git a/tests/analyzer/metadata/utils.py b/tests/analyzer/metadata/utils.py
new file mode 100644
index 0000000..9f0f5d8
--- /dev/null
+++ b/tests/analyzer/metadata/utils.py
@@ -0,0 +1,4 @@
+class MockWhoIs:
+    def __init__(self, date) -> None:
+        self.creation_date = date
+