Improve selected client certificate validation

Previously, we didn't check whether the pfx file actually contained
either a cert or a private key, where in either case it would be
unusable. We also missed some potential invalid format issues. Now we
catch each of these, and display different errors for specific failure
cases.

Author: pimterry

src/components/settings/connection-settings-card.tsx

@@ -8,7 +8,7 @@ import { WarningIcon, Icon } from '../../icons';
 import { trackEvent } from '../../metrics';
 
 import { uploadFile } from '../../util/ui';
-import { asError } from '../../util/error';
+import { UnreachableCheck, asError, unreachableCheck } from '../../util/error';
 
 import { UpstreamProxyType, RulesStore } from '../../model/rules/rules-store';
 import { ParsedCertificate, ValidationResult } from '../../model/crypto';
@@ -316,6 +316,9 @@ class ClientCertificateConfig extends React.Component<{ rulesStore: RulesStore }
     @observable
     clientCertState: undefined | 'encrypted' | 'processing' | 'error' | 'decrypted';
 
+    @observable
+    clientCertError: string | undefined;
+
     @action.bound
     onClientCertSelected(event: React.ChangeEvent<HTMLInputElement>) {
         const input = event.target;
@@ -326,6 +329,7 @@ class ClientCertificateConfig extends React.Component<{ rulesStore: RulesStore }
         fileReader.readAsArrayBuffer(file);
 
         this.clientCertState = 'processing';
+        this.clientCertError = undefined;
 
         const thisConfig = this; // fileReader events set 'this'
         fileReader.addEventListener('load', flow(function * () {
@@ -338,29 +342,18 @@ class ClientCertificateConfig extends React.Component<{ rulesStore: RulesStore }
 
             result = yield validatePKCS(thisConfig.clientCertData.pfx, undefined);
 
-            if (result === 'valid') {
-                thisConfig.clientCertState = 'decrypted';
-                thisConfig.clientCertData.passphrase = undefined;
-                return;
-            }
-
-            if (result === 'invalid-format') {
-                thisConfig.clientCertState = 'error';
-                return;
-            }
+            if (result === 'invalid-passphrase') {
+                // If it fails, try again with an empty key, since that is sometimes used for 'no passphrase'
+                result = yield validatePKCS(thisConfig.clientCertData.pfx, '');
 
-            // If it fails, try again with an empty key, since that is sometimes used for 'no passphrase'
-            result = yield validatePKCS(thisConfig.clientCertData.pfx, '');
+                if (result === 'valid') {
+                    thisConfig.clientCertData.passphrase = '';
+                }
 
-            if (result === 'valid') {
-                thisConfig.clientCertState = 'decrypted';
-                thisConfig.clientCertData.passphrase = '';
-                return;
+                thisConfig.handleClientCertValidationResult(result);
+            } else {
+                thisConfig.handleClientCertValidationResult(result);
             }
-
-            // If that still hasn't worked, it's encrypted. Mark is as such, and wait for the user
-            // to either cancel, or enter the correct passphrase.
-            thisConfig.clientCertState = 'encrypted';
         }));
 
         fileReader.addEventListener('error', () => {
@@ -371,15 +364,32 @@ class ClientCertificateConfig extends React.Component<{ rulesStore: RulesStore }
     readonly decryptClientCertData = flow(function * (this: ClientCertificateConfig) {
         const { pfx, passphrase } = this.clientCertData!;
 
-        let result: ValidationResult;
-
         this.clientCertState = 'processing';
-        result = yield validatePKCS(pfx, passphrase);
-        this.clientCertState = result === 'valid'
-            ? 'decrypted'
-            : 'encrypted';
+        this.clientCertError = undefined;
+
+        const result = yield validatePKCS(pfx, passphrase);
+        this.handleClientCertValidationResult(result);
     });
 
+    handleClientCertValidationResult(result: ValidationResult) {
+        this.clientCertError = undefined;
+
+        if (result === 'valid') {
+            this.clientCertState = 'decrypted';
+        } else if (result === 'invalid-passphrase') {
+            this.clientCertState = 'encrypted';
+        } else if (result === 'invalid-format') {
+            this.clientCertState = 'error';
+            this.clientCertError = 'Parsing failed';
+        } else if (result === 'missing-key') {
+            this.clientCertState = 'error';
+            this.clientCertError = 'No private key found';
+        } else if (result === 'missing-cert') {
+            this.clientCertState = 'error';
+            this.clientCertError = 'No certificate found';
+        } else unreachableCheck(result);
+    }
+
     @action.bound
     dropClientCertData() {
         this.clientCertData = undefined;
@@ -456,12 +466,14 @@ class ClientCertificateConfig extends React.Component<{ rulesStore: RulesStore }
                                 <Icon icon={['fas', 'undo']} title='Deselect this certificate' />
                             </SettingsButton>
                         </DecryptionInput>
-                    : <DecryptionInput>
-                        <p><WarningIcon /> Invalid certificate</p>
-                        <SettingsButton onClick={this.dropClientCertData}>
-                            <Icon icon={['fas', 'undo']} title='Deselect this certificate' />
-                        </SettingsButton>
-                    </DecryptionInput>
+                    : this.clientCertState === 'error'
+                        ? <DecryptionInput>
+                            <p><WarningIcon /> {this.clientCertError || 'Invalid certificate'}</p>
+                            <SettingsButton onClick={this.dropClientCertData}>
+                                <Icon icon={['fas', 'undo']} title='Deselect this certificate' />
+                            </SettingsButton>
+                        </DecryptionInput>
+                    : unreachableCheck(this.clientCertState)
                 }
                 <SettingsButton
                     disabled={

src/model/crypto.ts

@@ -5,8 +5,17 @@ require('node-forge/lib/asn1');
 require('node-forge/lib/pki');
 require('node-forge/lib/pkcs12');
 import * as forge from 'node-forge/lib/forge';
+import { isErrorLike } from '../util/error';
 
-export type ValidationResult = 'valid' | 'invalid-format' | 'invalid-passphrase';
+export type ValidationResult =
+    | 'valid'
+    | 'invalid-format'
+    | 'invalid-passphrase'
+    | 'missing-cert'
+    | 'missing-key';
+
+const pkcs12ContainsBag = (pfx: forge.pkcs12.Pkcs12Pfx, bagType: string) =>
+    pfx.getBags({ bagType })[bagType]?.length;
 
 export function validatePKCS12(
     data: ArrayBuffer,
@@ -23,11 +32,26 @@ export function validatePKCS12(
     try {
         // Decrypt the PKCS12 with the passphrase - we assume if this works, it's good.
         // Could be false (different key on cert within, who knows what else), but typically not.
-        forge.pkcs12.pkcs12FromAsn1(asn1Data, passphrase);
+        const pfx = forge.pkcs12.pkcs12FromAsn1(asn1Data, passphrase);
+
+        if (!pkcs12ContainsBag(pfx, forge.pki.oids.pkcs8ShroudedKeyBag)) {
+            return 'missing-key';
+            // There is also oids.keyBag, but AFAICT that's not what we need here generally.
+        }
+
+        if (!pkcs12ContainsBag(pfx, forge.pki.oids.certBag)) {
+            return 'missing-cert';
+        }
+
         return 'valid';
     } catch (e) {
         console.log(e);
-        return 'invalid-passphrase';
+        if (isErrorLike(e) && e.message?.includes('Invalid password')) {
+            return 'invalid-passphrase';
+        } else {
+            // E.g. "ASN.1 object is not an PKCS#12 PFX."
+            return 'invalid-format';
+        }
     }
 }