Improve Ephemeral Onion Service private key handling

[felipedau]

These changes seem to fix #190 and implement a different check for the type and blob provided by the user.

Right now it only checks if the key contains a ':' and in case the type is not 'NEW', makes sure the blob is not empty. Should we make sure both the type and blob are not empty? Also, as an extra AttributeError is being catched in case the user does not provide a string (well, something that does not split).

I am not sure about this, but I think it is a good practice to declare all attributes in __init__. Is it okay to declare hostname and private_key there (by just setting them to None)?

We should probably add tests for these too.

Let me know what you think. Thanks!

[coveralls]

Coverage decreased (-0.05%) to 99.89% when pulling db76fade87f8a104ff1e3bf8d72717b2628f547e on felipedau:develop into 7157e386a7041207171b374e1766c402c7ee03bf on meejah:master.

[codecov-io]

Codecov Report

Merging #230 into master will increase coverage by <.01%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   99.94%   99.94%   +<.01%     
==========================================
  Files          20       20              
  Lines        3647     3650       +3     
==========================================
+ Hits         3645     3648       +3     
  Misses          2        2

Impacted Files	Coverage Δ
txtorcon/torconfig.py	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7157e38...ae6b0ce. Read the comment docs.

[meejah]

Should we make sure both the type and blob are not empty?

Yes, probably! I'm not sure if passing no type to Tor is valid or not, but saying it isn't at this level is fine with me.

[felipedau]

I definitely wouldn't put hostname in, because it is (or should be!) a read-only attribute and the user has no business messing with it.

There's some argument for putting private_key in, but I would leave it out because I happen to have knowledge from the future ;). With the new Onion APIs, the default services will be ephemeral, and triggered off passing a private_key arg -- which if None will mean "let Tor create a new one" and otherwise is the keyblob. In any case, this is a sort of "opaque" thing -- the user shouldn't really ever create their own, merely echo back one they got by previously asking Tor to create them one.

Hmm, I did not suggest to receive hostname and private_key in __init__, but just declare them there so people know they exist. I remember the first time I was learning how to use the ephemeral stuff and I was confused if I could access those - later I noticed they were declared in the method below.

Currently, users would be able to mess with it after calling add_to_tor because they are regular attributes. Would it be more correct to declare (get) properties?

Another question: How important is it to provide the key type/blob in __init__? Couldn't it be provided just to add_to_tor instead?

[meejah]

Hmm, I did not suggest to receive hostname and private_key in init, but just declare them there so people know they exist

Ah, I see what you mean. They way they're done via __getattr__ is very gross. I've kind of been ignoring that as I'm working on getting "the new onion stuff" moved over from release-1.x.

Ultimately, onions will be created via a factory-method call on Tor in the very near future, and users shouldn't need to instantiate these classes themselves at all (nor use add_to_tor). That said, I will keep this one around and people can use it .. so if you want to fix how the hostname etc attributes are accessed that would be great.

[meejah]

p.s. it might be better to save ^ for a follow-on PR so we can get these improvements merged when they're ready and so this PR doesn't grow "too big"/long-running etc ;)

Trying to learn my lesson there :)

[felipedau]

I should definitely take a look at the release-1-x code, these are awesome additions.

So, what can I do to make your life easier when merging?

Does adding _is_valid_keyblob result in conflicts?

It is not very clear to me what to do now and later. Here is what I got so far:

• In this PR: improve the blob/type checks and add tests for them as well as the persistent key and hostname.

• In a new PR (after the new features are merged): make the key and hostname private and provide only read access to them.

Is that right?

[meejah]

yes, that sounds right. don't worry about conflicts, adding a helper is fine.

[felipedau]

yes, that sounds right. don't worry about conflicts, adding a helper is fine.

Cool! Thanks :)

I just amended that last check. I decided to use the regex [^ :]+:[^ :]+$. What do you think? I will then add the isinstance check to that conditional too.

I will move everything to the _is_valid_keyblob. Do you think that call should be the first instruction in the __init__ before assigning the attributes?

[coveralls]

Coverage increased (+5.0e-05%) to 99.945% when pulling ae6b0ceefa42dfadfb71d5168e6ed8bdfb085f1f on felipedau:develop into 7157e386a7041207171b374e1766c402c7ee03bf on meejah:master.

[coveralls]

Coverage increased (+5.0e-05%) to 99.945% when pulling ae6b0ceefa42dfadfb71d5168e6ed8bdfb085f1f on felipedau:develop into 7157e386a7041207171b374e1766c402c7ee03bf on meejah:master.

[coveralls]

Coverage increased (+5.0e-05%) to 99.945% when pulling ae6b0ceefa42dfadfb71d5168e6ed8bdfb085f1f on felipedau:develop into 7157e386a7041207171b374e1766c402c7ee03bf on meejah:master.

[felipedau]

Update:

Added the isinstance check and moved to _is_valid_keyblob. Let me know if you think it is a good idea to use a @classmethod.

Also fixed the wrong key-blob test expecting for a RuntimeError and added a few more wrong key-blobs to the test. Do you use anything (parametrization/fuzzy tests) that could make that test better?

Created another test to make sure the hostname a private key are persisted when the EOS is initialized with an existing key.

Let me know if you think the changes look like a good idea.

P.S. I took a quick look at the release-1.x branch and there are already properties for the hostname and key, so I think that second PR we discussed above is unnecessary.

Thanks!

[meejah]

great, thanks!