Hi!
Before the user successfully logged in to an account, unfortunately the jid might not be known. He might eg. use a burner jid for example. Unfortunately the smack-omemo keystore uses the jid as an identifier to store keys, trust decisions etc.
Because connection.getUser() returnes null before the user is logged in, smack-omemo has a workaround in place to retrieve the username from the AbstractConnection, which never returns null, but instead the returned value might still differ from the jid the user ends up with.
All this eventually leads to multiple OmemoManager instances being created along with multiple key stores.
In order to fix this, I propose to change the way OmemoManager instances are identified:
Instead of the tuple (BareJid, deviceID), we use a single String (identifier), which is independent from BareJid and deviceId. The deviceId is - instead of being the primary key to the accounts store, also a value inside the store.
That way, we can instanciate OmemoManager instances by using OmemoManager.getInstanceFor(connection, identifier) at all times, even pre-authentication. We might recommend to construct the identifier as the concatenation of the bareJid and the deviceId (eg “foo@bar.tld-123456”), but that’s not necessary.
I already started to create a PR that addresses the issue in the way I proposed and it seams as if not too many changes are necessary. Unfortunately I don’t see a very clear way for existing instances to migrate though, but maybe we’ll figure something out
Thanks Paul. I’ll try to summarize my Point of View.
So the OmemoManager, or more precise the OmemoStore, needs a unique identifier under which it can store its state, most notably the (pending) trust decisions. The natural identification would be a tuple of (bare Jid, device ID).
In fact the is mostly what the current code does. Now the issue is that you can create an OmemoManager at runtime which doesn’t know the bare JID yet, because it has never been connected. In XMPP we only know the bare JID after being connected at least once with the credentials.
Therefore the OmemoManager can be in two phases:
When it doesn’t know the local bare JID
When it does know the bare JID
As soon as the manager transitioned from phase 1. to phase 2. it can never go back (that is assured by the design of Smack).
I believe we can keep our easy to use and Smack idiomatic OmemoManager API
(and the OMEMO corner case getInstanceFor(XMPPConnection connection, int deviceId))
without introducing an additional identifier.
As a first start I would forbid all operations requiring the bare JID while the manager is in phase 1, by making them throw a meaningful exception.
As a second step, we could implement a mechanism that memorizes the operations requiring the bare JID, while that bare JID is not know, and performs them as soon as the connection got authenticated for the first time. Yes, this requires some more effort and logic to put in the library, but that’s what a library is for: to abstract complexity away from the user.
Hm, that sounds rather complicated, but I might give it a try.
When I tried to solve the issue the way I described above, I found that it might be ugly to test, whether the store is a “fresh” installation. Maybe we cut out the “fresh” logic completely and just generate missing keys on the fly? That will make it easier to debug things, since you can just delete unwanted keys without destroying the whole key store, while simultaneously making it more resistant against crashes due to missing keys.
On the other hand it might introduce some unforeseen security risks though, so what do you think?
I thought about it a little and came to the conclusion, that we’ll have to throw an exception for nearly every operation the OmemoManager offers.
The user wont even be capable of creating OMEMO keys in advance, since those are stored in the store, which is again identified by the jid. At this point I’m thinking, that it would probably make most sense to change the signature of getInstanceFor(connection, (deviceId)) to getInstanceFor(connection, barejid, (deviceId)). Sure, thats not the most elegant solution, but the easiest. I think client developers will be able to handle that. This would also allow the usage of OMEMO with persistent keys in the context of burner jids, which might be interesting.
Additionally we could let getInstanceFor(connection) and getInstanceFor(connection, deviceId) spaghetthi to getInstanceFor(connection, connection.getUser(), (deviceId)), which in turn throws an exception in case the user is not logged in → when getUser() is unreliable.
We want an easy to use API. The API design you propose makes it possible to use the method with incompatible arguments, which we can’t detect at invocation time in some cases
The only option I see is to go with the “throw an exception” for now, and to eventually make it transparent ot the user which requires, of course, a little bit of engineering effort, but that’s what libraries are for
The store is identified via the BareJid + deviceId
The connection does not have access to the BareJid when it is not authenticated
Most if not all usage scenarios depend on the store -> are only avail. when online
In order to solve the issue, while avoiding to make the API more complicated and error prone (both for users and devs), we need an identifier, which is available at all times during the lifetime of the connection.
As a workaround, I originally got access to the user-name, thinking it was the bareJid in String form. In hindsight I believe this is just the username the user uses to authenticate with the server.
Since this is most likely a stable value, which will not change during the lifetime of the account, another proposal would be to replace the BareJid in the tuple (BareJid, deviceId) with username + “@” + servicename. The identifier tuple would then be (username + “@” + servicename, deviceId).
We would of course have to make sure to use this identifier only to get access to the store, not for other operations.
I know this sounds a little sketchy too, but it would allow offline usage, which is a pretty big thing for me, considering smack-omemo will most likely end up being used in mobile applications.
That is correct, the “username” passed to and used by the SASL layer is technically not related to the localpart of the JID you get after authentication. Although it is often the same, we can not assure that, as otherwise it would break for the installations where this isn’t the case.
There are authentication mechanism, like SASL EXTERNAL, which don’t have a username. How do you deal with them?
That sounds like one can’t enable offline usage with my approach, which, I think, is not true.
My point is: If you want to allow offline usage, then you have to implement some merge logic which merges the decisions made locally on the device while it was offline, which the decisions that where made by other devices on the user's server state. And that logic is the complicated part. If you have it, keeping the `getInstanceFor()` method single parameterized is easy.
Wait: OmemoStore stores the Ratchet State and Trust Decissions?
It stores basically every type of persistent data required for OMEMO.
I was thinking about replacing the deviceId by a global identifier, but that again does not solve the issue that occurs when the user tries to use one store with a wrong jid . This whole thing is really tricky .
The current development goes in the following direction:
The only actions which require the BareJid of the user WHILE BEING OFFLINE are related to trust management.
This includes changing trust in identities, querying trust and showing fingerprints etc.
As a conclusion, we will strip trust related code out of the OmemoStore. Clients will have to store and manage trust information themselves. smack-omemo will provide a callback to gather trust information when needed.
The OmemoManager will have two states. When it is offline it does not have access to the OmemoStore (which now solely stores keys and the ratchet). The client should provide the OmemoManager with the TrustCallback, so that offline access to trust-related information is available.
As soon as the client goes online, the OmemoManager will be able to get its BareJid. This is then used to get access to the OmemoStore and its keys.
Operations requiring access to the OmemoStore will throw exceptions while the OmemoManager is in the offline state.
One usecase to cover is the following:
The user might want to display fingerprints while offline. For that purpose it needs access to the store (in theory). Flow suggested to have a method OmemoManager.setDefaultKeyFor(BareJid, Key) which can be used to give the OmemoManager access to the identityKey while offline.
One problem I see is, that the OmemoManager doesn’t know the type of the IdentityKey, so this method might be moved to the OmemoService class, which makes it a little intransparent in my opinion.
Also, what happens, with the provided key, when the client goes online? What if it doesn’t match the key in the store?
Otherwise, I figured that we can reuse most of the OmemoManagers API (eg. trustOmemoIdentity() can just be forwarded to the trustcallback), so upgrading might not be as much of a deal as I originally thought.
. This whole thing is really tricky 
.