JavaMail compatibility broken with implicit STARTTLS
dheinecke opened this issue · 2 comments
Hi!
The change to hide AUTH support until after STARTTLS is negotiated breaks JavaMail clients that require auth and use (implicit) STARTTLS (not implicit TLS, which is a different thing!) I realize that this is probably a very small demographic, but I am in this camp because I cannot make changes to our client (legacy application), and I do require STARTTLS support.
This simple demo illustrates the problem:
Server
public class Server {
public static final String host = "localhost";
public static final int port = 587;
public static final String userName = "ga";
public static final String password = "ga";
public static void main(String[] args) throws Exception {
System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "debug");
System.setProperty("javax.net.ssl.keyStore", "C:/Java/jre/lib/security/server.jks");
System.setProperty("javax.net.ssl.keyStorePassword", "changeit");
final SSLContext sslContext = SSLContext.getInstance("TLSv1.2");
final KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
final KeyStore ks = KeyStore.getInstance("JKS");
ks.load(new FileInputStream("C:/Java/JRE/lib/security/server.jks"), "changeit".toCharArray());
kmf.init(ks, "changeit".toCharArray());
sslContext.init(kmf.getKeyManagers(), null, new SecureRandom());
Wiser.create(
new SMTPServer.Builder()
.enableTLS(true)
.requireAuth(true)
.requireTLS(true)
.port(port)
.hostName(host)
.authenticationHandlerFactory(
new EasyAuthenticationHandlerFactory(
(String usr, String pwd, MessageContext mc) -> {
if(!userName.equalsIgnoreCase(usr) || !password.equals(pwd))
throw new LoginFailedException("bad credentials");
}
)
)
).start();
}
}
Client
public class Client {
public static void main(String[] args) throws Exception {
System.setProperty("javax.net.ssl.keyStore", "C:/Java/jre/lib/security/server.jks");
System.setProperty("javax.net.ssl.keyStorePassword", "changeit");
System.setProperty("javax.net.ssl.trustStore", "C:/Java/jre/lib/security/cacerts.jks");
System.setProperty("javax.net.ssl.trustStorePassword", "changeit");
Properties props = new Properties();
// props.put("mail.smtp.starttls.enable", "true"); // uses starttls implicitly
props.put("mail.smtp.host", Server.host);
props.put("mail.smtp.port", Server.port);
Session session = Session.getInstance(props,
new javax.mail.Authenticator() {
protected PasswordAuthentication
getPasswordAuthentication() {
return new PasswordAuthentication(Server.userName, Server.password);
}
});
Message message = new MimeMessage(session);
message.setFrom(new InternetAddress("from@server.com"));
message.setRecipients(Message.RecipientType.TO, InternetAddress.parse("to@server.com"));
message.setSubject("hello");
message.setText("Some content");
Transport.send(message, Server.userName, Server.password);
}
}
Server Output
SMTP server *:587 starting
SMTP server *:587 started
SMTP connection from 127.0.0.1/127.0.0.1, new connection count: 1
Server: 220 localhost ESMTP SubEthaSMTP null
Client: EHLO ...
Server: 250-localhost
250-8BITMIME
250-STARTTLS
250 Ok
Client: MAIL FROM:from@server.com
Server: 530 5.7.0 Authentication required
Client: RSET
Server: 530 Must issue a STARTTLS command first
Client: QUIT
Server: 221 Bye
Reverting change 9f9277e and re-running gives the following output:
SMTP server *:587 starting
SMTP server *:587 started
SMTP connection from 127.0.0.1/127.0.0.1, new connection count: 1
Server: 220 localhost ESMTP SubEthaSMTP null
Client: EHLO ...
Server: 250-localhost
250-8BITMIME
250-STARTTLS
250-AUTH PLAIN LOGIN
250 Ok
Client: AUTH LOGIN
Server: 530 Must issue a STARTTLS command first
Client: STARTTLS
Server: 220 Ready to start TLS
Cipher suite: TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
Client: AUTH LOGIN
Server: 334 VXNlcm5hbWU6
Server: 334 UGFzc3dvcmQ6
Server: 235 Authentication successful.
Client: MAIL FROM:from@server.com
Server: 250 Ok
Client: RCPT TO:to@server.com
Accepting mail from from@server.com to to@server.com
Server: 250 Ok
Client: DATA
Server: 354 End data with .
Delivering mail from from@server.com to to@server.com
Creating message from data with 462 bytes
Server: 250 Ok
Client: QUIT
Server: 221 Bye
On the face of it, the change seems like a good one since you don't want credentials being sent unencrypted on the wire if starttls is supported, but it appears that javamail has code in-place to mitigate this issue, though other client libraries might not have such inherent defensive measures. For the curious, the code that is responsible for the un-enabled starttls handling is in:
(v1.6.4)
com.sun.mail.smtp.SMTPTransport.Authenticator#authenticate()
{
...
if (this.resp == 530) {
**SMTPTransport.this.startTLS();**
if (ir != null) {
this.resp = SMTPTransport.this.simpleCommand("AUTH " + this.mech + " " + ir);
} else {
this.resp = SMTPTransport.this.simpleCommand("AUTH " + this.mech);
}
}
...
}
So during authentication, if the server responds with a 530, javamail then tries a STARTTLS regardless of whether the client has the option enabled. Ofc, the only way to get into the authenticate code in the first place is if the server publishes AUTH or AUTH=, which the change specifically prevents if we haven't already negotiated STARTTLS!
Sorry, I missed this. I'll look into it.
On the face of it, the change seems like a good one since you don't want credentials being sent unencrypted on the wire if starttls is supported, but it appears that javamail has code in-place to mitigate this issue, though other client libraries might not have such inherent defensive measures. For the curious, the code that is responsible for the un-enabled starttls handling is in:
(v1.6.4)
In the discussion of the referred to change #1 I say
So from what I see in the spec I think if requireTls() has been set then AUTH related capabilities should not be advertised in the pre-STARTTLS EHLO. Shall we make a change to support that statement?
That wasn't right, it is rather that the server may hide AUTH related capabilities pre STARTTLS.
So you suggest that we make the hiding of AUTH capabilities before STARTTLS optional? We can do that.
I could add .showAuthCapabilitiesBeforeSTARTTLS(boolean) and you would call with true, default would be false to prevent credentials going in the clear as you mentioned.