When the ephemeral hs is initialized with an existing key ehs.private and ehs.hostname remains uninitialized

[evilaliv3]

Right now when the when the ephemeral hs is initialized with an existing key ehs.private and ehs.hostname remains uninitialized.

In order to reproduce and unit tests the fix the following code could be used:

from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives.asymmetric import rsa
from cryptography.hazmat.primitives import serialization

private_key = rsa.generate_private_key(
    public_exponent=65537,
    key_size=1024,
    backend=default_backend()
)

privkey = private_key.private_bytes(
   encoding=serialization.Encoding.PEM,
   format=serialization.PrivateFormat.TraditionalOpenSSL,
   encryption_algorithm=serialization.NoEncryption()
)

privkey = privkey.replace('-----BEGIN RSA PRIVATE KEY-----', '')
privkey = privkey.replace('-----END RSA PRIVATE KEY-----', '')
privkey = privkey.replace('\n', '')
privkey = 'RSA1024:' + privkey

hs = txtorcon.EphemeralHiddenService(["80 127.0.0.1:8082"], privkey)
yield hs.add_to_tor(proto.tor_protocol)

it will be then possible to verify that when the callbacks are resolved hs.private_key and hs.hostname will be still undefined resulting in a KeyError if one access them.

[felipedau]

I added a main function to test the code you provided:

import txtorcon
from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives.asymmetric import rsa
from cryptography.hazmat.primitives import serialization
from twisted.internet.defer import inlineCallbacks
from twisted.internet.task import react

private_key = rsa.generate_private_key(
    public_exponent=65537,
    key_size=1024,
    backend=default_backend()
)

privkey = private_key.private_bytes(
   encoding=serialization.Encoding.PEM,
   format=serialization.PrivateFormat.TraditionalOpenSSL,
   encryption_algorithm=serialization.NoEncryption()
)

privkey = privkey.replace('-----BEGIN RSA PRIVATE KEY-----', '')
privkey = privkey.replace('-----END RSA PRIVATE KEY-----', '')
privkey = privkey.replace('\n', '')
privkey = 'RSA1024:' + privkey

@react
@inlineCallbacks
def main(reactor):
    tor = yield txtorcon.launch(reactor)
    hs = txtorcon.EphemeralHiddenService(["80 127.0.0.1:8082"], privkey)
    yield hs.add_to_tor(tor.protocol)
    print hs.hostname
    try:
        print hs.private_key
    except AttributeError:
        pass
    yield hs.remove_from_tor(tor.protocol)

A part of this issue has been fixed in the current txtorcon version - it does "persist" the hostname, because that is returned by the ADD_ONION command. The problem is that the private key is not (when you provide one).

The code persisting the private key in txtorcon/torconfig.py is:

 464         if self._key_blob == 'NEW:BEST':
 465             self.private_key = ans['PrivateKey']

Suggestions:

• Shouldn't the conditional check the key starts with 'NEW' (like __init__ does) or being more strict, that it matches both 'NEW:BEST' and 'NEW:RSA1024'?
• In __init__ it's also checked that the length of the key is correct, so maybe an else clause with self.private_key = self._key_blob would work.

What do you think?

[meejah]

Shouldn't the conditional check the key starts with 'NEW' (like init does) or being more strict, that it matches both 'NEW:BEST' and 'NEW:RSA1024'?

Yes. And probably just checking for "NEW:" is most future-proof (e.g. as Tor adds new key types).

Also, yes, persisting the private-key is a good idea.

There are new, more-complete Onion APIs coming "soon" (the last stuff on the release-1.x branch I haven't yet merged to master). However, the above API will continue to work as well so fixing it is not "lost work" or anything.

(Also, FWIW, the "new" APIs all use "onion" instead of "hidden")

[felipedau]

Yes. And probably just checking for "NEW:" is most future-proof (e.g. as Tor adds new key types).

So, do you think that the more strict checks could be done in __init__ and the more flexible ones everywhere else, as it would be assumed it was previously handled? For example, what happens if I provide 'NEW:FOO'? add_to_tor will fail? Should that kind of error be prevented early?

There are new, more-complete Onion APIs coming "soon" (the last stuff on the release-1.x branch I haven't yet merged to master). However, the above API will continue to work as well so fixing it is not "lost work" or anything.

(Also, FWIW, the "new" APIs all use "onion" instead of "hidden")

Oh, great to hear that :)

[meejah]

So, do you think that the more strict checks could be done in __init__ and the more flexible ones everywhere else, as it would be assumed it was previously handled? For example, what happens if I provide 'NEW:FOO'? add_to_tor will fail? Should that kind of error be prevented early?

In general, I try to keep things "as future-proof as reasonable". So, I would prefer to just take NEW: in this case and "wait for the error from Tor". Mostly this is how txtorcon is "supposed" to work. This isn't always true -- e.g. checking file-permissions that you "know" Tor will reject on hidden-service directories but "when in doubt" let Tor try (especially in a case like this where we know there will be more key-types added in the future).

This gives a good experience to users even if they're using an "older" txtorcon against a new (or unreleased) Tor. Also, I really try to avoid encoding any knowledge of Tor's version into txtorcon -- there's only one obscure exception to this right now.

Similarly: using "GETINFO" to discover what SETCONF options are available -- this works with any Tor version and avoids txtorcon having to know anything at all about Tor's specific options.

[felipedau]

Hmmm very interesting! Makes a lot of sense. Thanks for the explanation :)

Do you think the length check in __init__ could be simplified to just check it is not empty then?

[meejah]

Do you think the length check in init could be simplified to just check it is not empty then?

Yeah, the length check probably violates my own "rules" as stated above ;)

[meejah]

Oh, it auto-closes it. Hmm, well this is on master now but not yet released.

[meejah]

this is in 0.19.2