Opened 2 months ago

Closed 7 weeks ago

#26870 closed defect (fixed)

Spec: clarify inconsistency for [V]PADDING/DROP cell content vs. padding bytes

Reported by: dmr Owned by: dmr
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec, prop295-assigned-sponsor-v
Cc: teor, arma, nickm, atagar Actual Points:
Parent ID: #26869 Points:
Reviewer: nickm Sponsor: SponsorV

Description

This ticket is split off from #26228.

Here's the relevant description from that ticket:
Replying to dmr:

[...]

Unclear areas

Here are the points that need clarification / specification:

  • [...]
  • spec is a bit inconsistent with PADDING cells [1][2]

[...]

Inconsistency: PADDING cell payload

(see bullet above)

These references highlight the inconsistency:

[1] PADDING: Payload is unused. per sec 3 "Cell Packet format".

implies 0 bytes of payload, so the rest should be padded per that section

[2] The contents of a PADDING, VPADDING, or DROP cell SHOULD be chosen randomly, and MUST be ignored. per sec 7.2 "Link padding".

implies the payload of a PADDING cell actually is the rest of the size of the cell, and that it SHOULD be chosen randomly

The PADDING cells were mentioned in IRC but not discussed.
I think a simple change to make the spec consistent between the two sections would be this:

PADDING: Payload contains random data. (See Sec 7.2)

However, given the other points here, is that correct?

Child Tickets

Change History (8)

comment:1 Changed 2 months ago by dmr

Without repeating content here, the following comments from #26228 are relevant to this ticket:

As of the time of writing, the patch mentioned in 26228#comment:7 may actually add inconsistency/confusion.

comment:2 Changed 2 months ago by dmr

Cc: atagar added
Owner: set to dmr
Status: newassigned

Assigning to self. I'm fine with tackling the rewording of this; won't get to it for a few weeks yet, probably.

comment:3 Changed 2 months ago by teor

Milestone: Tor: 0.3.5.x-final
Parent ID: #26228
Status: assignedneeds_review

I fixed this in #26228.

comment:4 in reply to:  3 Changed 2 months ago by dmr

Replying to teor:

I fixed this in #26228.

Now tracking all of #26228, #26870, #26871 in this pull request:
https://github.com/torproject/torspec/pull/28

EDIT: only the tor-spec part of #26871 is tracked here.

Last edited 2 months ago by dmr (previous) (diff)

comment:5 Changed 8 weeks ago by asn

Reviewer: nickm

comment:6 Changed 8 weeks ago by teor

Keywords: prop295-assigned-sponsor-v added
Parent ID: #26228#26869
Sponsor: SponsorV

Flatten the tree, and make it Sponsor V.

comment:7 Changed 7 weeks ago by nickm

I've squashed and merged the spec branch; please close this ticket if there is no more to do here.

comment:8 Changed 7 weeks ago by teor

Resolution: fixed
Status: needs_reviewclosed

Thanks, we'll deal with the implementation in #26871.

Note: See TracTickets for help on using tickets.