HPCC-32678 Add support for TLS communication to Std.System.Email.SendEmail() #19128

dcamper · 2024-09-18T21:30:49Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

Testing performed by using a local SMTP dev tool (https://github.com/rnwood/smtp4dev) and monitoring the ESMTP conversation with various configurations.

github-actions · 2024-09-18T21:31:08Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32678

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

mckellyln · 2024-09-19T12:20:04Z

common/remote/rmtsmtp.cpp

@@ -472,8 +472,10 @@ class CMailInfo
    unsigned port;
    StringAttr sender;
    Owned<ISocket> socket;
+    Owned<ISecureSocket> secureSocket;


Is this used ?

Good catch. That was part of the test change and If forgot to remove it.

mckellyln · 2024-09-19T12:25:15Z

common/remote/rmtsmtp.cpp

+            char* buffPtr = inbuff;
+            while (buffPtr < (inbuff + inlen))
+            {
+                if (strncmp(buffPtr, "250", 3) == 0)


Could the "250" occur not at the beginning of buffPtr ?
And if in this block, could buffPtr go past inbuff + inlen ?

My understanding is that there will always be at least one 250 response that serves as an acknowledgement of the EHLO. In my tests, that line was "250-Nice to meet you." but the exact wording is not dictated.

Response codes will always be at the beginning of the buffer, and 250 responses should be first if there is no error.

I modified the size of the outer while() to abort earlier to avoid some scans outside of the buffer.

mckellyln

Looks good, approved.

dcamper · 2024-09-19T18:00:13Z

Thanks, Mark! @ghalliday Squashed and ready for merging.

ghalliday

@dcamper one issue that probably should be fixed, and a couple of questions.

ghalliday · 2024-09-20T10:23:13Z

common/remote/rmtsmtp.cpp

@@ -505,6 +506,20 @@ class CMailInfo
        validator.validateValue(subject.get(), "email subject");
    }

+    void convertToTLSSocket()
+    {
+        secureSocketContext.setown(createSecureSocketContext(ClientSocket));


Not for this PR, but does this need the ability to provide a client certificate for mtls? If so this will need enhanced configuration support (from a secret).

ghalliday · 2024-09-20T10:24:24Z

common/remote/rmtsmtp.cpp

+
+#if defined(_USE_OPENSSL)
+        bool useTLS = false;
+        if (inlen > 0)


if (inlen > 12), otherwise a very short response could access invalid memory

The while condition a couple of lines down guards against an invalid access. Aborting the loop earlier is slightly more efficient, though.

ghalliday · 2024-09-20T10:26:19Z

common/remote/rmtsmtp.cpp

+        bool useTLS = false;
+        if (inlen > 0)
+        {
+            // Walk the buffer, looking for the STARTTLS option


Worth commenting that 250 responses must come first. Given the comment my expectation was that this would keep skipping to the next line through the buffer.

250 codes should be the only non-error responses at this point, but the options that are returned can be in any order. So really, this is a line-by-line scan, aborting as soon as we can process '250 STARTTLS' or '250-STARTTLS'.

ghalliday · 2024-09-20T10:29:29Z

common/remote/rmtsmtp.cpp

+    void getEhloAndOptions(StringBuffer & out)
+    {
+        // Initial EHLO and gather reply, which should contain ESMTP options
+        getEhlo(out);


Does this need to fall back to HELO if EHLO not supported? Can we ignore for any modern mail server?

ESMTP came out in 1995 (RFC 1869). All mail servers should support ESMTP by now. That said, a misconfiguration on the server side could prompt a fallback to the original protocol. I will add that.

dcamper · 2024-09-20T14:00:54Z

@ghalliday I incorporated your feedback in this latest commit. It included a slight refactoring to make the flow more sensible. Please give it another look.

dcamper · 2024-09-20T14:05:53Z

Note additional change: After reviewiing the RFC I discovered that the option name (STARTTLS) should be tested case-insensitive. I made that change to the code as well.

ghalliday

@dcamper looks good. One trivial change. Please squash.

ghalliday · 2024-09-23T13:08:34Z

common/remote/rmtsmtp.cpp

+            // Log error as a warning
+            StringBuffer msg;
+            info.addToWarnings(e->errorMessage(msg).str());
+            EXCLOG(MCoperatorError, e, "WARNING");


MCoperatorWarning?

I copied that from a catch() a little further down in the code. Do you have a better way of doing this?

…Email()

dcamper · 2024-09-23T13:21:50Z

@ghalliday Squashed. Thanks!

ghalliday · 2024-09-25T16:38:42Z

@dcamper one final thought. What is the best version to target? Could this possibly break existing code?

dcamper · 2024-09-25T17:22:41Z

Any change introduces a possibility for things to go wrong. The most likely failure here lies with an errant external mail server. The possibilities include:

Server does not support ESMTP. The various RFCs state that the server should return a 502 code after we send EHLO. This would be detected as an error and an exception thrown. In this PR's modifications, the initial EHLO is wrapped in a try/catch for this eventuality, and the code reverts to sending HELO instead (the old behavior). This is an unlikely scenario, as ESMPT was introduced 29 years ago; every mail server that is currently running should support ESMTP.
ESMTP is supported but TLS is not. In this case the server will not reply with STARTTLS. This PR's modifications make enabling TLS optional, and will upgrade the connection only if the mail server indicates support.
Misconfigured server. Server detects that client is enabling ESMTP and in the course of activating ESMTP extensions, something goes wrong. This may cause the mail server to not complete the protocol. This would occur within our try/catch and then cause a fallback to SMTP, which should work because we presumably have been communicating with this server in the past, using plain SMTP.

The possibility of a introducing an outright "failure to communicate" is very small, I think. All of that said, we can certainly retarget to 9.8 or even master (9.10) in an abundance of caution.

dcamper · 2024-09-25T17:24:21Z

Final thought: If this PR is not safe for 9.6.x then I doubt it would be safe for 9.8.x, either. So the choices will be either 9.6.x or master.

ghalliday · 2024-09-26T11:36:22Z

Merged to 9.8.x for the moment because most production systems are not yet upgraded to that version. So I agree with your assesment of risk, a problem will not have the same potential impact.

dcamper requested a review from mckellyln September 18, 2024 21:30

mckellyln reviewed Sep 19, 2024

View reviewed changes

dcamper requested a review from mckellyln September 19, 2024 13:49

mckellyln approved these changes Sep 19, 2024

View reviewed changes

dcamper force-pushed the hpcc-32678-smtp-tls branch from da4f77d to 8e45a02 Compare September 19, 2024 17:58

ghalliday reviewed Sep 20, 2024

View reviewed changes

dcamper requested a review from ghalliday September 20, 2024 14:00

ghalliday approved these changes Sep 23, 2024

View reviewed changes

HPCC-32678 Add support for TLS communication to Std.System.Email.Send…

2c48403

…Email()

dcamper force-pushed the hpcc-32678-smtp-tls branch from a720b2d to 2c48403 Compare September 23, 2024 13:21

ghalliday changed the base branch from candidate-9.6.x to candidate-9.8.x September 26, 2024 11:34

ghalliday merged commit 2484ceb into hpcc-systems:candidate-9.8.x Sep 26, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32678 Add support for TLS communication to Std.System.Email.SendEmail() #19128

HPCC-32678 Add support for TLS communication to Std.System.Email.SendEmail() #19128

dcamper commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

mckellyln Sep 19, 2024

dcamper Sep 19, 2024

mckellyln Sep 19, 2024

dcamper Sep 19, 2024

dcamper Sep 19, 2024

mckellyln left a comment

dcamper commented Sep 19, 2024

ghalliday left a comment

ghalliday Sep 20, 2024

ghalliday Sep 20, 2024

dcamper Sep 20, 2024 •

edited

Loading

ghalliday Sep 20, 2024

dcamper Sep 20, 2024

ghalliday Sep 20, 2024

dcamper Sep 20, 2024

dcamper commented Sep 20, 2024

dcamper commented Sep 20, 2024

ghalliday left a comment

ghalliday Sep 23, 2024

dcamper Sep 23, 2024 •

edited

Loading

dcamper commented Sep 23, 2024

ghalliday commented Sep 25, 2024

dcamper commented Sep 25, 2024

dcamper commented Sep 25, 2024

ghalliday commented Sep 26, 2024

HPCC-32678 Add support for TLS communication to Std.System.Email.SendEmail() #19128

HPCC-32678 Add support for TLS communication to Std.System.Email.SendEmail() #19128

Conversation

dcamper commented Sep 18, 2024

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Sep 18, 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

mckellyln left a comment

Choose a reason for hiding this comment

dcamper commented Sep 19, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcamper Sep 20, 2024 • edited Loading

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

dcamper commented Sep 20, 2024

dcamper commented Sep 20, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcamper Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

dcamper commented Sep 23, 2024

ghalliday commented Sep 25, 2024

dcamper commented Sep 25, 2024

dcamper commented Sep 25, 2024

ghalliday commented Sep 26, 2024

dcamper Sep 20, 2024 •

edited

Loading

dcamper Sep 23, 2024 •

edited

Loading