From 21cc3d9176b31d47d642a3c424bb00d1966b9e4c Mon Sep 17 00:00:00 2001 From: Joe Steele Date: Mon, 21 Jul 2014 19:18:16 -0400 Subject: [PATCH] Remove ClientCertificateRequiredException With this commit, KeyChainKeyManager no longer throws the exception and AccountSetupCheckSettings no longer catches it. It was being thrown when the server requested a client certificate but no client certificate alias had been configured for the server. The code was making the incorrect assumption that the server would only request a client certificate when such a certificate was *required*. However, servers can be configured to accept multiple forms of authentication, including both password authentication and client certificate authentication. So a server may request a certificate without requiring it. If a user has not configured a client certificate, then that should not be treated as an error because the configuration may be valid and the server may accept it. The only indication that a certificate is *required* is when a SSLProtocolException is thrown, caused by a SSLHandshakeException resulting from a fatal handshake alert message received from the server. Unfortunately, such a message is fairly generic and only "indicates that the sender was unable to negotiate an acceptable set of security parameters given the options available." So there is no definitive way to know that a client certificate is required. Also, KeyChainKeyManager.getCertificateChain() and getPrivateKey() no longer throw IllegalStateException(). These methods are permitted to return null, and such a response is appropriate if the user has deleted client certificates from the device. Again, this may or may not cause the server to abort the connection, depending on whether the server *requires* a client certificate. --- res/values/strings.xml | 1 - .../setup/AccountSetupCheckSettings.java | 70 ----------------- .../ClientCertificateRequiredException.java | 48 ------------ src/com/fsck/k9/net/ssl/SslHelper.java | 12 +-- .../fsck/k9/security/KeyChainKeyManager.java | 78 ++----------------- 5 files changed, 9 insertions(+), 200 deletions(-) delete mode 100644 src/com/fsck/k9/mail/ClientCertificateRequiredException.java diff --git a/res/values/strings.xml b/res/values/strings.xml index 2177dd92a..b14762437 100644 --- a/res/values/strings.xml +++ b/res/values/strings.xml @@ -1124,7 +1124,6 @@ Please submit bug reports, contribute new features and ask questions at Use client certificate - This server requires a valid client certificate to be selected. No client certificate Remove client certificate selection diff --git a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java index f0ad53c54..ae9556fc8 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java @@ -25,14 +25,10 @@ import com.fsck.k9.fragment.ConfirmationDialogFragment; import com.fsck.k9.fragment.ConfirmationDialogFragment.ConfirmationDialogFragmentListener; import com.fsck.k9.mail.AuthenticationFailedException; import com.fsck.k9.mail.CertificateValidationException; -import com.fsck.k9.mail.ClientCertificateRequiredException; -import com.fsck.k9.mail.ServerSettings; import com.fsck.k9.mail.Store; import com.fsck.k9.mail.Transport; import com.fsck.k9.mail.store.WebDavStore; import com.fsck.k9.mail.filter.Hex; -import com.fsck.k9.security.KeyChainKeyManager; - import java.security.cert.CertificateException; import java.security.cert.CertificateEncodingException; import java.security.cert.X509Certificate; @@ -166,8 +162,6 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList afe.getMessage() == null ? "" : afe.getMessage()); } catch (final CertificateValidationException cve) { handleCertificateValidationException(cve); - } catch (final ClientCertificateRequiredException ccr) { - handleClientCertificateRequiredException(ccr); } catch (final Throwable t) { Log.e(K9.LOG_TAG, "Error while testing settings", t); showErrorDialog( @@ -196,70 +190,6 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList } } - private void handleClientCertificateRequiredException(ClientCertificateRequiredException ccr) { - if (K9.DEBUG) - Log.d(K9.LOG_TAG, "Client certificate alias required: " + ccr.getMessage()); - - String alias = null; - if (CheckDirection.INCOMING.equals(mDirection)) { - ServerSettings storeSettings = Store.decodeStoreUri(mAccount.getStoreUri()); - alias = storeSettings.clientCertificateAlias; - } else if (CheckDirection.OUTGOING.equals(mDirection)) { - ServerSettings transportSettings = Transport.decodeTransportUri(mAccount.getTransportUri()); - alias = transportSettings.clientCertificateAlias; - } - - if (K9.DEBUG) - Log.d(K9.LOG_TAG, "Client certificate alias is: " + alias); - - alias = KeyChainKeyManager.interactivelyChooseClientCertificateAlias( - AccountSetupCheckSettings.this, - ccr.getKeyTypes(), - ccr.getIssuers(), - ccr.getHostName(), - ccr.getPort(), - alias); - - // Note: KeyChainKeyManager gives back "" on cancel - if (alias != null && alias.equals("")) { - alias = null; - } - - // save client certificate alias - if (alias != null) { - if (CheckDirection.INCOMING.equals(mDirection)) { - if (K9.DEBUG) - Log.d(K9.LOG_TAG, "Setting store client certificate alias to: " + alias); - - // Set incoming server client certificate alias - String storeUri = mAccount.getStoreUri(); - ServerSettings incoming = Store.decodeStoreUri(storeUri); - ServerSettings newIncoming = incoming.newClientCertificateAlias(alias); - String newStoreUri = Store.createStoreUri(newIncoming); - mAccount.setStoreUri(newStoreUri); - } else if (CheckDirection.OUTGOING.equals(mDirection)) { - if (K9.DEBUG) - Log.d(K9.LOG_TAG, "Setting transport client certificate alias to: " + alias); - - // Set outgoing server client certificate alias - String transportUri = mAccount.getTransportUri(); - ServerSettings outgoing = Transport.decodeTransportUri(transportUri); - ServerSettings newOutgoing = outgoing.newClientCertificateAlias(alias); - String newTransportUri = Transport.createTransportUri(newOutgoing); - mAccount.setTransportUri(newTransportUri); - } - - // Save the account settings - mAccount.save(Preferences.getPreferences(AccountSetupCheckSettings.this)); - - // try again - AccountSetupCheckSettings.actionCheckSettings(AccountSetupCheckSettings.this, mAccount, - mDirection); - } else { - showErrorDialog(R.string.dialog_client_certificate_required); - } - } - @Override public void onDestroy() { super.onDestroy(); diff --git a/src/com/fsck/k9/mail/ClientCertificateRequiredException.java b/src/com/fsck/k9/mail/ClientCertificateRequiredException.java deleted file mode 100644 index 257352318..000000000 --- a/src/com/fsck/k9/mail/ClientCertificateRequiredException.java +++ /dev/null @@ -1,48 +0,0 @@ - -package com.fsck.k9.mail; - -import java.security.Principal; - -/** - * This exception is thrown when, during an SSL handshake, a client certificate - * alias is requested but we want the user to select one instead of using the - * previously selected one silently. This must be a RuntimeException because the - * implemented interface of X509ExtendedKeyManager (where it is thrown) does not - * allow anything else. - */ -public class ClientCertificateRequiredException extends RuntimeException { - public static final long serialVersionUID = -1; - - String[] mKeyTypes; - Principal[] mIssuers; - String mHostName; - int mPort; - - public ClientCertificateRequiredException(String[] keyTypes, - Principal[] issuers, - String hostName, - int port) { - super("interactive client certificate alias choice required"); - this.mKeyTypes = keyTypes; - this.mIssuers = issuers; - this.mHostName = hostName; - this.mPort = port; - } - - public String[] getKeyTypes() { - return mKeyTypes; - } - - public Principal[] getIssuers() { - return mIssuers; - } - - public String getHostName() { - return mHostName; - } - - public int getPort() { - return mPort; - } - -} diff --git a/src/com/fsck/k9/net/ssl/SslHelper.java b/src/com/fsck/k9/net/ssl/SslHelper.java index fbfc733db..12254c81c 100644 --- a/src/com/fsck/k9/net/ssl/SslHelper.java +++ b/src/com/fsck/k9/net/ssl/SslHelper.java @@ -29,16 +29,8 @@ public class SslHelper { Log.d(K9.LOG_TAG, "createSslContext: Client certificate alias: " + clientCertificateAlias); - KeyManager[] keyManagers = null; - if (clientCertificateAlias != null) { - keyManagers = new KeyManager[] { - new KeyChainKeyManager(clientCertificateAlias) - }; - } else { - keyManagers = new KeyManager[] { - new KeyChainKeyManager() - }; - } + KeyManager[] keyManagers = new KeyManager[] { new KeyChainKeyManager( + clientCertificateAlias) }; SSLContext sslContext = SSLContext.getInstance("TLS"); sslContext.init(keyManagers, diff --git a/src/com/fsck/k9/security/KeyChainKeyManager.java b/src/com/fsck/k9/security/KeyChainKeyManager.java index 67877886b..29bacdd58 100644 --- a/src/com/fsck/k9/security/KeyChainKeyManager.java +++ b/src/com/fsck/k9/security/KeyChainKeyManager.java @@ -5,23 +5,19 @@ import java.net.Socket; import java.security.Principal; import java.security.PrivateKey; import java.security.cert.X509Certificate; + import javax.net.ssl.X509ExtendedKeyManager; -import android.app.Activity; import android.os.Build; import android.security.KeyChain; -import android.security.KeyChainAliasCallback; import android.security.KeyChainException; import android.util.Log; import com.fsck.k9.K9; -import com.fsck.k9.mail.ClientCertificateRequiredException; /** * For client certificate authentication! Provide private keys and certificates - * during the TLS handshake using the Android 4.0 KeyChain API. If interactive - * selection is requested, we harvest the parameters during the handshake and - * abort with a custom (runtime) ClientCertificateRequiredException. + * during the TLS handshake using the Android 4.0 KeyChain API. */ public class KeyChainKeyManager extends X509ExtendedKeyManager { @@ -29,34 +25,16 @@ public class KeyChainKeyManager extends X509ExtendedKeyManager { private String mAlias; - public KeyChainKeyManager() { - mAlias = null; - if (K9.DEBUG) - Log.d(K9.LOG_TAG, "KeyChainKeyManager set to interactive prompting required"); - } - public KeyChainKeyManager(String alias) { if (alias == null || "".equals(alias)) { - throw new IllegalArgumentException( - "KeyChainKeyManager: The provided alias is null or empty!"); + mAlias = null; + } else { + mAlias = alias; } - - mAlias = alias; - if (K9.DEBUG) - Log.d(K9.LOG_TAG, "KeyChainKeyManager set up with for auto-selected alias " + alias); } @Override public String chooseClientAlias(String[] keyTypes, Principal[] issuers, Socket socket) { - if (mAlias == null) { - throw new ClientCertificateRequiredException(keyTypes, issuers, - socket.getInetAddress().getHostName(), socket.getPort()); - } - - if (K9.DEBUG) - Log.d(K9.LOG_TAG, "KeyChainKeyManager.chooseClientAlias returning preselected alias " - + mAlias); - return mAlias; } @@ -69,7 +47,7 @@ public class KeyChainKeyManager extends X509ExtendedKeyManager { X509Certificate[] chain = KeyChain.getCertificateChain(K9.app, alias); if (chain == null || chain.length == 0) { - throw new IllegalStateException("No certificate chain found for: " + alias); + Log.w(K9.LOG_TAG, "No certificate chain found for: " + alias); } return chain; @@ -103,7 +81,7 @@ public class KeyChainKeyManager extends X509ExtendedKeyManager { } if (key == null) { - throw new IllegalStateException("No private key found for: " + alias); + Log.w(K9.LOG_TAG, "No private key found for: " + alias); } return key; } catch (KeyChainException e) { @@ -140,46 +118,4 @@ public class KeyChainKeyManager extends X509ExtendedKeyManager { // not valid for client side throw new UnsupportedOperationException(); } - - public static String interactivelyChooseClientCertificateAlias(Activity activity, - String[] keyTypes, Principal[] issuers, String hostName, int port, - String preSelectedAlias) { - // defined as array to be able to set it inside the callback - final String[] selectedAlias = new String[1]; - - KeyChain.choosePrivateKeyAlias(activity, new KeyChainAliasCallback() { - @Override - public void alias(String alias) { - synchronized (selectedAlias) { - if (K9.DEBUG) - Log.d(K9.LOG_TAG, "User has selected client certificate alias:" + alias); - - // see below. not null is condition for breaking out of loop - if (alias == null) { - alias = ""; - } - selectedAlias[0] = alias; - selectedAlias.notifyAll(); - } - - } - }, keyTypes, issuers, hostName, port, preSelectedAlias); - - synchronized (selectedAlias) { - while (selectedAlias[0] == null) { - try { - selectedAlias.wait(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - break; - } - } - - if ("".equals(selectedAlias[0])) { - selectedAlias[0] = null; - } - } - - return selectedAlias[0]; - } }