Opened 7 years ago

Last modified 6 years ago

#6572 new defect

Replace key with hash to reduce avahi TXT size

Reported by: morgs Owned by: Collabora
Priority: normal Milestone: 9.1.0-cancelled
Component: presence-service Version:
Keywords: 9.1.0:? Cc: Collabora, mstone, jg, cscott, marcopg
Blocked By: Blocking:
Deployments affected: Action Needed:
Verified: no

Description

Morgan Collett wrote:

Ivan Krstić wrote:

On Feb 26, 2008, at 2:49 AM, Sjoerd Simons wrote:

Does sugar make any assumptions about the size of the key? IOW can we
instead of removing the key completely, use a smaller key?

As I've told daf, smcv et al many months ago in Boston, there's no point
in advertising the whole key. Advertising a digest is enough as long as
nodes support an on-demand operation that returns their whole key.

As long as Sugar gets a unique value as the key, nothing will break (for
very short term changes for testing...) so you could just replace the
key in the TXT record with its hash.

This patch for Presence Service replaces the public key with its sha1 hash.

I tested in jhbuild on salut and sharing Chat does work, and avahi-discover shows the shorter (40 byte) value for key.

Attachments (3)

ps_key_hash.diff (620 bytes) - added by morgs 7 years ago.
Updated patch with b64 encoding and SHA-256.
ps_key_hash256.diff (1.0 KB) - added by morgs 7 years ago.
SHA-256 with no b64 encoding. Down to 32 bytes.
6572_sugar_small_key.diff (648 bytes) - added by morgs 7 years ago.

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by morgs

Updated patch with b64 encoding and SHA-256.

comment:1 Changed 7 years ago by smcv

Still -1 from me. The 'key' property is defined to be a byte-array (D-Bus 'ay'), Salut puts it into mDNS as-is, mDNS is defined to be 8-bit-clean, and the mDNS draft explicitly says "don't bother Base64ing things".

Changed 7 years ago by morgs

SHA-256 with no b64 encoding. Down to 32 bytes.

comment:2 Changed 7 years ago by morgs

Updated patch attached. Without base64 encoding there may be side effects in gabble mode (sync_friends for instance) - still testing but this patch should be suitable for testing salut.

comment:3 Changed 7 years ago by daf

Hmm, not sure what we should do about sync_friends. Will base64 encoding it in the presence service before comparing it to the file on disk work? Can you look into this Morgan?

comment:4 Changed 7 years ago by morgs

Not base64 encoding the "key" makes current builds fail to get the buddy properties due to a D-Bus issue that needs a workaround. This means you don't see any buddies in mesh view, although they can see you.

So if we stop base64 encoding, we lose interoperability with the existing builds and need a flag day to upgrade. This is to save 12 bytes.

Is that worth it?

comment:5 Changed 7 years ago by morgs

Jim Gettys wrote:

OK, put like this, I agree with you. We can burn 12 bytes to save a
flag day conversion; we will already have saved a ton of bytes as it is;
12 bytes is no more than a 10% improvement on top....

The patch to consider is therefore my earlier one, ps_key_hash.diff

comment:6 Changed 7 years ago by morgs

I've tested this patch (ps_key_hash.diff) by applying to build 695 and verified that it works, and does interop with an XO without the patch. The key is base64 encoded, and 44 bytes.

comment:7 Changed 7 years ago by morgs

  • Keywords review? added

comment:8 Changed 7 years ago by morgs

The current patch has a regression in friending, but it's going into koji as sugar-presence-service-0.75.3 for joyride, for the performance testing.

I'll fix the friending soon.

comment:9 Changed 7 years ago by morgs

The current patch is in Joyride 1741, although it breaks deriving someone's JID from their key. This breaks friending.

comment:10 Changed 7 years ago by cscott

What's the status? If friending isn't fixed soon, this isn't going to make update.1.

comment:11 Changed 7 years ago by morgs

This is an experimental change, and fixing friending, or alternatively making this change only for salut and not gabble, will be more intrusive. PS depends on the key as the only true identifier for buddies. The exact value of the key isn't significant, but we derive the JID from the sha1 hexdigest of the base64 of the public key, and there's no way to derive it from the base64 sha256 non-hexdigest of the key.

Therefore, this patch should be seen as a testing tool, to validate the assumption that we can make the mesh significantly better by reducing the key payload in avahi. If we cannot test that now, or the gain is too low to consider the risk of changing something fundamental in PS, then we need to defer this.

Changed 7 years ago by morgs

comment:12 Changed 7 years ago by morgs

  • Cc Collabora mstone jg cscott marcopg added

And now for something completely different: Here is an alternative approach to reducing the key size in a consistent way that doesn't break friending: Replace the actual data in owner.key.pub - see 6572_sugar_small_key.diff. This changes the value at profile creating time - first boot.

Since PS identifies all buddies by key, the previous approach of substituting the key for a hash (or whatever) somewhere in the stack wasn't feasible as it broke the JIDs. (You calculate your own JID as a hash of owner.key.pub, others calculate your JID has a hash of the different thing you are sending out as your "key".) Fixing that required very intrusive changes to PS. I know that shipping the key around is unnecessary at this point, but since we will have a crypto design at some point, that's the proper time to make changes to PS.

With this patch, there are no changes needed to PS (except that sync_friends never worked but I'll file that separately).

The only other user of the key is Etoys, which (before Rainbow) used the key to sign projects and sandbox unsigned projects. With Rainbow on, it has no access to the key, and so does not sign or sandbox.

Comments?

comment:13 Changed 7 years ago by Blaketh

  • Keywords release? added

comment:14 Changed 7 years ago by daf

This will work as long as nothing uses the public key, but as soon as we do want to use it, we'll need the unhashed public key. Perhaps we should save the hash in a separate file and have Sugar read that?

comment:15 Changed 7 years ago by morgs

Seeing as there was no feedback on this in time for Update.1, we should do a proper redesign for Update.2 as we can be more intrusive.

Daf, any feedback from the meeting with Jonathan Herzog? Is the way PS is designed, using the key to identify buddies, the best approach (for bolting on crypto) - or should we change that?

comment:16 Changed 6 years ago by marco

  • Keywords 9.1.0? added; review? release? removed
  • Milestone changed from Never Assigned to 9.1.0

comment:17 Changed 6 years ago by marco

  • Keywords 9.1.0:? added; 9.1.0? removed
Note: See TracTickets for help on using tickets.