Skip to content

Commit 008cb4c

Browse files
committed
[*] 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: 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
1 parent 5733882 commit 008cb4c

File tree

11 files changed

+238
-100
lines changed

11 files changed

+238
-100
lines changed

packages/client-app/internal_packages/onboarding/lib/decorators/create-page-for-form.jsx

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,18 @@ const CreatePageForForm = (FormComponent) => {
122122
errorFieldNames.push('username');
123123
}
124124
if (err.statusCode === 401) {
125+
if (/smtp/i.test(err.message)) {
126+
errorFieldNames.push('smtp_username');
127+
errorFieldNames.push('smtp_password');
128+
}
129+
if (/imap/i.test(err.message)) {
130+
errorFieldNames.push('imap_username');
131+
errorFieldNames.push('imap_password');
132+
}
133+
// not sure what these are for -- backcompat?
125134
errorFieldNames.push('password')
126135
errorFieldNames.push('email');
127136
errorFieldNames.push('username');
128-
errorFieldNames.push('imap_username');
129-
errorFieldNames.push('smtp_username');
130-
errorFieldNames.push('imap_password');
131-
errorFieldNames.push('smtp_password');
132137
}
133138
if (NylasAPI.TimeoutErrorCodes.includes(err.statusCode)) { // timeout
134139
errorMessage = "We were unable to reach your mail provider. Please try again."

packages/client-app/internal_packages/onboarding/lib/onboarding-helpers.es6

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,14 @@ const IMAP_FIELDS = new Set([
1414
"imap_port",
1515
"imap_username",
1616
"imap_password",
17+
"imap_security",
18+
"imap_allow_insecure_ssl",
1719
"smtp_host",
1820
"smtp_port",
1921
"smtp_username",
2022
"smtp_password",
21-
"smtp_custom_config",
22-
"ssl_required",
23+
"smtp_security",
24+
"smtp_allow_insecure_ssl",
2325
]);
2426

2527
function base64url(inBuffer) {
@@ -133,7 +135,7 @@ export function runAuthRequest(accountInfo) {
133135
options: {
134136
path: '/auth',
135137
method: 'POST',
136-
timeout: 1000 * 90, // Connecting to IMAP could take up to 90 seconds, so we don't want to hang up too soon
138+
timeout: 1000 * 180, // Same timeout as server timeout (most requests are faster than 90s, but server validation can be slow in some cases)
137139
body: data,
138140
auth: noauth,
139141
},
@@ -144,7 +146,7 @@ export function runAuthRequest(accountInfo) {
144146
options: {
145147
path: `/auth`,
146148
method: 'POST',
147-
timeout: 1000 * 90, // Connecting to IMAP could take up to 90 seconds, so we don't want to hang up too soon
149+
timeout: 1000 * 180, // Same timeout as server timeout (most requests are faster than 90s, but server validation can be slow in some cases)
148150
body: data,
149151
auth: noauth,
150152
},
@@ -165,7 +167,10 @@ export function isValidHost(value) {
165167
export function accountInfoWithIMAPAutocompletions(existingAccountInfo) {
166168
const {email, type} = existingAccountInfo;
167169
const domain = email.split('@').pop().toLowerCase();
168-
const template = CommonProviderSettings[domain] || CommonProviderSettings[type] || {};
170+
let template = CommonProviderSettings[domain] || CommonProviderSettings[type] || {};
171+
if (template.alias) {
172+
template = CommonProviderSettings[template.alias];
173+
}
169174

170175
const usernameWithFormat = (format) => {
171176
if (format === 'email') {
@@ -177,18 +182,19 @@ export function accountInfoWithIMAPAutocompletions(existingAccountInfo) {
177182
return undefined;
178183
}
179184

180-
// always pre-fill SMTP / IMAP username, password and port.
181185
const defaults = {
182186
imap_host: template.imap_host,
183187
imap_port: template.imap_port || 993,
184188
imap_username: usernameWithFormat(template.imap_user_format),
185189
imap_password: existingAccountInfo.password,
190+
imap_security: template.imap_security || "SSL / TLS",
191+
imap_allow_insecure_ssl: template.imap_allow_insecure_ssl || false,
186192
smtp_host: template.smtp_host,
187193
smtp_port: template.smtp_port || 587,
188194
smtp_username: usernameWithFormat(template.smtp_user_format),
189195
smtp_password: existingAccountInfo.password,
190-
ssl_required: (template.ssl === '1'),
191-
smtp_custom_config: template.smtp_custom_config,
196+
smtp_security: template.smtp_security || "STARTTLS",
197+
smtp_allow_insecure_ssl: template.smtp_allow_insecure_ssl || false,
192198
}
193199

194200
return Object.assign({}, existingAccountInfo, defaults);

packages/client-app/internal_packages/onboarding/lib/page-account-settings-imap.jsx

Lines changed: 79 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,24 +56,94 @@ class AccountIMAPSettingsForm extends React.Component {
5656
this.props.onConnect();
5757
}
5858

59-
renderFieldsForType(type) {
60-
const {accountInfo, errorFieldNames, submitting, onFieldKeyPress, onFieldChange} = this.props;
59+
renderPortDropdown(protocol) {
60+
if (!["imap", "smtp"].includes(protocol)) {
61+
throw new Error(`Can't render port dropdown for protocol '${protocol}'`);
62+
}
63+
const {accountInfo, submitting, onFieldKeyPress, onFieldChange} = this.props;
64+
65+
if (protocol === "imap") {
66+
return (
67+
<span>
68+
<label htmlFor="imap_port">Port:</label>
69+
<select
70+
id="imap_port"
71+
tabIndex={0}
72+
value={accountInfo.imap_port}
73+
disabled={submitting}
74+
onKeyPress={onFieldKeyPress}
75+
onChange={onFieldChange}
76+
>
77+
<option value="143" key="143">143</option>
78+
<option value="993" key="993">993</option>
79+
</select>
80+
</span>
81+
)
82+
}
83+
if (protocol === "smtp") {
84+
return (
85+
<span>
86+
<label htmlFor="smtp_port">Port:</label>
87+
<select
88+
id="smtp_port"
89+
tabIndex={0}
90+
value={accountInfo.smtp_port}
91+
disabled={submitting}
92+
onKeyPress={onFieldKeyPress}
93+
onChange={onFieldChange}
94+
>
95+
<option value="25" key="25">25</option>
96+
<option value="465" key="465">465</option>
97+
<option value="587" key="587">587</option>
98+
</select>
99+
</span>
100+
)
101+
}
102+
return "";
103+
}
104+
105+
renderSecurityDropdown(protocol) {
106+
const {accountInfo, submitting, onFieldKeyPress, onFieldChange} = this.props;
61107

62108
return (
63109
<div>
64-
<FormField field={`${type}_host`} title={"Server"} {...this.props} />
65-
<div style={{textAlign: 'left'}}>
66-
<FormField field={`${type}_port`} title={"Port"} style={{width: 100, marginRight: 20}} {...this.props} />
110+
<span>
111+
<label htmlFor={`${protocol}_security`}>Security:</label>
112+
<select
113+
id={`${protocol}_security`}
114+
tabIndex={0}
115+
value={accountInfo[`${protocol}_security`]}
116+
disabled={submitting}
117+
onKeyPress={onFieldKeyPress}
118+
onChange={onFieldChange}
119+
>
120+
<option value="SSL / TLS" key="SSL">SSL / TLS</option>
121+
<option value="STARTTLS" key="STARTTLS">STARTTLS</option>
122+
<option value="none" key="none">none</option>
123+
</select>
124+
</span>
125+
<span style={{paddingLeft: '20px', paddingTop: '10px'}}>
67126
<input
68127
type="checkbox"
69-
id={`ssl_required`}
70-
className={(accountInfo.imap_host && errorFieldNames.includes(`ssl_required`)) ? 'error' : ''}
128+
id={`${protocol}_allow_insecure_ssl`}
71129
disabled={submitting}
72-
checked={accountInfo[`ssl_required`] || false}
130+
checked={accountInfo[`${protocol}_allow_insecure_ssl`] || false}
73131
onKeyPress={onFieldKeyPress}
74132
onChange={onFieldChange}
75133
/>
76-
<label htmlFor={`ssl_required`} className="checkbox">Require SSL</label>
134+
<label htmlFor={`${protocol}_allow_insecure_ssl"`} className="checkbox">Allow insecure SSL</label>
135+
</span>
136+
</div>
137+
)
138+
}
139+
140+
renderFieldsForType(type) {
141+
return (
142+
<div>
143+
<FormField field={`${type}_host`} title={"Server"} {...this.props} />
144+
<div style={{textAlign: 'left'}}>
145+
{this.renderPortDropdown(type)}
146+
{this.renderSecurityDropdown(type)}
77147
</div>
78148
<FormField field={`${type}_username`} title={"Username"} {...this.props} />
79149
<FormField field={`${type}_password`} title={"Password"} type="password" {...this.props} />

packages/cloud-api/src/routes/auth.es6

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ export default function registerAuthRoutes(server) {
129129
const tok = await GAuth.exchangeCodeForGoogleToken(client, code);
130130

131131
profile = await GAuth.fetchGoogleProfile(client);
132-
const settings = GAuth.imapSettings(tok, profile)
132+
const settings = AuthHelpers.googleSettings(tok, profile.email)
133133

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

packages/cloud-core/gmail-oauth-helpers.es6

Lines changed: 4 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -41,32 +41,12 @@ class GmailOAuthHelpers {
4141
})
4242
}
4343

44-
imapSettings(googleToken, googleProfile) {
45-
return {
46-
connection: {
47-
imap_username: googleProfile.email,
48-
imap_host: 'imap.gmail.com',
49-
imap_port: 993,
50-
smtp_username: googleProfile.email,
51-
smtp_host: 'smtp.gmail.com',
52-
smtp_port: 465,
53-
ssl_required: true,
54-
},
55-
credentials: {
56-
refresh_token: googleToken.refresh_token,
57-
expiry_date: googleToken.expiry_date,
58-
client_id: GMAIL_CLIENT_ID,
59-
client_secret: GMAIL_CLIENT_SECRET,
60-
},
61-
}
62-
}
63-
6444
async resolveIMAPSettings(imapSettings, logger) {
6545
const imap = await IMAPConnection.connect({
6646
logger: logger,
6747
settings: Object.assign({},
68-
imapSettings.connection,
69-
imapSettings.credentials
48+
imapSettings.connectionSettings,
49+
imapSettings.connectionCredentials,
7050
),
7151
db: {},
7252
})
@@ -80,8 +60,8 @@ class GmailOAuthHelpers {
8060
name: googleProfile.name,
8161
provider: Provider.Gmail,
8262
emailAddress: googleProfile.email,
83-
connectionSettings: imapSettings.connection,
84-
}, imapSettings.credentials)
63+
connectionSettings: imapSettings.connectionSettings,
64+
}, imapSettings.connectionCredentials)
8565
}
8666

8767
async refreshAccessToken(account) {

packages/isomorphic-core/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,5 @@ module.exports = {
2626
SendUtils: require('./src/send-utils'),
2727
executeJasmine: require('./spec/jasmine/execute').default,
2828
StringUtils: require('./src/string-utils'),
29+
TLSUtils: require('./src/tls-utils'),
2930
}

packages/isomorphic-core/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
"atob": "2.0.3",
1111
"btoa": "1.1.2",
1212
"imap": "github:jstejada/node-imap#fix-parse-body-list",
13-
"imap-provider-settings": "github:nylas/imap-provider-settings#054303cc2",
13+
"imap-provider-settings": "github:nylas/imap-provider-settings#2fdcd34d59b",
1414
"jasmine": "2.x.x",
1515
"joi": "8.4.2",
1616
"libhoney": "1.0.0-beta.2",

0 commit comments

Comments
 (0)