Remove the insecure ignoreCerts option.

Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit is contained in:
Anders Kaseorg
2020-05-13 04:06:50 -07:00
parent ca9ab6168e
commit d661895545
7 changed files with 24 additions and 95 deletions

View File

@@ -225,7 +225,6 @@ app.on('ready', () => {
) /* eslint-disable-line max-params */ => { ) /* eslint-disable-line max-params */ => {
// TODO: The entire concept of selectively ignoring certificate errors // TODO: The entire concept of selectively ignoring certificate errors
// is ill-conceived, and this handler needs to be deleted. // is ill-conceived, and this handler needs to be deleted.
event.preventDefault();
const {origin} = new URL(url); const {origin} = new URL(url);
const filename = CertificateUtil.getCertificate(encodeURIComponent(origin)); const filename = CertificateUtil.getCertificate(encodeURIComponent(origin));
@@ -237,6 +236,7 @@ app.on('ready', () => {
); );
if (certificate.data.replace(/[\r\n]/g, '') === if (certificate.data.replace(/[\r\n]/g, '') ===
savedCertificate.replace(/[\r\n]/g, '')) { savedCertificate.replace(/[\r\n]/g, '')) {
event.preventDefault();
callback(true); callback(true);
return; return;
} }
@@ -245,20 +245,11 @@ app.on('ready', () => {
} }
} }
page.send( dialog.showErrorBox(
'certificate-error', 'Certificate error',
webContents.id === mainWindow.webContents.id ? null : webContents.id, `The server presented an invalid certificate for ${origin}:
makeRendererCallback(ignore => {
callback(ignore);
if (!ignore) {
dialog.showErrorBox(
'Certificate error',
`The server presented an invalid certificate for ${origin}:
${error}` ${error}`
);
}
})
); );
}); });

View File

@@ -24,7 +24,6 @@ interface WebViewProps {
nodeIntegration: boolean; nodeIntegration: boolean;
preload: boolean; preload: boolean;
onTitleChange: () => void; onTitleChange: () => void;
ignoreCerts?: boolean;
hasPermission?: (origin: string, permission: string) => boolean; hasPermission?: (origin: string, permission: string) => boolean;
} }

View File

