Opened 5 years ago

Closed 4 years ago

#14784 closed enhancement (implemented)

Implement status/fresh-relay-descs controller command

Reported by: Sebastian Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: 027-triaged-1-out
Cc: atagar Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This command will generate a fresh relay/ei pair and return it to the controller.

Child Tickets

Change History (9)

comment:1 Changed 5 years ago by Sebastian

Spec patch is in bug14784 in my torspec repo, please check it atagar?

comment:2 Changed 5 years ago by Sebastian

Put an initial implementation idea int bug14784 in my tor repo. If you'd be available to take a peek, damian? Is that something you can work with?

comment:3 Changed 5 years ago by atagar

Fantastic! This is great, thanks Sebastian! Few thoughts are...


Lets have separate methods for getting the server and extrainfo descriptor. Unless I'm missing something there's no reason for 'GETINFO status/fresh-relay-descs' to return both. Maybe...

GETINFO status/fresh-descriptor/desc
GETINFO status/fresh-descriptor/extra-info

Just picking 'desc' here to be consistent with the earlier methods. Don't really care much about the name. :P


Nitpick but we're pretty inconsistent with descriptor naming...

  • 'GETINFO desc/*' and Stem call the main self-published descriptors 'server descriptors'.
  • The dir-spec calls them 'router descriptors'... usually. It opts for 'server descriptor' in section 6.2.
  • CollecTor calls them 'relay descriptors'.

It looks like you're opting for 'relay descriptors' here. That's fine, just pointing out that we're inconsistent. Maybe we should have another ticket to settle on a name? The specs and CollecTor can easily change, but Stem's classes would be a problem since we vend those.


+ DOC("status/fresh-relay-descs",
+ "A fresh relay/ei descriptor pair for Tor's current state. Not stored."),

I'm guessing the "/ei" is a typo?


