JAMES-3823 SMTP Require TLS Option #2460

Maxxx873 · 2024-10-20T18:49:54Z

Hi! The first step in the direction of SMTP Require TLS Option is a basic set of hooks. In the future I plan to enhance this PR by processing of new REQUIRETLS attribute, handling headers, remote delivery, etc.

src/site/resources/server/rfclist/smtp/rfc8689.txt

chibenwa

Hello,

Thanks a LOT for contributions on the topic!

A first remark is that this pull request only tackles incoming SMTP while it necessarily have impact on outgoing SMTP settings: we need to have RemoteDelivery setting up the REQUIRETLS mail parameter if needed... (section 4). I know this will eventually get takkled before we could claim support for REQUIRETLS.

We also need to tackle section 5: preserving those parameters for Bounce and DSNBounce mailets.

Maxxx873 · 2024-12-04T11:10:39Z

Hi! I'm still working. In this step, I added the requireStartTls() method for SmtpConfiguration in integration testing and implemented RemoteDelivery.

chibenwa

IMO we need a way to test this:

GIVEN From REQUIRETLS
WHEN remoteDelivery speaks to a server that do NOT allow startTLS
THEN it fails

Don't we have a way to tweak MockSMTP into allowing testing this.

And yes and VERY good work, by the way.

chibenwa · 2024-12-06T13:11:30Z

...ets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerToHost.java