@@ -360,7 +360,6 @@ class ServerManagerView {
url: server.url, url: server.url,
role: 'server', role: 'server',
name: CommonUtil.decodeString(server.alias), name: CommonUtil.decodeString(server.alias),
ignoreCerts: server.ignoreCerts,
hasPermission: (origin: string, permission: string) => hasPermission: (origin: string, permission: string) =>
origin === server.url && permission === 'notifications', origin === server.url && permission === 'notifications',
isActive: () => { isActive: () => {
@@ -816,21 +815,6 @@ class ServerManagerView {
}); });
} }
ipcRenderer.on('certificate-error', (
event: Event,
webContentsId: number | null,
rendererCallbackId: number
) => {
const ignore = webContentsId !== null &&
this.tabs.some(
({webview}) =>
!webview.loading &&
webview.$el.getWebContentsId() === webContentsId &&
webview.props.ignoreCerts
);
ipcRenderer.send('renderer-callback', rendererCallbackId, ignore);
});
ipcRenderer.on('permission-request', ( ipcRenderer.on('permission-request', (
event: Event, event: Event,
{webContentsId, origin, permission}: { {webContentsId, origin, permission}: {

View File

@@ -17,7 +17,6 @@ export interface ServerConf {
url: string; url: string;
alias?: string; alias?: string;
icon?: string; icon?: string;
ignoreCerts?: boolean;
} }
const logger = new Logger({ const logger = new Logger({
@@ -55,26 +54,14 @@ export function getDomain(index: number): ServerConf {
return db.getData(`/domains[${index}]`); return db.getData(`/domains[${index}]`);
} }
export function shouldIgnoreCerts(url: string): boolean {
const domains = getDomains();
for (const domain of domains) {
if (domain.url === url) {
return domain.ignoreCerts;
}
}
return null;
}
export function updateDomain(index: number, server: ServerConf): void { export function updateDomain(index: number, server: ServerConf): void {
reloadDB(); reloadDB();
db.push(`/domains[${index}]`, server, true); db.push(`/domains[${index}]`, server, true);
} }
export async function addDomain(server: ServerConf): Promise<void> { export async function addDomain(server: ServerConf): Promise<void> {
const {ignoreCerts} = server;
if (server.icon) { if (server.icon) {
const localIconUrl = await saveServerIcon(server, ignoreCerts); const localIconUrl = await saveServerIcon(server);
server.icon = localIconUrl; server.icon = localIconUrl;
db.push('/domains[]', server, true); db.push('/domains[]', server, true);
reloadDB(); reloadDB();
@@ -118,33 +105,16 @@ async function checkCertError(domain: string, serverConf: ServerConf, error: any
const certErrorMessage = Messages.certErrorMessage(domain, error); const certErrorMessage = Messages.certErrorMessage(domain, error);
const certErrorDetail = Messages.certErrorDetail(); const certErrorDetail = Messages.certErrorDetail();
const {response} = await dialog.showMessageBox({ await dialog.showMessageBox({
type: 'warning', type: 'error',
buttons: ['Yes', 'No'], buttons: ['OK'],
defaultId: 1,
message: certErrorMessage, message: certErrorMessage,
detail: certErrorDetail detail: certErrorDetail
}); });
if (response === 0) { throw new Error('Untrusted certificate.');
// Set ignoreCerts parameter to true in case user responds with yes
serverConf.ignoreCerts = true;
try {
return await getServerSettings(domain, serverConf.ignoreCerts);
} catch (_) {
if (error === Messages.noOrgsError(domain)) {
throw new Error(error);
}
return serverConf;
}
} else {
throw new Error('Untrusted certificate.');
}
} }
// ignoreCerts parameter helps in fetching server icon and export async function checkDomain(domain: string, silent = false): Promise<ServerConf> {
// other server details when user chooses to ignore certificate warnings
export async function checkDomain(domain: string, ignoreCerts = false, silent = false): Promise<ServerConf> {
if (!silent && duplicateDomain(domain)) { if (!silent && duplicateDomain(domain)) {
// Do not check duplicate in silent mode // Do not check duplicate in silent mode
throw new Error('This server has been added.'); throw new Error('This server has been added.');
@@ -155,12 +125,11 @@ export async function checkDomain(domain: string, ignoreCerts = false, silent =
const serverConf = { const serverConf = {
icon: defaultIconUrl, icon: defaultIconUrl,
url: domain, url: domain,
alias: domain, alias: domain
ignoreCerts
}; };
try { try {
return await getServerSettings(domain, serverConf.ignoreCerts); return await getServerSettings(domain);
} catch (error_) { } catch (error_) {
// Make sure that error is an error or string not undefined // Make sure that error is an error or string not undefined
// so validation does not throw error. // so validation does not throw error.
@@ -176,10 +145,10 @@ export async function checkDomain(domain: string, ignoreCerts = false, silent =
} }
} }
async function getServerSettings(domain: string, ignoreCerts = false): Promise<ServerConf> { async function getServerSettings(domain: string): Promise<ServerConf> {
const serverSettingsOptions = { const serverSettingsOptions = {
url: domain + '/api/v1/server_settings', url: domain + '/api/v1/server_settings',
...RequestUtil.requestOptions(domain, ignoreCerts) ...RequestUtil.requestOptions(domain)
}; };
return new Promise((resolve, reject) => { return new Promise((resolve, reject) => {
@@ -196,8 +165,7 @@ async function getServerSettings(domain: string, ignoreCerts = false): Promise<S
// Following check handles both the cases // Following check handles both the cases
icon: realm_icon.startsWith('/') ? realm_uri + realm_icon : realm_icon, icon: realm_icon.startsWith('/') ? realm_uri + realm_icon : realm_icon,
url: realm_uri, url: realm_uri,
alias: escape(realm_name), alias: escape(realm_name)
ignoreCerts
}); });
} else { } else {
reject(Messages.noOrgsError(domain)); reject(Messages.noOrgsError(domain));
@@ -209,13 +177,13 @@ async function getServerSettings(domain: string, ignoreCerts = false): Promise<S
}); });
} }
export async function saveServerIcon(server: ServerConf, ignoreCerts = false): Promise<string> { export async function saveServerIcon(server: ServerConf): Promise<string> {
const url = server.icon; const url = server.icon;
const domain = server.url; const domain = server.url;
const serverIconOptions = { const serverIconOptions = {
url, url,
...RequestUtil.requestOptions(domain, ignoreCerts) ...RequestUtil.requestOptions(domain)
}; };
// The save will always succeed. If url is invalid, downgrade to default icon. // The save will always succeed. If url is invalid, downgrade to default icon.
@@ -251,10 +219,9 @@ export async function saveServerIcon(server: ServerConf, ignoreCerts = false): P
export async function updateSavedServer(url: string, index: number): Promise<void> { export async function updateSavedServer(url: string, index: number): Promise<void> {
// Does not promise successful update // Does not promise successful update
const oldIcon = getDomain(index).icon; const oldIcon = getDomain(index).icon;
const {ignoreCerts} = getDomain(index);
try { try {
const newServerConf = await checkDomain(url, ignoreCerts, true); const newServerConf = await checkDomain(url, true);
const localIconUrl = await saveServerIcon(newServerConf, ignoreCerts); const localIconUrl = await saveServerIcon(newServerConf);
if (!oldIcon || localIconUrl !== '../renderer/img/icon.png') { if (!oldIcon || localIconUrl !== '../renderer/img/icon.png') {
newServerConf.icon = localIconUrl; newServerConf.icon = localIconUrl;
updateDomain(index, newServerConf); updateDomain(index, newServerConf);

View File

@@ -5,7 +5,6 @@ import backoff from 'backoff';
import request from 'request'; import request from 'request';
import Logger from './logger-util'; import Logger from './logger-util';
import * as RequestUtil from './request-util'; import * as RequestUtil from './request-util';
import * as DomainUtil from './domain-util';
const logger = new Logger({ const logger = new Logger({
file: 'domain-util.log', file: 'domain-util.log',
@@ -35,15 +34,10 @@ export default class ReconnectUtil {
async isOnline(): Promise<boolean> { async isOnline(): Promise<boolean> {
return new Promise(resolve => { return new Promise(resolve => {
try { try {
const ignoreCerts = DomainUtil.shouldIgnoreCerts(this.url);
if (ignoreCerts === null) {
return;
}
request( request(
{ {
url: `${this.url}/static/favicon.ico`, url: `${this.url}/static/favicon.ico`,
...RequestUtil.requestOptions(this.url, ignoreCerts) ...RequestUtil.requestOptions(this.url)
}, },
(error: Error, response: request.Response) => { (error: Error, response: request.Response) => {
const isValidResponse = const isValidResponse =

View File

@@ -20,12 +20,9 @@ interface RequestUtilResponse {
proxy: string | void | ProxyUtil.ProxyRule; proxy: string | void | ProxyUtil.ProxyRule;
ecdhCurve: 'auto'; ecdhCurve: 'auto';
headers: { 'User-Agent': string }; headers: { 'User-Agent': string };
rejectUnauthorized: boolean;
} }
// ignoreCerts parameter helps in fetching server icon and export function requestOptions(domain: string): RequestUtilResponse {
// other server details when user chooses to ignore certificate warnings
export function requestOptions(domain: string, ignoreCerts: boolean): RequestUtilResponse {
domain = formatUrl(domain); domain = formatUrl(domain);
const certificate = CertificateUtil.getCertificate( const certificate = CertificateUtil.getCertificate(
encodeURIComponent(domain) encodeURIComponent(domain)
@@ -56,8 +53,7 @@ export function requestOptions(domain: string, ignoreCerts: boolean): RequestUti
ca: certificateLocation ? certificateLocation : '', ca: certificateLocation ? certificateLocation : '',
proxy: proxyEnabled ? ProxyUtil.getProxy(domain) : '', proxy: proxyEnabled ? ProxyUtil.getProxy(domain) : '',
ecdhCurve: 'auto', ecdhCurve: 'auto',
headers: {'User-Agent': SystemUtil.getUserAgent()}, headers: {'User-Agent': SystemUtil.getUserAgent()}
rejectUnauthorized: !ignoreCerts
}; };
} }

View File

@@ -19,14 +19,12 @@ export function noOrgsError(domain: string): string {
} }
export function certErrorMessage(domain: string, error: string): string { export function certErrorMessage(domain: string, error: string): string {
return `Do you trust certificate from ${domain}? \n ${error}`; return `Certificate error for ${domain}\n${error}`;
} }
export function certErrorDetail(): string { export function certErrorDetail(): string {
return `The organization you're connecting to is either someone impersonating the Zulip server you entered, or the server you're trying to connect to is configured in an insecure way. return `The organization you're connecting to is either someone impersonating the Zulip server you entered, or the server you're trying to connect to is configured in an insecure way.
\nIf you have a valid certificate please add it from Settings>Organizations and try to add the organization again. \nIf you have a valid certificate please add it from Settings>Organizations and try to add the organization again.`;
\nUnless you have a good reason to believe otherwise, you should not proceed.
\nYou can click here if you'd like to proceed with the connection.`;
} }
export function enterpriseOrgError(length: number, domains: string[]): DialogBoxError { export function enterpriseOrgError(length: number, domains: string[]): DialogBoxError {