Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16235 closed defect (fixed)

identity-ed25519 is undocumented

Reported by: atagar Owned by: nickm
Priority: High Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: TorCoreTeam201508
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

On reflection suspect you'd rather #16227 to be specifically about documenting future expansion. Spec 228 doesn't include a proposal for what would be added to the dir-spec. As such controllers like Stem, metrics-lib, and BridgeDB don't have a clear description on how they should be parsing this new content.

Happy to review it when we have a proposed spec addition.

Child Tickets

Change History (12)

comment:1 Changed 5 years ago by atagar

Should the priority of this be bumped? Proposal 228 was incomplete, and if this wasn't an important security improvement I'd argue we should be rolling it back until that's fixed.

Karsten is moving forward with descriptor sanitation. The following was my reply to his request for comment...

Hi Karsten. My thought is that the existing ED stuff doesn't have a
specification, I don't know what it does, and think that should be
fixed before we worry about how it's sanitized. The spec was
incomplete and personally think we should either prioritize that or
roll back the feature. But maybe that's impractical. I already voiced
this earlier so feel free to proceed as you'd like. You have my
complete confidence. :P

Documenting (or rolling back) this feature should be easy so I'm surprised this is still an issue.

Apologies if this ruffles feathers, but personally I think the proposal process for this was mishandled. We shouldn't be merging major features that lack a spec, then leave them undocumented for a full month afterward. :(

comment:2 Changed 5 years ago by atagar

On reflection think I was looking at the wrong proposal. Proposal 220 discusses the descriptor fields, though it also lists itself as being a draft (not merged) so still tad confused about the state of things. :)

comment:3 Changed 5 years ago by atagar

Priority: normalmajor

Ok, think I'm gonna be a madman and bump the severity on this. Unless I'm missing something we merged a major feature and have left it completely out of the spec for months. I know we're all kept busy but this feels like a miss, and a tad frustrated it's been six weeks of hearing crickets.

comment:4 Changed 5 years ago by atagar

Summary: Proposal 228 needs spec documentationidentity-ed25519 is undocumented

Tweaking the ticket description to be more precise about what I care about. I'm waiting for this before Stem will recognize the new descriptor fields. Presently there's 45 server descriptors with the new fields...

% ./tor-prompt
>>> from stem.descriptor import remote
>>> downloader = remote.DescriptorDownloader()
>>> query = downloader.get_server_descriptors()
>>> results = query.run()
>>> for desc in results:
...   if desc.get_unrecognized_lines():
...     print desc.get_unrecognized_lines()
[u'identity-ed25519 \n-----BEGIN ED25519 CERT-----\nAQQABhpQARSbXrElkqVNjtI8JDPBdghiviwQmwoygc/rdTcnqCHiAQAgBACVQEQ/\nWeqTHXR+mXH/aCVJ9uZqUIpu3s0AuMwDm7Ki9jqXpHNbZfrPR3qaQhCM5I660RFM\nfCHYKQ1+DHJyJoh6s4nStl0JPkq+U5WEQfUlAHlU0bhSuimwNdxNM0oEQgU=\n-----END ED25519 CERT-----', u'master-key-ed25519 lUBEP1nqkx10fplx/2glSfbmalCKbt7NALjMA5uyovY', u'onion-key-crosscert \n-----BEGIN CROSSCERT-----\nnqsunFCwziRKIvVomzJRKtGEloFavu7NLAAZqdZeJjJPiFAcCdSvh1KG2fg/hhWQ\nrIqlFyNLXWAa1Mnhw/Ovcn4tzQexUU9vHnixZUOfITId4DGoGfCd17ZB8FzTQ306\naTlRwNYQVfmyv8TvHf8M/vKSFC+NC3TIzh4xfZ+z/KM=\n-----END CROSSCERT-----', u'ntor-onion-key-crosscert 0\n-----BEGIN ED25519 CERT-----\nAQoABhiFAZVARD9Z6pMddH6Zcf9oJUn25mpQim7ezQC4zAObsqL2AK34+b2/L0a9\nOyvXaT0Q8BJJjJ3VSm2E2+prUDOemahUxqdRXj1ICGLPVC3JYJ6hSLep/a2JPhpz\nIl6WsM9bkwE=\n-----END ED25519 CERT-----', u'router-sig-ed25519 V9jQvck/gA1p3VWi7yDOy71H44eo3pNroNiukWaPrvqfWYYlR0/zfwXLk/liLKi8SIpjCUKp+nUDPWn85tihDg']
[u'identity-ed25519 \n-----BEGIN ED25519 CERT-----\nAQQABhpoAScsjb4GMSGjE6T7s+22l5ZR4gNdu3GOgIAEPj8KjyaOAQAgBACRjdda\n5NzyMPPGKOiU8sI0JGUiElfupqmHHDgPeijms7yAq1jUQYB3uCyuaG2DhH4gxsd4\n4j3Ty1QiaDiiPLCXX3KP9E2Nw9FUrDBGSePo3J3AWzrdXOtAbnOgCaD4xgQ=\n-----END ED25519 CERT-----', u'master-key-ed25519 kY3XWuTc8jDzxijolPLCNCRlIhJX7qaphxw4D3oo5rM', u'onion-key-crosscert \n-----BEGIN CROSSCERT-----\nBQCW3hG5y/9wWDe9zwsXyeFKW62IoS7LNDCPletOmzlw2mo7wXnxugMBG0Y2VFH8\nGlenh0Wps7EpjGZ43caS0/mGaAaXWS4wPSPieikxYSlv5cIkCE6dneaHIO2PVVGU\noQSRbhKMzwdnXJXm+dCA/AKpCVFbpkbYNBTnWYPAeuE=\n-----END CROSSCERT-----', u'ntor-onion-key-crosscert 1\n-----BEGIN ED25519 CERT-----\nAQoABhh8AZGN11rk3PIw88Yo6JTywjQkZSISV+6mqYccOA96KOazABwUZc+dbkBP\nBUzzLzit8cKl6LRiWXRoURVNmY5zdnTQtwgQQbsoWfVSeuWgIRFTIxvp2HRn1QS8\nPu0nBlsuZw0=\n-----END ED25519 CERT-----', u'router-sig-ed25519 oWxFdlGijXjo5CdwoV1WEXUeDQwLyfjvLPTqtXvYUjhRAVf74PpfzkcN3CYhV684S+uh3PztadAYEMeszszODw']
...