@@ -123,17 +130,23 @@ public ExecutionResult tryDeliveryToHost(Mail mail, Collection<InternetAddress>
        try {
            transport = (SMTPTransport) session.getTransport(outgoingMailServer);
            transport.setLocalHost(props.getProperty(inContext(session, "mail.smtp.localhost"), configuration.getHeloNameProvider().getHeloName()));
+            if (isTlsRequired(mail)) {
+                transport.setStartTLS(true);


2 remarks:

If the transport layer is already configured with SSL there's no point setting SSL. That's the case EG relaying to a secure mail gateway other the internet.

If the receiver server do not support startTLS this need to throw.

Maxxx873 · 2024-12-10T12:56:39Z

IMO we need a way to test this:

GIVEN From REQUIRETLS WHEN remoteDelivery speaks to a server that do NOT allow startTLS THEN it fails

Don't we have a way to tweak MockSMTP into allowing testing this.

And yes and VERY good work, by the way.

Thanks so much for the review and remarks! Unfortunately, I don't know how to implement it correctly yet. I tried setting the MockSMTP behavior, but that's not what's needed as far as I understand. It is possible to make a tweak to MockSMTP to respond not only to commands, but also to parameters passed. Is it necessary? Further, it is not very clear how and at what point I can check the startTLS allowance for remoteDelivery. Also a question regarding Bounce and DSNBounce maillets. The REQUIRETLS email attribute will be saved as it is, what do I need to add additionally? I apologize for the large scope of questions.

chibenwa · 2024-12-10T13:16:43Z

...ery-integration-testing/src/test/java/org/apache/james/smtp/tls/SmtpRequireTlsRelayTest.java

+    void remoteDeliveryShouldFailsWhenServerNotAllowStartTls(DockerMockSmtp mockSmtp) throws Exception {
+        testIMAPClient.connect(LOCALHOST_IP, jamesServer.getProbe(ImapGuiceProbe.class).getImapPort())
+                .login(FROM, PASSWORD);
+
+        MockSMTPBehavior startTlsBehavior = new MockSMTPBehavior(
+                MAIL_FROM,
+                Condition.MATCH_ALL,
+                new Response(COMMAND_PARAMETER_NOT_IMPLEMENTED_504, "The server has not implemented a command parameter"),
+                MockSMTPBehavior.NumberOfAnswersPolicy.anytime());
+
+        mockSmtp.getConfigurationClient().setBehaviors(List.of(startTlsBehavior));
+
+        SMTPSClient smtpClient = initSMTPSClient();
+        smtpClient.mail("<" + FROM + "> REQUIRETLS");
+        smtpClient.rcpt("<" + RECIPIENT + ">");
+        smtpClient.sendShortMessageData("A short message...");
+
+        String dsnMessage = testIMAPClient.connect(LOCALHOST_IP, jamesServer.getProbe(ImapGuiceProbe.class).getImapPort())
+                .login(FROM, PASSWORD)
+                .select(TestIMAPClient.INBOX)
+                .awaitMessageCount(awaitAtMostOneMinute, 1)
+                .readFirstMessage();
+
+        Assertions.assertThat(dsnMessage).contains("504 The server has not implemented a command parameter");
+    }


No what we want is the mock server to HIDE that it implements STARTTLS (in the EHLO capability).

When that's the case the relay of the mail should fail.

This is different from failing at MAIL FROM

(What I mean is that the test is inaccurate).

No what we want is the mock server to HIDE that it implements STARTTLS (in the EHLO capability).

When that's the case the relay of the mail should fail.

This is different from failing at MAIL FROM

(What I mean is that the test is inaccurate).

I agree with you, and I propose a new version of the test

chibenwa · 2024-12-11T16:08:30Z

...ets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerToHost.java

@@ -131,6 +133,9 @@ public ExecutionResult tryDeliveryToHost(Mail mail, Collection<InternetAddress>
            transport = (SMTPTransport) session.getTransport(outgoingMailServer);
            transport.setLocalHost(props.getProperty(inContext(session, "mail.smtp.localhost"), configuration.getHeloNameProvider().getHeloName()));
            connect(outgoingMailServer, transport);
+            if (!transport.getLastServerResponse().contains("STARTTLS")) {


We should do this only if TLS is required

Wasn;t reviwing latest diff, sorry

chibenwa · 2024-12-11T16:15:54Z

...otocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/tls/SmtpTlsMessageHook.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SmtpTlsMessageHook implements JamesMessageHook {


Suggested change

public class SmtpTlsMessageHook implements JamesMessageHook {

public class SmtpRequireTlsMessageHook implements JamesMessageHook {

chibenwa · 2024-12-11T16:16:11Z

.../protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/tls/SmtpTlsEhloHook.java

+import org.apache.james.protocols.smtp.hook.HeloHook;
+import org.apache.james.protocols.smtp.hook.HookResult;
+
+public class SmtpTlsEhloHook implements HeloHook {


Suggested change

public class SmtpTlsEhloHook implements HeloHook {

public class SmtpRequireTlsEhloHook implements HeloHook {

chibenwa · 2024-12-11T16:16:32Z

...ocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/tls/SmtpTlsParameterHook.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SmtpTlsParameterHook implements MailParametersHook {


Suggested change

public class SmtpTlsParameterHook implements MailParametersHook {

public class SmtpRequireTlsParameterHook implements MailParametersHook {

chibenwa

Very good work.

I endorse this, and just find comments onto the naming of the hooks.

Maxxx873 · 2024-12-12T06:24:28Z

Very good work.

I endorse this, and just find comments onto the naming of the hooks.

Thank you! If there are any additional suggestions for improving this contribution, I am willing to follow up. It would be interesting if this could be tested on a test bench somehow. Thanks again for the code review and feedback on the work

chibenwa · 2024-12-20T21:32:07Z

Test failures seems related.

Could you have a look?

Maxxx873 · 2024-12-21T00:16:08Z

Test failures seems related.

Could you have a look?

I assume firstly that in order for CI to work properly when running tests with MAIL FROM parameters with no value, we need to update the version of the mock smtp server in the docker hub. See:
#2489

chibenwa · 2024-12-21T22:41:58Z

Will do tomorrow!

(sorry)

chibenwa · 2024-12-23T04:22:47Z

linagora/mock-smtp-server:0.7 is pushed, as promissed.

I did also re-trigger a build.

Maxxx873 · 2024-12-23T21:14:17Z

linagora/mock-smtp-server:0.7 is pushed, as promissed.

I did also re-trigger a build.

Thanks for updating the mock smtp server. I see that some of the related tests failed, sorry, I will need more time to find the reason.

Arsnael

The error looks unrelated on the CI so I restarted the build!

chibenwa reviewed Oct 21, 2024

View reviewed changes

src/site/resources/server/rfclist/smtp/rfc8689.txt Outdated Show resolved Hide resolved

chibenwa reviewed Oct 21, 2024

View reviewed changes

Maxxx873 mentioned this pull request Nov 5, 2024

[IMPROVEMENT] mock-smtp-server (parameters without values) #2489

Merged

Maxxx873 added 5 commits December 4, 2024 14:00

JAMES-3823 SMTP Require TLS Option

05a304c

fixup! JAMES-3823 SMTP Require TLS Option

831ce43

fixup! JAMES-3823 SMTP Require TLS Option

1db881c

fixup! JAMES-3823 SMTP Require TLS Option

c1e388f

fixup! JAMES-3823 SMTP Require TLS Option

2f8c0b7

Maxxx873 force-pushed the JAMES-3823 branch from 30ff634 to 2f8c0b7 Compare December 4, 2024 11:05

chibenwa reviewed Dec 6, 2024

View reviewed changes

fixup! JAMES-3823 SMTP Require TLS Option

52f0880

chibenwa reviewed Dec 10, 2024

View reviewed changes

Maxxx873 added 4 commits December 11, 2024 11:01

fixup! JAMES-3823 SMTP Require TLS Option

542bd66

fixup! JAMES-3823 SMTP Require TLS Option

64db1a8

fixup! JAMES-3823 SMTP Require TLS Option

2a07400

fixup! JAMES-3823 SMTP Require TLS Option

af59cc4

chibenwa reviewed Dec 11, 2024

View reviewed changes

fixup! JAMES-3823 SMTP Require TLS Option

cca388f

fixup! fixup! JAMES-3823 SMTP Require TLS Option

9ed5ad7

chibenwa approved these changes Dec 12, 2024

View reviewed changes

fixup! JAMES-3823 SMTP Require TLS Option

8fbf00d

fixup! JAMES-3823 SMTP Require TLS Option

e5915c1

Arsnael approved these changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JAMES-3823 SMTP Require TLS Option #2460

JAMES-3823 SMTP Require TLS Option #2460

Maxxx873 commented Oct 20, 2024

chibenwa left a comment •

edited

Loading

Maxxx873 commented Dec 4, 2024

chibenwa left a comment

chibenwa Dec 6, 2024

Maxxx873 commented Dec 10, 2024

chibenwa Dec 10, 2024

Maxxx873 Dec 11, 2024

chibenwa Dec 11, 2024

chibenwa Dec 11, 2024

chibenwa Dec 11, 2024

chibenwa Dec 11, 2024

chibenwa Dec 11, 2024

chibenwa left a comment

Maxxx873 commented Dec 12, 2024

chibenwa commented Dec 20, 2024

Maxxx873 commented Dec 21, 2024

chibenwa commented Dec 21, 2024

chibenwa commented Dec 23, 2024

Maxxx873 commented Dec 23, 2024

Arsnael left a comment

	public class SmtpTlsMessageHook implements JamesMessageHook {
	public class SmtpRequireTlsMessageHook implements JamesMessageHook {

	public class SmtpTlsEhloHook implements HeloHook {
	public class SmtpRequireTlsEhloHook implements HeloHook {

	public class SmtpTlsParameterHook implements MailParametersHook {
	public class SmtpRequireTlsParameterHook implements MailParametersHook {

JAMES-3823 SMTP Require TLS Option #2460

Are you sure you want to change the base?

JAMES-3823 SMTP Require TLS Option #2460

Conversation

Maxxx873 commented Oct 20, 2024

chibenwa left a comment • edited Loading

Choose a reason for hiding this comment

Maxxx873 commented Dec 4, 2024

chibenwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maxxx873 commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa left a comment

Choose a reason for hiding this comment

Maxxx873 commented Dec 12, 2024

chibenwa commented Dec 20, 2024

Maxxx873 commented Dec 21, 2024

chibenwa commented Dec 21, 2024

chibenwa commented Dec 23, 2024

Maxxx873 commented Dec 23, 2024

Arsnael left a comment

Choose a reason for hiding this comment

chibenwa left a comment •

edited

Loading