Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consider device_id when dealing with remote identity keys #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Consider device_id when dealing with remote identity keys #15

wants to merge 3 commits into from

Conversation

root-hardenedvault
Copy link
Contributor

No description provided.

@root-hardenedvault
Consider device_id when dealing with remote identity keys.
1b59878 
As signal protocol suggests, different clients under the same account are
distinguished by their device_id, so what should be uniquely stored and
queried should be the tuple of (addr_p->name, addr_p->device_id, identity_key),
not (addr_p->name, identity_key).

Signed-off-by: HardenedVault <root@hardenedvault.net>
@root-hardenedvault
Apply static storage class for semantic constants
845ca2a 
Auto variables must be initialized at every time the housing code blocks
({}) are called, even qualified with "const", so if semantic constants are
needed, they had better be stored statically, i.e. declared as global
"const" or "static const" in blocks.

By the way, it is better to initialize those provider structures with
global constant templates.

Signed-off-by: HardenedVault <root@hardenedvault.net>
@gkdr
Copy link
Owner

gkdr commented Dec 28, 2019

Thank you for your contribution! Changing the constants to static to prevent them from being re-initialized all the time sounds like a good idea. I don't understand how it is related to the title of the pull request, though. Is there something I don't see?
As for the device IDs - as far as I remember, there is only one identity key per account, which might even also apply to the (signed) pre keys. The device ID column only exists in the session store because that's when you need it. If you can point me to the part of the Signal Protocol spec you're referring to I could read for myself if I remember it right.
(In OMEMO, each device does in fact have its own identity key. I don't remember exactly, but I probably used the whole account+device_id "address" as the name. Your solution is probably more elegant anyway, I just need to think for a bit how much code I'd have to write to migrate the data to the new format.)

@root-hardenedvault
Copy link
Contributor Author

Hey gkdr,

Changing the constants to static to prevent them from being re-initialized all the time sounds like a good idea. I don't understand how it is related to the title of the pull request, though. Is there something I don't see?

The main purpose of this pull request is to correct the logic associated with device_id, so as its title. The transformation of semantic constant is a secondary purpose.

As for the device IDs - as far as I remember, there is only one identity key per account, which might even also apply to the (signed) pre keys.

https://signal.org/docs/specifications/sesame/#device-state

“Each device stores an identity key pair (a public key and private key) for cryptographic authentication. A device will always have the same DeviceID and identity key pair (to change these for some physical device the device must be logically deleted and then added with new values). Sesame supports two different models for key pairs: With per-user identity keys, all devices under a user share the same key pair. With per-device identity keys, each device may have a different key pair.With per-user identity keys, identity public keys for other devices are stored in UserRecords. With per-device identity keys, identity public keys for other devices are stored in DeviceRecords.”

Signal protocol allows "per-user identity keys" and "per-device identity keys" to be one of the two, so axc don 't have to assume "there is only one identity key per account".

In OMEMO, each device does in fact have its own identity key.

By contrast, this means that OMEMO uses "per-device identity keys", so that axc should implement it as a part of an implementation of OMEMO.

@gkdr
Copy link
Owner

gkdr commented Feb 16, 2020

The main purpose of this pull request is to correct the logic associated with device_id, so as its title. The transformation of semantic constant is a secondary purpose

I see.

https://signal.org/docs/specifications/sesame/#device-state

I see the cause of my confusion - when I started working on this (and that was after OMEMO was proposed and implemented), there was no "Sesame" specification (in fact, there weren't any specs and I was digging through their sourcecode). That's probably also the reason OMEMO describes some own ways to deal with (some of) the mentioned failure modes.
I believe the per-device keys used as a "hack" in OMEMO because of XMPP specifics were only considered later in the document you linked.

[...] OMEMO uses "per-device identity keys", so that axc should implement it as a part of an implementation of OMEMO.

You're right! I'll check how it affects my implementation so far and then merge your changes at some point. I'm sorry in advance that this might take a while.and thanks again for your contribution.

@Neustradamus
Copy link

@root-hardenedvault: I see a lot of PRs from you not merged, what is the status of this?

@gkdr
Copy link
Owner

gkdr commented Feb 12, 2021

as you can tell by the last comment, it's on me. it's a good PR with some thought put into it, and i did not have the time to check the implications on the existing code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@root-hardenedvault @gkdr @Neustradamus