From 6d0207c32eab0d7e7fdd0e696458e77385b8807d Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sat, 29 Nov 2014 03:58:43 +1100 Subject: [PATCH] More robust failure conditions for local repo keystore. Was experiencing some problems with local repo keystore on Android 2.3.4. Previously, the local repo keystore could fail to create, but would still return a LocalRepoKeyStore instance to be used. Now it throws a checked exception, and is dealt with in the relveant places. Also cleaned up some calls to "e.printStackTrace()" and replaced with more informative logging messages. --- .../fdroid/localrepo/LocalRepoKeyStore.java | 95 ++++++++++++++----- .../fdroid/localrepo/LocalRepoManager.java | 28 ++++-- .../fdroid/localrepo/LocalRepoService.java | 13 ++- src/org/fdroid/fdroid/net/LocalHTTPD.java | 6 +- .../fdroid/net/WifiStateChangeService.java | 34 ++++--- 5 files changed, 119 insertions(+), 57 deletions(-) diff --git a/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java b/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java index aafe89707..351a5b4d2 100644 --- a/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java +++ b/src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java @@ -34,7 +34,9 @@ import kellinwood.security.zipsigner.ZipSigner; // TODO Address exception handling in a uniform way throughout public class LocalRepoKeyStore { - private static final String TAG = "KerplappKeyStore"; + + private static final String TAG = "org.fdroid.fdroid.localrepo.LocalRepoKeyStore"; + public static final String INDEX_CERT_ALIAS = "fdroid"; public static final String HTTP_CERT_ALIAS = "https"; @@ -49,26 +51,49 @@ public class LocalRepoKeyStore { private KeyManager[] keyManagers; private File keyStoreFile; - public static LocalRepoKeyStore get(Context context) { + public static LocalRepoKeyStore get(Context context) throws InitException { if (localRepoKeyStore == null) localRepoKeyStore = new LocalRepoKeyStore(context); return localRepoKeyStore; } - private LocalRepoKeyStore(Context context) { + public static class InitException extends Exception { + public InitException(String detailMessage) { + super(detailMessage); + } + } + + private LocalRepoKeyStore(Context context) throws InitException { try { - Log.d(TAG, "generating LocalRepoKeyStore instance"); File appKeyStoreDir = context.getDir("keystore", Context.MODE_PRIVATE); + + Log.d(TAG, "Generating LocalRepoKeyStore instance: " + appKeyStoreDir.getAbsolutePath()); this.keyStoreFile = new File(appKeyStoreDir, "kerplapp.bks"); + + Log.d(TAG, "Using default KeyStore type: " + KeyStore.getDefaultType()); this.keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); - // If there isn't a persisted BKS keystore on disk we need to - // create a new empty keystore + if (keyStoreFile.exists()) { + try { + Log.d(TAG, "Keystore already exists, loading..."); + keyStore.load(new FileInputStream(keyStoreFile), "".toCharArray()); + } catch (IOException e) { + Log.e(TAG, "Error while loading existing keystore. Will delete and create a new one."); + + // NOTE: Could opt to delete and then re-create the keystore here, but that may + // be undesirable. For example - if you were to re-connect to an existing device + // that you have swapped apps with in the past, then you would really want the + // signature to be the same as last time. + throw new InitException("Could not initialize local repo keystore: " + e); + } + } + if (!keyStoreFile.exists()) { + // If there isn't a persisted BKS keystore on disk we need to + // create a new empty keystore // Init a new keystore with a blank passphrase + Log.d(TAG, "Keystore doesn't exist, creating..."); keyStore.load(null, "".toCharArray()); - } else { - keyStore.load(new FileInputStream(keyStoreFile), "".toCharArray()); } /* @@ -113,17 +138,23 @@ public class LocalRepoKeyStore { wrappedKeyManager }; } catch (UnrecoverableKeyException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (KeyStoreException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (NoSuchAlgorithmException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (CertificateException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (OperatorCreationException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Error loading keystore: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } } @@ -144,7 +175,8 @@ public class LocalRepoKeyStore { FDroidApp.ipAddressString); addToStore(HTTP_CERT_ALIAS, kerplappKeypair, indexCert); } catch (Exception e) { - e.printStackTrace(); + Log.e(TAG, "Failed to setup HTTPS certificate: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } } @@ -174,21 +206,29 @@ public class LocalRepoKeyStore { zipSigner.signZip(input.getAbsolutePath(), output.getAbsolutePath()); } catch (ClassNotFoundException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (IllegalAccessException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (InstantiationException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (KeyStoreException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (UnrecoverableKeyException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (NoSuchAlgorithmException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (GeneralSecurityException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to sign local repo index: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } } @@ -217,11 +257,14 @@ public class LocalRepoKeyStore { if (key instanceof PrivateKey) return keyStore.getCertificate(INDEX_CERT_ALIAS); } catch (KeyStoreException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to get certificate for local repo: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (UnrecoverableKeyException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to get certificate for local repo: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } catch (NoSuchAlgorithmException e) { - e.printStackTrace(); + Log.e(TAG, "Unable to get certificate for local repo: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } return null; } diff --git a/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java b/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java index cdd9faa97..165842d65 100644 --- a/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java +++ b/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java @@ -185,7 +185,8 @@ public class LocalRepoManager { symlinkIndexPageElsewhere("../../", repoCAPS); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Error writing local repo index: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } } @@ -249,13 +250,16 @@ public class LocalRepoManager { PackageInfo packageInfo = pm.getPackageInfo(packageName, PackageManager.GET_META_DATA); app.icon = getIconFile(packageName, packageInfo.versionCode).getName(); } catch (NameNotFoundException e) { - e.printStackTrace(); + Log.e(TAG, "Error adding app to local repo: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); return; } catch (CertificateEncodingException e) { - e.printStackTrace(); + Log.e(TAG, "Error adding app to local repo: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); return; } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Error adding app to local repo: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); return; } Log.i(TAG, "apps.put: " + packageName); @@ -317,7 +321,7 @@ public class LocalRepoManager { // TODO this needs to be ported to < android-8 @TargetApi(8) - private void writeIndexXML() throws TransformerException, ParserConfigurationException { + private void writeIndexXML() throws TransformerException, ParserConfigurationException, LocalRepoKeyStore.InitException { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); DocumentBuilder builder = factory.newDocumentBuilder(); @@ -480,12 +484,11 @@ public class LocalRepoManager { writeIndexXML(); } catch (Exception e) { Toast.makeText(context, R.string.failed_to_create_index, Toast.LENGTH_LONG).show(); - e.printStackTrace(); + Log.e(TAG, Log.getStackTraceString(e)); return; } - BufferedOutputStream bo = new BufferedOutputStream( - new FileOutputStream(xmlIndexJarUnsigned)); + BufferedOutputStream bo = new BufferedOutputStream(new FileOutputStream(xmlIndexJarUnsigned)); JarOutputStream jo = new JarOutputStream(bo); BufferedInputStream bi = new BufferedInputStream(new FileInputStream(xmlIndex)); @@ -504,8 +507,13 @@ public class LocalRepoManager { jo.close(); bo.close(); - LocalRepoKeyStore.get(context).signZip(xmlIndexJarUnsigned, xmlIndexJar); + try { + LocalRepoKeyStore.get(context).signZip(xmlIndexJarUnsigned, xmlIndexJar); + } catch (LocalRepoKeyStore.InitException e) { + throw new IOException("Could not sign index - keystore failed to initialize"); + } finally { + xmlIndexJarUnsigned.delete(); + } - xmlIndexJarUnsigned.delete(); } } diff --git a/src/org/fdroid/fdroid/localrepo/LocalRepoService.java b/src/org/fdroid/fdroid/localrepo/LocalRepoService.java index a8ed9f59b..fa31666a0 100644 --- a/src/org/fdroid/fdroid/localrepo/LocalRepoService.java +++ b/src/org/fdroid/fdroid/localrepo/LocalRepoService.java @@ -23,6 +23,7 @@ import org.fdroid.fdroid.FDroidApp; import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.Preferences.ChangeListener; import org.fdroid.fdroid.R; +import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.net.LocalHTTPD; import org.fdroid.fdroid.net.WifiStateChangeService; import org.fdroid.fdroid.views.swap.SwapActivity; @@ -226,7 +227,8 @@ public class LocalRepoService extends Service { Log.w(TAG, "port " + prev + " occupied, trying on " + FDroidApp.port + "!"); startService(new Intent(LocalRepoService.this, WifiStateChangeService.class)); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Could not start local repo HTTP server: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } Looper.loop(); // start the message receiving loop } @@ -279,7 +281,8 @@ public class LocalRepoService extends Service { jmdns = JmDNS.create(); jmdns.registerService(pairService); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Error while registering jmdns service: " + e); + Log.e(TAG, Log.getStackTraceString(e)); } } }).start(); @@ -300,11 +303,7 @@ public class LocalRepoService extends Service { pairService = null; } jmdns.unregisterAllServices(); - try { - jmdns.close(); - } catch (IOException e) { - e.printStackTrace(); - } + Utils.closeQuietly(jmdns); jmdns = null; } } diff --git a/src/org/fdroid/fdroid/net/LocalHTTPD.java b/src/org/fdroid/fdroid/net/LocalHTTPD.java index 1eaa2e67d..b5fd420f0 100644 --- a/src/org/fdroid/fdroid/net/LocalHTTPD.java +++ b/src/org/fdroid/fdroid/net/LocalHTTPD.java @@ -92,7 +92,11 @@ public class LocalHTTPD extends NanoHTTPD { localRepoKeyStore.getKeyManagers()); makeSecure(factory); } catch (IOException e) { - e.printStackTrace(); + Log.e(TAG, "Could not enable HTTPS: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); + } catch (LocalRepoKeyStore.InitException e) { + Log.e(TAG, "Could not enable HTTPS: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); } } diff --git a/src/org/fdroid/fdroid/net/WifiStateChangeService.java b/src/org/fdroid/fdroid/net/WifiStateChangeService.java index 6b4fda481..8c48da60e 100644 --- a/src/org/fdroid/fdroid/net/WifiStateChangeService.java +++ b/src/org/fdroid/fdroid/net/WifiStateChangeService.java @@ -82,9 +82,6 @@ public class WifiStateChangeService extends Service { return null; Context context = WifiStateChangeService.this.getApplicationContext(); - LocalRepoKeyStore localRepoKeyStore = LocalRepoKeyStore.get(context); - Certificate localCert = localRepoKeyStore.getCertificate(); - FDroidApp.repo.fingerprint = Utils.calcFingerprint(localCert); LocalRepoManager lrm = LocalRepoManager.get(context); lrm.setUriString(FDroidApp.repo.address); lrm.writeIndexPage(Utils.getSharingUri(context, FDroidApp.repo).toString()); @@ -92,17 +89,28 @@ public class WifiStateChangeService extends Service { if (isCancelled()) return null; - /* - * Once the IP address is known we need to generate a self - * signed certificate to use for HTTPS that has a CN field set - * to the ipAddressString. This must be run in the background - * because if this is the first time the singleton is run, it - * can take a while to instantiate. - */ - if (Preferences.get().isLocalRepoHttpsEnabled()) - localRepoKeyStore.setupHTTPSCertificate(); + try { + LocalRepoKeyStore localRepoKeyStore = LocalRepoKeyStore.get(context); + Certificate localCert = localRepoKeyStore.getCertificate(); + FDroidApp.repo.fingerprint = Utils.calcFingerprint(localCert); + + /* + * Once the IP address is known we need to generate a self + * signed certificate to use for HTTPS that has a CN field set + * to the ipAddressString. This must be run in the background + * because if this is the first time the singleton is run, it + * can take a while to instantiate. + */ + if (Preferences.get().isLocalRepoHttpsEnabled()) + localRepoKeyStore.setupHTTPSCertificate(); + + } catch (LocalRepoKeyStore.InitException e) { + Log.e(TAG, "Unable to configure a fingerprint or HTTPS for the local repo: " + e.getMessage()); + Log.e(TAG, Log.getStackTraceString(e)); + } + } catch (InterruptedException e) { - e.printStackTrace(); + Log.e(TAG, Log.getStackTraceString(e)); } return null; }