Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record size limit extension #338

Merged
merged 10 commits into from
Jan 16, 2019
Merged

Record size limit extension #338

merged 10 commits into from
Jan 16, 2019

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Jan 11, 2019

Implement record size limit extension from RFC 8449.


This change is Reviewable

@tomato42 tomato42 added the enhancement new feature to be implemented label Jan 11, 2019
@tomato42 tomato42 added this to the v0.8.0 milestone Jan 11, 2019
@tomato42 tomato42 self-assigned this Jan 11, 2019
@tomato42
Copy link
Member Author

This pull request fixes 3 alerts when merging c11fcda into 8243cf3 - view on LGTM.com

fixed alerts:

  • 3 for __eq__ not overridden when adding attributes

Comment posted by LGTM.com

@tomato42
Copy link
Member Author

This pull request fixes 3 alerts when merging 56c7c00 into 8243cf3 - view on LGTM.com

fixed alerts:

  • 3 for __eq__ not overridden when adding attributes

Comment posted by LGTM.com

@tomato42 tomato42 changed the title [WIP] Record size limit extension Record size limit extension Jan 15, 2019
@tomato42
Copy link
Member Author

This pull request fixes 3 alerts when merging 06cf5df into d61527a - view on LGTM.com

fixed alerts:

  • 3 for __eq__ not overridden when adding attributes

Comment posted by LGTM.com

Copy link
Collaborator

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 3 of 3 files at r3, 2 of 2 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tomato42)


tlslite/extensions.py, line 598 at r1 (raw file):

    @property
    def extData(self):
        """Return raw data encodin of the extension.

s/encodin/encoding/


unit_tests/test_tlslite_handshakesettings.py, line 419 at r4 (raw file):

        self.assertIn("record_size_limit", str(e.exception))

I would also add a test for the lower bound (< 64)

add abstract extension for handling extensions that have a single
integer value as their payload
make the string representation of few of them more readable in the process
to implement record size limit extension we need to be able to reject
records that are too large

at the same time, we need to provide updated hints to padding callbacks
when the sizes we can send are limited
make the server side of the test and client side of the test
have the same name
@tomato42
Copy link
Member Author

@ansasaki updated

@tomato42
Copy link
Member Author

This pull request fixes 3 alerts when merging 866017a into d61527a - view on LGTM.com

fixed alerts:

  • 3 for __eq__ not overridden when adding attributes

Comment posted by LGTM.com

Copy link
Collaborator

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+

Reviewed 12 of 12 files at r11, 3 of 3 files at r12, 3 of 3 files at r13, 2 of 2 files at r14, 1 of 1 files at r15, 2 of 2 files at r16, 1 of 1 files at r17, 1 of 1 files at r18, 1 of 1 files at r19, 1 of 1 files at r20.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tomato42)

@tomato42 tomato42 merged commit df7a087 into master Jan 16, 2019
@tomato42 tomato42 deleted the record_size_limit branch January 16, 2019 12:18
@tomato42
Copy link
Member Author

Thanks for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new feature to be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants