Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#21558 closed enhancement (implemented)

Add Ed25519 support

Reported by: patrickod Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

https://github.com/patrickod/stem/pull/1 introduces support for all Ed25519 identity and signing key descriptor properties associated with the implementation of props 220 and 228.

This unblocks https://trac.torproject.org/projects/tor/ticket/16670

Child Tickets

Change History (13)

comment:1 Changed 4 months ago by patrickod

  • Status changed from new to needs_review

comment:2 Changed 4 months ago by atagar

Hi Patrick. First thing I noticed was that this adds a dependency on pynacl. On #21086 you're swapping out pycrypto for cryptography - can we use that for this?

Also, a minorish thing but noticed the copyright header for certificates.py you list yourself rather than me. Including your name is fine, but could you list me too? Otherwise legally speaking things become a bit more of a headache...

https://stem.torproject.org/faq.html#what-is-the-copyright-for-patches

Speaking of which, these are really substantial improvements so I gotta ask: are you fine with all your contributions being in the public domain?

comment:3 Changed 4 months ago by patrickod

Just pushed a change to bring the copyright header for certificate.py up to scratch.

@atagar: I'm 100% ok with these changes being in the public domain :)

I will do some more research now to see if we can resolve these dependencies into one.

comment:4 Changed 4 months ago by patrickod

It appears that cryptography as yet does not support Ed25519, but may do in some future release https://github.com/pyca/cryptography/issues/2968

comment:5 Changed 4 months ago by atagar

Thanks Patrick! Spotting a few things that need to be tweaked but they're minor. Overall looks good! I'll try to get this merged tomorrow.

comment:6 Changed 4 months ago by atagar

Hi Patrick. Per chance did you forget to use 'git add' for a new file? Running the descriptor.certificate unit tests fails for me with...

Traceback (most recent call last):
  File "./run_tests.py", line 426, in <module>
    main()
  File "./run_tests.py", line 204, in main
    run_result = _run_test(args, test_class, output_filters, logging_buffer)
  File "./run_tests.py", line 398, in _run_test
    raise exc
AttributeError: 'module' object has no attribute 'certificate'

comment:7 Changed 4 months ago by patrickod

Hmm https://github.com/patrickod/stem/pull/1/files#diff-ae666c31dc370e3ab59989f4aad057ccR1 definitely shows the certificate.py file there, and when I check out that branch locally the tests pass for me.

comment:8 Changed 4 months ago by atagar

Hi Patrick. Sorry about that, as discussed on irc figured out what I was doing wrong. Made a few small changes to this branch but then realized it lacks python 3.x support. Running our unit tests fails with...

% python3 run_tests.py --unit
...
======================================================================
ERROR: test_ed25519_key_certificate_without_extensions
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/atagar/Desktop/stem/test/unit/descriptor/certificate.py", line 86, in test_ed25519_key_certificate_without_extensions
    validate = True
  File "/usr/lib/python3.2/unittest/case.py", line 1170, in deprecated_func
    return original_func(*args, **kwargs)
  File "/usr/lib/python3.2/unittest/case.py", line 1117, in assertRaisesRegex
    callable_obj(*args, **kwargs)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 66, in _parse_certificate
    return Ed25519KeyCertificate(raw_contents, master_key_bytes, validate = validate)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 148, in __init__
    super(Ed25519KeyCertificate, self).__init__(raw_contents, identity_key, validate = False)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 132, in __init__
    self.__set_certificate_entries(raw_contents)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 136, in __set_certificate_entries
    for key, func in Certificate.ATTRIBUTES.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'

======================================================================
ERROR: test_parse_extensions_invalid_certificate_extension_type
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/atagar/Desktop/stem/test/unit/descriptor/certificate.py", line 57, in test_parse_extensions_invalid_certificate_extension_type
    cert_bytes
  File "/usr/lib/python3.2/unittest/case.py", line 1170, in deprecated_func
    return original_func(*args, **kwargs)
  File "/usr/lib/python3.2/unittest/case.py", line 1117, in assertRaisesRegex
    callable_obj(*args, **kwargs)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 80, in _parse_extensions
    n_extensions = _bytes_to_long(raw_contents[39:40])
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 44, in _bytes_to_long
    return long(b.encode('hex'), 16)
NameError: global name 'long' is not defined

======================================================================
ERROR: test_parse_extensions_invalid_n_extensions_count
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/atagar/Desktop/stem/test/unit/descriptor/certificate.py", line 72, in test_parse_extensions_invalid_n_extensions_count
    cert_bytes
  File "/usr/lib/python3.2/unittest/case.py", line 1170, in deprecated_func
    return original_func(*args, **kwargs)
  File "/usr/lib/python3.2/unittest/case.py", line 1117, in assertRaisesRegex
    callable_obj(*args, **kwargs)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 80, in _parse_extensions
    n_extensions = _bytes_to_long(raw_contents[39:40])
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 44, in _bytes_to_long
    return long(b.encode('hex'), 16)
NameError: global name 'long' is not defined

======================================================================
ERROR: test_parse_extensions_truncated_extension
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/atagar/Desktop/stem/test/unit/descriptor/certificate.py", line 43, in test_parse_extensions_truncated_extension
    cert_bytes
  File "/usr/lib/python3.2/unittest/case.py", line 1170, in deprecated_func
    return original_func(*args, **kwargs)
  File "/usr/lib/python3.2/unittest/case.py", line 1117, in assertRaisesRegex
    callable_obj(*args, **kwargs)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 80, in _parse_extensions
    n_extensions = _bytes_to_long(raw_contents[39:40])
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 44, in _bytes_to_long
    return long(b.encode('hex'), 16)
NameError: global name 'long' is not defined

======================================================================
ERROR: test_with_invalid_type
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/atagar/Desktop/stem/test/unit/descriptor/certificate.py", line 30, in test_with_invalid_type
    None
  File "/usr/lib/python3.2/unittest/case.py", line 1170, in deprecated_func
    return original_func(*args, **kwargs)
  File "/usr/lib/python3.2/unittest/case.py", line 1117, in assertRaisesRegex
    callable_obj(*args, **kwargs)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 74, in _parse_certificate
    raise ValueError("Unknown Certificate type %s" % cert_type.encode('hex'))
LookupError: unknown encoding: hex

======================================================================
ERROR: test_with_invalid_version
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/atagar/Desktop/stem/test/unit/descriptor/certificate.py", line 20, in test_with_invalid_version
    None
  File "/usr/lib/python3.2/unittest/case.py", line 1170, in deprecated_func
    return original_func(*args, **kwargs)
  File "/usr/lib/python3.2/unittest/case.py", line 1117, in assertRaisesRegex
    callable_obj(*args, **kwargs)
  File "/home/atagar/Desktop/stem/stem/descriptor/certificate.py", line 76, in _parse_certificate
    raise ValueError("Unknown Certificate version %s" % version.encode('hex'))
LookupError: unknown encoding: hex

----------------------------------------------------------------------
Ran 7 tests in 0.015s

FAILED (errors=6, skipped=1)

Mind taking a peek? To build on the work I've been doing please fetch from the ed25519 branch of my personal repo (https://git.torproject.org/user/atagar/stem.git)...

https://gitweb.torproject.org/user/atagar/stem.git/log/?h=ed25519

Cheers! -Damian

comment:9 Changed 3 months ago by patrickod

Ok. I've cleaned up the Python 3 issues.

The latest ed25519 branch is https://github.com/patrickod/stem/tree/patrickod/ed25519-cleanup

It also includes the commit from https://trac.torproject.org/projects/tor/ticket/21583 (with author attribution) to fix Python 3 issues with the cryptography module.

The Python 3.6.0+ test suites break on test.unit.manual.test_parsing_with_example due to some OrderedDict ordering issues in the comparison. They appear to be unrelated to the changes made here, though I'll investigate fixing them separately.

comment:10 Changed 3 months ago by atagar

Hi Patrick, many thanks! Just a quick update that I haven't forgotten this. Working on this branch a bit every day but it's a pretty big one so may take a while. :P

comment:11 Changed 3 months ago by atagar

Bah. Hoped to have this merged by now but sadly been too busy with work. Flying out on a trip until the 26th during which I won't be able to push but I plan to work on this branch a bit during the flight.

Just a quick update since I hate leaving pull requests in a lurch. Sorry about the delay, hopefully I'll finally get this out soon after I get back. Thanks again for putting this together! It's a great addition. :)

comment:12 Changed 3 months ago by atagar

  • Resolution set to implemented
  • Status changed from needs_review to closed

Sorry about the delay! Merged support for ed25519 certs.

Thanks Patrick! Honestly overhauled this quite a bit (additional tests, tweaks, and most notably pynacl is now only required for validaton, not parsing). That said, your work was *very* useful - especially the crypto bits. Many thanks Patrick!

comment:13 Changed 3 months ago by patrickod

Fantastic! Excited to see it merged. Thanks for shepherding it into master!

I'll have a look at the changes that you committed on top of my commits for some code style feedback and the like to prevent the need for such refactors for future patches, but if you have any other feedback on the patch that'd be very welcome :)

Note: See TracTickets for help on using tickets.