if (router_build_fresh_descriptor(&r, &e) < 0) {

I don't know Tor's conventions but maybe != 0 is safer? In practice I know this is fine (presently it only returns negative or zero statuses) but not sure if this is like exit status codes where any non-zero indicates failure.


+ if (e) {
+ size += e->cache_info.signed_descriptor_len + 1;
+ }

Do you know the use cases where a server descriptor can be generated but an extrainfo can't? Didn't know that was possible.

Cheers! -Damian

comment:4 in reply to:  3 Changed 5 years ago by Sebastian

Replying to atagar:

Lets have separate methods for getting the server and extrainfo descriptor. Unless I'm missing something there's no reason for 'GETINFO status/fresh-relay-descs' to return both. Maybe...

GETINFO status/fresh-descriptor/desc
GETINFO status/fresh-descriptor/extra-info

Just picking 'desc' here to be consistent with the earlier methods. Don't really care much about the name. :P

I thought this too at first, but it's not possible to use this because the descriptors are temporary. To generate the relay descriptor, we have to generate the extra-info document too (to reference it from the descriptor). I could be convinced to add a method to just fetch the descriptor without extrainfo.

Nitpick but we're pretty inconsistent with descriptor naming...

  • 'GETINFO desc/*' and Stem call the main self-published descriptors 'server descriptors'.
  • The dir-spec calls them 'router descriptors'... usually. It opts for 'server descriptor' in section 6.2.
  • CollecTor calls them 'relay descriptors'.

It looks like you're opting for 'relay descriptors' here. That's fine, just pointing out that we're inconsistent. Maybe we should have another ticket to settle on a name? The specs and CollecTor can easily change, but Stem's classes would be a problem since we vend those.

I think this is for another ticket, another time

+ DOC("status/fresh-relay-descs",
+ "A fresh relay/ei descriptor pair for Tor's current state. Not stored."),

I'm guessing the "/ei" is a typo?

No, but extrainfo should be spelled out

if (router_build_fresh_descriptor(&r, &e) < 0) {

I don't know Tor's conventions but maybe != 0 is safer? In practice I know this is fine (presently it only returns negative or zero statuses) but not sure if this is like exit status codes where any non-zero indicates failure.

Imo that's fine.

+ if (e) {
+ size += e->cache_info.signed_descriptor_len + 1;
+ }

Do you know the use cases where a server descriptor can be generated but an extrainfo can't? Didn't know that was possible.

There's a code path. I don't know if there's any kind of way to trigger it in practice in current Tor. I don't think it is possible, but it's currently allowed.

comment:5 Changed 5 years ago by atagar

Status: newneeds_review

Hi Sebastian, sorry about the delay. Just gave this a whirl and works great. I look forward to it being merged!

atagar@odin:~/Desktop/stem$ cat /tmp/tor_test/torrc 
DataDirectory /tmp/tor_test
ControlPort 1111
ORPort 9090

atagar@odin:~/Desktop/stem$ ./tor-prompt --interface 1111
>>> GETINFO status/fresh-relay-descs
250+status/fresh-relay-descs=
router Unnamed 174.21.175.181 9090 0 0
platform Tor 0.2.6.2-alpha-dev on Linux
protocols Link 1 2 Circuit 1
published 2015-02-14 21:49:37
fingerprint 4A6F B0F6 E965 5B13 68B1 1C7B 83F8 9F55 8705 1A64
uptime 61
bandwidth 1073741824 1073741824 0
extra-info-digest 82C882B2FF0348925E443C77ADC1E856D34CE4EB
onion-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBAKLywDZG1euy6RuipuXgZ0SD7TaeNZn7NK8A5MtHgL+SxQD6QCOD6M7t
Mw5HijxG/e6N4iv9ABUmsq+KGy3cupSXK17W2oREIDzi/WllaKyOEvlSAmPWGlTa
DFlXi8LjAjGhjgg27XqCjCyLfYCotQwVkP0+YaVmiZf03/abt2rzAgMBAAE=
-----END RSA PUBLIC KEY-----
signing-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBAL1twVzVIdz7XdIytlqjZwQ33nVT6iD9fm1spl5qaROQSKmkQrvSWaSZ
wDGRSVxRl5IjkcCWnEu58HN/rOQ/aCXLgOOxm9dfXQUIVHeMe9HNNBxn8jcWXm9L
y1rPVWTOZVpr7lG5aRiIMIFrAjOe7q8dwJq9R6wgD4T5rU26f1l/AgMBAAE=
-----END RSA PUBLIC KEY-----
hidden-service-dir
ntor-onion-key LvvgQehe3LgWMuuVR1pN0DK9IojyWNE4h1RwtLqwrEc=
reject 0.0.0.0/8:*
reject 169.254.0.0/16:*
reject 127.0.0.0/8:*
reject 192.168.0.0/16:*
reject 10.0.0.0/8:*
reject 172.16.0.0/12:*
reject 174.21.175.181:*
reject *:25
reject *:119
reject *:135-139
reject *:445
reject *:563
reject *:1214
reject *:4661-4666
reject *:6346-6429
reject *:6699
reject *:6881-6999
accept *:*
router-signature
-----BEGIN SIGNATURE-----
Tgrn25vLbaYZA6pQd8wmfP/VMj8GTNUgqoCpLgvgwDo/oTUXn1eGOkRoi/ubByAs
AUUDcCZwSj2ip6jxrSPav4jzVj9e8gzu1kkkbdNR2GDkcRJxXSmUCYjKuj0B5O6W
ZROgadgtMvslim3nSEHZidcVR6lEsSpR9RmZGhnfAMM=
-----END SIGNATURE-----
extra-info Unnamed 4A6FB0F6E9655B1368B11C7B83F89F5587051A64
published 2015-02-14 21:49:37
router-signature
-----BEGIN SIGNATURE-----
htgHEamUr4O2VRa/hQkaNKiP5D2An4O1HFdnG85XRr/txifZVzVu1YA4TQbUP55s
Umz0P4mwTcSvSIPbdrFMHCEnLqyVpK87YL75exJ8toQAiVLLKt1wpJus6LL2COEX
3xHvc+egG4opRs6eitR1xeTv0Vpr4uPyNyKVKq8ABlQ=
-----END SIGNATURE-----
.
250 OK

Guessing this needs to be put in 'needs review' to get Nick's attention.

comment:6 Changed 5 years ago by Sebastian

This needs a bit more work on the code side, just wanted to make sure the spec is acceptable

comment:7 Changed 5 years ago by Sebastian

There, I think the branch can be reviewed now.

comment:8 Changed 4 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:9 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged, and cleaned a little. Some of the logic for handling end-of-descriptor stuff still looks a little screwy to me, but it should be harmless.

Note: See TracTickets for help on using tickets.