From cfe2971c2e179305140427680c9d34c3e74479b0 Mon Sep 17 00:00:00 2001 From: Christine Spang Date: Wed, 5 Apr 2017 17:34:13 -0700 Subject: [PATCH] [*] Revamp SSL options (including user-facing) Summary: Previously, the generic IMAP auth screen presented one security option to users: "Require SSL". This was ambiguous and difficult to translate into the correct security options behind the scenes, causing confusion and problems connecting some accounts. This patch does the following: * Separates security settings for IMAP and SMTP, as these different protocols may also require different SSL/TLS settings * Reworks the generic IMAP auth page to allow specifying security settings with higher fidelity. We looked at various different email apps and decided that the best solution to this problem was to allow more detailed specification of security settings and to ease the burden of more options by having sane defaults that work correctly in the majority of cases. This new screen allows users to pick from "SSL / TLS", "STARTTLS", or "none" for the security settings for a protocol, and also to instruct us that they're OK with us using known insecure SSL settings to connect to their server by checking a checkbox. We default to port 993 / SSL/TLS for IMAP and port 587 / STARTTLS for SMTP. These are the most common settings for providers these days and will work for most folks. * Significantly tightens our default security. Now that we can allow folks to opt-in to bad security, by default we should protect folks as best we can. * Removes some now-unnecessary jank like specifying the SSLv3 "cipher" in some custom SMTP configs. I don't think this was actually necessary as SSLv3 is a protocol and not a valid cipher, but these custom configs may have been necessary because of how the ssl_required flag was linked between IMAP and SMTP before (and thus to specify different settings for SMTP you'd have to override the SMTP config). * Removes hard-coding of Gmail & Office365 settings in several locations. (This was a major headache while working on the patch.) This depends on version 2.0.1 of imap-provider-settings, which has major breaking changes from version 1.0. See commit for more info: https://github.com/nylas/imap-provider-settings/commit/9851054f9153476b6896d04893a40b4febc8986e Among other things, I did a serious audit of the settings in this file and "upgraded" a few servers which weren't using the SSL-enabled ports for their provider to the secure ones. Hurray for nmap and openssl. Test Plan: manual Reviewers: evan, mark, juan, halla Reviewed By: juan, halla Differential Revision: https://phab.nylas.com/D4316 --- .../lib/decorators/create-page-for-form.jsx | 13 +- .../onboarding/lib/onboarding-helpers.es6 | 22 ++-- .../lib/page-account-settings-imap.jsx | 96 ++++++++++++-- packages/isomorphic-core/index.js | 1 + packages/isomorphic-core/package.json | 2 +- packages/isomorphic-core/src/auth-helpers.es6 | 120 ++++++++++++------ .../isomorphic-core/src/imap-connection.es6 | 37 ++++-- packages/isomorphic-core/src/smtp-errors.es6 | 10 +- packages/isomorphic-core/src/tls-utils.es6 | 15 +++ 9 files changed, 237 insertions(+), 79 deletions(-) create mode 100644 packages/isomorphic-core/src/tls-utils.es6 diff --git a/packages/client-app/internal_packages/onboarding/lib/decorators/create-page-for-form.jsx b/packages/client-app/internal_packages/onboarding/lib/decorators/create-page-for-form.jsx index b2312abed..0df8ff0a4 100644 --- a/packages/client-app/internal_packages/onboarding/lib/decorators/create-page-for-form.jsx +++ b/packages/client-app/internal_packages/onboarding/lib/decorators/create-page-for-form.jsx @@ -122,13 +122,18 @@ const CreatePageForForm = (FormComponent) => { errorFieldNames.push('username'); } if (err.statusCode === 401) { + if (/smtp/i.test(err.message)) { + errorFieldNames.push('smtp_username'); + errorFieldNames.push('smtp_password'); + } + if (/imap/i.test(err.message)) { + errorFieldNames.push('imap_username'); + errorFieldNames.push('imap_password'); + } + // not sure what these are for -- backcompat? errorFieldNames.push('password') errorFieldNames.push('email'); errorFieldNames.push('username'); - errorFieldNames.push('imap_username'); - errorFieldNames.push('smtp_username'); - errorFieldNames.push('imap_password'); - errorFieldNames.push('smtp_password'); } if (NylasAPI.TimeoutErrorCodes.includes(err.statusCode)) { // timeout errorMessage = "We were unable to reach your mail provider. Please try again." diff --git a/packages/client-app/internal_packages/onboarding/lib/onboarding-helpers.es6 b/packages/client-app/internal_packages/onboarding/lib/onboarding-helpers.es6 index e9943b9c8..55c848e80 100644 --- a/packages/client-app/internal_packages/onboarding/lib/onboarding-helpers.es6 +++ b/packages/client-app/internal_packages/onboarding/lib/onboarding-helpers.es6 @@ -14,12 +14,14 @@ const IMAP_FIELDS = new Set([ "imap_port", "imap_username", "imap_password", + "imap_security", + "imap_allow_insecure_ssl", "smtp_host", "smtp_port", "smtp_username", "smtp_password", - "smtp_custom_config", - "ssl_required", + "smtp_security", + "smtp_allow_insecure_ssl", ]); function base64url(inBuffer) { @@ -133,7 +135,7 @@ export function runAuthRequest(accountInfo) { options: { path: '/auth', method: 'POST', - timeout: 1000 * 90, // Connecting to IMAP could take up to 90 seconds, so we don't want to hang up too soon + timeout: 1000 * 180, // Same timeout as server timeout (most requests are faster than 90s, but server validation can be slow in some cases) body: data, auth: noauth, }, @@ -144,7 +146,7 @@ export function runAuthRequest(accountInfo) { options: { path: `/auth`, method: 'POST', - timeout: 1000 * 90, // Connecting to IMAP could take up to 90 seconds, so we don't want to hang up too soon + timeout: 1000 * 180, // Same timeout as server timeout (most requests are faster than 90s, but server validation can be slow in some cases) body: data, auth: noauth, }, @@ -165,7 +167,10 @@ export function isValidHost(value) { export function accountInfoWithIMAPAutocompletions(existingAccountInfo) { const {email, type} = existingAccountInfo; const domain = email.split('@').pop().toLowerCase(); - const template = CommonProviderSettings[domain] || CommonProviderSettings[type] || {}; + let template = CommonProviderSettings[domain] || CommonProviderSettings[type] || {}; + if (template.alias) { + template = CommonProviderSettings[template.alias]; + } const usernameWithFormat = (format) => { if (format === 'email') { @@ -177,18 +182,19 @@ export function accountInfoWithIMAPAutocompletions(existingAccountInfo) { return undefined; } - // always pre-fill SMTP / IMAP username, password and port. const defaults = { imap_host: template.imap_host, imap_port: template.imap_port || 993, imap_username: usernameWithFormat(template.imap_user_format), imap_password: existingAccountInfo.password, + imap_security: template.imap_security || "SSL / TLS", + imap_allow_insecure_ssl: template.imap_allow_insecure_ssl || false, smtp_host: template.smtp_host, smtp_port: template.smtp_port || 587, smtp_username: usernameWithFormat(template.smtp_user_format), smtp_password: existingAccountInfo.password, - ssl_required: (template.ssl === '1'), - smtp_custom_config: template.smtp_custom_config, + smtp_security: template.smtp_security || "STARTTLS", + smtp_allow_insecure_ssl: template.smtp_allow_insecure_ssl || false, } return Object.assign({}, existingAccountInfo, defaults); diff --git a/packages/client-app/internal_packages/onboarding/lib/page-account-settings-imap.jsx b/packages/client-app/internal_packages/onboarding/lib/page-account-settings-imap.jsx index e1d39b305..e330131d8 100644 --- a/packages/client-app/internal_packages/onboarding/lib/page-account-settings-imap.jsx +++ b/packages/client-app/internal_packages/onboarding/lib/page-account-settings-imap.jsx @@ -56,24 +56,94 @@ class AccountIMAPSettingsForm extends React.Component { this.props.onConnect(); } - renderFieldsForType(type) { - const {accountInfo, errorFieldNames, submitting, onFieldKeyPress, onFieldChange} = this.props; + renderPortDropdown(protocol) { + if (!["imap", "smtp"].includes(protocol)) { + throw new Error(`Can't render port dropdown for protocol '${protocol}'`); + } + const {accountInfo, submitting, onFieldKeyPress, onFieldChange} = this.props; + if (protocol === "imap") { + return ( + + + + + ) + } + if (protocol === "smtp") { + return ( + + + + + ) + } + return ""; + } + + renderSecurityDropdown(protocol) { + const {accountInfo, submitting, onFieldKeyPress, onFieldChange} = this.props; + + return ( +
+ + + + + + + + +
+ ) + } + + renderFieldsForType(type) { return (
- - - + {this.renderPortDropdown(type)} + {this.renderSecurityDropdown(type)}
diff --git a/packages/isomorphic-core/index.js b/packages/isomorphic-core/index.js index e6c97edfd..7dfd3d108 100644 --- a/packages/isomorphic-core/index.js +++ b/packages/isomorphic-core/index.js @@ -26,4 +26,5 @@ module.exports = { SendUtils: require('./src/send-utils'), executeJasmine: require('./spec/jasmine/execute').default, StringUtils: require('./src/string-utils'), + TLSUtils: require('./src/tls-utils'), } diff --git a/packages/isomorphic-core/package.json b/packages/isomorphic-core/package.json index e6eb773dc..431521832 100644 --- a/packages/isomorphic-core/package.json +++ b/packages/isomorphic-core/package.json @@ -10,7 +10,7 @@ "atob": "2.0.3", "btoa": "1.1.2", "imap": "github:jstejada/node-imap#fix-parse-body-list", - "imap-provider-settings": "github:nylas/imap-provider-settings#054303cc2", + "imap-provider-settings": "github:nylas/imap-provider-settings#2fdcd34d59b", "jasmine": "2.x.x", "joi": "8.4.2", "libhoney": "1.0.0-beta.2", diff --git a/packages/isomorphic-core/src/auth-helpers.es6 b/packages/isomorphic-core/src/auth-helpers.es6 index 0518bf03e..906825111 100644 --- a/packages/isomorphic-core/src/auth-helpers.es6 +++ b/packages/isomorphic-core/src/auth-helpers.es6 @@ -1,11 +1,16 @@ +/* eslint camelcase: 0 */ import _ from 'underscore' import Joi from 'joi' import atob from 'atob'; import nodemailer from 'nodemailer'; +import {CommonProviderSettings} from 'imap-provider-settings'; +import {INSECURE_TLS_OPTIONS, SECURE_TLS_OPTIONS} from './tls-utils'; import IMAPConnection from './imap-connection' import {NylasError, RetryableError} from './errors' import {convertSmtpError} from './smtp-errors' +const {GMAIL_CLIENT_ID, GMAIL_CLIENT_SECRET} = process.env; + const imapSmtpSettings = Joi.object().keys({ imap_host: [Joi.string().ip().required(), Joi.string().hostname().required()], imap_port: Joi.number().integer().required(), @@ -15,8 +20,14 @@ const imapSmtpSettings = Joi.object().keys({ smtp_port: Joi.number().integer().required(), smtp_username: Joi.string().required(), smtp_password: Joi.string().required(), + // new options - not required() for backcompat + smtp_security: Joi.string(), + imap_security: Joi.string(), + imap_allow_insecure_ssl: Joi.boolean(), + smtp_allow_insecure_ssl: Joi.boolean(), + // TODO: deprecated options - eventually remove! smtp_custom_config: Joi.object(), - ssl_required: Joi.boolean().required(), + ssl_required: Joi.boolean(), }).required(); const resolvedGmailSettings = Joi.object().keys({ @@ -36,34 +47,33 @@ export const SUPPORTED_PROVIDERS = new Set( ['gmail', 'office365', 'imap', 'icloud', 'yahoo', 'fastmail'] ); +export function googleSettings(googleToken, email) { + const connectionSettings = Object.assign({ + imap_username: email, + smtp_username: email, + }, CommonProviderSettings.gmail); + const connectionCredentials = { + expiry_date: googleToken.expiry_date, + }; + if (GMAIL_CLIENT_ID && GMAIL_CLIENT_SECRET) { + // cloud-only credentials + connectionCredentials.client_id = GMAIL_CLIENT_ID; + connectionCredentials.client_secret = GMAIL_CLIENT_SECRET; + connectionCredentials.access_token = googleToken.access_token; + connectionCredentials.refresh_token = googleToken.refresh_token; + } + if (googleToken.xoauth2) { + connectionCredentials.xoauth2 = googleToken.xoauth2; + } + return {connectionSettings, connectionCredentials} +} + export function credentialsForProvider({provider, settings, email}) { if (provider === "gmail") { - const connectionSettings = { - imap_username: email, - imap_host: 'imap.gmail.com', - imap_port: 993, - smtp_username: email, - smtp_host: 'smtp.gmail.com', - smtp_port: 465, - ssl_required: true, - } - const connectionCredentials = { - xoauth2: settings.xoauth2, - expiry_date: settings.expiry_date, - } + const {connectionSettings, connectionCredentials} = googleSettings(settings, email) return {connectionSettings, connectionCredentials} } else if (provider === "office365") { - const connectionSettings = { - imap_host: 'outlook.office365.com', - imap_port: 993, - ssl_required: true, - smtp_custom_config: { - host: 'smtp.office365.com', - port: 587, - secure: false, - tls: {ciphers: 'SSLv3'}, - }, - } + const connectionSettings = CommonProviderSettings[provider]; const connectionCredentials = { imap_username: email, @@ -74,10 +84,33 @@ export function credentialsForProvider({provider, settings, email}) { return {connectionSettings, connectionCredentials} } else if (SUPPORTED_PROVIDERS.has(provider)) { const connectionSettings = _.pick(settings, [ - 'imap_host', 'imap_port', - 'smtp_host', 'smtp_port', - 'ssl_required', 'smtp_custom_config', + 'imap_host', 'imap_port', 'imap_security', + 'smtp_host', 'smtp_port', 'smtp_security', + 'smtp_allow_insecure_ssl', + 'imap_allow_insecure_ssl', ]); + // BACKCOMPAT ONLY - remove eventually & make _security params required! + if (!connectionSettings.imap_security) { + switch (connectionSettings.imap_port) { + case 993: + connectionSettings.imap_security = "SSL / TLS"; + break; + default: + connectionSettings.imap_security = "none"; + break; + } + } + if (!connectionSettings.smtp_security) { + switch (connectionSettings.smtp_security) { + case 465: + connectionSettings.smtp_security = "SSL / TLS"; + break; + default: + connectionSettings.smtp_security = 'STARTTLS'; + break; + } + } + // END BACKCOMPAT const connectionCredentials = _.pick(settings, [ 'imap_username', 'imap_password', 'smtp_username', 'smtp_password', @@ -98,16 +131,19 @@ function bearerToken(xoauth2) { } export function smtpConfigFromSettings(provider, connectionSettings, connectionCredentials) { - let config; - const {smtp_host, smtp_port, ssl_required} = connectionSettings; - if (connectionSettings.smtp_custom_config) { - config = connectionSettings.smtp_custom_config; + const {smtp_host, smtp_port, smtp_security, smtp_allow_insecure_ssl} = connectionSettings; + const config = { + host: smtp_host, + port: smtp_port, + secure: smtp_security === 'SSL / TLS', + }; + if (smtp_security === 'STARTTLS') { + config.requireTLS = true; + } + if (smtp_allow_insecure_ssl) { + config.tls = INSECURE_TLS_OPTIONS; } else { - config = { - host: smtp_host, - port: smtp_port, - secure: ssl_required, - }; + config.tls = SECURE_TLS_OPTIONS; } if (provider === 'gmail') { @@ -117,6 +153,7 @@ export function smtpConfigFromSettings(provider, connectionSettings, connectionC } const token = bearerToken(xoauth2); + config.auth = { user: connectionSettings.smtp_username, xoauth2: token } } else if (SUPPORTED_PROVIDERS.has(provider)) { const {smtp_username, smtp_password} = connectionCredentials @@ -153,16 +190,17 @@ export function imapAuthHandler(upsertAccount) { const connectionChecks = []; const {connectionSettings, connectionCredentials} = credentialsForProvider(request.payload) - // All IMAP accounts require a valid SMTP server for sending, and we never - // want to allow folks to connect accounts and find out later that they - // entered the wrong SMTP credentials. So verify here also! const smtpConfig = smtpConfigFromSettings(provider, connectionSettings, connectionCredentials); const smtpTransport = nodemailer.createTransport(Object.assign({ connectionTimeout: 30000, }, smtpConfig)); + + // All IMAP accounts require a valid SMTP server for sending, and we never + // want to allow folks to connect accounts and find out later that they + // entered the wrong SMTP credentials. So verify here also! const smtpVerifyPromise = smtpTransport.verify().catch((error) => { throw convertSmtpError(error); - }) + }); connectionChecks.push(smtpVerifyPromise); connectionChecks.push(IMAPConnection.connect({ diff --git a/packages/isomorphic-core/src/imap-connection.es6 b/packages/isomorphic-core/src/imap-connection.es6 index 4ea072e37..b3c977968 100644 --- a/packages/isomorphic-core/src/imap-connection.es6 +++ b/packages/isomorphic-core/src/imap-connection.es6 @@ -2,7 +2,7 @@ import Imap from 'imap'; import _ from 'underscore'; import xoauth2 from 'xoauth2'; import EventEmitter from 'events'; -import CommonProviderSettings from 'imap-provider-settings'; +import {INSECURE_TLS_OPTIONS, SECURE_TLS_OPTIONS} from './tls-utils'; import PromiseUtils from './promise-utils'; import IMAPBox from './imap-box'; import {RetryableError} from './errors' @@ -14,11 +14,6 @@ import { } from './imap-errors'; -const MAJOR_IMAP_PROVIDER_HOSTS = Object.keys(CommonProviderSettings).reduce( - (hostnameSet, key) => { - hostnameSet.add(CommonProviderSettings[key].imap_host); - return hostnameSet; - }, new Set()) const Capabilities = { Gmail: 'X-GM-EXT-1', Quota: 'QUOTA', @@ -40,17 +35,41 @@ export default class IMAPConnection extends EventEmitter { } static asyncResolveIMAPSettings(baseSettings) { + let autotls; + // BACKCOMPAT ONLY - remove the if conditional on this eventually + if (baseSettings.imap_security) { + switch (baseSettings.imap_security) { + case 'STARTTLS': + autotls = 'required'; + break; + case 'SSL / TLS': + autotls = 'never'; + break; + default: + autotls = 'always'; + break; + } + } else { + // old code used the default value + autotls = 'never'; + } const settings = { host: baseSettings.imap_host, port: baseSettings.imap_port, user: baseSettings.imap_username, password: baseSettings.imap_password, - tls: baseSettings.ssl_required, + // TODO: ssl_required is a deprecated setting, remove eventually + tls: baseSettings.imap_security === 'SSL / TLS' || baseSettings.ssl_required, + autotls: autotls, socketTimeout: baseSettings.socketTimeout || DEFAULT_SOCKET_TIMEOUT_MS, authTimeout: baseSettings.authTimeout || AUTH_TIMEOUT_MS, } - if (!MAJOR_IMAP_PROVIDER_HOSTS.has(settings.host)) { - settings.tlsOptions = { rejectUnauthorized: false }; + // TODO: second part of || is for backcompat only, remove eventually (old + // settings were insecure by default) + if (baseSettings.imap_allow_insecure_ssl || baseSettings.imap_allow_insecure_ssl === undefined) { + settings.tlsOptions = INSECURE_TLS_OPTIONS; + } else { + settings.tlsOptions = SECURE_TLS_OPTIONS; } if (process.env.NYLAS_DEBUG) { diff --git a/packages/isomorphic-core/src/smtp-errors.es6 b/packages/isomorphic-core/src/smtp-errors.es6 index 4bffeaaf5..096a83934 100644 --- a/packages/isomorphic-core/src/smtp-errors.es6 +++ b/packages/isomorphic-core/src/smtp-errors.es6 @@ -44,8 +44,12 @@ export function convertSmtpError(err) { if (/(?:connection timeout)|(?:connect etimedout)/i.test(err.message)) { return new SMTPConnectionTimeoutError(err) } - if (/connection closed/i.test(err.message)) { - return new SMTPConnectionEndedError(err) + if (/(?:connection|socket) closed?/i.test(err.message)) { + const smtpErr = SMTPConnectionEndedError(err) + if (err.code) { + // e.g. https://github.com/nodemailer/nodemailer/blob/master/lib/smtp-transport/index.js#L184-L185 + smtpErr.code = err.code; + } } if (/error initiating tls/i.test(err.message)) { return new SMTPConnectionTLSError(err); @@ -56,7 +60,7 @@ export function convertSmtpError(err) { if (/unknown protocol/i.test(err.message)) { return new SMTPProtocolError(err); } - if (/(?:invalid login)|(?:username and password not accepted)|(?:incorrect username or password)/i.test(err.message)) { + if (/(?:invalid login)|(?:username and password not accepted)|(?:incorrect username or password)|(?:authentication failed)/i.test(err.message)) { return new SMTPAuthenticationError(err); } diff --git a/packages/isomorphic-core/src/tls-utils.es6 b/packages/isomorphic-core/src/tls-utils.es6 new file mode 100644 index 000000000..fea2d2265 --- /dev/null +++ b/packages/isomorphic-core/src/tls-utils.es6 @@ -0,0 +1,15 @@ +import constants from 'constants'; + +const INSECURE_TLS_OPTIONS = { + secureProtocol: 'SSLv23_method', + rejectUnauthorized: false, +} + +const SECURE_TLS_OPTIONS = { + secureProtocol: 'SSLv23_method', + // See similar code in cloud-core for explanation of each flag: + // https://github.com/nylas/cloud-core/blob/e70f9e023b880090564b62fca8532f56ec77bfc3/sync-engine/inbox/auth/generic.py#L397-L435 + secureOptions: constants.SSL_OP_NO_SSLv3 | constants.SSL_OP_NO_SSLv2 | constants.SSL_OP_NO_COMPRESSION | constants.SSL_OP_CIPHER_SERVER_PREFERENCE | constants.SSL_OP_SINGLE_DH_USE | constants.SSL_OP_SINGLE_ECDH_USE, +} + +export {SECURE_TLS_OPTIONS, INSECURE_TLS_OPTIONS};