Opened 2 years ago

Closed 19 months ago

#24342 closed defect (fixed)

Various spec fixes to dir-spec, rend-spec-v3

Reported by: filippo Owned by: asn
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor Version:
Severity: Normal Keywords: tor-spec, review-group-26, review-group-27, 033-triage-20180320, fast-fix, 033-included-20180326
Cc: filippo Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There are a few commits fixing various things in some of the specs, ranging from implementation mismatches to wording here:

https://github.com/FiloSottile/torspec/compare/53b7dee30b1044ae401338a9ce4b6c76e1c431e1...ab22bd1dce3b62b6120300fdead958c6924fe553

The commit messages offer more rationale.

Child Tickets

Change History (20)

comment:1 Changed 2 years ago by filippo

Status: newneeds_review

comment:2 Changed 2 years ago by filippo

Cc: filippo added
Summary: Various spec fixesVarious spec fixes to dir-spec, rend-spec-v3

comment:3 Changed 2 years ago by nickm

Milestone: Tor: 0.3.3.x-final

comment:4 Changed 23 months ago by dgoulet

Concerning commit https://github.com/FiloSottile/torspec/commit/73f26437470e4b4b360a484daaa1ce94efad317f ... I wonder if we should actually patch that upstream in tor. It has been released in tor-0.3.2.1-alpha so not stable yet. We could just fix that mistake right now and be done with it?

comment:5 Changed 23 months ago by filippo

It would disrupt v3 HS across versions, but it wouldn't impact their addresses. I'm happy either way. Seems to be a decision above my pay grade.

comment:6 Changed 23 months ago by nickm

Keywords: review-group-26 added

Creating review-group-26.

comment:7 Changed 23 months ago by nickm

(IMO, let's not break the existing 0.3.2.x Tors for this; it's a harmless spec change.)

comment:8 Changed 23 months ago by teor

Status: needs_reviewmerge_ready

Seems ok to me.

nickm, would you mind double-checking that the protover change is removing a spec redundancy?

Then we can merge.

comment:9 Changed 23 months ago by asn

Status: merge_readyneeds_revision

Hello Filipo,

many thanks for the detailed and intricate spec fixes!!! I like most of the prop224 fixes and I think they are well needed.

One thing that I would like to improve: In 42e31d5, we are introducing the RH and RH'` variables (which IIUC correspond to the extended secret key of ed25519 which is already a confusing subject), but we are not defining what they are and how they are used.

I think we should probably motivate those two variables before introducing them in the spec because otherwise they complicate the spec without helping out the reader. I know this is not an easy task and it will probably make that section a bit more dirty, but I think it's The Right Thing to do since we are fixing that part of the spec.

I'm marking this as needs_revision for the above comment, but please let me know if you disagree or if I'm mistaken, and we can move it back to merge_ready.

Thanks!

comment:10 Changed 23 months ago by nickm

Keywords: review-group-27 added

comment:11 Changed 21 months ago by dgoulet

Owner: set to asn
Status: needs_revisionassigned

comment:12 Changed 19 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:13 Changed 19 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:14 Changed 19 months ago by dgoulet

Keywords: fast-fix added; 033-removed-20180320 removed

We have to get this upstream asap.

comment:15 Changed 19 months ago by nickm

Keywords: 033-included-20180326 added

comment:16 Changed 19 months ago by asn

Status: assignedmerge_ready

Marking this as merge_ready since it contains very useful fixes that should be merged ASAP.

I opened #25678 to resolve the issues from comment:9 in the future, since I tried to do it on the spot but clarifying how extended private keys are used in ed25519 (and in hierarchical key derivation schemes) is not an easy fix.

comment:17 Changed 19 months ago by nickm

What are we merging here? The master branch at ab22bd1dce3b62b6120300fdead958c6924fe553?

comment:18 in reply to:  17 Changed 19 months ago by asn

Replying to nickm:

What are we merging here? The master branch at ab22bd1dce3b62b6120300fdead958c6924fe553?

Yes indeed. The four commits at that master branch.

comment:19 Changed 19 months ago by nickm

ok; merged!

Please either close #25678 , or remove its "parent ID" field as appropriate, so we can close this one?

comment:20 Changed 19 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Done (unparented #25678).

Note: See TracTickets for help on using tickets.