Put some actual hsdescs in the unit tests for parsing

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?

changed milestone to %Tor: 0.3.2.x-final

added component::core tor/tor easy milestone::Tor: 0.3.2.x-final owner::rl1987 points::0.1 priority::medium resolution::implemented review-group-19 reviewer::dgoulet severity::normal sponsor::R-can status::closed tor-hs type::enhancement labels

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

Trac:
Cc: N/A to rl1987@sdf.lonestar.org

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

Trac:
Owner: N/A to rl1987
Status: new to accepted

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.

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.

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?

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.

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

Trac:
Points: N/A to small/medium
Sponsor: N/A to N/A
Severity: N/A to Normal

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

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

Trac:
Keywords: N/A deleted, tor-03-unspecified-201612 added
Milestone: Tor: 0.3.??? to Tor: unspecified

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

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

Trac:
Milestone: Tor: unspecified to Tor: 0.3.2.x-final
Cc: rl1987@sdf.lonestar.org to rl1987@sdf.lonestar.org, dgoulet, asn
Sponsor: N/A to SponsorR-can
Reviewer: N/A to N/A
Status: accepted to new

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 (moved) 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.

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.

Trac:
Status: new to needs_review

Trac:
Keywords: N/A deleted, review-group-19 added

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".

Trac:
Points: small/medium to 0.1
Status: needs_review to needs_revision
Reviewer: N/A to dgoulet

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

Trac:
Status: needs_revision to needs_review

Looks good to me! Thanks David!

Trac:
Status: needs_review to merge_ready

squashed, added a changes file, merging. Thanks!

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

changed time estimate to 48m

mentioned in issue #15620 (moved)

moved to tpo/core/tor#15554 (closed)

mentioned in issue tpo/anti-censorship/bridgedb#15620 (closed)