Skip to content

Commit

Permalink
[*] Revamp SSL options (including user-facing)
Browse files Browse the repository at this point in the history
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:
nylas/imap-provider-settings@9851054

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
  • Loading branch information
spang committed Apr 6, 2017
1 parent 5733882 commit 008cb4c
Show file tree
Hide file tree
Showing 11 changed files with 238 additions and 100 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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') {
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<span>
<label htmlFor="imap_port">Port:</label>
<select
id="imap_port"
tabIndex={0}
value={accountInfo.imap_port}
disabled={submitting}
onKeyPress={onFieldKeyPress}
onChange={onFieldChange}
>
<option value="143" key="143">143</option>
<option value="993" key="993">993</option>
</select>
</span>
)
}
if (protocol === "smtp") {
return (
<span>
<label htmlFor="smtp_port">Port:</label>
<select
id="smtp_port"
tabIndex={0}
value={accountInfo.smtp_port}
disabled={submitting}
onKeyPress={onFieldKeyPress}
onChange={onFieldChange}
>
<option value="25" key="25">25</option>
<option value="465" key="465">465</option>
<option value="587" key="587">587</option>
</select>
</span>
)
}
return "";
}

renderSecurityDropdown(protocol) {
const {accountInfo, submitting, onFieldKeyPress, onFieldChange} = this.props;

return (
<div>
<FormField field={`${type}_host`} title={"Server"} {...this.props} />
<div style={{textAlign: 'left'}}>
<FormField field={`${type}_port`} title={"Port"} style={{width: 100, marginRight: 20}} {...this.props} />
<span>
<label htmlFor={`${protocol}_security`}>Security:</label>
<select
id={`${protocol}_security`}
tabIndex={0}
value={accountInfo[`${protocol}_security`]}
disabled={submitting}
onKeyPress={onFieldKeyPress}
onChange={onFieldChange}
>
<option value="SSL / TLS" key="SSL">SSL / TLS</option>
<option value="STARTTLS" key="STARTTLS">STARTTLS</option>
<option value="none" key="none">none</option>
</select>
</span>
<span style={{paddingLeft: '20px', paddingTop: '10px'}}>
<input
type="checkbox"
id={`ssl_required`}
className={(accountInfo.imap_host && errorFieldNames.includes(`ssl_required`)) ? 'error' : ''}
id={`${protocol}_allow_insecure_ssl`}
disabled={submitting}
checked={accountInfo[`ssl_required`] || false}
checked={accountInfo[`${protocol}_allow_insecure_ssl`] || false}
onKeyPress={onFieldKeyPress}
onChange={onFieldChange}
/>
<label htmlFor={`ssl_required`} className="checkbox">Require SSL</label>
<label htmlFor={`${protocol}_allow_insecure_ssl"`} className="checkbox">Allow insecure SSL</label>
</span>
</div>
)
}

renderFieldsForType(type) {
return (
<div>
<FormField field={`${type}_host`} title={"Server"} {...this.props} />
<div style={{textAlign: 'left'}}>
{this.renderPortDropdown(type)}
{this.renderSecurityDropdown(type)}
</div>
<FormField field={`${type}_username`} title={"Username"} {...this.props} />
<FormField field={`${type}_password`} title={"Password"} type="password" {...this.props} />
Expand Down
2 changes: 1 addition & 1 deletion packages/cloud-api/src/routes/auth.es6
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export default function registerAuthRoutes(server) {
const tok = await GAuth.exchangeCodeForGoogleToken(client, code);

profile = await GAuth.fetchGoogleProfile(client);
const settings = GAuth.imapSettings(tok, profile)
const settings = AuthHelpers.googleSettings(tok, profile.email)

request.logger.debug("Resolving IMAP connection")

Expand Down
28 changes: 4 additions & 24 deletions packages/cloud-core/gmail-oauth-helpers.es6
Original file line number Diff line number Diff line change
Expand Up @@ -41,32 +41,12 @@ class GmailOAuthHelpers {
})
}

imapSettings(googleToken, googleProfile) {
return {
connection: {
imap_username: googleProfile.email,
imap_host: 'imap.gmail.com',
imap_port: 993,
smtp_username: googleProfile.email,
smtp_host: 'smtp.gmail.com',
smtp_port: 465,
ssl_required: true,
},
credentials: {
refresh_token: googleToken.refresh_token,
expiry_date: googleToken.expiry_date,
client_id: GMAIL_CLIENT_ID,
client_secret: GMAIL_CLIENT_SECRET,
},
}
}

async resolveIMAPSettings(imapSettings, logger) {
const imap = await IMAPConnection.connect({
logger: logger,
settings: Object.assign({},
imapSettings.connection,
imapSettings.credentials
imapSettings.connectionSettings,
imapSettings.connectionCredentials,
),
db: {},
})
Expand All @@ -80,8 +60,8 @@ class GmailOAuthHelpers {
name: googleProfile.name,
provider: Provider.Gmail,
emailAddress: googleProfile.email,
connectionSettings: imapSettings.connection,
}, imapSettings.credentials)
connectionSettings: imapSettings.connectionSettings,
}, imapSettings.connectionCredentials)
}

async refreshAccessToken(account) {
Expand Down
1 change: 1 addition & 0 deletions packages/isomorphic-core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
}
2 changes: 1 addition & 1 deletion packages/isomorphic-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 008cb4c

Please sign in to comment.