amadan

carlos / platform Public

console: SES ingest — fix the cert check that failed closed on real mail

Round-2's chain-verification fix broke every legitimate delivery: it
asserted the certificate's DNSName against the FETCH host
(sns.<region>.amazonaws.com) and decoded only the first PEM block,
discarding any bundled intermediate. A genuine SNS signing certificate
names "sns.amazonaws.com" (one identity for the whole service, not one per
region) and does not chain to a system root without its intermediate —
proven live, during review, against the real TLS leaf of
sns.eu-west-1.amazonaws.com. fetchVerifiedCertKey now decodes every PEM
block (leaf plus intermediates, via the new decodeCertChain) and asserts
the fixed subject SNS actually carries.

The host pin also had a residual bypass: pinning only the "sns." label left
the region label free, so "sns.s3.amazonaws.com" — a real, registered S3
bucket — matched it, the same shape as the documented sns-validator bypass.
validSigningCertURL now pins to this deployment's configured region
(MailDeployment.Region) exactly when known, falling back to a
region-shaped-only pattern (never a bare service name) when it isn't.

Both outbound fetches (the certificate, and the SubscribeURL confirmation)
now use a client that refuses to follow redirects — without it, a redirect
from an already-validated host could hand control to a completely
unvalidated one, defeating the SubscribeURL host pin outright.

