From c84a1ecfff8d3f21ac83ef7b041f22f11e38936b Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 5 Jun 2014 14:06:07 +0200 Subject: [PATCH 1/6] import-log: add parcelable prototype --- .../keychain/pgp/OperationResultParcel.java | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java new file mode 100644 index 000000000..497c3dd71 --- /dev/null +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java @@ -0,0 +1,123 @@ +package org.sufficientlysecure.keychain.pgp; + +import android.R; +import android.os.Parcel; +import android.os.Parcelable; + +import java.util.ArrayList; + +/** Represent the result of an operation. + * + * This class holds a result and the log of an operation. It can be subclassed + * to include typed additional information specific to the operation. To keep + * the class structure (somewhat) simple, this class contains an exhaustive + * list (ie, enum) of all possible log types, which should in all cases be tied + * to string resource ids. + * + */ +public class OperationResultParcel implements Parcelable { + /** Holds the overall result. A value of 0 is considered a success, all + * other values may represent failure or varying degrees of success. */ + final int mResult; + + /// A list of log entries tied to the operation result. + final ArrayList mLog; + + public OperationResultParcel(int result, ArrayList log) { + mResult = result; + mLog = log; + } + + public OperationResultParcel(Parcel source) { + mResult = source.readInt(); + mLog = source.createTypedArrayList(LogEntryParcel.CREATOR); + } + + public boolean isSuccessful() { + return mResult == 0; + } + + /** One entry in the log. */ + public static class LogEntryParcel implements Parcelable { + final LogType mType; + final LogLevel mLevel; + final String[] mParameters; + + public LogEntryParcel(LogType type, LogLevel level, String[] parameters) { + mType = type; + mLevel = level; + mParameters = parameters; + } + + public LogEntryParcel(Parcel source) { + mType = LogType.values()[source.readInt()]; + mLevel = LogLevel.values()[source.readInt()]; + mParameters = source.createStringArray(); + } + + @Override + public int describeContents() { + return 0; + } + + @Override + public void writeToParcel(Parcel dest, int flags) { + dest.writeInt(mType.ordinal()); + dest.writeInt(mLevel.ordinal()); + dest.writeStringArray(mParameters); + } + + public static final Creator CREATOR = new Creator() { + public LogEntryParcel createFromParcel(final Parcel source) { + return new LogEntryParcel(source); + } + + public LogEntryParcel[] newArray(final int size) { + return new LogEntryParcel[size]; + } + }; + + } + + public static enum LogType { + // TODO add actual log entry types here + MSG_IMPORT_OK (R.string.copy), + MSG_IMPORT_FAILED (R.string.cancel); + + private int mMsgId; + LogType(int msgId) { + mMsgId = msgId; + } + } + + /** Enumeration of possible log levels. */ + public static enum LogLevel { + DEBUG, + INFO, + WARN, + /** If any ERROR log entry is included in the result, the overall operation should have failed. */ + ERROR, + } + + @Override + public int describeContents() { + return 0; + } + + @Override + public void writeToParcel(Parcel dest, int flags) { + dest.writeInt(mResult); + dest.writeTypedList(mLog); + } + + public static final Creator CREATOR = new Creator() { + public OperationResultParcel createFromParcel(final Parcel source) { + return new OperationResultParcel(source); + } + + public OperationResultParcel[] newArray(final int size) { + return new OperationResultParcel[size]; + } + }; + +} From b995b836a38ef8a87aa189f408f8542bf5a2c94c Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 6 Jun 2014 16:14:15 +0200 Subject: [PATCH 2/6] import-log: improve operationresultparcel, add indentation --- .../keychain/pgp/OperationResultParcel.java | 20 +++++++++----- .../keychain/provider/ProviderHelper.java | 27 ++++++++++++++++--- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java index 497c3dd71..f2da4389d 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java @@ -1,9 +1,10 @@ package org.sufficientlysecure.keychain.pgp; -import android.R; import android.os.Parcel; import android.os.Parcelable; +import org.sufficientlysecure.keychain.R; + import java.util.ArrayList; /** Represent the result of an operation. @@ -39,20 +40,26 @@ public class OperationResultParcel implements Parcelable { /** One entry in the log. */ public static class LogEntryParcel implements Parcelable { - final LogType mType; final LogLevel mLevel; + final LogType mType; final String[] mParameters; + final int mIndent; - public LogEntryParcel(LogType type, LogLevel level, String[] parameters) { - mType = type; + public LogEntryParcel(LogLevel level, LogType type, String[] parameters, int indent) { mLevel = level; + mType = type; mParameters = parameters; + mIndent = indent; + } + public LogEntryParcel(LogLevel level, LogType type, String[] parameters) { + this(level, type, parameters, 0); } public LogEntryParcel(Parcel source) { - mType = LogType.values()[source.readInt()]; mLevel = LogLevel.values()[source.readInt()]; + mType = LogType.values()[source.readInt()]; mParameters = source.createStringArray(); + mIndent = source.readInt(); } @Override @@ -62,9 +69,10 @@ public class OperationResultParcel implements Parcelable { @Override public void writeToParcel(Parcel dest, int flags) { - dest.writeInt(mType.ordinal()); dest.writeInt(mLevel.ordinal()); + dest.writeInt(mType.ordinal()); dest.writeStringArray(mParameters); + dest.writeInt(mIndent); } public static final Creator CREATOR = new Creator() { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java index ca7e622bb..83b55cc14 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java @@ -29,6 +29,9 @@ import android.support.v4.util.LongSparseArray; import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.pgp.KeyRing; +import org.sufficientlysecure.keychain.pgp.OperationResultParcel; +import org.sufficientlysecure.keychain.pgp.OperationResultParcel.LogType; +import org.sufficientlysecure.keychain.pgp.OperationResultParcel.LogLevel; import org.sufficientlysecure.keychain.pgp.PgpHelper; import org.sufficientlysecure.keychain.pgp.PgpKeyHelper; import org.sufficientlysecure.keychain.pgp.UncachedKeyRing; @@ -59,12 +62,21 @@ import java.util.List; import java.util.Set; public class ProviderHelper { - private Context mContext; - private ContentResolver mContentResolver; + private final Context mContext; + private final ContentResolver mContentResolver; + private final ArrayList mLog; + private int mIndent; public ProviderHelper(Context context) { - this.mContext = context; - this.mContentResolver = context.getContentResolver(); + this(context, null, 0); + } + + public ProviderHelper(Context context, ArrayList log, + int indent) { + mContext = context; + mContentResolver = context.getContentResolver(); + mLog = log; + mIndent = indent; } public static class NotFoundException extends Exception { @@ -76,6 +88,13 @@ public class ProviderHelper { } } + public void log(LogLevel level, LogType type) { + mLog.add(new OperationResultParcel.LogEntryParcel(level, type, null, mIndent)); + } + public void log(LogLevel level, LogType type, String[] parameters) { + mLog.add(new OperationResultParcel.LogEntryParcel(level, type, parameters, mIndent)); + } + // If we ever switch to api level 11, we can ditch this whole mess! public static final int FIELD_TYPE_NULL = 1; // this is called integer to stay coherent with the constants in Cursor (api level 11) From 787f6edf3260056030025388b921e9d6ce996d92 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 6 Jun 2014 16:15:27 +0200 Subject: [PATCH 3/6] import-log: add log statements in import routine --- .../keychain/pgp/OperationResultParcel.java | 40 ++- .../keychain/provider/ProviderHelper.java | 282 ++++++++++++------ 2 files changed, 225 insertions(+), 97 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java index f2da4389d..8110590b1 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java @@ -88,9 +88,43 @@ public class OperationResultParcel implements Parcelable { } public static enum LogType { - // TODO add actual log entry types here - MSG_IMPORT_OK (R.string.copy), - MSG_IMPORT_FAILED (R.string.cancel); + MSG_IP_APPLY_BATCH (R.string.msg_ip_apply_batch), + MSG_IP_BAD_TYPE_SECRET (R.string.msg_ip_bad_type_secret), + MSG_IP_DELETE_OLD_FAIL (R.string.msg_ip_delete_old_fail), + MSG_IP_DELETE_OLD_OK (R.string.msg_ip_delete_old_ok), + MSG_IP_ENCODE_FAIL (R.string.msg_ip_encode_fail), + MSG_IP_FAIL_IO_EXC (R.string.msg_ip_fail_io_exc), + MSG_IP_FAIL_OP_EX (R.string.msg_ip_fail_op_ex), + MSG_IP_FAIL_REMOTE_EX (R.string.msg_ip_fail_remote_ex), + MSG_IP_IMPORTING (R.string.msg_ip_importing), + MSG_IP_INSERT_KEYRING (R.string.msg_ip_insert_keyring), + MSG_IP_INSERT_SUBKEY (R.string.msg_ip_insert_subkey), + MSG_IP_INSERT_SUBKEYS (R.string.msg_ip_insert_subkeys), + MSG_IP_PRESERVING_SECRET (R.string.msg_ip_preserving_secret), + MSG_IP_REINSERT_SECRET (R.string.msg_ip_reinsert_secret), + MSG_IP_SUCCESS (R.string.msg_ip_success), + MSG_IP_TRUST_RETRIEVE (R.string.msg_ip_trust_retrieve), + MSG_IP_TRUST_USING (R.string.msg_ip_trust_using), + MSG_IP_TRUST_USING_SEC (R.string.msg_ip_trust_using_sec), + MSG_IP_UID_CERT_BAD (R.string.msg_ip_uid_cert_bad), + MSG_IP_UID_CERT_ERROR (R.string.msg_ip_uid_cert_error), + MSG_IP_UID_CERT_GOOD (R.string.msg_ip_uid_cert_good), + MSG_IP_UID_CERTS_UNKNOWN (R.string.msg_ip_uid_certs_unknown), + MSG_IP_UID_CLASSIFYING (R.string.msg_ip_uid_classifying), + MSG_IP_UID_INSERT (R.string.msg_ip_uid_insert), + MSG_IP_UID_PROCESSING (R.string.msg_ip_uid_processing), + MSG_IP_UID_SELF_BAD (R.string.msg_ip_uid_self_bad), + MSG_IP_UID_SELF_GOOD (R.string.msg_ip_uid_self_good), + MSG_IP_UID_SELF_IGNORING_OLD (R.string.msg_ip_uid_self_ignoring_old), + MSG_IP_UID_SELF_NEWER (R.string.msg_ip_uid_self_newer), + MSG_IS_BAD_TYPE_PUBLIC (R.string.msg_is_bad_type_public), + MSG_IS_IMPORTING (R.string.msg_is_importing), + MSG_IS_IMPORTING_SUBKEYS (R.string.msg_is_importing_subkeys), + MSG_IS_IO_EXCPTION (R.string.msg_is_io_excption), + MSG_IS_SUBKEY_NONEXISTENT (R.string.msg_is_subkey_nonexistent), + MSG_IS_SUBKEY_OK (R.string.msg_is_subkey_ok), + MSG_IS_SUCCESS (R.string.msg_is_success), + ; private int mMsgId; LogType(int msgId) { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java index 83b55cc14..170fc4df2 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java @@ -241,136 +241,208 @@ public class ProviderHelper { * Saves PGPPublicKeyRing with its keys and userIds in DB */ @SuppressWarnings("unchecked") - public void savePublicKeyRing(UncachedKeyRing keyRing) throws IOException { + public OperationResultParcel savePublicKeyRing(UncachedKeyRing keyRing) { if (keyRing.isSecret()) { - throw new RuntimeException("Tried to save secret keyring as public! " + - "This is a bug, please file a bug report."); + log(LogLevel.ERROR, LogType.MSG_IP_BAD_TYPE_SECRET); + return new OperationResultParcel(1, mLog); } UncachedPublicKey masterKey = keyRing.getPublicKey(); long masterKeyId = masterKey.getKeyId(); + log(LogLevel.INFO, LogType.MSG_IP_IMPORTING, + new String[]{Long.toString(masterKeyId)}); // IF there is a secret key, preserve it! - UncachedKeyRing secretRing = null; + UncachedKeyRing secretRing; try { secretRing = getWrappedSecretKeyRing(masterKeyId).getUncached(); + log(LogLevel.DEBUG, LogType.MSG_IP_PRESERVING_SECRET); } catch (NotFoundException e) { - Log.e(Constants.TAG, "key not found!"); + secretRing = null; } // delete old version of this keyRing, which also deletes all keys and userIds on cascade try { mContentResolver.delete(KeyRingData.buildPublicKeyRingUri(Long.toString(masterKeyId)), null, null); + log(LogLevel.DEBUG, LogType.MSG_IP_DELETE_OLD_OK); } catch (UnsupportedOperationException e) { Log.e(Constants.TAG, "Key could not be deleted! Maybe we are creating a new one!", e); + log(LogLevel.DEBUG, LogType.MSG_IP_DELETE_OLD_FAIL); } // insert new version of this keyRing ContentValues values = new ContentValues(); values.put(KeyRingData.MASTER_KEY_ID, masterKeyId); - values.put(KeyRingData.KEY_RING_DATA, keyRing.getEncoded()); - Uri uri = KeyRingData.buildPublicKeyRingUri(Long.toString(masterKeyId)); - mContentResolver.insert(uri, values); + try { + values.put(KeyRingData.KEY_RING_DATA, keyRing.getEncoded()); + } catch (IOException e) { + log(LogLevel.ERROR, LogType.MSG_IP_ENCODE_FAIL); + return new OperationResultParcel(1, mLog); + } // save all keys and userIds included in keyRing object in database ArrayList operations = new ArrayList(); - int rank = 0; - for (UncachedPublicKey key : new IterableIterator(keyRing.getPublicKeys())) { - operations.add(buildPublicKeyOperations(masterKeyId, key, rank)); - ++rank; - } + try { - // get a list of owned secret keys, for verification filtering - LongSparseArray allKeyRings = - getUncachedMasterKeys(KeyRingData.buildSecretKeyRingUri()); - // special case: available secret keys verify themselves! - if (secretRing != null) { - allKeyRings.put(secretRing.getMasterKeyId(), secretRing.getPublicKey()); - } + log(LogLevel.INFO, LogType.MSG_IP_INSERT_KEYRING); + Uri uri = KeyRingData.buildPublicKeyRingUri(Long.toString(masterKeyId)); + operations.add(ContentProviderOperation.newInsert(uri).withValues(values).build()); - // classify and order user ids. primary are moved to the front, revoked to the back, - // otherwise the order in the keyfile is preserved. - List uids = new ArrayList(); + log(LogLevel.INFO, LogType.MSG_IP_INSERT_SUBKEYS); + mIndent += 1; + int rank = 0; + for (UncachedPublicKey key : new IterableIterator(keyRing.getPublicKeys())) { + log(LogLevel.DEBUG, LogType.MSG_IP_INSERT_SUBKEY, new String[] { + PgpKeyHelper.convertKeyIdToHex(masterKeyId) + }); + operations.add(buildPublicKeyOperations(masterKeyId, key, rank)); + ++rank; + } + mIndent -= 1; - for (String userId : new IterableIterator( - masterKey.getUnorderedUserIds().iterator())) { - UserIdItem item = new UserIdItem(); - uids.add(item); - item.userId = userId; + log(LogLevel.DEBUG, LogType.MSG_IP_TRUST_RETRIEVE); + // get a list of owned secret keys, for verification filtering + LongSparseArray trustedKeys = + getUncachedMasterKeys(KeyRingData.buildSecretKeyRingUri()); + // special case: available secret keys verify themselves! + if (secretRing != null) { + trustedKeys.put(secretRing.getMasterKeyId(), secretRing.getPublicKey()); + log(LogLevel.INFO, LogType.MSG_IP_TRUST_USING_SEC, new String[]{ + Integer.toString(trustedKeys.size()) + }); + } else { + log(LogLevel.INFO, LogType.MSG_IP_TRUST_USING, new String[] { + Integer.toString(trustedKeys.size()) + }); + } - // look through signatures for this specific key - for (WrappedSignature cert : new IterableIterator( - masterKey.getSignaturesForId(userId))) { - long certId = cert.getKeyId(); - try { - // self signature - if (certId == masterKeyId) { - cert.init(masterKey); - if (!cert.verifySignature(masterKey, userId)) { - // not verified?! dang! TODO notify user? this is kinda serious... - Log.e(Constants.TAG, "Could not verify self signature for " + userId + "!"); - continue; - } - // is this the first, or a more recent certificate? - if (item.selfCert == null || - item.selfCert.getCreationTime().before(cert.getCreationTime())) { + // classify and order user ids. primary are moved to the front, revoked to the back, + // otherwise the order in the keyfile is preserved. + log(LogLevel.DEBUG, LogType.MSG_IP_UID_CLASSIFYING); + mIndent += 1; + List uids = new ArrayList(); + for (String userId : new IterableIterator( + masterKey.getUnorderedUserIds().iterator())) { + UserIdItem item = new UserIdItem(); + uids.add(item); + item.userId = userId; + + int unknownCerts = 0; + + log(LogLevel.INFO, LogType.MSG_IP_UID_PROCESSING, new String[] { userId }); + mIndent += 1; + // look through signatures for this specific key + for (WrappedSignature cert : new IterableIterator( + masterKey.getSignaturesForId(userId))) { + long certId = cert.getKeyId(); + try { + // self signature + if (certId == masterKeyId) { + cert.init(masterKey); + if (!cert.verifySignature(masterKey, userId)) { + // Bad self certification? That's kinda bad... + log(LogLevel.ERROR, LogType.MSG_IP_UID_SELF_BAD); + return new OperationResultParcel(1, mLog); + } + + // if we already have a cert.. + if (item.selfCert != null) { + // ..is this perchance a more recent one? + if (item.selfCert.getCreationTime().before(cert.getCreationTime())) { + log(LogLevel.DEBUG, LogType.MSG_IP_UID_SELF_NEWER); + } else { + log(LogLevel.DEBUG, LogType.MSG_IP_UID_SELF_IGNORING_OLD); + continue; + } + } else { + log(LogLevel.DEBUG, LogType.MSG_IP_UID_SELF_GOOD); + } + + // save certificate as primary self-cert item.selfCert = cert; item.isPrimary = cert.isPrimaryUserId(); item.isRevoked = cert.isRevocation(); + } - } - // verify signatures from known private keys - if (allKeyRings.indexOfKey(certId) >= 0) { - cert.init(allKeyRings.get(certId)); - if (cert.verifySignature(masterKey, userId)) { - item.trustedCerts.add(cert); + + // verify signatures from known private keys + if (trustedKeys.indexOfKey(certId) >= 0) { + UncachedPublicKey trustedKey = trustedKeys.get(certId); + cert.init(trustedKey); + if (cert.verifySignature(masterKey, userId)) { + item.trustedCerts.add(cert); + log(LogLevel.INFO, LogType.MSG_IP_UID_CERT_GOOD, new String[] { + PgpKeyHelper.convertKeyIdToHex(trustedKey.getKeyId()) + }); + } else { + log(LogLevel.WARN, LogType.MSG_IP_UID_CERT_BAD); + } } + + unknownCerts += 1; + + } catch (PgpGeneralException e) { + log(LogLevel.WARN, LogType.MSG_IP_UID_CERT_ERROR, new String[]{ + PgpKeyHelper.convertKeyIdToHex(cert.getKeyId()) + }); } - } catch (PgpGeneralException e) { - Log.e(Constants.TAG, "Signature verification failed! " - + PgpKeyHelper.convertKeyIdToHex(masterKey.getKeyId()) - + " from " - + PgpKeyHelper.convertKeyIdToHex(cert.getKeyId()), e); + } + mIndent -= 1; + + if (unknownCerts > 0) { + log(LogLevel.DEBUG, LogType.MSG_IP_UID_CERTS_UNKNOWN, new String[] { + Integer.toString(unknownCerts) + }); + } + + } + mIndent -= 1; + + log(LogLevel.INFO, LogType.MSG_IP_UID_INSERT); + // primary before regular before revoked (see UserIdItem.compareTo) + // this is a stable sort, so the order of keys is otherwise preserved. + Collections.sort(uids); + // iterate and put into db + for (int userIdRank = 0; userIdRank < uids.size(); userIdRank++) { + UserIdItem item = uids.get(userIdRank); + operations.add(buildUserIdOperations(masterKeyId, item, userIdRank)); + // no self cert is bad, but allowed by the rfc... + if (item.selfCert != null) { + operations.add(buildCertOperations( + masterKeyId, userIdRank, item.selfCert, Certs.VERIFIED_SELF)); + } + // don't bother with trusted certs if the uid is revoked, anyways + if (item.isRevoked) { + continue; + } + for (int i = 0; i < item.trustedCerts.size(); i++) { + operations.add(buildCertOperations( + masterKeyId, userIdRank, item.trustedCerts.get(i), Certs.VERIFIED_SECRET)); } } - } - // primary before regular before revoked (see UserIdItem.compareTo) - // this is a stable sort, so the order of keys is otherwise preserved. - Collections.sort(uids); - // iterate and put into db - for (int userIdRank = 0; userIdRank < uids.size(); userIdRank++) { - UserIdItem item = uids.get(userIdRank); - operations.add(buildUserIdOperations(masterKeyId, item, userIdRank)); - // no self cert is bad, but allowed by the rfc... - if (item.selfCert != null) { - operations.add(buildCertOperations( - masterKeyId, userIdRank, item.selfCert, Certs.VERIFIED_SELF)); - } - // don't bother with trusted certs if the uid is revoked, anyways - if (item.isRevoked) { - continue; - } - for (int i = 0; i < item.trustedCerts.size(); i++) { - operations.add(buildCertOperations( - masterKeyId, userIdRank, item.trustedCerts.get(i), Certs.VERIFIED_SECRET)); - } - } - - try { + log(LogLevel.DEBUG, LogType.MSG_IP_APPLY_BATCH); mContentResolver.applyBatch(KeychainContract.CONTENT_AUTHORITY, operations); + } catch (IOException e) { + log(LogLevel.ERROR, LogType.MSG_IP_FAIL_IO_EXC); } catch (RemoteException e) { - Log.e(Constants.TAG, "applyBatch failed!", e); + log(LogLevel.ERROR, LogType.MSG_IP_FAIL_REMOTE_EX); } catch (OperationApplicationException e) { - Log.e(Constants.TAG, "applyBatch failed!", e); + log(LogLevel.ERROR, LogType.MSG_IP_FAIL_OP_EX); } // Save the saved keyring (if any) if (secretRing != null) { + log(LogLevel.DEBUG, LogType.MSG_IP_REINSERT_SECRET); + mIndent += 1; saveSecretKeyRing(secretRing); + mIndent -= 1; } + log(LogLevel.INFO, LogType.MSG_IP_SUCCESS); + return new OperationResultParcel(0, mLog); + } private static class UserIdItem implements Comparable { @@ -398,13 +470,29 @@ public class ProviderHelper { * Saves a PGPSecretKeyRing in the DB. This will only work if a corresponding public keyring * is already in the database! */ - public void saveSecretKeyRing(UncachedKeyRing keyRing) throws IOException { + public OperationResultParcel saveSecretKeyRing(UncachedKeyRing keyRing) { if (!keyRing.isSecret()) { - throw new RuntimeException("Tried to save publkc keyring as secret! " + - "This is a bug, please file a bug report."); + log(LogLevel.ERROR, LogType.MSG_IS_BAD_TYPE_PUBLIC); + return new OperationResultParcel(1, mLog); } long masterKeyId = keyRing.getMasterKeyId(); + log(LogLevel.INFO, LogType.MSG_IS_IMPORTING, + new String[]{ Long.toString(masterKeyId) }); + + // save secret keyring + try { + ContentValues values = new ContentValues(); + values.put(KeyRingData.MASTER_KEY_ID, masterKeyId); + values.put(KeyRingData.KEY_RING_DATA, keyRing.getEncoded()); + // insert new version of this keyRing + Uri uri = KeyRingData.buildSecretKeyRingUri(Long.toString(masterKeyId)); + mContentResolver.insert(uri, values); + } catch (IOException e) { + Log.e(Constants.TAG, "Failed to encode key!", e); + log(LogLevel.ERROR, LogType.MSG_IS_IO_EXCPTION); + return new OperationResultParcel(1, mLog); + } { Uri uri = Keys.buildKeysUri(Long.toString(masterKeyId)); @@ -416,24 +504,30 @@ public class ProviderHelper { values.put(Keys.HAS_SECRET, 1); // then, mark exactly the keys we have available + log(LogLevel.INFO, LogType.MSG_IS_IMPORTING_SUBKEYS); + mIndent += 1; for (Long sub : new IterableIterator(keyRing.getAvailableSubkeys().iterator())) { - mContentResolver.update(uri, values, Keys.KEY_ID + " = ?", new String[] { + int upd = mContentResolver.update(uri, values, Keys.KEY_ID + " = ?", new String[] { Long.toString(sub) }); + if(upd == 0) { + log(LogLevel.DEBUG, LogType.MSG_IS_SUBKEY_OK, new String[] { + PgpKeyHelper.convertKeyIdToHex(sub) + }); + } else { + log(LogLevel.WARN, LogType.MSG_IS_SUBKEY_NONEXISTENT, new String[] { + PgpKeyHelper.convertKeyIdToHex(sub) + }); + } } + mIndent -= 1; + // this implicitly leaves all keys which were not in the secret key ring // with has_secret = 0 } - // save secret keyring - { - ContentValues values = new ContentValues(); - values.put(KeyRingData.MASTER_KEY_ID, masterKeyId); - values.put(KeyRingData.KEY_RING_DATA, keyRing.getEncoded()); - // insert new version of this keyRing - Uri uri = KeyRingData.buildSecretKeyRingUri(Long.toString(masterKeyId)); - mContentResolver.insert(uri, values); - } + log(LogLevel.INFO, LogType.MSG_IS_SUCCESS); + return new OperationResultParcel(0, mLog); } From e083ccc37029bea5c130582756dc119920f577be Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 6 Jun 2014 16:15:45 +0200 Subject: [PATCH 4/6] import-log: add import log string resources --- OpenKeychain/src/main/res/values/strings.xml | 38 ++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index eeb6b3742..0755d9fb2 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -520,4 +520,42 @@ Encoding error No encryption subkey available! + + Applying insert batch operation. + Tried to import secret keyring as public. This is a bug, please file a report! + No old key deleted (creating a new one?) + Deleted old key from database + Operation failed due to encoding error + Operation failed due to i/o error + Operation failed due to database error + Operation failed due to internal error + Importing public keyring + Inserting keyring data + Inserting subkey %s + Inserting subkeys + Preserving available secret key + Re-inserting secret key + Successfully inserted secret keyring + Retrieving trusted keys + Using %s trusted keys + Secret key available, self certificates are trusted + Encountered bad certificate! + Error processing certificate! + Found good certificate from %s + Ignored %s certificates from unknown pubkeys + Classifying user ids + Inserting user ids + Processing user id %s + Bad self certificate encountered! + Found good self certificate + Ignoring older self certificate + Using more recent good self certificate + Tried to import public keyring as secret. This is a bug, please file a report! + Importing secret key %s + Processing secret subkeys + Error encoding keyring + Subkey %s unavailable in public key + Marked %s as available + Successfully inserted secret keyring + From 118225d7d2b6401006352897386a40131c6cd854 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 6 Jun 2014 17:28:36 +0200 Subject: [PATCH 5/6] import-log: add output to logcat (for debugging) --- .../keychain/pgp/OperationResultParcel.java | 5 ++++- .../keychain/pgp/PgpImportExport.java | 15 ++++++++------- .../keychain/provider/ProviderHelper.java | 7 +++++++ 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java index 8110590b1..ccb4b935c 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java @@ -126,10 +126,13 @@ public class OperationResultParcel implements Parcelable { MSG_IS_SUCCESS (R.string.msg_is_success), ; - private int mMsgId; + private final int mMsgId; LogType(int msgId) { mMsgId = msgId; } + public int getMsgId() { + return mMsgId; + } } /** Enumeration of possible log levels. */ diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpImportExport.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpImportExport.java index 14ec67e64..5ce0b11dd 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpImportExport.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpImportExport.java @@ -152,13 +152,14 @@ public class PgpImportExport { } } - mProviderHelper.savePublicKeyRing(key); - /*switch(status) { - case RETURN_UPDATED: oldKeys++; break; - case RETURN_OK: newKeys++; break; - case RETURN_BAD: badKeys++; break; - }*/ - // TODO proper import feedback + mProviderHelper.resetLog(); + OperationResultParcel result = mProviderHelper.savePublicKeyRing(key); + for(OperationResultParcel.LogEntryParcel loge : result.mLog) { + Log.d(Constants.TAG, + loge.mIndent + + new String(new char[loge.mIndent]).replace("\0", " ") + + mContext.getString(loge.mType.getMsgId(), (Object[]) loge.mParameters)); + } newKeys += 1; } catch (PgpGeneralException e) { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java index 170fc4df2..b3a08a063 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java @@ -79,6 +79,13 @@ public class ProviderHelper { mIndent = indent; } + public void resetLog() { + if(mLog != null) { + mLog.clear(); + mIndent = 0; + } + } + public static class NotFoundException extends Exception { public NotFoundException() { } From c36b311d5f5c2b43ec3f1549d9a059fb0de3bde2 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 6 Jun 2014 17:29:39 +0200 Subject: [PATCH 6/6] import-log: better stripped key logging --- .../keychain/pgp/OperationResultParcel.java | 1 + .../keychain/pgp/UncachedKeyRing.java | 5 +- .../keychain/provider/ProviderHelper.java | 51 ++++++++++++++----- OpenKeychain/src/main/res/values/strings.xml | 3 +- 4 files changed, 43 insertions(+), 17 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java index ccb4b935c..216e4b497 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OperationResultParcel.java @@ -123,6 +123,7 @@ public class OperationResultParcel implements Parcelable { MSG_IS_IO_EXCPTION (R.string.msg_is_io_excption), MSG_IS_SUBKEY_NONEXISTENT (R.string.msg_is_subkey_nonexistent), MSG_IS_SUBKEY_OK (R.string.msg_is_subkey_ok), + MSG_IS_SUBKEY_STRIPPED (R.string.msg_is_subkey_stripped), MSG_IS_SUCCESS (R.string.msg_is_success), ; diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java index 02e5411ca..1264c8c36 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -19,6 +19,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.ArrayList; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Vector; @@ -149,13 +150,13 @@ public class UncachedKeyRing { aos.close(); } - public ArrayList getAvailableSubkeys() { + public HashSet getAvailableSubkeys() { if(!isSecret()) { throw new RuntimeException("Tried to find available subkeys from non-secret keys. " + "This is a programming error and should never happen!"); } - ArrayList result = new ArrayList(); + HashSet result = new HashSet(); // then, mark exactly the keys we have available for (PGPSecretKey sub : new IterableIterator( ((PGPSecretKeyRing) mRing).getSecretKeys())) { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java index b3a08a063..5c8bf6752 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java @@ -68,7 +68,7 @@ public class ProviderHelper { private int mIndent; public ProviderHelper(Context context) { - this(context, null, 0); + this(context, new ArrayList(), 0); } public ProviderHelper(Context context, ArrayList log, @@ -96,10 +96,14 @@ public class ProviderHelper { } public void log(LogLevel level, LogType type) { - mLog.add(new OperationResultParcel.LogEntryParcel(level, type, null, mIndent)); + if(mLog != null) { + mLog.add(new OperationResultParcel.LogEntryParcel(level, type, null, mIndent)); + } } public void log(LogLevel level, LogType type, String[] parameters) { - mLog.add(new OperationResultParcel.LogEntryParcel(level, type, parameters, mIndent)); + if(mLog != null) { + mLog.add(new OperationResultParcel.LogEntryParcel(level, type, parameters, mIndent)); + } } // If we ever switch to api level 11, we can ditch this whole mess! @@ -258,6 +262,7 @@ public class ProviderHelper { long masterKeyId = masterKey.getKeyId(); log(LogLevel.INFO, LogType.MSG_IP_IMPORTING, new String[]{Long.toString(masterKeyId)}); + mIndent += 1; // IF there is a secret key, preserve it! UncachedKeyRing secretRing; @@ -301,7 +306,7 @@ public class ProviderHelper { int rank = 0; for (UncachedPublicKey key : new IterableIterator(keyRing.getPublicKeys())) { log(LogLevel.DEBUG, LogType.MSG_IP_INSERT_SUBKEY, new String[] { - PgpKeyHelper.convertKeyIdToHex(masterKeyId) + PgpKeyHelper.convertKeyIdToHex(key.getKeyId()) }); operations.add(buildPublicKeyOperations(masterKeyId, key, rank)); ++rank; @@ -433,10 +438,19 @@ public class ProviderHelper { mContentResolver.applyBatch(KeychainContract.CONTENT_AUTHORITY, operations); } catch (IOException e) { log(LogLevel.ERROR, LogType.MSG_IP_FAIL_IO_EXC); + Log.e(Constants.TAG, "IOException during import", e); + mIndent -= 1; + return new OperationResultParcel(1, mLog); } catch (RemoteException e) { log(LogLevel.ERROR, LogType.MSG_IP_FAIL_REMOTE_EX); + Log.e(Constants.TAG, "RemoteException during import", e); + mIndent -= 1; + return new OperationResultParcel(1, mLog); } catch (OperationApplicationException e) { log(LogLevel.ERROR, LogType.MSG_IP_FAIL_OP_EX); + Log.e(Constants.TAG, "OperationApplicationException during import", e); + mIndent -= 1; + return new OperationResultParcel(1, mLog); } // Save the saved keyring (if any) @@ -448,6 +462,7 @@ public class ProviderHelper { } log(LogLevel.INFO, LogType.MSG_IP_SUCCESS); + mIndent -= 1; return new OperationResultParcel(0, mLog); } @@ -513,17 +528,25 @@ public class ProviderHelper { // then, mark exactly the keys we have available log(LogLevel.INFO, LogType.MSG_IS_IMPORTING_SUBKEYS); mIndent += 1; - for (Long sub : new IterableIterator(keyRing.getAvailableSubkeys().iterator())) { - int upd = mContentResolver.update(uri, values, Keys.KEY_ID + " = ?", new String[] { - Long.toString(sub) - }); - if(upd == 0) { - log(LogLevel.DEBUG, LogType.MSG_IS_SUBKEY_OK, new String[] { - PgpKeyHelper.convertKeyIdToHex(sub) - }); + Set available = keyRing.getAvailableSubkeys(); + for (UncachedPublicKey sub : + new IterableIterator(keyRing.getPublicKeys())) { + long id = sub.getKeyId(); + if(available.contains(id)) { + int upd = mContentResolver.update(uri, values, Keys.KEY_ID + " = ?", + new String[] { Long.toString(id) }); + if (upd == 1) { + log(LogLevel.DEBUG, LogType.MSG_IS_SUBKEY_OK, new String[]{ + PgpKeyHelper.convertKeyIdToHex(id) + }); + } else { + log(LogLevel.WARN, LogType.MSG_IS_SUBKEY_NONEXISTENT, new String[]{ + PgpKeyHelper.convertKeyIdToHex(id) + }); + } } else { - log(LogLevel.WARN, LogType.MSG_IS_SUBKEY_NONEXISTENT, new String[] { - PgpKeyHelper.convertKeyIdToHex(sub) + log(LogLevel.INFO, LogType.MSG_IS_SUBKEY_STRIPPED, new String[]{ + PgpKeyHelper.convertKeyIdToHex(id) }); } } diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index 0755d9fb2..6f21900fe 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -535,7 +535,7 @@ Inserting subkeys Preserving available secret key Re-inserting secret key - Successfully inserted secret keyring + Successfully inserted public keyring Retrieving trusted keys Using %s trusted keys Secret key available, self certificates are trusted @@ -556,6 +556,7 @@ Error encoding keyring Subkey %s unavailable in public key Marked %s as available + Marked %s as stripped Successfully inserted secret keyring