Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:  closed
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:  fixed
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  dgoulet |Sponsor:
|  SponsorR-must
+--
Changes (by nickm):

 * status:  merge_ready => closed
 * resolution:   => fixed


Comment:

 Great; merged!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:
|  merge_ready
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  dgoulet |Sponsor:
|  SponsorR-must
+--
Changes (by dgoulet):

 * status:  needs_information => merge_ready


Comment:

 Replying to [comment:12 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?)
 >
 > 2. 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`

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:
|  needs_information
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  dgoulet |Sponsor:
|  SponsorR-must
+--
Changes (by jryans):

 * status:  needs_revision => needs_information


Comment:

 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?)

 2. 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.)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:
|  needs_revision
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  dgoulet |Sponsor:
|  SponsorR-must
+--
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 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 :).

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:
|  needs_review
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  dgoulet |Sponsor:
|  SponsorR-must
+--
Changes (by jryans):

 * status:  needs_revision => needs_review


Comment:

 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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:
|  needs_revision
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  dgoulet |Sponsor:
|  SponsorR-must
+--
Changes (by dgoulet):

 * reviewer:   => dgoulet


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:
|  needs_revision
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  |Sponsor:
|  SponsorR-must
+--
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 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 :).

 2. I see some extra changes that have nothing to do with the commit:

 {{{
 -_cert) < 0) {
 +_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!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:
|  needs_review
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  |Sponsor:
|  SponsorR-must
+--
Changes (by jryans):

 * status:  assigned => needs_review


Comment:

 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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:  jryans
 Type:  defect  | Status:  assigned
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  |Sponsor:
|  SponsorR-must
+--
Changes (by jryans):

 * owner:   => jryans
 * status:  new => assigned


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:
 Type:  defect  | Status:  new
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  |Sponsor:
|  SponsorR-must
+--

Comment (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`

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:
 Type:  defect  | Status:  new
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  |Sponsor:
|  SponsorR-must
+--
Changes (by jryans):

 * cc: jryans@… (added)


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:
 Type:  defect  | Status:  new
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  |Sponsor:
|  SponsorR-must
+--
Changes (by dgoulet):

 * keywords:  tor-hs, prop224, TorCoreTeam201611 => tor-hs, prop224,
 TorCoreTeam201612


Comment:

 Replying to [comment:2 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 for now, 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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
+--
 Reporter:  dgoulet |  Owner:
 Type:  defect  | Status:  new
 Priority:  High|  Milestone:  Tor:
|  0.3.0.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201611  |  Actual Points:
Parent ID:  | Points:  0.5
 Reviewer:  |Sponsor:
|  SponsorR-must
+--

Comment (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?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

[tor-bugs] #20572 [Core Tor/Tor]: hs: Remove the private key material from hs_descriptor.h

[Tor Bug Tracker & Wiki]

#20572: hs: Remove the private key material from hs_descriptor.h
---+
 Reporter:  dgoulet|  Owner:
 Type:  defect | Status:  new
 Priority:  High   |  Milestone:  Tor: 0.3.0.x-final
Component:  Core Tor/Tor   |Version:
 Severity:  Normal |   Keywords:  tor-hs, prop224
Actual Points: |  Parent ID:
   Points:  0.5|   Reviewer:
  Sponsor:  SponsorR-must  |
---+
 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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs