fix(ses_auth): harden comment stripping and alarm evaluation window (#103)

SES-AR-01: treat ")" at depth 0 as an unmatched close, rejecting the
value as not well-formed so a ")(...)" pair cannot manufacture a depth-0
gap where a smuggled dkim=pass clause gets parsed.

SES-AR-02: emit a space when a comment is removed so comments act as
CFWS folding whitespace (RFC 5322). Without this, "dk(z)im=pass" would
become "dkim=pass" and an attacker comment could glue unrelated tokens.

Alarm: widen evaluation_periods from 3→6 (30-min window) with
datapoints_to_alarm still at 2, closing the sparse-outage residual where
rejections >10-15 min apart fail to place breaching datapoints in 2 of 3
consecutive periods.

Both ses_auth.py copies stay byte-identical. All 86 tests pass.

Co-authored-by: amoussa1229 <166072409+amoussa1229@users.noreply.github.com>

cdk/po_stack.py

@@ -121,12 +121,15 @@ def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic
     # drift outage (allowlist wrong / signing-domain changed) may only produce
     # a trickle of rejects -- one every few minutes -- that never sums to 3 in
     # any window, so the outage never pages. Instead: >=1 reject per 5-min
-    # period, alarming when 2 of the last 3 periods breach (evaluation_periods=3
-    # / datapoints_to_alarm=2, the same idiom as the duration alarm). A single
-    # stray spoof probe (one lone period) is tolerated and self-clears, but a
-    # sustained reject condition trips within ~10-15 min even at one reject per
-    # period. default_value=0 on the metric filter keeps the series continuous
-    # so NOT_BREACHING only applies before the first datapoint ever arrives.
+    # period, alarming when 2 of the last 6 periods breach (evaluation_periods=6
+    # / datapoints_to_alarm=2). Six periods (30 min) with only 2 required
+    # datapoints closes the sparse-outage residual: even rejections >10-15 min
+    # apart can still place two breaching datapoints in a single 30-min
+    # evaluation window. A single stray spoof probe (one lone period) is
+    # tolerated and self-clears, but a sustained reject condition trips even
+    # at very low arrival rates. default_value=0 on the metric filter keeps the
+    # series continuous so NOT_BREACHING only applies before the first datapoint
+    # ever arrives.
     cloudwatch.Metric(
         namespace=_SENDER_AUTH_METRIC_NAMESPACE,
         metric_name=metric_name,
@@ -141,7 +144,7 @@ def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic
             "(possible allowlist/DKIM-domain drift silently dropping real mail)"
         ),
         threshold=1,
-        evaluation_periods=3,
+        evaluation_periods=6,
         datapoints_to_alarm=2,
         comparison_operator=cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
         treat_missing_data=cloudwatch.TreatMissingData.NOT_BREACHING,

cdk/wo_stack.py

@@ -121,12 +121,15 @@ def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic
     # drift outage (allowlist wrong / signing-domain changed) may only produce
     # a trickle of rejects -- one every few minutes -- that never sums to 3 in
     # any window, so the outage never pages. Instead: >=1 reject per 5-min
-    # period, alarming when 2 of the last 3 periods breach (evaluation_periods=3
-    # / datapoints_to_alarm=2, the same idiom as the duration alarm). A single
-    # stray spoof probe (one lone period) is tolerated and self-clears, but a
-    # sustained reject condition trips within ~10-15 min even at one reject per
-    # period. default_value=0 on the metric filter keeps the series continuous
-    # so NOT_BREACHING only applies before the first datapoint ever arrives.
+    # period, alarming when 2 of the last 6 periods breach (evaluation_periods=6
+    # / datapoints_to_alarm=2). Six periods (30 min) with only 2 required
+    # datapoints closes the sparse-outage residual: even rejections >10-15 min
+    # apart can still place two breaching datapoints in a single 30-min
+    # evaluation window. A single stray spoof probe (one lone period) is
+    # tolerated and self-clears, but a sustained reject condition trips even
+    # at very low arrival rates. default_value=0 on the metric filter keeps the
+    # series continuous so NOT_BREACHING only applies before the first datapoint
+    # ever arrives.
     cloudwatch.Metric(
         namespace=_SENDER_AUTH_METRIC_NAMESPACE,
         metric_name=metric_name,
@@ -141,7 +144,7 @@ def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic
             "(possible allowlist/DKIM-domain drift silently dropping real mail)"
         ),
         threshold=1,
-        evaluation_periods=3,
+        evaluation_periods=6,
         datapoints_to_alarm=2,
         comparison_operator=cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
         treat_missing_data=cloudwatch.TreatMissingData.NOT_BREACHING,

lambdas/po/email_processor/ses_auth.py

@@ -99,15 +99,28 @@ def _strip_comments(value: str) -> tuple:
     Dropping comments first means comment-embedded ``header.i=`` / ``;``
     fakes can never influence clause splitting or domain extraction.
 
+    Every removed comment is replaced by a single space so comments act as
+    folding whitespace (RFC 5322 CFWS semantics). A ``)`` that appears
+    outside any comment (depth 0) is an unmatched close and makes the value
+    not well-formed, closing the ``)(`` clause-injection gap.
+
+    CFWS also means a comment abutting a token *ends* that token:
+    ``header.d=seahaven.com(note)`` reads as domain ``seahaven.com``, where
+    the pre-CFWS gluing behaviour read ``seahaven.comnote`` (rejected).
+    This is deliberate -- ``header.d`` is written verbatim by SES from a
+    *verified* signature's ``d=`` tag, so a comment in that position is
+    never attacker-controlled.
+
     Returns ``(stripped_text, well_formed)``. ``well_formed`` is False when
-    the value ends inside an unterminated comment or quoted-string, i.e. the
-    parens/quotes are unbalanced. Callers reject on ``not well_formed`` so a
-    malformed header (which could otherwise be mis-tokenised) fails closed
-    rather than being partially parsed.
+    the value ends inside an unterminated comment or quoted-string, or when
+    an unmatched ``)`` appears at the top level. Callers reject on
+    ``not well_formed`` so a malformed header (which could otherwise be
+    mis-tokenised) fails closed rather than being partially parsed.
     """
     out = []
     depth = 0  # comment nesting depth
     in_quote = False  # inside a quoted-string (only tracked at depth 0)
+    extra_close = False  # SES-AR-01: ")" at depth 0 (no matching open)
     i = 0
     n = len(value)
     while i < n:
@@ -121,6 +134,9 @@ def _strip_comments(value: str) -> tuple:
                 depth += 1
             elif c == ")":
                 depth -= 1
+                if depth == 0:
+                    # SES-AR-02: comment acts as folding whitespace (RFC 5322).
+                    out.append(" ")
             i += 1
             continue
         if in_quote:
@@ -138,11 +154,18 @@ def _strip_comments(value: str) -> tuple:
             depth += 1
             i += 1
             continue
+        if c == ")":
+            # SES-AR-01: ")" at depth 0 is an unmatched close -- refuse to
+            # tokenise rather than letting a ")(...)" pair manufacture a
+            # gap where attacker text leaks out at depth 0.
+            extra_close = True
+            i += 1
+            continue
         if c == '"':
             in_quote = True
         out.append(c)
         i += 1
-    well_formed = depth == 0 and not in_quote
+    well_formed = depth == 0 and not in_quote and not extra_close
     return "".join(out), well_formed
 
 

lambdas/wo/email_processor/ses_auth.py

@@ -99,15 +99,28 @@ def _strip_comments(value: str) -> tuple:
     Dropping comments first means comment-embedded ``header.i=`` / ``;``
     fakes can never influence clause splitting or domain extraction.
 
+    Every removed comment is replaced by a single space so comments act as
+    folding whitespace (RFC 5322 CFWS semantics). A ``)`` that appears
+    outside any comment (depth 0) is an unmatched close and makes the value
+    not well-formed, closing the ``)(`` clause-injection gap.
+
+    CFWS also means a comment abutting a token *ends* that token:
+    ``header.d=seahaven.com(note)`` reads as domain ``seahaven.com``, where
+    the pre-CFWS gluing behaviour read ``seahaven.comnote`` (rejected).
+    This is deliberate -- ``header.d`` is written verbatim by SES from a
+    *verified* signature's ``d=`` tag, so a comment in that position is
+    never attacker-controlled.
+
     Returns ``(stripped_text, well_formed)``. ``well_formed`` is False when
-    the value ends inside an unterminated comment or quoted-string, i.e. the
-    parens/quotes are unbalanced. Callers reject on ``not well_formed`` so a
-    malformed header (which could otherwise be mis-tokenised) fails closed
-    rather than being partially parsed.
+    the value ends inside an unterminated comment or quoted-string, or when
+    an unmatched ``)`` appears at the top level. Callers reject on
+    ``not well_formed`` so a malformed header (which could otherwise be
+    mis-tokenised) fails closed rather than being partially parsed.
     """
     out = []
     depth = 0  # comment nesting depth
     in_quote = False  # inside a quoted-string (only tracked at depth 0)
