Opened 3 years ago

Closed 4 months ago

#15554 closed enhancement (implemented)

Put some actual hsdescs in the unit tests for parsing

Reported by: arma Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, easy, review-group-19
Cc: rl1987@…, dgoulet, asn Actual Points:
Parent ID: Points: 0.1
Reviewer: dgoulet Sponsor: SponsorR-can

Description

Right now our test_rend_fns() function in src/test/test.c builds a "rend_service_descriptor_t *generated", and then it encodes it, and then it parses it, to make sure it gets the expected descriptor back. That's a great start.

But there are now several different versions of the descriptor format in play, and our unit tests by definition only test the one that the current Tor version generates.

Also, it would be easier to play around with "how Tor handles it if you take that stanza out of the descriptor" experiments, if there are some actual descriptors in the unit tests.

Good idea / bad idea?

Child Tickets

Change History (21)

comment:1 Changed 3 years ago by arma

dgoulet points us to #14847 which he says has a branch as part of its unit tests that contains a valid hidden service descriptor.

comment:2 Changed 3 years ago by rl1987

Cc: rl1987@… added

comment:3 Changed 3 years ago by isis

I added support for HS descriptor generation to Leekspin. See #15620.

comment:4 Changed 3 years ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:5 Changed 3 years ago by arma

rl1987: it might be best for stem to do this, using some sort of hs desc generation code, and the hsdesc post feature, on the control port, as part of the 'make test-stem' process. Rather than cluttering Tor's unit tests with this. Nick would have an opinion here.

comment:6 Changed 3 years ago by atagar

Personally I'm very much in favor of this. Sebastian was expanding the control spec to include a GETINFO method for 'gimme the server/extrainfo descriptor tor would generate for me right now'.

The goal for it was to test Tor's descriptor generation using Stem. Stem already does a lot of validation when it reads descriptors so this is a cheap and easy way of getting much better Tor test coverage.

comment:7 in reply to:  5 Changed 3 years ago by isis

Replying to arma:

rl1987: it might be best for stem to do this, using some sort of hs desc generation code, and the hsdesc post feature, on the control port, as part of the 'make test-stem' process. Rather than cluttering Tor's unit tests with this. Nick would have an opinion here.


If you want the HS descriptors to be generated, rather than actually including real HS descriptors in the unittests, then does that mean that you want Stem to include/merge Leekspin?

comment:8 Changed 3 years ago by atagar

If you want the HS descriptors to be generated, rather than actually including real HS descriptors in the unittests, then does that mean that you want Stem to include/merge Leekspin?

Stem has the ability to generate descriptors too. I just haven't put that functionality in the library (presently its in a testing helper). We can move it to the library if it's of use to tor.

comment:9 Changed 21 months ago by nickm

Points: small/medium
Severity: Normal

Easy once you know how to do it; harder to do it if you don't know how. So calling it small/medium.

comment:10 Changed 11 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:11 Changed 10 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:12 Changed 5 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:13 Changed 4 months ago by nickm

Cc: dgoulet asn added
Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Sponsor: SponsorR-can
Status: acceptednew

I believe this might already be done in master, but if not, we should consider it critical.

comment:14 Changed 4 months ago by asn

I actually don't think we are parsing an actual hardcoded v2 HS desc right now. We only parse self-generated descriptors.

It's true that that #14847 added an actual HS desc in the unittests (see hs_desc_content in test_hs.c) but I don't think we are actually parsing it anywhere right now. We just use it to check the output of the control port commands.

We should probably add a test that parses that v2 descriptor. I added it to my TODO list will try to do it RSN.

comment:15 Changed 4 months ago by asn

Please check my branch bug15554 for a minimal test that checks that our hardcoded v2 HS descriptor is parseable. A more advanced test here would also test the internal descriptor fields.

Also, the next step here is to do the same for v3 descriptors. That's a bit more involved since it involves more crypto, but it's definitely possible.

comment:16 Changed 4 months ago by asn

Status: newneeds_review

comment:17 Changed 4 months ago by nickm

Keywords: review-group-19 added

comment:18 Changed 4 months ago by dgoulet

Points: small/medium0.1
Reviewer: dgoulet
Status: needs_reviewneeds_revision

The test fails with:

FAIL src/test/test_hs.c:147: assert(ret OP_EQ 1): 0 vs 1

And in that case, there is a memleak. It's easily fixable with moving the tor_free(desc.desc_str); before the tt_int_op() test.

The failure is due to:

[warn] parse error: Malformed object: bad begin line

There are many condition to hit that and I narrow it down to strcmp_len(eol-5, "-----", 5) which returns -32 in our case. So I think all those \r to the end of the line are not making the parsing function happy. If your remove them, it works but then the control port test is unhappy.

*So* we either need to copy is in another const string without the \r or do some magic removal \r :). I say go with the easy "copy without \r".

comment:19 Changed 4 months ago by dgoulet

Status: needs_revisionneeds_review

See my branch bug15554_032_01 with the fix above proposed in an extra fixup.

comment:20 Changed 4 months ago by asn

Status: needs_reviewmerge_ready

Looks good to me! Thanks David!

comment:21 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

squashed, added a changes file, merging. Thanks!

Note: See TracTickets for help on using tickets.