Presently only spot this is mentioned is spec 220...

% torspec$ grep -R 'identity-ed25519' ./*
./proposals/220-ecc-id-keys.txt:      "identity-ed25519" NL "-----BEGIN ED25519 CERT-----" NL certificate
./proposals/220-ecc-id-keys.txt:   When an identity-ed25519 element is present, there must also be a
./proposals/220-ecc-id-keys.txt:      * If the identity-ed25519 line is present, it must be well-formed,
./proposals/220-ecc-id-keys.txt:   Extra-info documents now include "identity-ed25519" and

comment:5 Changed 5 years ago by nickm

Keywords: TorCoreTeam201508 added
Milestone: Tor: 0.2.7.x-final
Owner: set to nickm
Status: newaccepted

comment:6 Changed 5 years ago by nickm

I've started this as a branch called ed25519. More work to do.

comment:7 Changed 5 years ago by nickm

Status: acceptedneeds_review

comment:8 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

I'm merging the branch as-is as "better than nothing". Please reopen, or open a new ticket, if more is needed.

comment:9 Changed 5 years ago by atagar

Resolution: implemented
Status: closedreopened

Thanks Nick! Added Stem support for the new fields. Spec's almost perfect, only a couple minor questions...

"identity-ed25519" NL "-----BEGIN ED25519 CERT-----" NL certificate
       "-----END ED25519 CERT-----" NL

    [At most once, in second or first position in document]

Above the 'router' says it's at the start and appears exactly once. How can identity-ed25519 be in the first position? Is there a reason we care about its position in the server descriptor? If not then might as well just say "[At most once]" like everything else.

"id" SP "ed25519" SP base64-encoded-ed25519-identity NL

  [At most once]

Lets merge this with the above entry for 'id rsa1024'. Can microdescriptors and router status entries have multiple 'id' lines? This makes it sound like it can but Stem assumed from the rsa id line's wording that it wouldn't.

comment:10 Changed 5 years ago by nickm

Resolution: fixed
Status: reopenedclosed

How can identity-ed25519 be in the first position?

It will in some future version of the spec. But for now, it's second-position.

Is there a reason we care about its position in the server descriptor? If not then might as well just say "[At most once]" like everything else.

I'd like implementors to be able to do a parsing algorithm that gets the key and checks the signature first, and only does parsing if the key checks.

Can microdescriptors and router status entries have multiple 'id' lines?

Yes, at most once per key type. Tried to clarify that in the spec.

Changes are in commit 09ff9e202d4a169e95d6962c710bb05fcf062a49 . Thanks!

comment:11 Changed 5 years ago by atagar

Great, thanks Nick!

Just for the future the prior reading of the 'id rsa1024' lines read to me as 'there is only one id line'...

"id" SP "rsa1024" SP base64-encoded-identity-digest NL

   [At most once]
...

Not at all a big whoop, but I'll need to rejigger things a bit in Stem in ways I try to avoid. For me how many times a field appears is important since it determines the data structures I use. If we can keep forward comparability in mind early it's helpful.

The field as it is now is perfect though - thanks!

comment:12 Changed 5 years ago by atagar

Added support for multiple 'id' lines to Stem (change).

Note: See TracTickets for help on using tickets.