python-tls / tls

A pure Python implementation of the Transport Layer Security protocol version 1.2, using existing libraries for crypto math.
Other
164 stars 44 forks source link

Add an optional length_field_size for use with TLS vectors that use a… #110

Closed ashfall closed 7 years ago

ashfall commented 7 years ago

… length prefix that's not UBInt16 format.

I need this for https://github.com/pyca/tls/issues/109, Certificate uses UBInt32 to represent the length of the certificate_list.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling e74c0a4f31431063bef97facd3a635f1f280b224 on ashfall:custom-len-tlsprearr into 5d48cc44364d44ba7251e470168276638224de6f on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling e74c0a4f31431063bef97facd3a635f1f280b224 on ashfall:custom-len-tlsprearr into 5d48cc44364d44ba7251e470168276638224de6f on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling e74c0a4f31431063bef97facd3a635f1f280b224 on ashfall:custom-len-tlsprearr into 5d48cc44364d44ba7251e470168276638224de6f on pyca:master.

ashfall commented 7 years ago

@markrwilliams Do you have thoughts about how to add the non-default length case test? I feel like the whole point of using fixtures is to avoid having to repeat writing similar tests with different sets of inputs, and yet I keep defaulting to creating a new test case for the non-default case and repeating code. How would you do it?

markrwilliams commented 7 years ago

@ashfall I spent some time thinking about this. I'm no py.test expert, and I don't think I have a great answer, but I'll share what I know.

Let's consider a test like the one you linked to:

import pytest
import construct

@pytest.mark.parametrize(
    'obj,serialized',
    [(1, '\x01')],
)
class TestSerialization(object):
    @pytest.fixture
    def con(self):
        return construct.UBInt8("A")

    def test(self, con, obj, serialized):
        assert con.build(obj) == serialized

We can't do this:

import pytest
import construct

@pytest.fixture
def con():
    return construct.UBInt8("A")

@pytest.mark.parametrize(
    'obj,serialized,con',
    [(1, '\x01', con)],
)
class TestSerialization(object):
    def test(self, obj, serialized, con):
        assert con.build(obj) == serialized 

Because con is a py.test fixture, not an actual object.

We don't want to do this:

import pytest
import construct

@pytest.fixture
def con():
    return construct.UBInt8("A")

@pytest.mark.parametrize(
    'obj,serialized',
    [(1, '\x01')],
)
class TestSerialization(object):
    def test(self, obj, serialized, con):
        assert con.build(obj) == serialized

Because we've still hard coded the association between con and our test.

We could use parametrized fixtures:

@pytest.fixture(params=[construct.UBInt8("A")])
def con(request):
    return request.param

@pytest.mark.parametrize(
    'obj,serialized',
    [(1, '\x01')],
)
class TestSerialization(object):
    def test(self, obj, serialized, con):
        assert con.build(obj) == serialized

But this will break if we add another construct to con, because it'll run our parametrized inputs against all the parameters:

import pytest
import construct

@pytest.fixture(params=[construct.UBInt8("A"),
                        construct.UBInt16("B")])
def con(request):
    return request.param

@pytest.mark.parametrize(
    'obj,serialized',
    [(1, '\x01')],
)
class TestSerialization(object):
    def test(self, obj, serialized, con):
        assert con.build(obj) == serialized

#     def test(self, obj, serialized, con):
# >       assert con.build(obj) == serialized
# E       assert '\x00\x01' == '\x01'

This test already relies on the fact that constructs are immutable, so we could do this:

import pytest
import construct

UBINT8 = construct.UBInt8("A")
UBINT16 = construct.UBInt16("B")

@pytest.mark.parametrize(
    'obj,serialized,con',
    [
        (1, '\x01', UBINT8),
        (1, '\x00\x01', UBINT16),
    ],
)
class TestSerialization(object):
    def test(self, obj, serialized, con):
        assert con.build(obj) == serialized

But I didn't really like relying static, global "fixtures".

We could also do this:

import pytest
import construct

UBINT8 = construct.UBInt8("A")
UBINT16 = construct.UBInt16("B")

@pytest.fixture(params=[
        (1, '\x01', UBINT8),
        (1, '\x00\x01', UBINT16),
    ])
def conArgs(request):
    return request.param

class TestSerialization(object):
    def test(self, conArgs):
        obj, serialized, con = conArgs
        assert con.build(obj) == serialized

But unpacking the arguments in the test seemed hard to read.

Historically py.test hasn't had a good way to nest fixtures, which is what we really want. For that reason, and also to make selecting individual tests easier, I've favored the same approach you followed.

It turns out a py.test plugin recently became available that might help! It's called pytest-lazy-fixture. Note that it needs a very recent version of py.test -- I used 3.0.5. Here's how it allows us to rewrite our test:

import pytest
import construct

@pytest.fixture
def UBInt8A():
    return construct.UBInt8("A")

@pytest.fixture
def UBInt16B():
    return construct.UBInt16("B")

@pytest.mark.parametrize(
    "obj,serialized,con",
    [
        (1, '\x01', pytest.lazy_fixture("UBInt8A")),
        (1, '\x00\x01', pytest.lazy_fixture("UBInt16B")),
    ])
class TestSerialization(object):
    def test(self, obj, serialized, con):
        assert con.build(obj) == serialized

This is pretty nice, but maybe too magical. I'm also not sure how you'd just run the UBInt16 case. What do you think?

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 9aaef9742fc016206a0ca7bfb87ceadb7eacf82e on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 51d8bfb06043d21567ff552b102e02d3fad892dc on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 51d8bfb06043d21567ff552b102e02d3fad892dc on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 51d8bfb06043d21567ff552b102e02d3fad892dc on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 51d8bfb06043d21567ff552b102e02d3fad892dc on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 51d8bfb06043d21567ff552b102e02d3fad892dc on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 0ed946cd39325d45d0dc560f7b7f796de2b38850 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 0ed946cd39325d45d0dc560f7b7f796de2b38850 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 0ed946cd39325d45d0dc560f7b7f796de2b38850 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 0ed946cd39325d45d0dc560f7b7f796de2b38850 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 50c8397f471791a064d8d4fa1632289b1d524553 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 50c8397f471791a064d8d4fa1632289b1d524553 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 216c5e30593950c3fed5737c9569f4bcb39b266d on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 216c5e30593950c3fed5737c9569f4bcb39b266d on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 216c5e30593950c3fed5737c9569f4bcb39b266d on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 5d04708be1edc5f51ff4ce64868252efc557be70 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 5d04708be1edc5f51ff4ce64868252efc557be70 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 5d04708be1edc5f51ff4ce64868252efc557be70 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 577060b4f7c6e39bbc91b18a744848ab26f54d37 on ashfall:custom-len-tlsprearr into dfc6109caf06c8f43fd03d64684c8f3126ff0f7a on pyca:master.