Add support for AES-CCM TLS1.2 and 1.3 ciphers, both with full and tr…

[inikolcev]

…uncated tag.
This fixes #205

---

This change is 

[lgtm-com]

This pull request introduces 4 alerts when merging 5416765 into 75dbe54 - view on LGTM.com

new alerts:

• 4 for Unused import

[tomato42]

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 30 unresolved discussions (waiting on @inikolcev)

a discussion (no related file):
FYI, if you use "fixes", instead of "addresses" then the bug will get auto-closed when the PR is merged

---

a discussion (no related file):
please address the issues from lgtm.com

---

a discussion (no related file):
This is rather pushing the definition of "Make commits of logical units and describe them properly in commits"...

---

a discussion (no related file):

createAESCCM

test coverage missing to verify that the default implementation list is ["python"]

---

---

tests/tlstest.py, line 1910 at r1 (raw file):

                continue
            if cipher in ("aes128ccm", "aes128ccm_8",
                          "aes256ccm", "aes256ccm_8") and \

why not combine it with the "chacha20-poly1305" test?
afaics, the chacha was separate from aes-gcm because aes-gcm also has pycrypto implementation

---

tlslite/constants.py, line 1237 at r1 (raw file):

    certSuites.append(TLS_RSA_WITH_AES_256_CCM)
    certSuites.append(TLS_RSA_WITH_AES_128_CCM_8)
    certSuites.append(TLS_RSA_WITH_AES_256_CCM_8)

the order matters, please put the ones with the 16 byte tag after GCM and the one with 8 byte tag after CBC

---

tlslite/constants.py, line 1261 at r1 (raw file):

    dheCertSuites.append(TLS_DHE_RSA_WITH_AES_256_CCM)
    dheCertSuites.append(TLS_DHE_RSA_WITH_AES_128_CCM_8)
    dheCertSuites.append(TLS_DHE_RSA_WITH_AES_256_CCM_8)

the order matters, please put the ones with the 16 byte tag after GCM and the one with 8 byte tag after CBC

---

tlslite/constants.py, line 1306 at r1 (raw file):

    ecdheEcdsaSuites.append(TLS_ECDHE_ECDSA_WITH_AES_256_CCM)
    ecdheEcdsaSuites.append(TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8)
    ecdheEcdsaSuites.append(TLS_ECDHE_ECDSA_WITH_AES_256_CCM_8)

the order matters, please put the ones with the 16 byte tag after GCM and the one with 8 byte tag after CBC

---

tlslite/handshakesettings.py, line 18 at r1 (raw file):

                "aes256gcm", "aes128gcm",
                "aes256", "aes128",
                "3des", "aes128ccm", "aes128ccm_8", "aes256ccm", "aes256ccm_8"]

the order matters, please put the ones with the 16 byte tag after GCM
and I don't think we want to enable the ones with 8 byte tag by default, please add them to ALL_CIPHER_NAMES

---

tlslite/utils/aesccm.py, line 16 at r1 (raw file):

    # AES-CCM implementation per RFC3610

    def __init__(self, key, implementation, rawAesEncrypt, tagLength=16):

camelCase is unpythonic for parameter names and variable names, please use snake_case

---

tlslite/utils/aesccm.py, line 17 at r1 (raw file):

    def __init__(self, key, implementation, rawAesEncrypt, tagLength=16):
        self.isBlockCipher = False

but keep this camelCase here, for isAEAD and tagLength, as it's part of the interface that ciphers need to implement

---

tlslite/utils/aesccm.py, line 22 at r1 (raw file):

        if len(self.key) == 16 and tagLength == 8:
            self.name = "aes128ccm_8"
        elif len(self.key) == 16:

shouldn't we check that the tag length is 16 here?

---

tlslite/utils/aesccm.py, line 29 at r1 (raw file):

            self.name = "aes256ccm"
        else:
            raise AssertionError()

wouldn't writing it as:

        elif len(self.key) == 32 and tagLength == 8:
            self.name = "aes256ccm_8"
        else:
            assert len(self.key) == 32 and tagLength == 16
            self.name = "aes256ccm"

make it easier to provide 100% branch coverage?

---

tlslite/utils/aesccm.py, line 30 at r1 (raw file):

        else:
            raise AssertionError()
        self.rawAesEncrypt = rawAesEncrypt

why keep it public?

---

tlslite/utils/aesccm.py, line 36 at r1 (raw file):

    def _pad_with_zeroes(self, data):

couldn't this be a @staticmethod?

---

