Add an Amazon SES email sender using the SES v2 API and IAM credentials by lorexoff · Pull Request #52361 · keycloak/keycloak

lorexoff · 2026-09-03T10:55:33Z

Closes #52360
Discussion: #52359

What

Adds an EmailSenderProvider with the id aws-ses that delivers Keycloak's transactional email through the Amazon SES v2 API, signed with AWS SigV4, instead of through SES's SMTP interface. It does nothing unless selected with --spi-email-sender--provider=aws-ses.

Why

SES SMTP requires a username and password derived from a long-lived IAM user's secret access key. It cannot be an assumed role, AWS does not rotate it, and it lives in realm.smtpServer.password. A Keycloak on EKS, ECS or EC2 cannot use the identity its platform already gives it. email.adoc documents the dead end today: "XOAUTH2 is not supported by the AWS-SMTP service. The AWS-service requires the use of a password."

How

No new dependencies. SigV4 signing, the AWS credential chain, JSON and MIME are implemented against what the server already ships: HttpClientProvider (Apache HttpClient), JsonSerialization (Jackson) and jakarta.mail. No AWS SDK, no new Maven module, no pom changes — one package under services/src/main/java/org/keycloak/email/aws/ and one appended line in META-INF/services/org.keycloak.email.EmailSenderProviderFactory, the same shape as Sending Mails via SMTP and XOAUTH2 authentication mechanism #36665.
Message parity. The MIME is built exactly as DefaultEmailSenderProvider builds it — same multipart/alternative, same header order, same RFC 2047 subject encoding, Reply-To defaulting to From, the same IDN conversion convertEmail performs — and sent as Content.Raw, so a realm that switches transport changes nothing a recipient can observe. FromEmailAddress and the MIME From header are derived from a single InternetAddress, so the parameter and the header cannot disagree.
One difference from the SMTP path is deliberate: MimeMessage.writeTo emits bare LF inside 7bit parts, which the mail implementation's own CRLF stream hides on the SMTP socket and nothing hides on an API call, so line endings are normalised before the message is base64-encoded.
Realm settings keep their meaning: from, fromDisplayName, replyTo, replyToDisplayName, timeout, connectionTimeout. envelopeFrom maps to FeedbackForwardingEmailAddress; that mapping is partial — SES fixes the SMTP envelope sender to the identity's MAIL FROM domain — and is documented and warned about rather than presented as lossless.
Credentials come only from the standard AWS chain (system properties, environment, web identity token, shared config files, container credentials, IMDSv2), never from Keycloak configuration, because kc.sh show-config prints unregistered SPI options unmasked. The container and instance-metadata lookups are constrained to the documented AWS endpoints, and IMDSv1 is not implemented.
No retries. SES SendEmail has no idempotency token, so a retried request that reached AWS would put a second activation email in a user's inbox. Note that the shared HTTP client installs DefaultHttpRequestRetryHandler regardless, and turns it into a resend-after-send when spi-connections-http-client-default-max-retries is set; since that cannot be opted out of per request, the factory logs a warning when it sees it.
Inert by default. order() stays 0, so the provider cannot win default-provider resolution by accident. A misconfiguration is fatal at boot only when this provider is the one the server was told to use — a Keycloak that cannot send an activation link should not start pretending it can — and is silent otherwise.

Testing

234 unit tests in services/src/test/java/org/keycloak/email/aws/: SigV4 against AWS's published signing test vectors, MIME parity with the SMTP sender, the request body, error mapping, and every credential source. JUnit 5 + Hamcrest, no mocking framework — the three collaborators that touch the outside world (HTTP, the process environment, the clock) are interfaces with hand-written fakes.
AwsSesEmailSenderTest in tests/base (org.keycloak.tests.providers, already registered in Base3, so check-base-suites.sh needs no change) runs a real server with the sender selected, points the provider's endpoint option at the @InjectHttpServer stub, triggers an admin execute-actions email, and asserts on the bytes that left the server: the SigV4 credential scope, and the base64-decoded MIME's structure, To, From and line endings. A second test answers 400 with x-amzn-ErrorType: MessageRejected and asserts the failure reaches the administrator. No AWS account, no credentials, no network.
Additionally run against real quay.io/keycloak/keycloak containers for 26.0.7 and 26.7.3 to confirm the compatibility decision below, including a negative control (with the option removed the run fails, so the test cannot pass for the wrong reason).

Compatibility

validate(Map) is declared without @Override: it does not exist on 26.0.x, 26.1.x or 26.3.0–26.3.2 and is abstract on newer releases, so this is what lets one implementation work across the range. Verified on both ends: on 26.7.3 a realm update with a malformed from returns 400 with this provider's own message, i.e. the server really does dispatch into it; on 26.0.7 the same update returns 204.

Nothing existing changes behaviour: the default sender stays default, and the only edit to an existing file is the appended service-registration line.

Documentation

server_admin/topics/realms/email.adoc: a section inside the existing ifeval::[{project_community}==true] guard, next to the XOAUTH2 vendor notes, carrying the same "provided as-is" disclaimer.
A release note in release_notes/topics/26_8_0.adoc.

Notes for the reviewer