+    extra_close = False  # SES-AR-01: ")" at depth 0 (no matching open)
     i = 0
     n = len(value)
     while i < n:
@@ -121,6 +134,9 @@ def _strip_comments(value: str) -> tuple:
                 depth += 1
             elif c == ")":
                 depth -= 1
+                if depth == 0:
+                    # SES-AR-02: comment acts as folding whitespace (RFC 5322).
+                    out.append(" ")
             i += 1
             continue
         if in_quote:
@@ -138,11 +154,18 @@ def _strip_comments(value: str) -> tuple:
             depth += 1
             i += 1
             continue
+        if c == ")":
+            # SES-AR-01: ")" at depth 0 is an unmatched close -- refuse to
+            # tokenise rather than letting a ")(...)" pair manufacture a
+            # gap where attacker text leaks out at depth 0.
+            extra_close = True
+            i += 1
+            continue
         if c == '"':
             in_quote = True
         out.append(c)
         i += 1
-    well_formed = depth == 0 and not in_quote
+    well_formed = depth == 0 and not in_quote and not extra_close
     return "".join(out), well_formed
 
 

tests/test_ses_auth.py

@@ -212,6 +212,63 @@ class TestParseAuthenticationResults:
         )
         assert passing == frozenset({"seahaven.com"})
 
