Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 26, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 99 additions & 4 deletions common/remote/rmtsmtp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

#include "jlib.hpp"
#include "jlog.hpp"
#include "jsocket.hpp"
#include "securesocket.hpp"
#include "jbuff.hpp"

#include "rmtsmtp.hpp"
Expand Down Expand Up @@ -472,8 +472,9 @@ class CMailInfo
unsigned port;
StringAttr sender;
Owned<ISocket> socket;
Owned<ISecureSocketContext> secureSocketContext;
StringBuffer lastAction;
char inbuff[200];
char inbuff[2048];
unsigned inlen;
bool highPriority;
bool termJobOnFail;
Expand Down Expand Up @@ -505,6 +506,20 @@ class CMailInfo
validator.validateValue(subject.get(), "email subject");
}

void convertToTLSSocket()
{
secureSocketContext.setown(createSecureSocketContext(ClientSocket));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Owned<ISecureSocket> ssock = secureSocketContext->createSecureSocket(socket.getClear());
int status = ssock->secure_connect(SSLogNone);
if (status < 0)
{
ssock->close();
VStringBuffer errmsg("Secure connect failed: %d", status);
THROWJSOCKEXCEPTION_MSG(JSOCKERR_connection_failed, errmsg);
}
socket.setown(ssock.getClear());
}

void open()
{
SocketEndpoint address(mailServer.get());
Expand Down Expand Up @@ -633,6 +648,70 @@ class CMailInfo
out.append("HELO ").append(mailServer.get()).append("\r\n");
}

void getEhlo(StringBuffer & out) const
{
out.append("EHLO ").append(mailServer.get()).append("\r\n");
}

void processEsmtpOptions(StringBuffer & out)
{
#if defined(_USE_OPENSSL)
bool useTLS = false;
if (inlen > 12) // 12 == '250 STARTTLS'
{
// Walk the buffer, looking for the STARTTLS option
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
buffPtr += 3;
bool hasMoreOptions = (*buffPtr == '-');
++buffPtr;
if (strnicmp(buffPtr, "STARTTLS", 8) == 0) // case-insensitive
{
useTLS = true;
break;
}
if (hasMoreOptions)
{
// Skip to the character past the end of the line
while (buffPtr < (inbuff + inlen))
{
buffPtr++;
if (*buffPtr == '\n')
{
buffPtr++;
break;
}
}
}
else
{
break;
}
}
else
{
break;
}
}
}

if (useTLS)
{
// Tell the server we're starting TLS
out.set("STARTTLS\r\n");
writeAndAck(out.str(), out.length());
// Upgrade existing connection to TLS
convertToTLSSocket();
// Start the EHLO conversation again
getEhlo(out.clear());
writeAndAck(out.str(), out.length());
}
#endif
}

void getMailFrom(StringBuffer & out) const
{
out.append("MAIL FROM:<").append(sender.get()).append(">\r\n");
Expand Down Expand Up @@ -824,9 +903,25 @@ static void doSendEmail(CMailInfo & info, CMailPart const & part)
StringBuffer outbuff;

info.read(0);
info.getHelo(outbuff);

info.writeAndAck(outbuff.str(), outbuff.length());
// Try to use ESMTP and fall back to SMTP if necessary
try
{
info.getEhlo(outbuff.clear());
info.writeAndAck(outbuff.str(), outbuff.length());
info.processEsmtpOptions(outbuff);
}
catch(IException * e)
{
// Log error as a warning
StringBuffer msg;
info.addToWarnings(e->errorMessage(msg).str());
EXCLOG(MCoperatorError, e, "WARNING");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCoperatorWarning?

Copy link
Contributor Author

@dcamper dcamper Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

e->Release();
// Fall back to SMTP
info.getHelo(outbuff.clear());
info.writeAndAck(outbuff.str(), outbuff.length());
}

info.getMailFrom(outbuff.clear());

Expand Down
Loading