Parse `client_certificate_url` extension and support CertificateURL message #112

ashfall · 2016-12-01T13:16:39Z

A few things I'd love to hear thoughts on:

What is the correct place to validate the padding byte? Should/Can this be moved to tls.constructs and or be cleaner than what I have now?
When an extension’s extension_data is supposed to be empty (e.g. client_certificate_url), should it still be mentioned in tls._construct.Extension’s EnumSwitch? What I have now works, but I think @markrwilliams's idea of creating a helper extension-builder in tls._constructs will be useful here.
I've put a couple of XXXs in the code that could use some discussion. @reaperhulk had raised a good point when I was working on the TLS PRF that being too specific with the errors and exceptions is not always a good idea. Same could be true for the InvalidPaddingException

… extension

…ATE_URL extension

…when we serialize it, we get the extension_data.length bytes (if only '\x00\x00') properly

…ructs containing CertificateURL messages

coveralls · 2016-12-01T13:19:31Z

Coverage remained the same at 100.0% when pulling 3e1758a on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

markrwilliams

The PR looks great!

A couple of inline comments that kind of address the questions you asked.

Here's some more on two of those questions:

When an extension’s extendion_data is supposed to be empty (e.g. client_certificate_url), should it still be mentioned in tls._construct.Extension’s EnumSwitch? What I have now works, but I think @markrwilliams's idea of creating a helper extension-builder in tls._constructs will be useful here.

I think it could be. Let's write it up in a new PR and how it goes!!

@reaperhulk had raised a good point when I was working on the TLS PRF that being too specific with the errors and exceptions is not always a good idea. Same could be true for the InvalidPaddingException

I definitely don't want to leak sensitive data. As I mentioned in review comment, though, I really want tls to allow for meaningful error reporting. In particular I'd like tls to be usable as a library for people investigating TLS issues. openssl isn't great for such a use case :(

I think a good compromise is to start developing an exception hierarchy. We can start with a root TLSException from which all other exceptions inherit. Lower-level parsing code, like we're working on these days, can then raise very specific exceptions that higher-level code can always catch with except TLSError:. That code can then raise a new exception that contains suitable information. This would hopefully end up looking like Python 3's improved OSError hierarchy.

(Inheritance is of course not great, but Python's except mechanism seems to kind of expect it.)

markrwilliams · 2016-12-01T16:36:53Z

tls/exceptions.py

+
+
+class InvalidPaddingException(Exception):
+    # XXX: Is leaking this info in CertificateURL (i.e. revealing the specific


I think that at this level the code should be as specific as possible. I'd really like tls to have the ability to do better than OpenSSL alert handshake failure.

Upper level code can always suppress these exceptions.

Good idea, thanks! I'll add the TLSException definition, and spin up a new small PR to update UnsupportedExtensionException and UnsupportedCipherException to inherit from it as well.

markrwilliams · 2016-12-02T01:04:27Z

tls/message.py

+    padding = attr.ib()
+    sha1_hash = attr.ib()
+
+    # XXX: The "padding" byte MUST be 0x01.  It is


You could use an attrs validator here, as it'd cover both serialization and deserialization.

But since this is a parsing issue, it might be nice to fail early when reading potentially malicious data. You can use construct.OneOf to do that:

>>> construct.OneOf(construct.UBInt8('a'), [1]).parse('\x01') 1 >>> construct.OneOf(construct.UBInt8('a'), [1]).build(1) '\x01' >>> construct.OneOf(construct.UBInt8('a'), [1]).parse('\x02') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 188, in parse return self.parse_stream(BytesIO(data)) File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 198, in parse_stream return self._parse(stream, Container()) File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 288, in _parse return self._decode(self.subcon._parse(stream, context), context) File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 412, in _decode raise ValidationError("invalid object", obj) construct.adapters.ValidationError: ('invalid object', 2) >>> construct.OneOf(construct.UBInt8('a'), [1]).build(2) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 212, in build self.build_stream(obj, stream) File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 219, in build_stream self._build(obj, stream, Container()) File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 290, in _build self.subcon._build(self._encode(obj, context), stream, context) File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 415, in _encode return self._decode(obj, context) File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 412, in _decode raise ValidationError("invalid object", obj) construct.adapters.ValidationError: ('invalid object', 2)

Note that failing early on parsing means failing late on serialization. Maybe it's not a bad idea to have an attrs validator and construct.OneOf?

Thanks, this is useful! No obvious thoughts on failing early/late, let me play around with this and see what the tests look like. We can do multiple iterations on this until everyone is happy! Updates coming soon!

markrwilliams · 2016-12-02T01:31:09Z

tls/_common/enums.py

@@ -45,6 +45,8 @@ class HandshakeType(Enum):
    CERTIFICATE_VERIFY = 15
    CLIENT_KEY_EXCHANGE = 16
    FINISHED = 20
+    CERTIFICATE_URL = 21
+    CERTIFICATE_STATUS = 22


Do you think a later PR can make the Handshake construct use EnumSwitch to dispatch to the various handshake types?

Yes!! I'll play around with that next, blends in well with the #106 branch I was thinking of tackling.

coveralls · 2016-12-03T04:21:13Z

Coverage remained the same at 100.0% when pulling 17a3280 on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

… TLSValidationException that's not a ConstructError so that Range implementation (and hence TLSPrefixedArray) doesn't break

coveralls · 2016-12-03T05:54:43Z

Coverage decreased (-0.07%) to 99.931% when pulling 800ad1f on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

coveralls · 2016-12-03T10:14:12Z

Coverage remained the same at 100.0% when pulling 3ede2d6 on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

coveralls · 2016-12-03T10:21:14Z

Coverage remained the same at 100.0% when pulling fe03620 on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

coveralls · 2016-12-03T10:21:14Z

Coverage remained the same at 100.0% when pulling fe03620 on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

coveralls · 2016-12-03T10:21:14Z

Coverage remained the same at 100.0% when pulling fe03620 on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

coveralls · 2016-12-03T10:24:29Z

Coverage remained the same at 100.0% when pulling 77e451d on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

coveralls · 2016-12-03T10:37:56Z

Coverage remained the same at 100.0% when pulling 77e451d on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

markrwilliams

Some minor comments and well-deserved praise. I'm most particular about the test against the empty dict.

markrwilliams · 2016-12-05T15:24:45Z

tls/_common/_constructs.py

@@ -211,6 +213,35 @@ def EnumSwitch(type_field, type_enum, value_field, value_choices,  # noqa
                             default=default))