While writing the integration test I hit what looks like a pre-existing bug in the test framework: HttpServerUtil.sendResponse sets the response headers after calling exchange.sendResponseHeaders(...), so they are never sent. Its only current caller does not check them, which is why it has gone unnoticed. I have sent a separate one-line fix; this PR carries its own small respond() helper so the two are independent.

Disclosure per CONTRIBUTING: AI agents were used in researching and writing this change.

Copilot

🟡 Changes recommended

Endpoint security, partition support, temporary-credential expiry, and documented credential behavior need correction.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds an opt-in Amazon SES v2 email sender using SigV4 and AWS-provided credentials.

Changes:

Implements SES MIME delivery, signing, HTTP transport, configuration, and error handling.
Adds an AWS credential provider chain for common workload identities.
Adds documentation and extensive unit/integration coverage.

File summaries

File	Description
`services/src/main/java/org/keycloak/email/aws/AwsHttpRequest.java`	Defines immutable AWS HTTP requests.
`services/src/main/java/org/keycloak/email/aws/AwsHttpResponse.java`	Models bounded AWS HTTP responses.
`services/src/main/java/org/keycloak/email/aws/AwsHttpTransport.java`	Defines the transport contract.
`services/src/main/java/org/keycloak/email/aws/AwsSesConfig.java`	Resolves SES configuration.
`services/src/main/java/org/keycloak/email/aws/AwsSesConfigurationException.java`	Represents configuration failures.
`services/src/main/java/org/keycloak/email/aws/AwsSesEmailSenderProvider.java`	Implements the email sender SPI.
`services/src/main/java/org/keycloak/email/aws/AwsSesEmailSenderProviderFactory.java`	Registers and configures the provider.
`services/src/main/java/org/keycloak/email/aws/AwsV4Signer.java`	Implements SigV4 signing.
`services/src/main/java/org/keycloak/email/aws/CrlfNormalizingOutputStream.java`	Normalizes MIME line endings.
`services/src/main/java/org/keycloak/email/aws/KeycloakHttpTransport.java`	Uses Keycloak’s shared HTTP client.
`services/src/main/java/org/keycloak/email/aws/SesClient.java`	Calls SES v2 SendEmail.
`services/src/main/java/org/keycloak/email/aws/SesErrorResponse.java`	Parses SES error responses.
`services/src/main/java/org/keycloak/email/aws/SesRawMessage.java`	Composes raw MIME messages.
`services/src/main/java/org/keycloak/email/aws/credentials/AwsCredentials.java`	Models AWS credentials.
`services/src/main/java/org/keycloak/email/aws/credentials/AwsCredentialsException.java`	Represents credential failures.
`services/src/main/java/org/keycloak/email/aws/credentials/AwsCredentialsProvider.java`	Defines credential sources.
`services/src/main/java/org/keycloak/email/aws/credentials/AwsCredentialsProviderChain.java`	Resolves and caches credentials.
`services/src/main/java/org/keycloak/email/aws/credentials/AwsEnvironment.java`	Abstracts process configuration.
`services/src/main/java/org/keycloak/email/aws/credentials/ContainerCredentialsProvider.java`	Supports ECS/EKS credentials.
`services/src/main/java/org/keycloak/email/aws/credentials/EnvironmentVariableCredentialsProvider.java`	Reads environment credentials.
`services/src/main/java/org/keycloak/email/aws/credentials/InstanceMetadataCredentialsProvider.java`	Supports EC2 IMDSv2 credentials.
`services/src/main/java/org/keycloak/email/aws/credentials/ProfileCredentialsProvider.java`	Reads static AWS profiles.
`services/src/main/java/org/keycloak/email/aws/credentials/SystemPropertiesCredentialsProvider.java`	Reads JVM credentials.
`services/src/main/java/org/keycloak/email/aws/credentials/WebIdentityTokenCredentialsProvider.java`	Supports EKS web identity.
`services/src/main/resources/META-INF/services/org.keycloak.email.EmailSenderProviderFactory`	Registers the SES factory.
`services/src/test/java/org/keycloak/email/aws/AwsSesConfigTest.java`	Tests SES configuration.
`services/src/test/java/org/keycloak/email/aws/AwsSesEmailSenderProviderFactoryTest.java`	Tests factory activation behavior.
`services/src/test/java/org/keycloak/email/aws/AwsSesEmailSenderProviderTest.java`	Tests provider wiring.
`services/src/test/java/org/keycloak/email/aws/AwsV4SignerTest.java`	Tests SigV4 vectors.
`services/src/test/java/org/keycloak/email/aws/CrlfNormalizingOutputStreamTest.java`	Tests line normalization.
`services/src/test/java/org/keycloak/email/aws/FakeTransport.java`	Provides an HTTP test fake.
`services/src/test/java/org/keycloak/email/aws/KeycloakHttpTransportTest.java`	Tests actual HTTP behavior.
`services/src/test/java/org/keycloak/email/aws/SesClientTest.java`	Tests SES requests and responses.
`services/src/test/java/org/keycloak/email/aws/SesErrorResponseTest.java`	Tests error parsing.
`services/src/test/java/org/keycloak/email/aws/SesRawMessageTest.java`	Tests MIME parity.
`services/src/test/java/org/keycloak/email/aws/TestEnvironment.java`	Provides isolated environment state.
`services/src/test/java/org/keycloak/email/aws/credentials/AwsCredentialsProviderChainTest.java`	Tests chain precedence and caching.
`services/src/test/java/org/keycloak/email/aws/credentials/ContainerCredentialsProviderTest.java`	Tests container credentials.
`services/src/test/java/org/keycloak/email/aws/credentials/InstanceMetadataCredentialsProviderTest.java`	Tests IMDSv2 behavior.
`services/src/test/java/org/keycloak/email/aws/credentials/ProfileCredentialsProviderTest.java`	Tests profile parsing.
`services/src/test/java/org/keycloak/email/aws/credentials/StaticCredentialsProvidersTest.java`	Tests static credential sources.
`services/src/test/java/org/keycloak/email/aws/credentials/WebIdentityTokenCredentialsProviderTest.java`	Tests STS web identity.
`tests/base/src/test/java/org/keycloak/tests/providers/AwsSesEmailSenderTest.java`	Adds server-level integration coverage.
`docs/documentation/server_admin/topics/realms/email.adoc`	Documents setup and credentials.
`docs/documentation/release_notes/topics/26_8_0.adoc`	Announces the SES provider.

