Opened 3 years ago

Closed 3 years ago

#20572 closed defect (fixed)

hs: Remove the private key material from hs_descriptor.h

Reported by: dgoulet Owned by: jryans
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, TorCoreTeam201612
Cc: jryans@… Actual Points:
Parent ID: Points: 0.5
Reviewer: dgoulet Sponsor: SponsorR-must

Description

Basically, we need to remove the private keys from some data structure in hs_descriptor.h. Namely:

    ed25519_keypair_t signing_kp;
    curve25519_keypair_t curve25519;

Those private keys should be in a dedicated data structure on the service side which we don't have yet so only put public keys there.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by dgoulet

Keywords: TorCoreTeam201611 added

comment:2 Changed 3 years ago by jryans

I'd like to make an attempt here, assuming that's okay.

It appears the private key from signing_kp is used to sign the descriptor in desc_encode_v3, so if the private key is removed from hs_desc_plaintext_data_t, is there a specific place it should be moved so that signing can still happen?

comment:3 in reply to:  2 Changed 3 years ago by dgoulet

Keywords: TorCoreTeam201612 added; TorCoreTeam201611 removed

Replying to jryans:

I'd like to make an attempt here, assuming that's okay.

Great thanks for this jryans!

It appears the private key from signing_kp is used to sign the descriptor in desc_encode_v3, so if the private key is removed from hs_desc_plaintext_data_t, is there a specific place it should be moved so that signing can still happen?

No, the secret key material will go in data structure that are being designed/implemented (from #20657) so they should be removed from the current HS descriptor code and it should only manage public key material.

Last edited 3 years ago by dgoulet (previous) (diff)

comment:4 Changed 3 years ago by jryans

Cc: jryans@… added

comment:5 Changed 3 years ago by jryans

From talking more to dgoulet, we believe that the cases of private keys to remove are:

  • signing_kp in hs_desc_plaintext_data_t
  • blinded_kp in hs_desc_plaintext_data_t

while on the other hand, the following are okay since they are known on the client side:

  • curve25519 in hs_desc_intro_point_t

comment:6 Changed 3 years ago by jryans

Owner: set to jryans
Status: newassigned

comment:7 Changed 3 years ago by jryans

Status: assignedneeds_review

Branch available at https://github.com/jryans/tor/commits/hs-rm-private-key.

hs_desc_encode_descriptor takes a new arg for the signing keypair so signing the descriptor can still be done (from tests, etc). Not sure if it's okay to expose ed25519_keypair_t at this level, since it could be v3 specific, but perhaps that can be revised in follow up work.

comment:8 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

Comments:

  1. We only use a changes file if the patch is a feature or actually fixing something that has been released. In this case, we don't need one. I'm not sure if we documented that somewhere but at least here it is :).
  1. I see some extra changes that have nothing to do with the commit:
-                    &encoded_cert) < 0) {
+                                &encoded_cert) < 0) {

or

+  version = desc->plaintext_data.version;
+  if (!hs_desc_is_supported_version(version)) {

It's good stuff! and fine for now but next time, simply put them in different commit which would be great!

So basically, if you can resubmit an extra fixup commit in the branch to remove the change file it would be great and ready to go! (git commit --fixup=). THANKS!

comment:9 Changed 3 years ago by dgoulet

Reviewer: dgoulet

comment:10 Changed 3 years ago by jryans

Status: needs_revisionneeds_review

Thanks for the clarification about the changes file! I filed #20932 to update CodingStandards.md to mention that the changes file isn't needed with unreleased work.

Good to know about keeping style changes in separate commits. I'll be more vigilant in the future! :)

I added an additional fixup commit as requested to remove the changes file.

comment:11 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

Looks good!

So I kind of fucked it up and I'm sorry about that. curve25519_keypair_t curve25519 should actually be _only_ the public key (curve25519_public_key_t) :S

Only the public key is published in the descriptor and only that public key is used by the client for encryption so the private key is a service specific key material. It shouldn't be difficult to change. What I suggest is that you pass a curve25519 keypair to the right function for encoding which means you'll have to change 3 or 4 functions signature to bring that keypair up to the right place. (as a fixup commit)

With the service implementation (#20657), the hs_desc_encode_descriptor() function will probably change to take a high level structure for "key material" from which we'll be able to handle the versioning much cleaner but for now this is fine.

Let me know if you don't have the time to do it, I'll just take it from your hands, no worries :).

comment:12 Changed 3 years ago by jryans

Status: needs_revisionneeds_information

Okay, I am happy to take a look at this as well! I have a few questions:

  1. Since curve25519 is part of hs_desc_intro_point_t and the descriptor can have a variable number of intro points, should hs_desc_encode_descriptor() be passed a list of keypairs, one for each intro point? (Would it be better to create the higher level structure for key material here instead of waiting for #20657?)
  1. It seems like the legacy path (using crypto_pk_t *legacy;) also contains a private key. Should that also be cleaned up as well?

As a meta-question, I think I would normally add a separate regular commit to the branch (not a fixup) for this additional work, since it feels like an independent task and less like correcting an error noticed during review. Is that okay? (Still trying to get a feel for the desired Tor patch workflow, sorry for the mechanical questions.)

comment:13 in reply to:  12 Changed 3 years ago by dgoulet

Status: needs_informationmerge_ready

Replying to jryans:

Okay, I am happy to take a look at this as well! I have a few questions:

  1. Since curve25519 is part of hs_desc_intro_point_t and the descriptor can have a variable number of intro points, should hs_desc_encode_descriptor() be passed a list of keypairs, one for each intro point? (Would it be better to create the higher level structure for key material here instead of waiting for #20657?)
  1. It seems like the legacy path (using crypto_pk_t *legacy;) also contains a private key. Should that also be cleaned up as well?

Ok indeed, that is another problem. I've just spoke with jryans on IRC and basically we'll delay this change for the IPs in the service implementation (#20657). HSDir do not care about that section as it's encrypted. I've opened #21008 about this.

As a meta-question, I think I would normally add a separate regular commit to the branch (not a fixup) for this additional work, since it feels like an independent task and less like correcting an error noticed during review. Is that okay? (Still trying to get a feel for the desired Tor patch workflow, sorry for the mechanical questions.)

Yes that's perfectly fine!

I've reviewed and autosquash jryans branch in with minor addition to the commit message: bug20572_030_01

comment:14 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Great; merged!

Note: See TracTickets for help on using tickets.