+class TLSExprValidator(construct.Validator):


This is a great way to implement TLSOneOf! We can reuse TLSExprValidator for other constructs. Thanks!!

:)

Took this idea from construct's OneOf.

markrwilliams · 2016-12-06T06:06:50Z

tls/_common/_constructs.py

+    :py:class:`tls.exceptions.TLSValidationException` instead of a
+    ``ConstructError`` subclass on mismatch.
+
+    This is necessary because any ConstructError signifies the end of


It might be nice to put an explanation like this into TLSExprValidator's doc string.

markrwilliams · 2016-12-06T06:07:34Z

tls/_common/test/test_constructs.py

+    Tests for :py:class:`tls._common._constructs.TLSExprValidator`.
+    """
+    @pytest.fixture
+    def data_class(self):


Good name - no need for # noqa. I think we should use this approach more!

markrwilliams · 2016-12-06T06:12:13Z

tls/test/test_hello_message.py

+        assert len(record.extensions) == 1
+        assert (record.extensions[0].type ==
+                enums.ExtensionType.CLIENT_CERTIFICATE_URL)
+        assert record.extensions[0].data == {}


I'd prefer this be

assert record.extensions[0].data == Container()

Since data is actually a Container and I prefer to forget that Container subclasses dict.

…g to '{}' (Container subclasses dict)

…mplement it

coveralls · 2016-12-06T06:47:57Z

Coverage remained the same at 100.0% when pulling dd69365 on ashfall:client_certificate_url into 5a7a0ec on pyca:master.

markrwilliams

Looks great!

ashfall added 16 commits December 1, 2016 06:49

Implement CertificateURL constructs

9c4e4f5

Add some new alerts to the AlertDescription enum

d43b754

Define an object representation of a URLAndHash struct

dbcb7db

Define CertificateURL.as_bytes()

6fa7c53

Something about not repeating yourself

8c732bb

A barebones CertificateURL.from_bytes

f1d59a1

Implement CertificateURL.from_bytes

eba6d74

Raise an exception when the padding byte is not 1

a322860

implement CertificateURL.as_bytes()

ca55815

whitespace

188d7a3

Remove duplicate note

c1917f2

Add a test to parse a ClientHello message with CLIENT_CERTIFICATE_URL…

41c8714

… extension

Add a test for serializing a ClientHello message with CLIENT_CERTIFIC…

346a63b

…ATE_URL extension

Define a minimal construct for the ClientCertificateURL extension so …

3aa50d1

…when we serialize it, we get the extension_data.length bytes (if only '\x00\x00') properly

pep8

4be9ced

Add CertificateURL as a handshake message type and parse Handshake st…

3e1758a

…ructs containing CertificateURL messages

ashfall changed the title ~~Parse client_certificate_url message and support CertificateURL message~~ Parse client_certificate_url extension and support CertificateURL message Dec 1, 2016

markrwilliams reviewed Dec 2, 2016

View reviewed changes

Define a TLSException that other more specific exceptions inherit from

17a3280

ashfall added 4 commits December 2, 2016 20:22

Validate padding early, and a failing test

18c869d

Define a custom TLSOneOf method that uses TLSExprValidator to raise a…

f839d07

… TLSValidationException that's not a ConstructError so that Range implementation (and hence TLSPrefixedArray) doesn't break

pep8 + docstrings

e9b6c84

Delete outdated import

800ad1f

ashfall added 3 commits December 3, 2016 01:59

Tests for TLSExprValidator and TLSOneOf

fe03620

Use list comprehension to build the url_and_hash_list

3ede2d6

Tell travis that subconstruct is a thing

cd8d518

remove inline XXX

77e451d

ashfall mentioned this pull request Dec 3, 2016

TLS ABC PEP draft Lukasa/peps#1

Open

markrwilliams reviewed Dec 6, 2016

View reviewed changes

ashfall added 2 commits December 5, 2016 22:40

Data is really a Container, asserting to that is better than assertin…

1cc0cd4

…g to '{}' (Container subclasses dict)

Update docstring for TLSExprValidator to include why we needed to rei…

dd69365

…mplement it

markrwilliams approved these changes Dec 6, 2016

View reviewed changes

markrwilliams merged commit d4f268a into python-tls:master Dec 6, 2016

ashfall deleted the client_certificate_url branch December 9, 2016 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse `client_certificate_url` extension and support CertificateURL message #112

Parse `client_certificate_url` extension and support CertificateURL message #112

ashfall commented Dec 1, 2016 •

edited

Loading

coveralls commented Dec 1, 2016 •

edited

Loading

markrwilliams left a comment

markrwilliams Dec 1, 2016

ashfall Dec 2, 2016

markrwilliams Dec 2, 2016

ashfall Dec 2, 2016

markrwilliams Dec 2, 2016

ashfall Dec 2, 2016

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016

coveralls commented Dec 3, 2016

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

markrwilliams left a comment

markrwilliams Dec 5, 2016

ashfall Dec 6, 2016

markrwilliams Dec 6, 2016

markrwilliams Dec 6, 2016

markrwilliams Dec 6, 2016

coveralls commented Dec 6, 2016 •

edited

Loading

markrwilliams left a comment



		class InvalidPaddingException(Exception):
		# XXX: Is leaking this info in CertificateURL (i.e. revealing the specific

		@@ -211,6 +213,35 @@ def EnumSwitch(type_field, type_enum, value_field, value_choices, # noqa
		default=default))


		class TLSExprValidator(construct.Validator):

Parse client_certificate_url extension and support CertificateURL message #112

Parse client_certificate_url extension and support CertificateURL message #112

Conversation

ashfall commented Dec 1, 2016 • edited Loading

coveralls commented Dec 1, 2016 • edited Loading

markrwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 3, 2016 • edited Loading

coveralls commented Dec 3, 2016 • edited Loading

coveralls commented Dec 3, 2016 • edited Loading

coveralls commented Dec 3, 2016 • edited Loading

coveralls commented Dec 3, 2016

coveralls commented Dec 3, 2016

coveralls commented Dec 3, 2016 • edited Loading

coveralls commented Dec 3, 2016 • edited Loading

markrwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 6, 2016 • edited Loading

markrwilliams left a comment

Choose a reason for hiding this comment

Parse `client_certificate_url` extension and support CertificateURL message #112

Parse `client_certificate_url` extension and support CertificateURL message #112

ashfall commented Dec 1, 2016 •

edited

Loading

coveralls commented Dec 1, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 3, 2016 •

edited

Loading

coveralls commented Dec 6, 2016 •

edited

Loading