+    # --- SES-AR-01: ")" at depth 0 (extra close) must fail closed ---
+
+    def test_extra_close_paren_fails_closed(self, ses_auth):
+        # SES-AR-01: a ")" at depth 0 makes the value not well-formed,
+        # preventing a ")(...)" pair from manufacturing a depth-0 gap
+        # where a smuggled "dkim=pass" clause is parsed.
+        authserv_id, passing = ses_auth.parse_authentication_results(
+            "amazonses.com; spf=pass (legit)) ; dkim=pass header.d=seahaven.com (rest)"
+        )
+        assert authserv_id == ""
+        assert passing == frozenset()
+
+    def test_extra_close_paren_at_start_fails_closed(self, ses_auth):
+        authserv_id, passing = ses_auth.parse_authentication_results(
+            "amazonses.com;) dkim=pass header.d=seahaven.com"
+        )
+        assert authserv_id == ""
+        assert passing == frozenset()
+
+    # --- SES-AR-02: comment removal inserts space (CFWS semantics) ---
+
+    def test_comment_adjacent_to_result_still_terminates_token(self, ses_auth):
+        # Regression: "dkim=pass(good signature)" must still read as
+        # "dkim=pass" after the comment is replaced by a space.
+        _, passing = ses_auth.parse_authentication_results(
+            "amazonses.com; dkim=pass(good signature) header.i=@seahaven.com"
+        )
+        assert passing == frozenset({"seahaven.com"})
+
+    def test_comment_glue_does_not_create_dkim_clause(self, ses_auth):
+        # SES-AR-02: "dk(z)im=pass" must NOT become "dkim=pass" when the
+        # comment is stripped. The space inserted for the comment produces
+        # "dk zim=pass", which does not start with "dkim".
+        _, passing = ses_auth.parse_authentication_results(
+            "amazonses.com; dk(z)im=pass header.i=@seahaven.com"
+        )
+        assert passing == frozenset()
+
+    def test_comment_separates_adjacent_tokens(self, ses_auth):
+        # SES-AR-02: "sea(x)haven.com" must NOT become "seahaven.com".
+        # The space keeps the tokens separate.
+        _, passing = ses_auth.parse_authentication_results(
+            "amazonses.com; sea(x)haven.com dkim=pass header.i=@seahaven.com"
+        )
+        assert passing == frozenset()
+
+    def test_comment_after_header_d_ends_the_domain_token(self, ses_auth):
+        # CFWS: a comment abutting header.d ends the token, so the domain
+        # reads as "seahaven.com" (pre-CFWS gluing read "seahaven.comnote",
+        # rejected). Deliberate: header.d is written verbatim by SES from a
+        # *verified* signature's d= tag, so a comment in that position is
+        # never attacker-controlled.
+        _, passing = ses_auth.parse_authentication_results(
+            "amazonses.com; dkim=pass header.d=seahaven.com(note)"
+        )
+        assert passing == frozenset({"seahaven.com"})
+
     def test_unbalanced_quote_fails_closed(self, ses_auth):
         # An unterminated quoted-string is malformed; refuse to parse it so a
         # dkim=pass clause "swallowed" by the runaway quote can't be salvaged