Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Tor Trac #28279]: control: Add a key to GETINFO to fetch the circuit onion handshake rephist values #1907

Closed
wants to merge 14 commits into from
Closed

[Tor Trac #28279]: control: Add a key to GETINFO to fetch the circuit onion handshake rephist values #1907

wants to merge 14 commits into from

Conversation

Reviewers

@dgoulet-tor dgoulet-tor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@neelchauhan @coveralls @dgoulet-tor @torproject-pusher
@neelchauhan
Copy link
Contributor

@neelchauhan neelchauhan commented May 28, 2020

Ticket: https://trac.torproject.org/projects/tor/ticket/28279
@coveralls
Copy link

@coveralls coveralls commented May 28, 2020 •
edited

Pull Request Test Coverage Report for Build 9333

  • 16 of 23 (69.57%) changed or added relevant lines in 2 files are covered.
  • 5324 unchanged lines in 41 files lost coverage.
  • Overall coverage increased (+0.1%) to 64.337%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/feature/control/control_getinfo.c 16 17 94.12%
src/feature/stats/rephist.c 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
src/core/or/versions.c 3 96.23%
src/feature/dirauth/voteflags.c 3 29.44%
src/feature/dirauth/shared_random.c 4 85.0%
src/feature/nodelist/routerlist.h 4 22.22%
src/feature/nodelist/dirlist.c 6 77.78%
src/feature/nodelist/routerset.c 8 94.55%
src/feature/nodelist/fmt_routerstatus.c 11 82.86%
src/app/config/resolve_addr.c 14 90.54%
src/core/or/scheduler_kist.c 14 79.76%
src/lib/tls/buffers_tls.c 18 39.68%
Totals Coverage Status
Change from base Build 9007: 0.1%
Covered Lines: 51245
Relevant Lines: 79651
💛 - Coveralls

neelchauhan added 3 commits May 28, 2020
@neelchauhan
control: Add a key to GETINFO to fetch the circuit onion handshake re…
b4eb5e9 
…phist values
@neelchauhan
Add GETINFO "rephist/{tap,ntor}/onion_handshakes_{assigned/requested}…
9c12886 
…" test
@neelchauhan
Add changes file for Ticket #28279
2289fff
dgoulet-tor
dgoulet-tor reviewed Jun 25, 2020
View changes
src/feature/control/control_getinfo.c Outdated
@@ -1436,6 +1437,37 @@ getinfo_helper_liveness(control_connection_t *control_conn,
return 0;
}

/** Implementation helper for GETINFO: answers queries about shared random
* value. */
Copy link
Contributor

@dgoulet-tor dgoulet-tor Jun 25, 2020

Comment is not accurate. Most likely copy paste :).

src/feature/control/control_getinfo.c Outdated
result =
rep_hist_get_circuit_handshake_requested(ONION_HANDSHAKE_TYPE_TAP);
}
/* Else statement here is unrecognized key so do nothing. */
Copy link
Contributor

@dgoulet-tor dgoulet-tor Jun 25, 2020

Shouldn't we send an error here? Spec specifies:

The server sends a 551 or 552 error on failure.

src/feature/control/control_getinfo.c Outdated
{
(void) control_conn;
(void) errmsg;
int result = -1;
Copy link
Contributor

@dgoulet-tor dgoulet-tor Jun 25, 2020

I would not assign it here so the compiler can tell us when we use it unassigned. And I don't think we should ever use -1 as for an unknown question, an control error code should be returned?

src/feature/stats/rephist.c
/** Get the circuit handshake value that is requested. */
MOCK_IMPL(int,
rep_hist_get_circuit_handshake_requested, (uint16_t type))
{
Copy link
Contributor

@dgoulet-tor dgoulet-tor Jun 25, 2020

We should be extra extra careful here with type so I would recommend you BUG() on type > MAX_ONION_HANDSHAKE_TYPE and return 0 as in "no stats".

src/feature/control/control_getinfo.c
ITEM("rephist/tap/onion_handshakes_assigned", rephist,
"Assigned TAP circuit handshake stats."),
ITEM("rephist/tap/onion_handshakes_requested", rephist,
"Requested TAP circuit handshake stats."),
Copy link
Contributor

@dgoulet-tor dgoulet-tor Jun 25, 2020

See torspec comment about the naming. We'll wait on a decision there.

Copy link
Contributor

@dgoulet-tor dgoulet-tor Jul 1, 2020

So, the PREFIX are not correct.

But also I would propose we use stats/ntor/requested and stats/ntor/assigned.

Reason I say that is that I think onion and handshake are a bit redundant since ntor is by definition an onion handshake.

neelchauhan added 11 commits Jun 25, 2020
@neelchauhan
fixup! Add changes file for Ticket #28279
e988a61
@neelchauhan
fixup! Add GETINFO "rephist/{tap,ntor}/onion_handshakes_{assigned/req…
0c03641 
…uested}" test
@neelchauhan
fixup! control: Add a key to GETINFO to fetch the circuit onion hands…
e8c7949 
…hake rephist values
@neelchauhan
fixup! control: Add a key to GETINFO to fetch the circuit onion hands…
45ebdf3 
…hake rephist values
@neelchauhan
fixup! Add GETINFO "rephist/{tap,ntor}/onion_handshakes_{assigned/req…
b06cba7 
…uested}" test
@neelchauhan
fixup! control: Add a key to GETINFO to fetch the circuit onion hands…
693f1e3 
…hake rephist values
@neelchauhan
fixup! Add GETINFO "rephist/{tap,ntor}/onion_handshakes_{assigned/req…
012f9aa 
…uested}" test
@neelchauhan
fixup! Add changes file for Ticket #28279
79aa7a7
@neelchauhan
Merge branch 'bug28279' of github.com:neelchauhan/tor into bug28279
14f3535
@neelchauhan
fixup! control: Add a key to GETINFO to fetch the circuit onion hands…
3eb59a9 
…hake rephist values
@neelchauhan
fixup! control: Add a key to GETINFO to fetch the circuit onion hands…
af6568c 
…hake rephist values
@torproject-pusher torproject-pusher deleted the branch torproject:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment