From ebe262015a53bbe8ecb6abf9093b66f314ba2272 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Mon, 15 Jan 2018 18:16:48 +0100 Subject: [PATCH] Change Autocrypt logic to more closely match the spec --- .../operations/results/SaveKeyringResult.java | 8 +- .../AutocryptPeerDataAccessObject.java | 17 +++ .../keychain/provider/KeychainProvider.java | 35 ++--- .../keychain/remote/AutocryptInteractor.java | 136 +++++++++++------- .../keychain/remote/OpenPgpService.java | 8 +- 5 files changed, 118 insertions(+), 86 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/SaveKeyringResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/SaveKeyringResult.java index bdb1ce66d..8b41a52f0 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/SaveKeyringResult.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/SaveKeyringResult.java @@ -24,12 +24,12 @@ import org.sufficientlysecure.keychain.pgp.CanonicalizedKeyRing; public class SaveKeyringResult extends OperationResult { - public final long mRingMasterKeyId; + public final Long savedMasterKeyId; public SaveKeyringResult(int result, OperationLog log, CanonicalizedKeyRing ring) { super(result, log); - mRingMasterKeyId = ring != null ? ring.getMasterKeyId() : Constants.key.none; + savedMasterKeyId = ring != null ? ring.getMasterKeyId() : null; } // Some old key was updated @@ -46,13 +46,13 @@ public class SaveKeyringResult extends OperationResult { public SaveKeyringResult(Parcel source) { super(source); - mRingMasterKeyId = source.readLong(); + savedMasterKeyId = source.readLong(); } @Override public void writeToParcel(Parcel dest, int flags) { super.writeToParcel(dest, flags); - dest.writeLong(mRingMasterKeyId); + dest.writeLong(savedMasterKeyId); } public static Creator CREATOR = new Creator() { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/AutocryptPeerDataAccessObject.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/AutocryptPeerDataAccessObject.java index 76895496b..7fa579894 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/AutocryptPeerDataAccessObject.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/AutocryptPeerDataAccessObject.java @@ -153,6 +153,23 @@ public class AutocryptPeerDataAccessObject { return null; } + public Date getLastSeenGossip(String autocryptId) { + Cursor cursor = queryInterface.query(ApiAutocryptPeer.buildByPackageNameAndAutocryptId(packageName, autocryptId), + null, null, null, null); + + try { + if (cursor != null && cursor.moveToFirst()) { + long lastUpdated = cursor.getColumnIndex(ApiAutocryptPeer.GOSSIP_LAST_SEEN_KEY); + return new Date(lastUpdated); + } + } finally { + if (cursor != null) { + cursor.close(); + } + } + return null; + } + public void updateLastSeen(String autocryptId, Date date) { ContentValues cv = new ContentValues(); cv.put(ApiAutocryptPeer.LAST_SEEN, date.getTime()); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainProvider.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainProvider.java index d8d83eef3..d303eb0ca 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainProvider.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainProvider.java @@ -1114,42 +1114,25 @@ public class KeychainProvider extends ContentProvider { break; } case AUTOCRYPT_PEERS_BY_PACKAGE_NAME_AND_TRUST_ID: { - ContentValues actualValues = new ContentValues(); String packageName = uri.getPathSegments().get(2); - actualValues.put(ApiAutocryptPeer.PACKAGE_NAME, packageName); - actualValues.put(ApiAutocryptPeer.IDENTIFIER, uri.getLastPathSegment()); + String identifier = uri.getLastPathSegment(); + values.put(ApiAutocryptPeer.PACKAGE_NAME, packageName); + values.put(ApiAutocryptPeer.IDENTIFIER, identifier); - Long newLastSeen = values.getAsLong(ApiAutocryptPeer.LAST_SEEN); - if (newLastSeen != null) { - actualValues.put(ApiAutocryptPeer.LAST_SEEN, newLastSeen); - db.replace(Tables.API_AUTOCRYPT_PEERS, null, actualValues); - break; + int updated = db.update(Tables.API_AUTOCRYPT_PEERS, values, + ApiAutocryptPeer.PACKAGE_NAME + "=? AND " + ApiAutocryptPeer.IDENTIFIER + "=?", + new String[] { packageName, identifier }); + if (updated == 0) { + db.insertOrThrow(Tables.API_AUTOCRYPT_PEERS, null, values); } Long masterKeyId = values.getAsLong(ApiAutocryptPeer.MASTER_KEY_ID); if (masterKeyId != null) { - Long lastSeenKey = values.getAsLong(ApiAutocryptPeer.LAST_SEEN_KEY); - Long preferEncryptState = values.getAsLong(ApiAutocryptPeer.IS_MUTUAL); - - actualValues.put(ApiAutocryptPeer.MASTER_KEY_ID, masterKeyId); - actualValues.put(ApiAutocryptPeer.LAST_SEEN_KEY, lastSeenKey); - actualValues.put(ApiAutocryptPeer.IS_MUTUAL, preferEncryptState); - - db.replace(Tables.API_AUTOCRYPT_PEERS, null, actualValues); contentResolver.notifyChange(KeyRings.buildGenericKeyRingUri(masterKeyId), null); - break; } - Long gossipMasterKeyId = values.getAsLong(ApiAutocryptPeer.GOSSIP_MASTER_KEY_ID); - if (gossipMasterKeyId != null) { - Long gossipLastSeenKey = values.getAsLong(ApiAutocryptPeer.GOSSIP_LAST_SEEN_KEY); - - actualValues.put(ApiAutocryptPeer.GOSSIP_MASTER_KEY_ID, gossipMasterKeyId); - actualValues.put(ApiAutocryptPeer.GOSSIP_LAST_SEEN_KEY, gossipLastSeenKey); - - db.replace(Tables.API_AUTOCRYPT_PEERS, null, actualValues); + if (gossipMasterKeyId != null && !gossipMasterKeyId.equals(masterKeyId)) { contentResolver.notifyChange(KeyRings.buildGenericKeyRingUri(gossipMasterKeyId), null); - break; } break; diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/AutocryptInteractor.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/AutocryptInteractor.java index bfdea532e..0d56522f3 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/AutocryptInteractor.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/AutocryptInteractor.java @@ -5,15 +5,17 @@ import java.io.IOException; import java.util.Date; import android.content.Context; +import android.support.annotation.Nullable; import org.openintents.openpgp.AutocryptPeerUpdate; import org.openintents.openpgp.AutocryptPeerUpdate.PreferEncrypt; import org.sufficientlysecure.keychain.Constants; +import org.sufficientlysecure.keychain.operations.results.SaveKeyringResult; import org.sufficientlysecure.keychain.pgp.UncachedKeyRing; import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; import org.sufficientlysecure.keychain.provider.AutocryptPeerDataAccessObject; import org.sufficientlysecure.keychain.provider.KeyWritableRepository; -import org.sufficientlysecure.keychain.util.Log; +import timber.log.Timber; class AutocryptInteractor { @@ -33,74 +35,100 @@ class AutocryptInteractor { this.keyWritableRepository = keyWritableRepository; } - void updateAutocryptPeerState(String autocryptPeerId, AutocryptPeerUpdate autocryptPeerUpdate) - throws PgpGeneralException, IOException { - if (autocryptPeerUpdate == null) { - return; - } - - Long newMasterKeyId; - if (autocryptPeerUpdate.hasKeyData()) { - UncachedKeyRing uncachedKeyRing = UncachedKeyRing.decodeFromData(autocryptPeerUpdate.getKeyData()); - if (uncachedKeyRing.isSecret()) { - Log.e(Constants.TAG, "Found secret key in autocrypt id! - Ignoring"); - return; - } - // this will merge if the key already exists - no worries! - keyWritableRepository.savePublicKeyRing(uncachedKeyRing); - newMasterKeyId = uncachedKeyRing.getMasterKeyId(); - } else { - newMasterKeyId = null; - } - + void updateAutocryptPeerState(String autocryptPeerId, AutocryptPeerUpdate autocryptPeerUpdate) { Date effectiveDate = autocryptPeerUpdate.getEffectiveDate(); - autocryptPeerDao.updateLastSeen(autocryptPeerId, effectiveDate); - if (newMasterKeyId == null) { + // 1. If the message’s effective date is older than the peers[from-addr].autocrypt_timestamp value, then no changes are required, and the update process terminates. + Date lastSeenAutocrypt = autocryptPeerDao.getLastSeenKey(autocryptPeerId); + if (lastSeenAutocrypt != null && lastSeenAutocrypt.after(effectiveDate)) { return; } - Date lastSeenKey = autocryptPeerDao.getLastSeenKey(autocryptPeerId); - if (lastSeenKey == null || effectiveDate.after(lastSeenKey)) { - boolean isMutual = autocryptPeerUpdate.getPreferEncrypt() == PreferEncrypt.MUTUAL; - autocryptPeerDao.updateKey(autocryptPeerId, effectiveDate, newMasterKeyId, isMutual); + // 2. If the message’s effective date is more recent than peers[from-addr].last_seen then set peers[from-addr].last_seen to the message’s effective date. + Date lastSeen = autocryptPeerDao.getLastSeen(autocryptPeerId); + if (lastSeen == null || lastSeen.after(effectiveDate)) { + autocryptPeerDao.updateLastSeen(autocryptPeerId, effectiveDate); } + + // 3. If the Autocrypt header is unavailable, no further changes are required and the update process terminates. + if (!autocryptPeerUpdate.hasKeyData()) { + return; + } + + SaveKeyringResult saveKeyringResult = parseAndImportAutocryptKeyData(autocryptPeerUpdate); + if (saveKeyringResult == null) { + return; + } + + // 4. Set peers[from-addr].autocrypt_timestamp to the message’s effective date. + // 5. Set peers[from-addr].public_key to the corresponding keydata value of the Autocrypt header. + Long newMasterKeyId = saveKeyringResult.savedMasterKeyId; + // 6. Set peers[from-addr].prefer_encrypt to the corresponding prefer-encrypt value of the Autocrypt header. + boolean isMutual = autocryptPeerUpdate.getPreferEncrypt() == PreferEncrypt.MUTUAL; + + autocryptPeerDao.updateKey(autocryptPeerId, effectiveDate, newMasterKeyId, isMutual); } - - void updateAutocryptPeerGossipState(String autocryptPeerId, AutocryptPeerUpdate autocryptPeerUpdate) - throws PgpGeneralException, IOException { - if (autocryptPeerUpdate == null) { - return; - } - - Long newMasterKeyId; - if (autocryptPeerUpdate.hasKeyData()) { - UncachedKeyRing uncachedKeyRing = UncachedKeyRing.decodeFromData(autocryptPeerUpdate.getKeyData()); - if (uncachedKeyRing.isSecret()) { - Log.e(Constants.TAG, "Found secret key in autocrypt id! - Ignoring"); - return; - } - // this will merge if the key already exists - no worries! - keyWritableRepository.savePublicKeyRing(uncachedKeyRing); - newMasterKeyId = uncachedKeyRing.getMasterKeyId(); - } else { - newMasterKeyId = null; - } - - Date lastSeen = autocryptPeerDao.getLastSeen(autocryptPeerId); + void updateAutocryptPeerGossipState(String autocryptPeerId, AutocryptPeerUpdate autocryptPeerUpdate) { Date effectiveDate = autocryptPeerUpdate.getEffectiveDate(); - if (newMasterKeyId == null) { + + // 1. If gossip-addr does not match any recipient in the mail’s To or Cc header, the update process terminates (i.e., header is ignored). + // -> This should be taken care of in the mail client that sends us this data! + + // 2. If peers[gossip-addr].gossip_timestamp is more recent than the message’s effective date, then the update process terminates. + Date lastSeenGossip = autocryptPeerDao.getLastSeenGossip(autocryptPeerId); + if (lastSeenGossip != null && lastSeenGossip.after(effectiveDate)) { return; } - Date lastSeenKey = autocryptPeerDao.getLastSeenKey(autocryptPeerId); - if (lastSeenKey != null && effectiveDate.before(lastSeenKey)) { + if (!autocryptPeerUpdate.hasKeyData()) { return; } - if (lastSeen == null || effectiveDate.after(lastSeen)) { - autocryptPeerDao.updateKeyGossip(autocryptPeerId, effectiveDate, newMasterKeyId); + SaveKeyringResult saveKeyringResult = parseAndImportAutocryptKeyData(autocryptPeerUpdate); + if (saveKeyringResult == null) { + return; } + + // 3. Set peers[gossip-addr].gossip_timestamp to the message’s effective date. + // 4. Set peers[gossip-addr].gossip_key to the value of the keydata attribute. + Long newMasterKeyId = saveKeyringResult.savedMasterKeyId; + + autocryptPeerDao.updateKeyGossip(autocryptPeerId, effectiveDate, newMasterKeyId); + } + + @Nullable + private SaveKeyringResult parseAndImportAutocryptKeyData(AutocryptPeerUpdate autocryptPeerUpdate) { + UncachedKeyRing uncachedKeyRing = parseAutocryptKeyData(autocryptPeerUpdate); + if (uncachedKeyRing != null) { + return importAutocryptKeyData(uncachedKeyRing); + } + return null; + } + + @Nullable + private SaveKeyringResult importAutocryptKeyData(UncachedKeyRing uncachedKeyRing) { + SaveKeyringResult saveKeyringResult = keyWritableRepository.savePublicKeyRing(uncachedKeyRing); + if (!saveKeyringResult.success()) { + Timber.e(Constants.TAG, "Error inserting key - ignoring!"); + return null; + } + return saveKeyringResult; + } + + @Nullable + private UncachedKeyRing parseAutocryptKeyData(AutocryptPeerUpdate autocryptPeerUpdate) { + UncachedKeyRing uncachedKeyRing; + try { + uncachedKeyRing = UncachedKeyRing.decodeFromData(autocryptPeerUpdate.getKeyData()); + } catch (IOException | PgpGeneralException e) { + Timber.e(Constants.TAG, "Error parsing public key! - Ignoring"); + return null; + } + if (uncachedKeyRing.isSecret()) { + Timber.e(Constants.TAG, "Found secret key in autocrypt id! - Ignoring"); + return null; + } + return uncachedKeyRing; } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java index 75c52a1da..9550f161b 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java @@ -762,7 +762,9 @@ public class OpenPgpService extends Service { String autocryptPeerId = data.getStringExtra(OpenPgpApi.EXTRA_AUTOCRYPT_PEER_ID); AutocryptPeerUpdate autocryptPeerUpdate = data.getParcelableExtra(OpenPgpApi.EXTRA_AUTOCRYPT_PEER_UPDATE); - autocryptInteractor.updateAutocryptPeerState(autocryptPeerId, autocryptPeerUpdate); + if (autocryptPeerUpdate != null) { + autocryptInteractor.updateAutocryptPeerState(autocryptPeerId, autocryptPeerUpdate); + } } if (data.hasExtra(OpenPgpApi.EXTRA_AUTOCRYPT_PEER_GOSSIP_UPDATES)) { @@ -770,7 +772,9 @@ public class OpenPgpService extends Service { for (String address : updates.keySet()) { Timber.d(Constants.TAG, "Updating gossip state: " + address); AutocryptPeerUpdate update = updates.getParcelable(address); - autocryptInteractor.updateAutocryptPeerGossipState(address, update); + if (update != null) { + autocryptInteractor.updateAutocryptPeerGossipState(address, update); + } } }