Opened 2 weeks ago

Last modified 25 hours ago

#31589 needs_review defect

hs-v3: Simplify decrypt_desc_layer interface

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs easy refactoring tech-debt
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:

Description

Here is how decrypt_desc_layer is called:

  superencrypted_len = decrypt_desc_layer(desc,
                                 desc->plaintext_data.superencrypted_blob,
                                 desc->plaintext_data.superencrypted_blob_size,
                                 NULL, 1, &superencrypted_plaintext);
  encrypted_len = decrypt_desc_layer(desc,
                                 desc->superencrypted_data.encrypted_blob,
                                 desc->superencrypted_data.encrypted_blob_size,
                                 descriptor_cookie, 0, &encrypted_plaintext);

There is no point in passing desc->superencrypted_data.encrypted_blob and desc->superencrypted_data.encrypted_blob_size since we are already passing the whole desc and is_superencrypted_layer which should be enough to figure out which fields to use.

We could either of the following two things:

  • Ditch desc as an argument and pass desc->plaintext_data.blinded_pubkey explicitly.
  • Ditch encrypted_blob and encrypted_blob_size as arguments and get them off desc.

I prefer the first, but I'm fine with either, since it will make the interface cleaner.

Child Tickets

Change History (9)

comment:1 Changed 2 weeks ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:2 Changed 2 weeks ago by neel

Cc: neel removed
Owner: neel deleted

comment:3 Changed 2 weeks ago by neel

Status: assignednew

comment:4 Changed 8 days ago by ltbringer

desc is passed down to build_secret_key_iv_mac -> build_kdf_key -> build_secret_input.

That means, either we:

  1. Remove desc and provide desc->plaintext_data.blinded_pubkey, desc->subcredential and desc->plaintext_data.revision_counter
  2. Ditch encrypted_blob and encrypted_blob_size as arguments and get them off desc. (same as you suggest)

My limited experience says, it would be neater to just pass desc but I would like to know why that wasn't your preference as either solutions are easy.

comment:6 Changed 8 days ago by dgoulet

Status: newneeds_review

comment:7 in reply to:  4 Changed 8 days ago by asn

Replying to ltbringer:

desc is passed down to build_secret_key_iv_mac -> build_kdf_key -> build_secret_input.

That means, either we:

  1. Remove desc and provide desc->plaintext_data.blinded_pubkey, desc->subcredential and desc->plaintext_data.revision_counter
  2. Ditch encrypted_blob and encrypted_blob_size as arguments and get them off desc. (same as you suggest)

My limited experience says, it would be neater to just pass desc but I would like to know why that wasn't your preference as either solutions are easy.

Hello, the reason I suggested the other approach is to be more explicit on what that function needs, in the sense where if a reader reads the function prototype they understand exactly what the function needs instead of the amorphous desc which could be anything. That said, you are right that this would add 3 more arguments to the function...

I haven't checked your patch yet, FWIW.

comment:8 Changed 35 hours ago by asn

Reviewer: asn

comment:9 Changed 25 hours ago by teor

Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Note: See TracTickets for help on using tickets.