-
Notifications
You must be signed in to change notification settings - Fork 81
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
Point extension #517
base: master
Are you sure you want to change the base?
Point extension #517
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have test coverage to verify that
- it is actually negotiated (compressed representation is used)
- that if one side doesn't advertise support for compressed, that uncompressed is used
Reviewed 5 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @gstarovo)
.github/workflows/ci.yml
line 8 at r1 (raw file):
- master - tlslite-ng-0.7 - format_extension
we don't need this here, please squash it with the second commit
tlslite/handshakesettings.py
line 411 at r1 (raw file):
self.keyExchangeNames = list(KEY_EXCHANGE_NAMES) self.cipherImplementations = list(CIPHER_IMPLEMENTATIONS) self.ecPointFormats = [ECPointFormat.uncompressed,
nit: trailing whitespace
also, all new class fields should use snake_case
not camelCase
tlslite/handshakesettings.py
line 413 at r2 (raw file):
self.ecPointFormats = [ECPointFormat.uncompressed, ECPointFormat.ansiX962_compressed_char2, ECPointFormat.ansiX962_compressed_prime]
the values should be copied and validated that they are recognised, check how other settings are handled
tlslite/keyexchange.py
line 707 at r1 (raw file):
kex = ECDHKeyExchange(self.group_id, self.serverHello.server_version) self.ecdhXs = kex.get_random_private_key() ext_negotiated = 0
this shouldn't be a 0
, it should be ECPointFormat.uncompressed
to document that this is the fallback value
tlslite/keyexchange.py
line 714 at r1 (raw file):
for ext in ext_c.formats: if ext in ext_s.formats and ext_negotiated is None: ext_negotiated = ext
- we don't need to set ext_negotiated to None
- we're looking for first match, so after we find a value that is in both, we should
break
from the loop
tlslite/keyexchange.py
line 734 at r1 (raw file):
kex = ECDHKeyExchange(self.group_id, self.serverHello.server_version) ext_supported = [0]
we should be explicit that this is ECPointFormat.uncompressed
tlslite/keyexchange.py
line 761 at r1 (raw file):
ecdhXc = kex.get_random_private_key() ext_negotiated = 0 ext_supported = [0]
same here, please use ECPointFormat.uncompressed
tlslite/keyexchange.py
line 767 at r1 (raw file):
ext_supported = [] for ext in ext_c.formats: ext_negotiated = None
shouldn't this be outside the loop?
tlslite/keyexchange.py
line 957 at r1 (raw file):
return bytesToNumber(getRandomBytes(needed_bytes)) def calc_public_value(self, private, frm_negotiated=None):
the doc string should explain that this is added here for API compat and has no effect on FFDH
tlslite/keyexchange.py
line 981 at r1 (raw file):
return bytesToNumber(peer_share) def calc_shared_key(self, private, peer_share, frm_supported_=None):
doc string should explain that this is added for API compatibility, not because it's needed for FFDH
tlslite/keyexchange.py
line 1040 at r1 (raw file):
@staticmethod def _get_ext_name(ext): """Get extension name from the numeric value."""
it's not "extension name", it's "point format"?
tlslite/keyexchange.py
line 1041 at r1 (raw file):
def _get_ext_name(ext): """Get extension name from the numeric value.""" transform = {0: 'uncompressed', 1: 'compressed', 2: 'compressed'}
here too it would be better to use defines from ECPointFormat instead of magic values
tlslite/keyexchange.py
line 1046 at r1 (raw file):
def calc_public_value(self, private, frm_negotiated=0): """Calculate public value for given private key.""" extension = self._get_ext_name(frm_negotiated)
I think point_fmt
would work better here as the variable name
tlslite/keyexchange.py
line 1057 at r1 (raw file):
return bytearray(point.to_bytes(encoding=extension)) def calc_shared_key(self, private, peer_share, frm_supported_=set([0])):
why _
at the end of frm_supported_
?
also, the parameters should be documented in the doc string
tlslite/tlsconnection.py
line 748 at r1 (raw file):
group_id = getattr(GroupName, group_name) key_share = self._genKeyShareEntry(group_id, (3, 4), settings.ecPointFormats[0])
why this is needed?
tlslite/tlsconnection.py
line 766 at r1 (raw file):
groups.extend(self._curveNamesToList(settings)) extensions.append(ECPointFormatsExtension().\ create(settings.ecPointFormats))
if the list is empty, we shouldn't send an extensions
tlslite/tlsconnection.py
line 987 at r1 (raw file):
ext_negotiated = ext key_share = self._genKeyShareEntry(group_id, (3, 4), ext_negotiated)
key shares are TLS 1.2 specific, they don't use point format negotiation
see section 4.2.8.2 of RFC 8446:
Note: Versions of TLS prior to 1.3 permitted point format
negotiation; TLS 1.3 removes this feature in favor of a single point
format for each curve.
tlslite/tlsconnection.py
line 1187 at r1 (raw file):
@classmethod def _genKeyShareEntry(cls, group, version, ext_negotiated):
same here, not needed
tlslite/tlsconnection.py
line 1224 at r1 (raw file):
"advertised group.") kex = self._getKEX(sr_kex.group, self.version) ext_supported = set([0])
please us a define
tlslite/tlsconnection.py
line 2731 at r1 (raw file):
ext_negotiated = 0 ext_supported = set([0])
please use defines
tlslite/tlsconnection.py
line 2745 at r1 (raw file):
"No negotiated point extension") key_share = self._genKeyShareEntry(selected_group,
again, key shares are tls 1.3 specific
unit_tests/test_tlslite_keyexchange.py
line 2532 at r1 (raw file):
with self.assertRaises(NotImplementedError): kex.calc_shared_key(None, None, None)
those changes shouldn't be needed, the code should work the same as it did before introduction of the options
dd118da
to
d683eb5
Compare
aca7813
to
076956c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @gstarovo)
scripts/tls.py
line 420 at r3 (raw file):
for item in cipher.split(',')] # CHANGED settings.ec_point_formats = []
this seems like debugging code we shouldn't be including...
scripts/tls.py
line 575 at r3 (raw file):
# CHANGED settings.ec_point_formats = [2, 0]
same here, looks like debugging code to me...
tests/tlstest.py
line 312 at r3 (raw file):
connection.handshakeClientCert(settings=settings) testConnClient(connection) assert connection.session.ec_point_format == ECPointFormat.ansiX962_compressed_char2
This is actually incorrect: secp256r1
, secp384r1
and secp521r1
can use either uncompressed
or ansiX962_compressed_prime
, they can't use ansiX962_compressed_char2
the char2
encoding is for curves like the sect163r2
or the sect233r1
, but don't bother searching for them: we don't have support for "characteristic-2" curves, only for "prime field" curves
tlslite/handshakesettings.py
line 361 at r3 (raw file):
:vartype ec_point_formats: list :ivat ec_point_formats: Enabeled point format extension for
nit: Enabled
nit: :ivar
tlslite/handshakesettings.py
line 609 at r3 (raw file):
not 64 <= other.record_size_limit <= 2**14 + 1: raise ValueError("record_size_limit cannot exceed 2**14+1 bytes")
nit: whitespace
tlslite/handshakesettings.py
line 613 at r3 (raw file):
i not in EC_POINT_FORMATS] if bad_ec_ext: raise ValueError("Unknown ec point format extension: "
not a fan of phrasing it like this... maybe "Unknown EC point formats provided: {0}" ?
tlslite/session.py
line 78 at r3 (raw file):
:vartype ec_point_format: int :ivar ec_point_format: used ec point extension format; created for testing
don't think we need the last part; but I think I'd rephrase it to something like: "Used EC point format for the ECDH key exchange"
tlslite/tlsconnection.py
line 663 at r3 (raw file):
if ext_c and ext_s: ext_ec_point = [i for i in ext_c.formats \ if i in ext_s.formats][0]
If we go for iterators, then probably we should go for a solution that uses them fully, something like:
ext_ec_point = next((i for i in ext_c.formats if i in ext_s.formats)
and speaking of corner cases: we probably should handle the situation when there is no overlap: that's a protocol violation, but we need to detect it and handle correctly
tlslite/tlsconnection.py
line 776 at r3 (raw file):
else: extensions.append(ECPointFormatsExtension().\ create(list([ECPointFormat.uncompressed])))
this will still send the extension... why?
tlslite/tlsconnection.py
line 2287 at r3 (raw file):
else: extensions.append(ECPointFormatsExtension().\ create(list([ECPointFormat.uncompressed])))
didn't we agree that if the list is empty, the extension should be omitted?
tlslite/tlsconnection.py
line 2432 at r3 (raw file):
ext_ec_point = ECPointFormat.uncompressed if ext_c and ext_s: ext_ec_point = [i for i in ext_c.formats if i in ext_s.formats][0]
same here, next()
will be more pythonic
unit_tests/test_tlslite_keyexchange.py
line 23 at r3 (raw file):
from tlslite.constants import CipherSuite, CertificateType, AlertDescription, \ HashAlgorithm, SignatureAlgorithm, GroupName, ECCurveType, \ SignatureScheme, ECPointFormat
unused?
unit_tests/test_tlslite_keyexchange.py
line 37 at r3 (raw file):
from tlslite import VerifierDB from tlslite.extensions import SupportedGroupsExtension, SNIExtension, \ ECPointFormatsExtension
unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gstarovo)
tests/tlstest.py
line 310 at r4 (raw file):
settings.maxVersion = (3, 3) settings.eccCurves = ["secp256r1", "secp384r1", "secp521r1", "x25519", "x448"] settings.ec_point_formats = [ECPointFormat.ansiX962_compressed_prime, ECPointFormat.uncompressed]
why we need to set it? shouldn't this be the default?
tests/tlstest.py
line 2217 at r4 (raw file):
settings.maxVersion = (3, 3) settings.eccCurves = ["secp256r1", "secp384r1", "secp521r1", "x25519", "x448"] settings.ec_point_formats = [ECPointFormat.ansiX962_compressed_prime, ECPointFormat.uncompressed]
same here, this should be the default...
tests/tlstest.py
line 2221 at r4 (raw file):
privateKey=x509ecdsaKey, settings=settings) testConnServer(connection) print(connection.session.ec_point_format)
leftover debug ?
tlslite/tlsconnection.py
line 664 at r4 (raw file):
ext_ec_point = next((i for i in ext_c.formats \ if i in ext_s.formats), \ ECPointFormat.uncompressed)
no, this will just use uncompressed when there's no overlap, we can't do it like this, when there's no overlap we need to send an illegal_parameter
alert
tlslite/tlsconnection.py
line 2429 at r4 (raw file):
ext_ec_point = next((i for i in ext_c.formats \ if i in ext_s.formats),\ ECPointFormat.uncompressed)
same here, we need to abort in case of no overlap
failure in |
Yes, yes, I'm trying to understand why it is only with Python 3.6, or maybe the problem is somewhere else. |
I don't think it's py3.6 related, I think it's tackpy related |
line too long errors from codeclimate are valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 7 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gstarovo)
tlslite/keyexchange.py
line 1040 at r1 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
it's not "extension name", it's "point format"?
this doesn't look to be fixed...
tlslite/keyexchange.py
line 1114 at r10 (raw file):
ecdh.load_received_public_key_bytes(peer_share, valid_encodings= \ valid_encodings)
nit: as codeclimate indicates: over indented (also, the \
is not necessary inside ()
tlslite/tlsconnection.py
line 667 at r10 (raw file):
ext_ec_point = next((i for i in ext_c.formats \ if i in ext_s.formats))
nit: leftover whitespace
tlslite/tlsconnection.py
line 2440 at r10 (raw file):
ext_ec_point = next((i for i in ext_c.formats \ if i in ext_s.formats))
nit: leftover whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code itself is fine, but as it is now, it breaks ECDH in tlsfuzzer (as we've discussed in person), so technically it's incomplete
Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gstarovo)
There was a problem hiding this 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 r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gstarovo)
@gstarovo that fixes the tlsfuzzer issue? should I try it and if it works, merge this PR, or do you want to to some additional tweaks? |
dd3ba8a
to
82bfabc
Compare
7bffdb9
to
2659e77
Compare
line too long issues from codeclimate are valid |
check if the client adverties the uncompressed point format extension error when uncompressed is not supported codeclimat
2659e77
to
818a4de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 4 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gstarovo)
fixes #103
This change is