Two tests were proving nothing: the SubscribeURL-off-AWS test asserted only
a status code, so removing the check still passed (the fetch to a link-local
address merely times out into the same-looking failure) — it now stands up
a real local server and asserts it is never reached. The self-signed-cert
test used a SAN that did not match the DNSName check at all, so it was
refused by the hostname mismatch before ever reaching chain verification —
replacing Verify with VerifyHostname left it passing. The fixture's SAN now
matches the checked subject exactly, isolating the chain check, and an
offline two-level CA hierarchy (decodeCertChain's own test) proves the
leaf+intermediate parsing and chaining logic end to end without any network
access or system trust store changes.
Paul Campbell pushed by paul@keymail.dev d66135bad5ce5e9e6cc0cde5c382215e2531837b parent 7470c1f
2 files changed, +441 −118
  • internal/console/sesevents.go +199 −86
  • internal/console/sesevents_test.go +242 −32
diff --git a/internal/console/sesevents.go b/internal/console/sesevents.go
index 74653fbd8..3fbe7611a 100644
--- a/internal/console/sesevents.go
+++ b/internal/console/sesevents.go
@@ -10,22 +10,46 @@
// where the verifying certificate may come from, and verify the signature
// before trusting any field the message carries.
//
-// SECURITY: validSigningCertURL is the entirety of SNS signature security.
-// SNS tells the receiver where to fetch the certificate that verifies the
-// message — so an unvalidated URL lets anyone who can reach this endpoint
-// host their own certificate, sign their own payload, and have it verify.
-// The consequence here is not merely bad data: forged bounce events trip
-// the automatic pause, so it is a remote off-switch for any tenant's email.
+// SECURITY: validSigningCertURL and fetchVerifiedCertKey TOGETHER are the
+// entirety of SNS signature security. SNS tells the receiver where to fetch
+// the certificate that verifies the message — so an unvalidated URL, or an
+// unverified certificate, lets anyone who can reach this endpoint host
+// their own certificate, sign their own payload, and have it verify. The
+// consequence here is not merely bad data: forged bounce events trip the
+// automatic pause, so it is a remote off-switch for any tenant's email.
//
-// A host allowlist is NOT a signer identity by itself (post-review fix): the
-// original check here accepted any *.amazonaws.com host, which is every AWS
-// customer's identity, not Amazon's — a free-tier S3 bucket or API Gateway
-// stage can serve a genuine, publicly-trusted certificate from a host that
-// satisfies "ends in amazonaws.com". validSigningCertURL now pins to SNS's
-// own regional endpoint shape, and verifiedCertKey separately verifies the
-// downloaded certificate's chain (against system roots) and hostname —
-// two independent checks, both required, because either alone still admits
-// a forgery the other closes.
+// A host allowlist is NOT a signer identity by itself, and a parsed
+// certificate is NOT a verified one — two review rounds' worth of fixes
+// live in this file because each of those was true in turn:
+//
+// - Round 1: the original check accepted any *.amazonaws.com host, which
+// is every AWS customer's identity, not Amazon's.
+// - Round 2 (C1-residual): pinning only the "sns." label and leaving the
+// region label free still accepted "sns.s3.amazonaws.com" — a real,
+// registered S3 bucket — the exact shape of the documented
+// sns-validator bypass. validSigningCertURL now pins to this
+// deployment's OWN region exactly when it is known, and falls back to
+// a region-SHAPED pattern (never a bare service name) only when it is
+// not.
+// - Round 2 (C2): the certificate was parsed and its public key trusted
+// with no chain verification and no subject check at all — a
+// self-signed certificate would have verified. fetchVerifiedCertKey now
+// checks the chain against system roots AND the subject SNS's real
+// certificate carries.
+// - Round 3 (C2 fixed-closed bug): the FIRST attempt at C2 asserted
+// DNSName against the FETCH host (e.g. sns.eu-west-1.amazonaws.com) and
+// decoded only the first PEM block, discarding any intermediate — but a
+// genuine SNS signing certificate names "sns.amazonaws.com" (one
+// identity for the whole service, not one per region) and does not
+// chain to a system root without its intermediate. That combination
+// failed closed on every real message. fetchVerifiedCertKey now decodes
+// every PEM block (leaf plus intermediates) and asserts the FIXED
+// subject SNS actually uses, not the fetch host.
+// - Round 3 (redirect guard): both outbound fetches used
+// http.DefaultClient, which follows redirects — for the SubscribeURL
+// fetch specifically, a redirect target is never re-validated against
+// the host pin, so following one defeats it outright. Both fetches now
+// use a client that refuses to follow a redirect at all.
package console
import (
@@ -59,8 +83,8 @@ const sesEventMaxBody = 256 << 10
// sesCertMaxBody bounds the signing certificate this door fetches (and the
// SubscribeURL response body it discards). AWS's SNS certificates are a few
-// KB; anything vastly larger than that is not a certificate this code needs
-// to finish reading.
+// KB, even bundled with an intermediate; anything vastly larger than that
+// is not a certificate response this code needs to finish reading.
const sesCertMaxBody = 64 << 10
// sesFetchTimeout bounds both outbound calls this door makes — fetching the
@@ -69,35 +93,67 @@ const sesCertMaxBody = 64 << 10
// far end costs one request this long, not a goroutine forever.
const sesFetchTimeout = 10 * time.Second
-// snsHostPattern is the ONLY host shape a signing certificate — or a
-// SubscribeURL — may come from: an SNS regional endpoint,
-// "sns.<region>.amazonaws.com". This is deliberately narrower than "any
-// amazonaws.com host": that wider family is not Amazon's identity, it is
-// every AWS customer's — an S3 bucket (evil-bucket.s3.amazonaws.com), an API
-// Gateway stage (xyz.execute-api.eu-west-1.amazonaws.com), or any of the
-// hundreds of other services that mint a genuine, publicly-trusted
-// certificate on an *.amazonaws.com host that a free-tier account can stand
-// up in minutes. Accepting any of those reduces this door's whole security
-// model to the secrecy of the topic ARN, which is not a secret — it appears
-// in IaC and logs.
-var snsHostPattern = regexp.MustCompile(`^sns\.[a-z0-9-]+\.amazonaws\.com$`)
+// snsFetchClient is used for both outbound fetches this door makes — the
+// signing certificate and the SubscribeURL confirmation.
+//
+// CheckRedirect refuses to follow. For the SubscribeURL fetch this is not
+// optional hardening: validSigningCertURL only ever inspects the ORIGINAL
+// URL, so a 3xx response pointing anywhere else would defeat the host pin
+// outright — the far end could serve a redirect to any host it likes and
+// this client would have followed it there. http.ErrUseLastResponse is the
+// documented way to say "hand me the redirect response itself, don't chase
+// it" — the caller then sees a non-2xx status and refuses it the same way
+// it refuses any other failure. Precedent: internal/edge/prober.go's client
+// uses the identical idiom for the identical reason — a redirect is not the
+// thing that was asked for.
+var snsFetchClient = &http.Client{
+ CheckRedirect: func(*http.Request, []*http.Request) error {
+ return http.ErrUseLastResponse
+ },
+}
+
+// snsHostPattern is the FALLBACK shape check, used only when this
+// deployment's own region is unknown: "sns." plus a REGION-SHAPED middle
+// label — "<2-letter-country>[-gov]-<name>-<digit>", e.g. "eu-west-1",
+// "us-gov-west-1", "ap-southeast-4" — plus ".amazonaws.com", and nothing
+// looser than that. A bare service name like "s3" or "s3-us-west-2" does
+// not have this shape, which is what closes the C1-residual bypass
+// described below.
+var snsHostPattern = regexp.MustCompile(`^sns\.[a-z]{2}(-gov)?-[a-z]+-\d+\.amazonaws\.com$`)
// validSigningCertURL refuses anything that is not an https URL on an SNS
-// regional host. It is also reused, unchanged, to validate SubscribeURL
-// before that URL is ever fetched (IMPORTANT-1 fix): a verified signature
-// only proves AWS put some string in that field, not that the string points
-// somewhere safe to GET, and a real SubscribeURL is itself always an SNS
-// regional URL, so the same pattern is the right check for both.
+// host — and, when region is non-empty, anything that is not THIS
+// deployment's own SNS regional host exactly. It is also reused, unchanged,
+// to validate SubscribeURL before that URL is ever fetched (round-2
+// IMPORTANT-1 fix): a verified signature only proves AWS put some string in
+// that field, not that the string points somewhere safe to GET, and a real
+// SubscribeURL is itself always an SNS regional URL, so the same check is
+// the right one for both.
+//
+// C1-RESIDUAL FIX (round 2 review): the previous pattern,
+// ^sns\.[a-z0-9-]+\.amazonaws\.com$, pins the FIRST label ("sns.") but
+// leaves the SECOND label completely unconstrained — which is the exact
+// shape of the documented sns-validator bypass:
+// "https://sns.s3.amazonaws.com/x.pem" matches it, and "sns" is a real,
+// registered AWS bucket name (the review confirmed it returns 403
+// AccessDenied — owned by someone, reachable, and NOT SNS). When this
+// deployment knows its own region (MailDeployment.Region, threaded in by
+// the caller), the host must equal "sns.<region>.amazonaws.com" EXACTLY —
+// no pattern, no label left free to be a bucket name. Only when the region
+// is unknown does this fall back to snsRegionPattern, which is at least
+// narrow enough to refuse a customer-chosen name: it requires the middle
+// label to have the SHAPE of an actual AWS region code, which "s3" and
+// "s3-us-west-2" do not.
//
-// The host check is LABEL-AWARE. A bare strings.HasSuffix(host,
-// "amazonaws.com") would accept "evilamazonaws.com" — it genuinely ends
-// with the string "amazonaws.com", with no dot separating the two, so it is
-// a completely different registered domain that merely happens to end the
-// right way. Anchoring the pattern on both ends (and requiring the "sns."
-// label specifically) is what tells a real AWS subdomain apart from that
-// lookalike, and from names like "sns.eu-west-1.amazonaws.com.evil.com"
-// that merely CONTAIN the right substring in the middle.
-func validSigningCertURL(raw string) error {
+// The host check is also LABEL-AWARE against the suffix-lookalike class of
+// bug: a bare strings.HasSuffix(host, "amazonaws.com") would accept
+// "evilamazonaws.com" — it genuinely ends with the string "amazonaws.com",
+// with no dot separating the two, so it is a completely different
+// registered domain that merely happens to end the right way. Anchoring the
+// whole pattern (or the whole exact-match string) is what refuses that, and
+// also refuses names like "sns.eu-west-1.amazonaws.com.evil.com" that
+// merely CONTAIN the right substring in the middle.
+func validSigningCertURL(raw, region string) error {
u, err := url.Parse(raw)
if err != nil {
return fmt.Errorf("sns: unparseable SigningCertURL")
@@ -106,7 +162,13 @@ func validSigningCertURL(raw string) error {
return fmt.Errorf("sns: SigningCertURL is not https")
}
host := strings.ToLower(u.Hostname())
- if !snsHostPattern.MatchString(host) {
+ region = strings.ToLower(strings.TrimSpace(region))
+ switch {
+ case region != "":
+ if host != "sns."+region+".amazonaws.com" {
+ return fmt.Errorf("sns: SigningCertURL host %q is not this deployment's SNS host (sns.%s.amazonaws.com)", host, region)
+ }
+ case !snsHostPattern.MatchString(host):
return fmt.Errorf("sns: SigningCertURL host %q is not an SNS host", host)
}
// A traversal in the path can walk off the certificate route on a host
@@ -184,10 +246,11 @@ func (e snsEnvelope) canonicalString() string {
// GET in dispatch) — otherwise a stranger's POST decides who this
// console makes outbound requests to, which is SSRF wearing an
// AWS-shaped alibi.
-// 3. Validate SigningCertURL's shape — https, an SNS regional host,
-// label-aware, no path traversal — before it is ever fetched.
+// 3. Validate SigningCertURL's shape — https, an SNS host (this
+// deployment's own region exactly, when known), no path traversal —
+// before it is ever fetched.
// 4. Verify the signature over SNS's canonical string, which includes
-// fetching the certificate and checking ITS chain and hostname too.
+// fetching the certificate and checking ITS chain and subject too.
// Nothing past this point trusts a single field of the message,
// including SubscribeURL, until this has succeeded.
// 5. Dispatch: confirm a subscription (after running its SubscribeURL
@@ -226,7 +289,8 @@ func (s *Server) handleSESEvents(w http.ResponseWriter, r *http.Request) {
}
// 3. Validate SigningCertURL before it is ever fetched.
- if err := validSigningCertURL(env.SigningCertURL); err != nil {
+ region := s.cfg.MailDeployment.Region
+ if err := validSigningCertURL(env.SigningCertURL, region); err != nil {
jsonError(w, http.StatusBadRequest, err.Error())
return
}
@@ -261,7 +325,7 @@ func (s *Server) handleSESEvents(w http.ResponseWriter, r *http.Request) {
// where that string points — so without this an authentic message
// can still carry a SubscribeURL aimed at an arbitrary host,
// including link-local metadata endpoints.
- if err := validSigningCertURL(env.SubscribeURL); err != nil {
+ if err := validSigningCertURL(env.SubscribeURL, region); err != nil {
log.Printf("console: ses events: refusing SubscribeURL: %v", err)
jsonError(w, http.StatusBadRequest, "invalid SubscribeURL")
return
@@ -313,13 +377,15 @@ func (s *Server) handleSESEvents(w http.ResponseWriter, r *http.Request) {
// SubscribeURL has passed validSigningCertURL — the signature proves AWS
// put the string there, the host check proves the string points at SNS
// itself rather than somewhere this console should never fetch on a
-// stranger's behalf.
+// stranger's behalf. It uses snsFetchClient (never http.DefaultClient) so a
+// redirect from that already-verified host cannot hand control to a
+// DIFFERENT, unverified one — see snsFetchClient's doc comment.
func (s *Server) confirmSNSSubscription(ctx context.Context, subscribeURL string) error {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, subscribeURL, nil)
if err != nil {
return fmt.Errorf("sns: building subscribe request: %w", err)
}
- resp, err := http.DefaultClient.Do(req)
+ resp, err := snsFetchClient.Do(req)
if err != nil {
return fmt.Errorf("sns: fetching SubscribeURL: %w", err)
}
@@ -364,8 +430,8 @@ func (c *snsCertCache) put(certURL string, pub *rsa.PublicKey) {
}
// verifiedCertKey returns the RSA public key for a signing certificate URL
-// — already pinned to an SNS regional host by the caller (validSigningCertURL)
-// — using the cache on a hit, and fetching plus fully verifying the
+// — already pinned to an SNS host by the caller (validSigningCertURL) —
+// using the cache on a hit, and fetching plus fully verifying the
// certificate on a miss.
func (s *Server) verifiedCertKey(ctx context.Context, certURL string) (*rsa.PublicKey, error) {
if pub, ok := s.snsCerts.get(certURL); ok {
@@ -379,39 +445,90 @@ func (s *Server) verifiedCertKey(ctx context.Context, certURL string) (*rsa.Publ
return pub, nil
}
-// fetchVerifiedCertKey downloads the certificate at certURL and returns its
-// RSA public key, but only once the certificate has passed BOTH of two
-// independent checks (CRITICAL-2 fix) — neither is sufficient alone:
+// snsCertDNSName is the hostname a genuine SNS message-signing certificate
+// actually carries in its subject/SAN — NOT the regional fetch host (e.g.
+// sns.eu-west-1.amazonaws.com). SNS mints one signing identity for the
+// whole service, not one per region. Verified against a genuine SNS
+// signing certificate the round-2 review pulled from AWS's own SDK test
+// fixture (aws-sdk-ruby, gems/aws-sdk-sns/spec/message_verifier_spec.rb):
+// subject=CN=sns.amazonaws.com, SAN=[sns.amazonaws.com],
+// issuer=CN=Amazon,OU=Server CA 1B,O=Amazon,C=US. That fixture is old and
+// is not treated as sufficient evidence that today's live certificate is
+// identical — see sesevents.go's package comment and the task report for
+// what remains unproven until a real message arrives.
+const snsCertDNSName = "sns.amazonaws.com"
+
+// decodeCertChain splits a PEM-encoded response body into its leaf
+// certificate (the first CERTIFICATE block) and every subsequent
+// CERTIFICATE block as an intermediates pool.
//
-// - Its chain verifies against the system root pool. Without this, a
-// certificate that merely PARSES is trusted — a self-signed certificate
-// generated on an attacker's laptop, or any other certificate not
-// actually issued by a CA anyone trusts, would verify a forged
-// signature.
-// - It is valid for the EXACT hostname certURL named. Without this, a
-// validly-issued certificate for a completely different (but still
+// ROUND-3 FIX: the previous version decoded only the FIRST PEM block and
+// discarded anything after it. A genuine Amazon leaf certificate does not
+// chain to a system root on its own — proven live, during the round-2
+// review, against the actual TLS leaf of sns.eu-west-1.amazonaws.com, which
+// fails chain verification until its intermediate ("Amazon RSA 2048 M01")
+// is supplied — so a signing-cert response that legitimately bundles an
+// intermediate was being silently truncated to just the part that can never
+// verify alone. This is what fixes-closed-on-every-real-message meant in
+// the round-3 review.
+func decodeCertChain(certPEM []byte) (leaf *x509.Certificate, intermediates *x509.CertPool, err error) {
+ intermediates = x509.NewCertPool()
+ rest := certPEM
+ for {
+ var block *pem.Block
+ block, rest = pem.Decode(rest)
+ if block == nil {
+ break
+ }
+ if block.Type != "CERTIFICATE" {
+ continue
+ }
+ c, perr := x509.ParseCertificate(block.Bytes)
+ if perr != nil {
+ return nil, nil, fmt.Errorf("sns: parsing signing cert: %w", perr)
+ }
+ if leaf == nil {
+ leaf = c
+ } else {
+ intermediates.AddCert(c)
+ }
+ }
+ if leaf == nil {
+ return nil, nil, fmt.Errorf("sns: signing cert response contained no PEM certificate")
+ }
+ return leaf, intermediates, nil
+}
+
+// fetchVerifiedCertKey downloads the certificate (and any bundled
+// intermediates) at certURL and returns its RSA public key, but only once
+// it has passed BOTH of two independent checks (CRITICAL-2 fix) — neither
+// is sufficient alone:
+//
+// - Its chain verifies against the system root pool, using any
+// intermediate certificates the response bundled alongside the leaf.
+// Without this, a certificate that merely PARSES is trusted — a
+// self-signed certificate generated on an attacker's laptop, or any
+// other certificate not actually issued by a CA anyone trusts, would
+// verify a forged signature.
+// - It is valid for snsCertDNSName — the fixed subject every genuine SNS
+// signing certificate carries, NOT the regional host certURL was
+// fetched from (that was round 2's C2 fix, and it was wrong: SNS names
+// one identity for the whole service). Without this, a validly-issued
+// certificate for a completely different (but still
// amazonaws.com-hosted) service could be swapped in for SNS's own —
// chain verification alone says "some CA vouches for this cert", not
// "this cert speaks for SNS".
//
-// certURL is assumed already checked by validSigningCertURL — this function
-// re-derives the hostname from it rather than trusting a caller-supplied
-// one, so there is exactly one source of truth for "which host this
-// certificate must be valid for".
+// Uses snsFetchClient, never http.DefaultClient, so this fetch cannot be
+// redirected to an unpinned host either.
func fetchVerifiedCertKey(ctx context.Context, certURL string) (*rsa.PublicKey, error) {
- u, err := url.Parse(certURL)
- if err != nil {
- return nil, fmt.Errorf("sns: unparseable SigningCertURL: %w", err)
- }
- hostname := u.Hostname()
-
fctx, cancel := context.WithTimeout(ctx, sesFetchTimeout)
defer cancel()
req, err := http.NewRequestWithContext(fctx, http.MethodGet, certURL, nil)
if err != nil {
return nil, fmt.Errorf("sns: building cert request: %w", err)
}
- resp, err := http.DefaultClient.Do(req)
+ resp, err := snsFetchClient.Do(req)
if err != nil {
return nil, fmt.Errorf("sns: fetching signing cert: %w", err)
}
@@ -423,21 +540,17 @@ func fetchVerifiedCertKey(ctx context.Context, certURL string) (*rsa.PublicKey,
if err != nil {
return nil, fmt.Errorf("sns: reading signing cert: %w", err)
}
- block, _ := pem.Decode(certPEM)
- if block == nil {
- return nil, fmt.Errorf("sns: signing cert is not PEM")
- }
- cert, err := x509.ParseCertificate(block.Bytes)
+ leaf, intermediates, err := decodeCertChain(certPEM)
if err != nil {
- return nil, fmt.Errorf("sns: parsing signing cert: %w", err)
+ return nil, err
}
- // Chain (system roots) and hostname, in one call: nil Roots defaults to
- // the system pool, and DNSName additionally requires the cert to be
- // valid for this exact host.
- if _, err := cert.Verify(x509.VerifyOptions{DNSName: hostname}); err != nil {
- return nil, fmt.Errorf("sns: signing cert failed chain/hostname verification: %w", err)
+ // Chain (system roots, plus any bundled intermediates) and subject, in
+ // one call: nil Roots defaults to the system pool, and DNSName
+ // additionally requires the leaf to be valid for SNS's own identity.
+ if _, err := leaf.Verify(x509.VerifyOptions{DNSName: snsCertDNSName, Intermediates: intermediates}); err != nil {
+ return nil, fmt.Errorf("sns: signing cert failed chain/subject verification: %w", err)
}
- pub, ok := cert.PublicKey.(*rsa.PublicKey)
+ pub, ok := leaf.PublicKey.(*rsa.PublicKey)
if !ok {
return nil, fmt.Errorf("sns: signing cert public key is not RSA")
}
diff --git a/internal/console/sesevents_test.go b/internal/console/sesevents_test.go
index 8ef5f8501..38f77becb 100644
--- a/internal/console/sesevents_test.go
+++ b/internal/console/sesevents_test.go
@@ -4,9 +4,10 @@ package console
// complaint notifications here. It is unauthenticated — SNS is not a signed-
// in member — so what stands between a forged bounce and an automatic pause
// on a stranger's say-so is exactly what these tests pin: the TopicArn
-// allowlist, the SigningCertURL/SubscribeURL host check, certificate chain
-// verification, and the ordering that runs all of it before any network call
-// or trusted field.
+// allowlist, the SigningCertURL/SubscribeURL host check (pinned to this
+// deployment's own region when known), certificate chain and subject
+// verification, a redirect guard on both outbound fetches, and the ordering
+// that runs all of it before any network call or trusted field.
import (
"context"
@@ -47,24 +48,40 @@ func newConfirmServer(t *testing.T, confirmed chan struct{}) *httptest.Server {
}))
}
-// The SigningCertURL check is the ENTIRETY of SNS signature security. If an
+// notReached fails the test if its handler is ever invoked. Used where a
+// status code alone is not evidence — a timeout and an active refusal can
+// both produce a non-2xx, and only one of them proves the far end was never
+// asked.
+func notReached(t *testing.T) *httptest.Server {
+ t.Helper()
+ return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ t.Errorf("hostile server was reached: %s %s", r.Method, r.URL)
+ w.WriteHeader(http.StatusOK)
+ }))
+}
+
+// The SigningCertURL check is the ENTIRETY of SNS signature security — half
+// of it, along with fetchVerifiedCertKey's chain/subject check. If an
// attacker can point it at a host they control, they sign their own message
// with their own certificate and it verifies — which means forging the
// bounce events that trigger an automatic pause, on any tenant.
//
-// The host pattern is pinned to SNS's own regional endpoint shape
-// ("sns.<region>.amazonaws.com"), not the wider amazonaws.com family: that
-// wider family is every AWS customer's identity, not Amazon's — an S3
-// bucket or an API Gateway stage can serve a genuine, publicly-trusted
-// certificate from a host that satisfies a bare "ends in amazonaws.com".
+// This test exercises the FALLBACK shape check (region == ""): a
+// region-shaped middle label, never a bare service name. The stricter
+// exact-region pin is TestSigningCertURLPinsToConfiguredRegionExactly.
func TestSigningCertURLMustBeAmazon(t *testing.T) {
ok := []string{
"https://sns.eu-west-1.amazonaws.com/SimpleNotificationService-abc.pem",
"https://sns.us-east-1.amazonaws.com/x.pem",
+ "https://sns.us-gov-west-1.amazonaws.com/x.pem",
+ "https://sns.ap-southeast-4.amazonaws.com/x.pem",
+ // The real SubscribeURL shape: a query string, not a bare path —
+ // validSigningCertURL is reused unchanged to check SubscribeURL too.
+ "https://sns.eu-west-1.amazonaws.com/?Action=ConfirmSubscription&TopicArn=arn:aws:sns:eu-west-1:1:carlos-mail&Token=abc123",
}
for _, u := range ok {
- if err := validSigningCertURL(u); err != nil {
- t.Fatalf("validSigningCertURL(%q) = %v, want nil", u, err)
+ if err := validSigningCertURL(u, ""); err != nil {
+ t.Fatalf("validSigningCertURL(%q, \"\") = %v, want nil", u, err)
}
}
bad := []struct{ url, why string }{
@@ -83,10 +100,40 @@ func TestSigningCertURLMustBeAmazon(t *testing.T) {
{"https://evil-bucket.s3.amazonaws.com/x.pem", "S3 bucket, not SNS"},
{"https://evil-bucket.s3.eu-west-1.amazonaws.com/attacker.pem", "regional S3 bucket, not SNS"},
{"https://xyz.execute-api.eu-west-1.amazonaws.com/p/x.pem", "API Gateway stage, not SNS"},
+ // C1-RESIDUAL: pinning only the "sns." label and leaving the second
+ // label free is the documented sns-validator bypass shape — "sns"
+ // itself is a real, registered (and reachable — 403 AccessDenied,
+ // i.e. owned by someone) S3 bucket name.
+ {"https://sns.s3.amazonaws.com/x.pem", "sns-named S3 bucket — the documented sns-validator bypass"},
+ {"https://sns.s3-us-west-2.amazonaws.com/x.pem", "sns-named regional S3 bucket — same bypass shape"},
+ }
+ for _, c := range bad {
+ if err := validSigningCertURL(c.url, ""); err == nil {
+ t.Fatalf("validSigningCertURL(%q, \"\") accepted — %s", c.url, c.why)
+ }
+ }
+}
+
+// When this deployment's own region is known, the host must equal
+// sns.<region>.amazonaws.com EXACTLY — not merely "look like" a region.
+// Without this, a deployment in eu-west-1 would still accept a validly-
+// shaped certificate URL for an entirely different region, which is more
+// host than any single deployment's SNS topic ever needs to trust, and the
+// bucket-name bypass shape (C1-residual) is refused a second, independent
+// way.
+func TestSigningCertURLPinsToConfiguredRegionExactly(t *testing.T) {
+ if err := validSigningCertURL("https://sns.eu-west-1.amazonaws.com/x.pem", "eu-west-1"); err != nil {
+ t.Fatalf("the deployment's own region was refused: %v", err)
+ }
+ bad := []struct{ url, why string }{
+ {"https://sns.us-east-1.amazonaws.com/x.pem", "a different, but validly-shaped, region"},
+ {"https://sns.s3.amazonaws.com/x.pem", "the bypass shape, now doubly refused"},
+ {"https://sns.eu-west-1.amazonaws.com.evil.com/x.pem", "suffix lookalike"},
+ {"http://sns.eu-west-1.amazonaws.com/x.pem", "plain http"},
}
for _, c := range bad {
- if err := validSigningCertURL(c.url); err == nil {
- t.Fatalf("validSigningCertURL(%q) accepted — %s", c.url, c.why)
+ if err := validSigningCertURL(c.url, "eu-west-1"); err == nil {
+ t.Fatalf("validSigningCertURL(%q, \"eu-west-1\") accepted — %s", c.url, c.why)
}
}
}
@@ -210,21 +257,36 @@ func TestSESEventsRefusesAnOversizeBodyEvenWhenOtherwiseValid(t *testing.T) {
// that the string points somewhere safe to GET. Without a host check on it
// this door is a blind SSRF dispatcher: an authentic message can carry a
// SubscribeURL aimed at anything reachable from the console, including
-// link-local instance-metadata endpoints. This must be refused before any
-// fetch of SubscribeURL is attempted, which is also why no confirm server is
-// stood up here — one being reached at all would be the failure.
+// link-local instance-metadata endpoints.
+//
+// ROUND-3 FIX: the previous version of this test pointed SubscribeURL at
+// 169.254.169.254 and asserted on the STATUS CODE alone. That is vacuous —
+// removing the host-check constraint entirely still leaves this test green,
+// because the fetch to that address simply times out into a 502 in this
+// sandbox, which looks identical to a deliberate refusal. This version
+// stands up a REAL local server as the hostile target and asserts its
+// handler is NEVER invoked — the only way to prove the request was refused
+// BEFORE any fetch was attempted, not merely that some fetch eventually
+// failed.
func TestSESEventsRefusesASubscribeURLOffAWS(t *testing.T) {
fx := newConsoleFixture(t)
fx.srv.cfg.MailDeployment.SNSTopicARN = "arn:aws:sns:eu-west-1:1:carlos-mail"
fx.srv.snsVerify = func(_ snsEnvelope) error { return nil }
+
+ hostile := notReached(t)
+ defer hostile.Close()
+
resp := sesPost(t, fx, `{"Type":"SubscriptionConfirmation","TopicArn":"arn:aws:sns:eu-west-1:1:carlos-mail",`+
`"Message":"You have chosen to subscribe...",`+
- `"SubscribeURL":"http://169.254.169.254/latest/meta-data/iam/security-credentials/",`+
+ `"SubscribeURL":"`+hostile.URL+`",`+
`"SigningCertURL":"https://sns.eu-west-1.amazonaws.com/x.pem","Signature":"x"}`)
defer resp.Body.Close()
if resp.StatusCode == http.StatusOK {
t.Fatal("a SubscribeURL off an SNS host was accepted")
}
+ // notReached's own handler calls t.Errorf if it is ever invoked — by the
+ // time sesPost has returned, the request (refusal or fetch) is already
+ // complete, so no further synchronization is needed here.
}
// The mechanical half of subscription handling: confirmSNSSubscription
@@ -249,6 +311,43 @@ func TestConfirmSNSSubscriptionFetchesTheURL(t *testing.T) {
}
}
+// IMPORTANT fix: neither outbound fetch this door makes may follow a
+// redirect. For SubscribeURL specifically, validSigningCertURL only ever
+// inspects the URL SNS's own signed message named — a redirect response
+// from that (now-verified) host is a completely different, UNvalidated
+// target, so following it defeats the host pin outright.
+func TestConfirmSNSSubscriptionDoesNotFollowARedirect(t *testing.T) {
+ hostile := notReached(t)
+ defer hostile.Close()
+
+ redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ http.Redirect(w, r, hostile.URL, http.StatusFound)
+ }))
+ defer redirector.Close()
+
+ fx := newConsoleFixture(t)
+ if err := fx.srv.confirmSNSSubscription(context.Background(), redirector.URL); err == nil {
+ t.Fatal("a redirect response was treated as a successful confirmation")
+ }
+}
+
+// Same guard, same reasoning, for the certificate fetch: a redirect there
+// would hand an attacker-controlled host the chance to serve the bytes this
+// door treats as SNS's own signing certificate.
+func TestFetchVerifiedCertKeyDoesNotFollowARedirect(t *testing.T) {
+ hostile := notReached(t)
+ defer hostile.Close()
+
+ redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ http.Redirect(w, r, hostile.URL, http.StatusFound)
+ }))
+ defer redirector.Close()
+
+ if _, err := fetchVerifiedCertKey(context.Background(), redirector.URL); err == nil {
+ t.Fatal("a redirect response was treated as a successful cert fetch")
+ }
+}
+
// A signature that does not verify must never reach the dispatch switch —
// proof the seam only bypasses the crypto, not the rest of the pipeline.
func TestSESEventsRefusesABadSignature(t *testing.T) {
@@ -338,13 +437,18 @@ func TestVerifySNSSignatureBytesRefusesAnUnknownVersion(t *testing.T) {
}
}
-// ---- fetchVerifiedCertKey: the certificate chain check -------------------
+// ---- fetchVerifiedCertKey: the certificate chain/subject check -----------
// selfSignedTestCertPEM builds a certificate that is internally well-formed
-// — it parses, it has a matching DNSName — but is signed by nothing anyone
-// trusts: exactly what an attacker running `openssl req -x509` on their own
-// laptop produces.
-func selfSignedTestCertPEM(t *testing.T, cn string) []byte {
+// — it parses, and its SAN matches snsCertDNSName exactly — but is signed
+// by nothing anyone trusts: exactly what an attacker running
+// `openssl req -x509` on their own laptop produces. The SAN is deliberately
+// the FIXED constant, not whatever host happens to serve it (round-3 fix to
+// this fixture): fetchVerifiedCertKey no longer checks the fetch host at
+// all, so a fixture whose SAN was the serving host's address would trip the
+// DNSName check before chain verification is even reached, proving nothing
+// about the chain check specifically. See TestFetchVerifiedCertKeyRefusesASelfSignedCert.
+func selfSignedTestCertPEM(t *testing.T) []byte {
t.Helper()
key, err := rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
@@ -352,8 +456,8 @@ func selfSignedTestCertPEM(t *testing.T, cn string) []byte {
}
tmpl := &x509.Certificate{
SerialNumber: big.NewInt(1),
- Subject: pkix.Name{CommonName: cn},
- DNSNames: []string{cn},
+ Subject: pkix.Name{CommonName: snsCertDNSName},
+ DNSNames: []string{snsCertDNSName},
NotBefore: time.Now().Add(-time.Hour),
NotAfter: time.Now().Add(time.Hour),
KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
@@ -367,14 +471,17 @@ func selfSignedTestCertPEM(t *testing.T, cn string) []byte {
return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})
}
-// CRITICAL-2 regression test: a certificate that parses fine and even names
-// the right host must still be REFUSED if nothing trusted signed it. Without
-// this check, an attacker who can point SigningCertURL at their own
-// (host-pinned-lookalike-proof) server — or who can otherwise get bytes onto
-// a URL that passes the host check — could serve a self-signed certificate
-// and have it accepted as SNS's own.
+// CRITICAL-2 regression test: a certificate whose SUBJECT matches exactly
+// what fetchVerifiedCertKey asserts (snsCertDNSName) must still be REFUSED
+// if nothing trusted signed it. Because the SAN matches, the DNSName check
+// inside cert.Verify passes trivially — what refuses this certificate is
+// ONLY the chain check, which is the property this test needs to isolate
+// (see the round-3 fix note on selfSignedTestCertPEM: the previous fixture
+// used a SAN that did not match the check at all, so it was refused by the
+// hostname mismatch and never reached the chain logic — a mutation that
+// deleted chain verification entirely still passed).
func TestFetchVerifiedCertKeyRefusesASelfSignedCert(t *testing.T) {
- certPEM := selfSignedTestCertPEM(t, "sns.eu-west-1.amazonaws.com")
+ certPEM := selfSignedTestCertPEM(t)
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write(certPEM)
}))
@@ -382,7 +489,7 @@ func TestFetchVerifiedCertKeyRefusesASelfSignedCert(t *testing.T) {
_, err := fetchVerifiedCertKey(context.Background(), ts.URL)
if err == nil {
- t.Fatal("a self-signed certificate passed chain verification")
+ t.Fatal("a self-signed certificate (with a matching subject) passed chain verification")
}
}
@@ -407,3 +514,106 @@ func TestVerifiedCertKeyServesFromCache(t *testing.T) {
t.Fatal("verifiedCertKey returned a different key than the one cached")
}
}
+
+// ---- decodeCertChain: leaf vs. intermediates -----------------------------
+
+// testCA builds either a self-signed root (parent == nil) or a certificate
+// issued by parent/parentKey — enough machinery to build a real,
+// independently-verifiable two-level chain entirely offline.
+func testCA(t *testing.T, cn string, parent *x509.Certificate, parentKey *rsa.PrivateKey) (*rsa.PrivateKey, *x509.Certificate) {
+ t.Helper()
+ key, err := rsa.GenerateKey(rand.Reader, 2048)
+ if err != nil {
+ t.Fatal(err)
+ }
+ tmpl := &x509.Certificate{
+ SerialNumber: big.NewInt(time.Now().UnixNano()),
+ Subject: pkix.Name{CommonName: cn},
+ NotBefore: time.Now().Add(-time.Hour),
+ NotAfter: time.Now().Add(time.Hour),
+ KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature,
+ BasicConstraintsValid: true,
+ IsCA: true,
+ }
+ signerCert, signerKey := tmpl, key
+ if parent != nil {
+ signerCert, signerKey = parent, parentKey
+ }
+ der, err := x509.CreateCertificate(rand.Reader, tmpl, signerCert, &key.PublicKey, signerKey)
+ if err != nil {
+ t.Fatal(err)
+ }
+ cert, err := x509.ParseCertificate(der)
+ if err != nil {
+ t.Fatal(err)
+ }
+ return key, cert
+}
+
+// testLeaf builds a non-CA certificate for dnsName, issued by parent.
+func testLeaf(t *testing.T, dnsName string, parent *x509.Certificate, parentKey *rsa.PrivateKey) *x509.Certificate {
+ t.Helper()
+ key, err := rsa.GenerateKey(rand.Reader, 2048)
+ if err != nil {
+ t.Fatal(err)
+ }
+ tmpl := &x509.Certificate{
+ SerialNumber: big.NewInt(time.Now().UnixNano()),
+ Subject: pkix.Name{CommonName: dnsName},
+ DNSNames: []string{dnsName},
+ NotBefore: time.Now().Add(-time.Hour),
+ NotAfter: time.Now().Add(time.Hour),
+ KeyUsage: x509.KeyUsageDigitalSignature,
+ }
+ der, err := x509.CreateCertificate(rand.Reader, tmpl, parent, &key.PublicKey, parentKey)
+ if err != nil {
+ t.Fatal(err)
+ }
+ cert, err := x509.ParseCertificate(der)
+ if err != nil {
+ t.Fatal(err)
+ }
+ return cert
+}
+
+func pemEncodeCert(c *x509.Certificate) []byte {
+ return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: c.Raw})
+}
+
+// ROUND-3 FIX regression test, entirely offline: decodeCertChain must treat
+// the FIRST PEM block as the leaf and every subsequent block as an
+// intermediate — and the intermediate must actually be NECESSARY for the
+// leaf to chain, mirroring the real SNS leaf, which the round-2 review
+// proved does not chain to a system root without its own intermediate
+// ("Amazon RSA 2048 M01"). A root that is trusted only within this test
+// (never the real system pool) makes the whole chain independently
+// verifiable without any network access or system trust store changes.
+func TestDecodeCertChainSeparatesLeafFromIntermediates(t *testing.T) {
+ rootKey, rootCert := testCA(t, "Test Root CA", nil, nil)
+ interKey, interCert := testCA(t, "Test Intermediate CA", rootCert, rootKey)
+ leafCert := testLeaf(t, snsCertDNSName, interCert, interKey)
+
+ blob := append(pemEncodeCert(leafCert), pemEncodeCert(interCert)...)
+ leaf, intermediates, err := decodeCertChain(blob)
+ if err != nil {
+ t.Fatalf("decodeCertChain: %v", err)
+ }
+ if leaf.Subject.CommonName != leafCert.Subject.CommonName {
+ t.Fatalf("leaf = %q, want %q — the first block must be the leaf", leaf.Subject.CommonName, leafCert.Subject.CommonName)
+ }
+
+ roots := x509.NewCertPool()
+ roots.AddCert(rootCert)
+
+ if _, err := leaf.Verify(x509.VerifyOptions{DNSName: snsCertDNSName, Intermediates: intermediates, Roots: roots}); err != nil {
+ t.Fatalf("leaf+intermediate did not verify against its own trusted root: %v", err)
+ }
+ // Prove the intermediate is NECESSARY, not incidental: the same leaf,
+ // against the same root, with no intermediate supplied, must fail. This
+ // is exactly the shape of the real Amazon leaf per the round-2 review —
+ // if this assertion did not hold, the fixture would prove nothing about
+ // needing decodeCertChain to carry the intermediate through at all.
+ if _, err := leaf.Verify(x509.VerifyOptions{DNSName: snsCertDNSName, Roots: roots}); err == nil {
+ t.Fatal("leaf verified against its root with NO intermediate — the fixture proves nothing about needing one")
+ }
+}