tlslite/utils/aesccm.py, line 47 at r1 (raw file):

        flags = 64 * (len(aad) > 0)
        flags += 8 * ((self.tagLength - 2) // 2)
        flags += 1 * (L - 1)

this code definitely warrants a comment to explain what it is doing...

---

tlslite/utils/aesccm.py, line 90 at r1 (raw file):

pep8: there should be just one line of whitespace between fields of a class

---

tlslite/utils/aesccm.py, line 119 at r1 (raw file):

        enc_msg = bytearray(len(msg))

        for i in range(0, len(msg)):

0 is the default min value for range()

but indexing stings (x[i]) is fairly expensive, why not do the following instead:

enc_msg = bytearray(i ^ j for i, j in zip(msg, s_n))

also, given that this is an internal loop, I wonder if we couldn't gain a lot by iterating over long longs rather than unsigned chars...
something like:

msg_mv = memoryview(msg).cast('Q')
s_n_mv = memoryview(s_n).cast('Q')
enc_msg = array.array('Q', i ^ j for i, j in zip(msg_mv, s_n_mv))
enc_bytes = enc.msg.tobytes()

then the number of xors we need to perform would be reduced by a factor of 8... (albeit only on python3)

---

tlslite/utils/aesccm.py, line 157 at r1 (raw file):

        for i in range(1, int(counter_lmt) + 1):
            s_n += self.rawAesEncrypt(bytearray([flags]) +
                   nonce + numberToByteArray(i, L))

that's a copy-pasta from seal()?
AFAICT, it's the same AES-CTR as is in AES-GCM, shouldn't we reuse the code from there? (as in, modify aes-gcm so that it uses shared code?)

---

tlslite/utils/cipherfactory.py, line 79 at r1 (raw file):

def createAESCCM(key, implList=None):
    """ Create a new AESCCM object.

nit: for consistency we keep the line with """ of a multi-line doc string empty

---

tlslite/utils/cipherfactory.py, line 82 at r1 (raw file):

    :type key: bytearray
    :param key: A 16 byte byte array.

yes, but what's it used for? and what about aes256ccm?

---

tlslite/utils/cipherfactory.py, line 99 at r1 (raw file):

def createAESCCM_8(key, implList=None):
    """ Create a new AESCCM object with
    truncated tag.

pep257: the summary line of multi-line commit should be on a line of its own

---

tlslite/utils/cipherfactory.py, line 102 at r1 (raw file):

    :type key: bytearray
    :param key: A 16 byte byte array.

again, what it's used for? and what about aes256ccm?

---

tlslite/utils/python_aesccm.py, line 10 at r1 (raw file):

def new(key, tagLength=16):
    return AESCCM(key, "python", rijndael(key, 16).encrypt, tagLength)

rijndael is the deprecated name of this class, please use Rijndael

---

unit_tests/test_tlslite_utils_aesccm.py, line 19 at r1 (raw file):

        self.assertIsNotNone(aesCCM)

pep8 nit: the class field definitions should be separated by a single line of whitespace

---

unit_tests/test_tlslite_utils_aesccm.py, line 38 at r1 (raw file):

        encData = aesCCM.seal(nonce, plaintext, bytearray(0))

        self.assertEqual(bytearray(b'%}Q.\x99\xa3\r\xae\xcbMc\xf2\x16,^\xff' +

unnecessary + for byte literals inside ()
(same for other instances of it)

---

unit_tests/test_tlslite_utils_aesccm.py, line 42 at r1 (raw file):

                                   b'\xe3\xb8\xa2'), encData)

    def test_seal_trunc(self):

shouldn't that be "_small_tag"?

also, I think it would be nice to have the same tests (this and the test_seal) but with 256 bit keys

---

unit_tests/test_tlslite_utils_aesccm.py, line 66 at r1 (raw file):

        self.assertEqual(len(plaintext), 16)

        with self.assertRaises(ValueError):

if we're raising a generic error, I'd say we should verify the message is the expected one

---

unit_tests/test_tlslite_utils_aesccm.py, line 83 at r1 (raw file):

        self.assertEqual(plaintext, bytearray(b'text to encrypt.'))

    def test_open_trunc(self):

as with seal: _small_tag and 128 vs 256

---

unit_tests/test_tlslite_utils_aesccm.py, line 136 at r1 (raw file):

            b'z1:\xf6}\xa7\\@\xba\x11\xd8r\xdf#K\xd4')

        with self.assertRaises(ValueError):

same as with seal() - check message

---

unit_tests/test_tlslite_utils_aesccm.py, line 244 at r1 (raw file):

        aesCCM = AESCCM(key, "python", Rijndael(key, 16).encrypt)

        self.assertEqual(aesCCM.name, "aes256ccm")

shouldn't this be tested with in the __init__ test?

---

unit_tests/test_tlslite_utils_aesccm.py, line 260 at r1 (raw file):

        aesCCM = AESCCM(key, "python", Rijndael(key, 16).encrypt)

        self.assertEqual(aesCCM.name, "aes256ccm")

same here, shouldn't this be in __init__ test?

[lgtm-com]

This pull request introduces 2 alerts when merging 7867f4f into 75dbe54 - view on LGTM.com

new alerts:

• 2 for Variable defined multiple times

[tomato42]

Reviewed 1 of 10 files at r2, 9 of 9 files at r3.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @inikolcev)

---

tlslite/tlsconnection.py, line 2355 at r3 (raw file):

                cipher = createAESCCM(key, settings.cipherImplementations)
            elif settings.ticketCipher in ("aes128ccm_8", "aes256ccm_8"):
                cipher = createAESCCM_8(key, settings.cipherImplementations)

do we have test coverage for tickets encrypted with AES-CCM?

---

tlslite/utils/aesccm.py, line 72 at r3 (raw file):

        # we need to run in through AES-CBC with 0 IV

        cbc = Python_AES(self.key, 2, bytearray(b'\x00' * 16))

doesn't it cause new key derivation for every encrypted and decrypted AEAD ciphertext?
couldn't we reuse the same Python_AES object for multiple MAC calculations? that should improve performance quite significantly

---

tlslite/utils/aesccm.py, line 143 at r3 (raw file):

        # We decrypt the message
        for i in range(0, len(ciphertext) - self.tagLength):
            msg[i] = ciphertext[i] ^ s_n[i]

here we should do the same memoryview stuff as we do in seal()

---

tlslite/utils/aesccm.py, line 149 at r3 (raw file):

        # We decrypt the auth value
        for i in range(0, self.tagLength):

nit: this is equivalent to rage(self.tagLength):

---

tlslite/utils/aesccm.py, line 159 at r3 (raw file):

    def _construct_s_n(self, ciphertext, flags, nonce, L):
        s_n = bytearray()
        if len(ciphertext) % 16 == 0:

divceil() would probably be cleaner

[tomato42]

regarding counter mode and where the "2" came from in CBC: https://www.python.org/dev/peps/pep-0272/

[lgtm-com]

This pull request introduces 1 alert when merging 4725c26 into 3ea78db - view on LGTM.com

new alerts:

• 1 for Variable defined multiple times

[tomato42]

Reviewed 10 of 10 files at r4, 9 of 9 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @inikolcev)

---

tests/tlstest.py, line 1096 at r5 (raw file):

    connection = connect()
    settings = HandshakeSettings()
    settings.minVersion = (3,4)

nit: space missing after , in tuple

---

tests/tlstest.py, line 1099 at r5 (raw file):

    # force HRR
    settings.keyShares = []
    settings.ticketCipher = "aes128ccm"

that's not necessary for client side configuration – client has no way of knowing what kind of encryption server uses

---

tests/tlstest.py, line 1112 at r5 (raw file):

    synchro.recv(1)
    settings = HandshakeSettings()
    settings.minVersion = (3,4)

nit: space missing after , in tuple

---

tests/tlstest.py, line 2177 at r5 (raw file):

    connection = connect()
    settings = HandshakeSettings()
    settings.minVersion = (3,4)

nit: space missing after , in tuple

---

tlslite/utils/aesccm.py, line 33 at r4 (raw file):

        self.implementation = implementation
        self.nonceLength = 12
        self.cbc = Python_AES(self.key, 2, bytearray(b'\x00' * 16))

shouldn't this be a private field, like _rawAesEncrypt?

---

tlslite/utils/aesccm.py, line 154 at r4 (raw file):

    def _xor(self, inp, s_n):
        if sys.version_info[0] >= 3:

shouldn't we rather define different method based on which python we run under? like:

if PY3:
    def foo():
        pass
else:
    def foo():
        pass

[tomato42]

Reviewed 10 of 10 files at r6, 9 of 9 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @inikolcev)

---

tlslite/utils/aesccm.py, line 104 at r6 (raw file):

        if sys.version_info[0] >= 3:
            enc_msg = self._xor(msg, s_n)

no, I meant that this should happen during class definition, like here:
https://github.com/tomato42/tlslite-ng/blob/3d63ee4ed3f1cd8ee3564face508cb2dc5f257a3/tlslite/utils/codec.py#L13-L50

[tomato42]

Reviewed 10 of 10 files at r8, 9 of 9 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @inikolcev)

[tomato42]

now, question: you'd prefer to merge it now and risk need for fixup commits later or would you prefer to create a tlsfuzzer test case, test it manually locally and only when both are ready, merge this one?

[inikolcev]

now, question: you'd prefer to merge it now and risk need for fixup commits later or would you prefer to create a tlsfuzzer test case, test it manually locally and only both are ready, merge this one?

Lets leave it for now, I'll write the fuzzing test first and if it is all good we can merge it.

[tomato42]

Reviewed 10 of 10 files at r10, 9 of 9 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @inikolcev)