Opened 3 months ago

Closed 10 days ago

#24854 closed enhancement (implemented)

Extract the authority list from config.c

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: torspec, tor-dirauth, 029-backport, 031-backport, 032-backport, 033-backport, review-group-32, review-group-33, review-group-34, 034-triage-20180328
Cc: beastr0@… Actual Points:
Parent ID: #24818 Points: 0.2
Reviewer: ahf Sponsor:

Description (last modified by teor)

There is a list of default_authorities in src/or/config.c.
We want it in a separate file at src/or/auth_dirs.inc.

This should be implemented like the default_fallbacks array, which includes the fallback list from src/or/fallback_dirs.inc.

We will need to implement two branches for backporting:

  • a branch on maint-0.2.9 for 0.2.9 and later. It has IPv6 addresses.
  • a branch on maint-0.2.5 for 0.2.5. It has no IPv6 addresses.

(Then, after we have moved it into a separate file, we want to automatically generate the file, in a new format. See the rest of the children of #24818.)

Child Tickets

Attachments (3)

config.c (298.4 KB) - added by beastr0 3 months ago.
24854 edit to config.c
auth_dirs.inc (1.6 KB) - added by beastr0 3 months ago.
dependency for 24854 edit to config.c
config_c.patch (1.9 KB) - added by beastr0 3 months ago.
patch file of config.c for 0.3.4

Download all attachments as: .zip

Change History (34)

comment:1 Changed 3 months ago by beastr0

Cc: beastr0@… added

comment:2 Changed 3 months ago by beastr0

Should this function/method (can't remember which one it's called in C) be written in the config.c file or somewhere else?

comment:3 Changed 3 months ago by teor

I don't think we need to modify any functions for this patch.

This patch should #include "auth_dirs.inc" in config.c at compile time, like we do a few lines later in config.c with "fallback_dirs.inc".

(Sorry, that wasn't very obvious.)

comment:4 Changed 3 months ago by beastr0

I've been crawling through the files, just getting myself oriented, and I think I see where I went wrong. Ha, wow, nice and easy fix. Should I also create the auth_dirs.inc file or is that #24853?

comment:5 Changed 3 months ago by teor

Please create the file by copying and pasting the current contents of the array.
Then we can modify the format of the file in #24851, and backport it in #24853.

comment:6 Changed 3 months ago by teor

Keywords: 025-backport removed

We won't backport this to 0.2.5, because the format is missing ipv6. Instead, we will manually backport any authority changes that happen before May 2018 when 0.2.5 is EOL.

comment:7 Changed 3 months ago by teor

Description: modified (diff)

comment:8 Changed 3 months ago by teor

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

We have time to do these, let's do them well in 0.3.4

comment:9 Changed 3 months ago by beastr0

So Teor (or anyone else who's reading this and knows), how and where do I upload these patches?

comment:10 Changed 3 months ago by teor

You can attach them to this ticket using the "Attachments" button near the top of the ticket.
(If you don't see it, let me know, and I will enable it for you.)

Or you can clone https://git.torproject.org/tor.git , create a new branch, commit the patches to that branch, and push the branch to GitHub or a similar public repository.

Changed 3 months ago by beastr0

Attachment: config.c added

24854 edit to config.c

comment:11 Changed 3 months ago by teor

Keywords: 030-backport removed
Status: newneeds_revision

Hi, can you also upload a copy of the auth_dirs.inc file?

We usually use the "diff" command to produce a small patch file containing all the changed files. If you would like, you can also upload a patch file containing the changes to config.c, and the new auth_dirs.inc file.

Changed 3 months ago by beastr0

Attachment: auth_dirs.inc added

dependency for 24854 edit to config.c

comment:12 Changed 3 months ago by teor

Status: needs_revisionneeds_review

This looks good to me.

I still need to:

  • check that the authority information is exactly the same,
  • run the unit tests, and
  • put the changed code into a branch.

Changed 3 months ago by beastr0

Attachment: config_c.patch added

patch file of config.c for 0.3.4

comment:13 Changed 3 months ago by beastr0

Do you need any help with those tasks? or do you have any other specific tickets that you would like me to work on?

comment:14 in reply to:  13 Changed 3 months ago by teor

Replying to beastr0:

Do you need any help with those tasks? or do you have any other specific tickets that you would like me to work on?

Ticket #24851 is the next step: we want to generate the auth_dirs.inc file using a script.
That way, we can update it automatically when authority details change.
If you know python, or you're willing to learn, it's a good ticket.

Otherwise, I'm not actively working on anything in C at the moment, but I'll let you know. Or you can look for tickets.

comment:15 Changed 3 months ago by beastr0

Yeah, sure, I can take that one on. I know Python, C and Java so for future reference you guys can hit me up with any projects based on those languages.

comment:16 Changed 2 months ago by nickm

Reviewer: teor

(teor, it sounds like you'd like me to mark you reviewer on this one. please let me know if that's not right.)

comment:17 Changed 2 months ago by nickm

Keywords: review-group-32 added

comment:18 Changed 2 months ago by nickm

Keywords: review-group-34 added

comment:19 Changed 2 months ago by nickm

Keywords: review-group-33 added; review-group-34 removed

comment:20 Changed 7 weeks ago by nickm

Keywords: review-group-34 added

comment:21 Changed 3 weeks ago by nickm

Keywords: 034-triage-20180328 added

comment:22 Changed 3 weeks ago by teor

Reviewer: teor

In Rome, we decided on a 1 week deadline for reviews.
I'm not going to get time review this, so it should be assigned to someone else.

To review:

  • make sure that the code compiles
  • make sure that the authority details were moved between files, without being changed

comment:23 Changed 3 weeks ago by dgoulet

Reviewer: ahf

Reviewer assignment for week 02/04/2018

comment:24 Changed 2 weeks ago by ahf

Hey beastr0,

Thanks for your patches. I think they look good, but it's not possible for us to integrate them right now in the current form as we need them in a Git patch format. I can do this for you if you want me to.

Otherwise you can do this yourself as it will make the life of the patch reviewers much easier: create a branch in Git with your changes in, do a git add <file> for each <file> you have modified, then do git commit and write a commit message. For Tor we normally also want a 'changes' file that describes the change for use in ChangeLog messages when we make the release. Once this is done you can push your branch to, for example, Github and post a link here for where we can review it :-)

We have some documentation on this process here: https://gitweb.torproject.org/tor.git/tree/doc/HACKING/GettingStarted.md

Unless I hear back from you, I will proceed with turning your changes into a Git style patch and credit you as beastro0 in the ChangeLog entry!

Thanks

comment:25 Changed 2 weeks ago by ahf

Status: needs_reviewneeds_revision

comment:26 Changed 2 weeks ago by ahf

Status: needs_revisionneeds_review

The patches have been combined in https://github.com/torproject/tor/pull/43

comment:27 Changed 2 weeks ago by teor

Status: needs_reviewmerge_ready

Looks good to me!
As long as it unit tests fine, and tor bootstraps on the public network, we should be good here.

comment:28 Changed 2 weeks ago by teor

Keywords: 033-backport added
Status: merge_readyneeds_revision

Oh, this branch needs to be based on 0.2.9, because it will be backported to 0.2.9, 0.3.1, 0.3.2, and 0.3.3.

comment:29 Changed 12 days ago by ahf

Status: needs_revisionneeds_review

Pushed new version to https://github.com/torproject/tor/pull/43 (based of on master). The new version have the auth_dirs.inc file added to the set of included files when we make releases (ORHEADERS in this case).

I opened https://github.com/torproject/tor/pull/44 which is based of on maint-0.2.9.

comment:30 Changed 11 days ago by teor

Status: needs_reviewmerge_ready

Lets get it merged

comment:31 Changed 10 days ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to 0.2.9 and forward. Pfew! Good work, everybody!

Note: See TracTickets for help on using tickets.