Review details

Suppressed comments (1)

services/src/main/java/org/keycloak/email/aws/AwsSesConfig.java:137

This accepts any URI scheme with a host, such as ftp://host, even though the transport only supports HTTP(S); boot succeeds and every send then fails with an unsupported protocol. Reject schemes other than http and https during configuration parsing.

 
8000
           URI parsed = new URI(base);
            if (parsed.getScheme() == null || parsed.getHost() == null) {
                throw new AwsSesConfigurationException("'" + endpoint + "' is not a usable Amazon SES endpoint URL:"
                        + " it must be absolute, for example https://email.eu-central-1.amazonaws.com");

Files reviewed: 45/45 changed files
Comments generated: 11
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

lorexoff · 2026-09-03T11:44:20Z

Thanks — I went through each comment and verified it against the code before acting. Nine were right and are fixed in the amended commit; two I could not reproduce and have left alone, with the evidence below.

Fixed

STS endpoint built from an unvalidated AWS_REGION. This was the serious one. The region is now validated before it reaches a hostname, in both the SES and the STS path, and the STS provider fails with a clear message instead of building a URL from the value. Test: refusesARegionThatWouldRedirectTheTokenToAnotherHost asserts no request is made at all, so the token cannot leave the process.
aws-cn partition. Both default endpoints now derive the DNS suffix from the region (AwsRegion.dnsSuffix), so cn-north-1 resolves email.cn-north-1.amazonaws.com.cn and sts.cn-north-1.amazonaws.com.cn. Covered by usesThePartitionSuffixWhenBuildingTheDefaultEndpoint and usesThePartitionSuffixForTheRegionalStsEndpoint.
envelopeFrom bypassed validation on the send path. Correct, and it matters precisely on the servers where validate(Map) is never called. It now goes through the same requireSendableAddress as every other address, so it is validated and IDN-converted. Two tests: a malformed value fails before any request, and bounces@società.it reaches SES as bounces@xn--societ-nta.it.
Container credentials accepted without Token or Expiration. Right, and the second half was worse than described: the code's own javadoc said an unreadable expiry must not be treated as "never expires", while a missing one silently was — so the chain would have cached role credentials for the life of the process. Both fields are now required, in the container and the IMDS provider.
postInit warning on servers that do not use this sender. Correct: an ambient AWS_REGION is enough to build a configuration, so the warning could fire on any AWS-hosted Keycloak. Now guarded by isSelectedEmailSender().
Endpoint scheme (the suppressed comment). Also fixed: anything other than http/https is rejected at configuration time rather than failing on every send.
ses:SendRawEmail in the docs. Agreed. IAM authorises operations, and Content.Raw is a field of the single v2 SendEmail operation, not a second one; ses:SendRawEmail belongs to the v1 API and the SMTP interface. The documentation now asks for ses:SendEmail and explains that a policy inherited from an SMTP deployment already covers this.
"standard AWS chain" overstated the profile support. True — the profile source reads static keys only, and silently skips role_arn, credential_process and SSO profiles. Documented as such rather than implemented: those are three more credential flows, and I would rather add them in a follow-up than smuggle them into this one.

Not changed — I could not reproduce these two

Both comments assume ObjectMapper.readTree returns null for a whitespace 8000 -only body. It returns MissingNode. Probed against the exact Jackson the server ships (2.17.2):

input=""      -> node=  isNull=false class=MissingNode
input="   "   -> node=  isNull=false class=MissingNode
input="\n\t " -> node=  isNull=false class=MissingNode
input="not json" -> THREW JsonParseException

So SesClient.messageId takes the documented "returned no message id" path (get on a MissingNode is null), and SesErrorResponse.parse treats it as an unstructured error, which is the intended behaviour for a proxy's HTML page. SesClientTest and SesErrorResponseTest already cover the empty, whitespace and non-JSON bodies. Happy to add a null guard anyway if you would rather not depend on that behaviour.

The suite is now 244 unit tests plus the integration test; all green, and the container run against 26.0.7 and 26.7.3 was repeated after these changes.

Copilot

🟡 Changes recommended

Response parsing, temporary credential caching, endpoint handling, and documentation contain unresolved correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

services/src/main/java/org/keycloak/email/aws/SesErrorResponse.java:59

A whitespace-only error body is non-empty but readTree returns null; calling firstText then masks the SES status and request ID with a NullPointerException. Skip field extraction when no JSON node was parsed.

                JsonNode body = JsonSerialization.mapper.readTree(response.body());
                if (errorCode == null) {
                    errorCode = sanitizeErrorType(firstText(body, "__type", "code", "Code"));
                }
                message = firstText(body, "message", "Message");

Files reviewed: 46/46 changed files
Comments generated: 7
Review effort level: Balanced

+        JsonNode document;
+        try {
+            document = JsonSerialization.mapper.readTree(response.body());
+        } catch (IOException e) {
+            // The cause is dropped on purpose: Jackson quotes the offending source in its message,
+            // and that source is the credential document itself.
+            throw new AwsCredentialsException("EC2 instance metadata service returned an unreadable credential"
+                    + " document for role " + role);
+        }


lorexoff · 2026-09-03T11:58:13Z

Second pass. Five of the seven are real and are fixed; the other two repeat a premise I cannot reproduce.

Fixed

ses:SendRawEmail does not cover the v2 operation. You are right and I was wrong in the other direction: I had written that an SMTP policy "already covers this", which is not true — a policy granting only ses:SendRawEmail does not authorize SendEmail. The documentation now says the policy has to gain ses:SendEmail, and that keeping the old action alongside it is what preserves the ability to switch back to SMTP.
Release note used the single-dash selector while the admin guide used the double-dash one. Made consistent.
stripTrailingSlash removed only one slash. Fixed to strip the whole run; appendsTheOperationPathToAnEndpointHoweverItIsWritten covers the bare, single-slash and triple-slash forms.
A blank MessageId was accepted. True and separate from the null claim below: a textual "" passed the check and was returned as the message id — i.e. a send recorded as successful with nothing to trace it by. Now rejected; test added.
Temporary credentials with no known expiry were cached forever. Good catch. After the previous round the container, IMDS and STS sources all require an Expiration, so the shape can now only come from a profile file or the process environment — and the profile file is the one an operator can fix without a restart. Those credentials are no longer cached at all, so a refreshed ~/.aws/credentials takes effect on the next send. Test: doesNotCacheTemporaryCredentialsWithNoKnownExpiry.

Not changed — same premise as last round, and I still cannot reproduce it

Four comments (SesClient, SesErrorResponse, ContainerCredentialsProvider, InstanceMetadataCredentialsProvider) assume ObjectMapper.readTree returns null for an empty or whitespace-only body. It returns MissingNode. Run against the Jackson the server ships, 2.17.2, with readTree(byte[]) exactly as the code calls it:

input=""      -> node=  isNull=false class=MissingNode
input="   "   -> node=  isNull=false class=MissingNode
input="\n\t " -> node=  isNull=false class=MissingNode
input="not json" -> THREW JsonParseException

readTree has returned MissingNode rather than null for empty input since Jackson 2.10. So get/path on the result are safe, and each of those four call sites takes its documented failure path — all four are covered by existing tests that feed empty, whitespace-only and non-JSON bodies.

I have left them as they are rather than adding four null checks for a value that cannot be null, but if you would rather not have the code depend on that Jackson behaviour, say so and I will add them.

249 unit tests plus the integration test, all green; the container run against 26.0.7 and 26.7.3 was repeated after these changes.

Copilot

🟡 Changes recommended

Endpoint validation permits unsafe plaintext targets, and several empty HTTP responses cause unchecked null dereferences.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

services/src/main/java/org/keycloak/email/aws/credentials/ContainerCredentialsProvider.java:230

A successful credential endpoint response with an empty body causes readTree to return null, followed by a NullPointerException in text. Convert this malformed response into an AwsCredentialsException so the configured source fails predictably.

            body = JsonSerialization.mapper.readTree(response.body());
        } catch (IOException e) {
            throw new AwsCredentialsException("Container credential endpoint " + host
                    + " returned a body that is not JSON", e);

services/src/main/java/org/keycloak/email/aws/credentials/InstanceMetadataCredentialsProvider.java:245

An empty or whitespace-only 200 credential document is parsed as Java null, after which text(document, ...) throws NullPointerException. Reject the empty document with AwsCredentialsException instead of leaking an unchecked failure from the credential chain.

            document = JsonSerialization.mapper.readTree(response.body());
        } catch (IOException e) {
            // The cause is dropped on purpose: Jackson quotes the offending source in its message,
            // and that source is the credential document itself.
            throw new AwsCredentialsException("EC2 instance metadata service returned an unreadable credential"
                    + " document for role " + role);

Files reviewed: 46/46 changed files
Comments generated: 4
Review effort level: Balanced

+    private static final List<String> ALLOWED_LINK_LOCAL_HOSTS =
+            List.of("169.254.170.2", "169.254.170.23", "fd00:ec2::23");
+
+    private static final Pattern LOOPBACK_IPV4 = Pattern.compile("127(?:\\.\\d{1,3}){3}");


lorexoff · 2026-09-03T12:36:28Z

Third pass. One of the two new findings is real and fixed; the other turned out not to be reachable, and I have pinned that rather than leave it to chance. The four null-dereference comments are the same premise as the previous two rounds.

Fixed — the IPv6 range was too wide

AWS_EC2_METADATA_SERVICE_ENDPOINT was accepted for anything starting fd00:ec2:, i.e. the whole /32, where AWS documents a single address. Now compared against fd00:ec2::254 itself. Two tests: fd00:ec2::dead is refused with no request made, and the documented address is accepted however it is spelled.

Investigated — the invalid-octet case, and what I found

You were right that the two guards disagreed: the instance-metadata one validates octets, the container one matched 127(\.\d{1,3}){3}, which accepts 127.999.999.999. That divergence is the actual defect — the same rule written twice, once correctly — so both now go through one helper, and addresses are compared as addresses rather than as strings.

The exploit does not hold, though, and I would rather say so than claim a fix I did not make: java.net.URI refuses an authority that looks like an IPv4 address and is not one, so getHost() returns null and the endpoint is rejected one layer earlier, before the guard ever runs. I found this by writing the test you implied and watching it fail for the wrong reason. Both layers are now pinned — refusesAnAddressShapedNameThatIsNotAnAddress for the URI-level rejection, and refusesALoopbackLookalikeThatUriAccepts (127.0.0.1.example.com) for the case that does reach the guard, which is the one that matters.

Still not changed — readTree does not return null

The four remaining comments assume ObjectMapper.readTree returns null for an empty or whitespace-only body. Against Jackson 2.17.2, the version the server ships, called with readTree(byte[]) exactly as the code calls it:

input=""      -> node=  isNull=false class=MissingNode
input="   "   -> node=  isNull=false class=MissingNode
input="\n\t " -> node=  isNull=false class=MissingNode
input="not json" -> THREW JsonParseException

MissingNode, not null, since Jackson 2.10 — so get/path/asText on the result are safe and each call site takes its documented failure path. Existing tests feed empty, whitespace-only and non-JSON bodies to all four.

I am happy to add explicit null guards anyway if you would prefer the code not to rest on that behaviour; it is four lines and I have no strong feeling about it. What I did not want to do was add them while describing it as a bug fix.

254 unit tests plus the integration test, green; container matrix on 26.0.7 and 26.7.3 re-run after these changes.

Copilot

🟡 Changes recommended

Credential proxying can expose bearer tokens, while credential-chain compatibility and error redaction also need correction.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 47/47 changed files
Comments generated: 5
Review effort level: Balanced

+            httpRequest.setHeader(header.getKey(), header.getValue());
+        }
+
+        try (CloseableHttpResponse response = httpClient.execute(httpRequest)) {


lorexoff · 2026-09-03T13:06:52Z

Fourth pass — all five acted on. The proxy one was the good catch of this round and it changed the design.

Credential endpoints no longer share the server's HTTP client

You are right, and the second half of the comment is the part that matters: under a wildcard proxy the address checks in front of the container and metadata calls are not just bypassed, they are meaningless, because the host being validated stops being the host being spoken to. Documenting a no_proxy requirement was not a good enough answer.

So the two sources that answer on this machine now go through a DirectHttpTransport built with HttpClient.Builder.NO_PROXY — direct by construction, no configuration to get wrong. The split is explicit rather than implicit: AwsCredentialsProvider.requiresDirectConnection() defaults to false and is overridden by exactly those two, and the chain routes each source accordingly. The STS call of the web-identity flow deliberately keeps the shared client, because it talks to a real AWS endpoint and should inherit the server's proxy and truststore.

Still no new dependency — it is the JDK client, and the response-size bound is shared with the Apache 10BC8 one rather than written twice. DirectHttpTransportTest asserts the no-proxy property itself, plus the round trip, the bodyless PUT the IMDS token call makes, that a redirect is returned instead of followed, and the bound.

The credential chain now matches the SDK on system properties

Also correct, and it was the same omission three times: aws.profile, aws.sharedCredentialsFile, aws.configFile, aws.webIdentityTokenFile, aws.roleArn and aws.roleSessionName are read before their environment equivalents, through one helper rather than six ad-hoc reads. A server started with -Daws.profile=… would otherwise have sent email as one identity while every other AWS client in the same JVM used another. Tests cover the precedence for the profile name, the credentials-file location and the web-identity settings.

Parse failure no longer carries Jackson's cause

Right — the instance-metadata provider already dropped it for exactly this reason and the container one did not. Jackson quotes the offending source in its message, and here that source is the credential document.

264 unit tests plus the integration test, green; container matrix on 26.0.7 and 26.7.3 re-run.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated 3 comments.

lorexoff · 2026-09-03T15:13:37Z

Rebased on current main — the conflict was in 26_8_0.adoc, where the Helm chart note landed at the same insertion point; both sections are kept.

The three comments from this round were all real and are fixed.

canonicalUri read the decoded path, and the query decoder followed form-body rules

Both are genuine deviations from the signing spec, and both are the kind that only surface once someone signs a URI this provider does not build today.

The canonical URI now comes from getRawPath(), split on /, each segment percent-decoded and re-encoded. Reading getPath() turned a %2F inside a segment back into a separator, so /v2/a%2Fb and /v2/a/b signed identically. A test now asserts the two produce different signatures — an assertion that needs no golden value and would have failed before.
URLDecoder is gone. It decodes application/x-www-form-urlencoded, where + is a space; in a URI it is a literal plus, so ?a=1%2B2 was being canonicalised as a=1%202. Replaced with a percent-decoder that works on the UTF-8 bytes (so a multi-byte sequence split across triplets reassembles) and leaves + alone. Pinned against a golden signature, plus direct assertions on the decoder.

AWS's published get-vanilla vectors still pass unchanged, which is what makes the change safe to believe.

DirectHttpTransport ignored the per-request connect timeout

Correct, and it contradicted the interface's own contract rather than merely being undocumented. It now keeps one client per connect-timeout value, created on demand. In practice the map holds a single entry — both callers ask for a second — but the contract holds, and it is cheaper than building a client, and its threads, on every credential lookup.

Dead constants in InstanceMetadataCredentialsProvider

Right, and worth more than tidiness: leaving a validation pattern behind after the logic moved to LocalAddress invites the next reader to believe it is what is enforced. IPV4_OCTET, IPV4_LITERAL, the now-unused isLoopbackLiteral and their imports are gone.

267 unit tests plus the integration test, green; container matrix on 26.0.7 and 26.7.3 re-run after the rebase.

lorexoff · 2026-09-03T15:20:13Z

One follow-up on the readTree comments, which have now come back three 8 times.< 10BC8 /p>

I have added the null guards. Not because I found the failure — I could not reproduce it, and the probe below still holds — but because the code should not rest on a library behaviour, and a reader landing on this thread should not have to relitigate it.

Jackson 2.17.2, readTree(byte[]) exactly as the code calls it:
  ""       -> MissingNode
  "   "    -> MissingNode
  "\n\t "  -> MissingNode
  "not json" -> JsonParseException

So the four sites take their documented paths today, and the guards are unreachable with the parser the server ships. Each one says so in a comment rather than pretending to be a bug fix, and each lands in the same place the corresponding malformed-body path already goes — the SES message-id error, the unstructured-error branch, and the two "unreadable credential document" failures.

I have not written tests for them: a test would have to make the parser return null, which this Jackson will not do, and a test that cannot fail is worse than none. The reachable behaviour — empty, whitespace-only and non-JSON bodies — is already covered.

267 unit tests plus the integration test, green; container matrix on 26.0.7 and 26.7.3 re-run.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated 5 comments.

Suppressed comments (1)

services/src/test/java/org/keycloak/email/aws/DirectHttpTransportTest.java:1

This assertion relies on referential equality (sameInstance) for the proxy selector, which can be brittle across JDK implementations (the Optional may not return the same singleton instance even if behavior is identical). Consider asserting behaviorally (e.g., proxy is present and configured for no-proxy semantics, or that requests are not routed via a proxy) rather than depending on identity equality.

+ * header the caller set, must not follow redirects and must not retry a request whose bytes already
+ * reached the wire: the first two would break the SigV4 signature, the third would send a
+ * transactional email twice, because SES {@code SendEmail} has no idempotency token.


+    public EmailSenderProvider create(KeycloakSession session) {
+        if (config == null) {
+            throw configurationError;
+        }
+        HttpClientProvider httpClientProvider = session.getProvider(HttpClientProvider.class);
+        return new AwsSesEmailSenderProvider(config, client,
+                new KeycloakHttpTransport(httpClientProvider.getHttpClient()), clock);
+    }


+        String code = text(document, "Code");
+        if (!"Success".equals(code)) {
+            throw new AwsCredentialsException("EC2 instance metadata service reported Code=" + code
+                    + " for role " + role);
+        }
+        String accessKeyId = text(document, "AccessKeyId");
+        String secretAccessKey = text(document, "SecretAccessKey");


Copilot

🟡 Changes recommended

Credential response reads can hang indefinitely, and detailed SES errors can leak recipient addresses into server logs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 50/50 changed files
Comments generated: 2
Review effort level: Balanced

+            HttpResponse<java.io.InputStream> response =
+                    clientFor(request).send(builder.build(), HttpResponse.BodyHandlers.ofInputStream());
+            return new AwsHttpResponse(response.statusCode(), headersOf(response),
+                    HttpBodies.readBounded(response.body()));


+        if (!response.isSuccessful()) {
+            SesErrorResponse error = SesErrorResponse.parse(response);
+            logger.warnf("Amazon SES refused an email: %s", error.describeForLog());
+            throw new EmailException(error.describe());


lorexoff · 2026-09-03T18:05:18Z

Four of the five are real and fixed. One I checked and could not reproduce.

Fixed

The document == null guard sat after the first use of document. Correct, and it was mine, added in the previous round — a guard placed below text(document, "Code") protects nothing. Moved above the first use.
create() could throw null. If it ever ran before init(), both config and configurationError are null and the throw surfaces as a NullPointerException with nothing in it. It now throws a configuration error that says the sender was not initialised.
The transport contract overstated what an implementation controls. "Must not retry" is not something KeycloakHttpTransport can promise: it runs on the server's shared Apache client, which installs its own retry handler and turns it into a resend-after-send when spi-connections-http-client-default-max-retries is set, with no per-request opt-out. The javadoc now says "must not itself retry" and spells out that caveat, which is also why the factory warns at startup.
Option spellings were inconsistent between the guide and the code. Deliberate now rather than accidental, and documented: the guide shows the modern double-dash form, while the provider's own error messages use the legacy single-dash one — because that is the spelling that works on every supported server, and an operator reading a boot failure should be able to paste what they see. The guide now says so.
I also checked the environment-variable claim I had made without testing it, since operators copy that line. Against stock images, with the option set and no provider jar present, so that a refused boot proves the option was read:
```
26.7.3  KC_SPI_EMAIL_SENDER__PROVIDER  -> "Failed to find provider aws-ses"  (honoured)
26.7.3  KC_SPI_EMAIL_SENDER_PROVIDER   -> "Failed to find provider aws-ses"  (honoured)
```
The NO_PROXY assertion was identity-based. Fair — it now asserts the behaviour: the selector returns Proxy.NO_PROXY for the link-local credential URL.

Not reproduced

The AsciiDoc table is not malformed: it opens with |===, not ||===. grep -n '||===' email.adoc returns nothing.

267 unit tests plus the integration test, green; container matrix on 26.0.7 and 26.7.3 re-run.

One thing worth flagging for whoever picks this up: the last two Copilot passes both reported Copilot couldn't run its full agentic review because it didn't start before the timeout, so neither was a complete pass over the current code.

lorexoff · 2026-09-03T18:12:36Z

Both real. The first one is more serious than the comment suggests, because the remedy it proposes does not work.

The credential body read could hang — and switching body handlers does not fix it

I wrote the suggested fix first, then tested it. HttpRequest#timeout does not bound the body read for any of the JDK's handlers, not just ofInputStream. Against a handler that sends its headers and then stalls for three seconds, with timeout(500ms):

ofByteArrayConsumer -> IOException after 3046 ms
ofByteArray         -> IOException after 3008 ms
ofInputStream       -> IOException after 3008 ms

All three waited for the peer; none returned at 500 ms, and the exception came only because the stub eventually closed. A peer that simply never closes would hold the caller forever.

So the transport now sends asynchronously and waits on the future with an explicit deadline, cancelling the exchange when it expires — the calling thread is freed whatever the peer does, which is the property that matters when that thread is inside a Keycloak transaction. The byte cap stays. A regression test asserts the caller returns in well under the three seconds the stub stalls for; before the change it took the full three.

AWS's rejection text reached the server log with the recipient in it

Correct, and my earlier reasoning was wrong: I had kept the text in the exception and sanitised only the provider's own WARN, on the assumption that the message stayed in the administrator's response. It does not — UserResource calls ServicesLogger.LOGGER.failedToSendActionsEmail(e), which logs the whole exception.

Rather than dropping the text — "Email address is not verified" is exactly what makes the failure actionable — the address is now redacted out of it, so the message keeps the diagnosis and loses the identity. The error code and the AWS request id are untouched. Test asserts the address is absent and everything else survives.

269 unit tests plus the integration test, green; container matrix on 26.0.7 and 26.7.3 re-run.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated 3 comments.

+     * did not cover, for instance — AWS answers 403 with nothing to explain why.
+     */
+    private static String hostHeader(URI uri) {
+        return uri.getPort() == -1 ? uri.getHost() : uri.getHost() + ":" + uri.getPort();


+            DocumentBuilder builder = factory.newDocumentBuilder();
+            builder.setErrorHandler(STRICT_ERROR_HANDLER);
+            return builder.parse(new ByteArrayInputStream(xml));
+        } catch (ParserConfigurationException | SAXException | IOException e) {


lorexoff · 2026-09-03T18:28:11Z

One of the two holds; the other does not.

Fixed — the XML factory could throw out of the contract

Correct. setAttribute is allowed to refuse an attribute it does not recognise, and on a JAXP implementation other than the JDK's that IllegalArgumentException would have escaped unchecked from a method whose callers are promised an AwsCredentialsException. Added to the catch, with a comment saying why it is there.

No test: making it fire needs a different JAXP on the classpath, and a test that cannot fail is worse than none.

Not reproduced — the IPv6 Host header is already correct

URI#getHost() returns the bracketed form for an IPv6 literal, so the concatenation produces a valid header:

http://[::1]:8500/x         -> getHost()=[::1]              getPort()=8500   -> Host: [::1]:8500
https://[fd00:ec2::254]/x   -> getHost()=[fd00:ec2::254]    getPort()=-1     -> Host: [fd00:ec2::254]

Adding brackets around that would produce [[::1]]:8500. I have pinned the behaviour in a test rather than only saying so here, because the Host header is signed and a mismatch is a 403 with nothing to explain it.

270 unit tests plus the integration test, green; container matrix on 26.0.7 and 26.7.3 re-run.

Since it keeps coming up in the summaries: the last three passes all reported Copilot couldn't run its full agentic review because it didn't start before the timeout, so none of them was a complete review of the current code.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated 4 comments.

+        ByteArrayOutputStream body = new ByteArrayOutputStream();
+        CompletableFuture<HttpResponse<Void>> exchange = clientFor(request).sendAsync(builder.build(),
+                responseInfo -> HttpResponse.BodySubscribers.ofByteArrayConsumer(
+                        chunk -> chunk.ifPresent(bytes -> append(body, bytes))));


+        if (body.size() + chunk.length > HttpBodies.MAX_RESPONSE_BYTES) {
+            // Unchecked because the consumer cannot declare one; unwrapped by the caller.
+            throw new UncheckedIOException(
+                    new IOException("Response body exceeded " + HttpBodies.MAX_RESPONSE_BYTES + " bytes"));
+        }
+        body.write(chunk, 0, chunk.length);


+ * produces a 403 on every request.</li>
+ * <li>Only the headers passed in are signed. Whatever the HTTP client adds afterwards (Host when
+ * unset, Content-Length, User-Agent, Accept-Encoding, Connection) must stay out of the signature,
+ * so the caller sets every signed header explicitly and this class refuses the hop-by-hop ones.</li>


+        List<AwsCredentialsProvider> providers = new ArrayList<>();
+        providers.add(new SystemPropertiesCredentialsProvider(environment));
+        providers.add(new EnvironmentVariableCredentialsProvider(environment));
+        providers.add(new WebIdentityTokenCredentialsProvider(environment));
+        providers.add(new ProfileCredentialsProvider(environment));
+        providers.add(new ContainerCredentialsProvider(environment));
+        providers.add(new InstanceMetadataCredentialsProvider(environment));
+        return new AwsCredentialsProviderChain(Collections.unmodifiableList(providers));


Keycloak's only route to Amazon SES is its SMTP interface, which requires a username and password derived from a long-lived IAM user secret and stored in the realm. A server running on AWS cannot use the identity its platform already gives it, and the credential cannot be rotated by AWS. The new provider (SPI id aws-ses) signs SES v2 SendEmail requests with SigV4 and resolves credentials from the standard AWS chain. It adds no dependency: SigV4, the credential chain, JSON and MIME are implemented against HttpClientProvider, JsonSerialization and jakarta.mail. The message is composed exactly as DefaultEmailSenderProvider composes it, from the same realm settings, and sent as Content.Raw, so switching transport changes nothing a recipient can observe. It is inert unless selected with --spi-email-sender--provider=aws-ses, and order() stays 0 so it cannot win default-provider resolution by accident. The endpoint option makes the integration test hermetic: AwsSesEmailSenderTest points it at the injected HTTP server, sends a real execute-actions email through a real server, and asserts the signed request and the decoded MIME, with no AWS account and no network. SigV4 is verified against AWS's published test vectors. Closes keycloak#52360 Signed-off-by: lorexoff <rocca.lorenzo04@gmail.com>

lorexoff · 2026-09-03T18:49:20Z

Two of the four hold, both cosmetic; the thread-safety pair does not.

Fixed

The signer's javadoc said it "refuses" hop-by-hop headers when it silently excludes them from the signature. Reworded to match what the code does — a reader should not have to check.
Collections.unmodifiableList before a constructor that already does List.copyOf was redundant. Removed.

Not reproduced — the body accumulator is not racy

The premise is that ByteArrayOutputStream is not thread-safe and the subscriber callback may run on several client threads. Neither half holds:

$ javap java.io.ByteArrayOutputStream
  public synchronized void write(int);
  public synchronized void write(byte[], int, int);
  public synchronized byte[] toByteArray();
  public synchronized int size();

Every method used here is synchronized, so size() and write() cannot interleave with another thread's write. And they never need to: Flow.Subscriber requires onNext to be signalled serially, so the consumer is invoked one call at a time. The hand-off to the caller is covered too — the array is read after CompletableFuture#get, which establishes happens-before with the completion that follows the subscriber.

I would rather not add a lock that documents a race which cannot occur; if you would prefer the accumulation to be visibly single-threaded anyway, say so and I will make it explicit rather than synchronised.

270 unit tests plus the integration test, green; container matrix on 26.0.7 and 26.7.3 re-run.

ahus1 · 2026-09-03T19:08:40Z

I see that there is a lot of feedback in the PR from Copilot that makes it difficult to discuss. I'll comment on the parent issue.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

stianst · 2026-09-10T07:43:04Z

I'm afraid we can't accept this PR; we simply can't support and maintain proprietary mechanisms for sending email.

Copilot AI balanced review requested due to automatic review settings September 3, 2026 10:55

lorexoff requested a review from a team as a code owner September 3, 2026 10:55

lorexoff mentioned this pull request Sep 3, 2026

Send the response headers HttpServerUtil is given #52363

Merged

Copilot started reviewing on behalf of lorexoff September 3, 2026 11:09

View session