Opened 6 weeks ago

Closed 3 weeks ago

#31589 closed defect (implemented)

hs-v3: Simplify decrypt_desc_layer interface

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.4.2.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 (18)

comment:1 Changed 6 weeks ago by neel

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

comment:2 Changed 6 weeks ago by neel

Cc: neel removed
Owner: neel deleted

comment:3 Changed 6 weeks ago by neel

Status: assignednew

comment:4 Changed 5 weeks 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 5 weeks ago by dgoulet

Status: newneeds_review

comment:7 in reply to:  4 Changed 5 weeks 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 4 weeks ago by asn

Reviewer: asn

comment:9 Changed 4 weeks ago by teor

Milestone: Tor: unspecifiedTor: 0.4.3.x-final

comment:10 Changed 4 weeks ago by asn

Status: needs_reviewneeds_revision

Patch looks good to me. Thanks for the code!

Two small things:

comment:11 Changed 4 weeks ago by ltbringer

I'll do both of these things and push the code.

comment:12 Changed 4 weeks ago by ltbringer

I have converted the int to a bool and added the changes file.

comment:13 Changed 3 weeks ago by asn

Status: needs_revisionneeds_review

comment:14 Changed 3 weeks ago by asn

Status: needs_reviewneeds_revision

Looks good to me. Almost there. Two more things:

  • Can you please squash all these commits into a single commit with a nice commit messge?
  • Please add the subsystem (onion services) to the type of change in the changes file.

comment:15 Changed 3 weeks ago by ltbringer

Status: needs_revisionneeds_review

Pushed with a sqaush-rebase and subtype added to changes/ticket31589

comment:16 Changed 3 weeks ago by asn

Status: needs_reviewmerge_ready

Usually we want a descriptive commit message that describes what happened: for example "hs-v3: Simplify decrypt_desc_layer interface".

In any case, I think this is good enough for now. Thanks for the code!

comment:17 Changed 3 weeks ago by nickm

Merged!

comment:18 Changed 3 weeks ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.2.x-final
Resolution: implemented
Status: merge_readyclosed

Oh shoot. I figured this was supposed to go into 0.4.2 since it is pure refactoring. Oh well; I hope it doesn't break anything :/

Note: See TracTickets for help on using tickets.