delegated credentials

[gstarovo]

WIP: implementing delegated credentials client side

x509.py: Delegated Credential class and parsing; handshakesettings.py: DC in settings template; extensions.py: DC extension clientHello/Certificate

tlslite/tlsconnection.py: validating the delegated credential

tlslite/connection.py: alert added when server sends dc when client does not support it

tests/serverX509.pem: new certificate

---

This change is 

[tomato42]

Reviewed 3 of 6 files at r1, 2 of 4 files at r2, 1 of 1 files at r8.
Reviewable status: 6 of 8 files reviewed, 13 unresolved discussions

---

tlslite/extensions.py line 1354 at r8 (raw file):

class DelegatedCredentialExtension(TLSExtension):

to be consistent with other extensions it should be
DelegatedCredentialCertExtension
then the list of signatures can be called DelegatedCredentialExtension

---

tlslite/extensions.py line 1437 at r8 (raw file):

class SignatureSchemeExtension(_SigListExt):

as above: DelegatedCredentialExtension is more appropriate name

---

tlslite/tlsconnection.py line 1460 at r8 (raw file):

            cert_entry = certificate.certificate_list[0]
            if ExtensionType.delegated_credential in cert_entry.extensions:
                if settings.delegated_credential:

isn't this check inverted?

---

tlslite/tlsconnection.py line 2046 at r8 (raw file):

                    AlertDescription.illegal_parameter,
                    "The delegated credential is missing, though the peer "\
                    "provided extension for delegated credential."):

nope, the server is not obliged to send a delegated credential if client advertises support for it

---

tlslite/tlsconnection.py line 2051 at r8 (raw file):

            for result in self._sendError(
                    AlertDescription.illegal_parameter,
                    "Server sent multiple delegated credentials "\

\ not needed

---

tlslite/tlsconnection.py line 2057 at r8 (raw file):

        #  The algorithm field MUST be of a type advertised by the client in the
        # "signature_algorithms" extension of the ClientHello message

this while line should be before the comment

---

scripts/tls.py line 143 at r8 (raw file):

  --echo - function as an echo server
  --groups - specify what key exchange groups should be supported
  --dc KEY - the private key of the delegated credential

--dckey or maybe even --dc-keyand--dc-pub` I think would work better

---

scripts/tls.py line 144 at r8 (raw file):

  --groups - specify what key exchange groups should be supported
  --dc KEY - the private key of the delegated credential
  --dcpub KEY - the public key of the delegated credential

don't we need the whole delegated credential here? signature included (i.e. the DelegatedCredential struct)?

---

scripts/tls.py line 592 at r8 (raw file):

    if (cert_chain and not privateKey) or (not cert_chain and privateKey):
        raise SyntaxError("Must specify CERT and KEY together")

with delegated credentials we still present the certificate, but we shouldn't need the private key... shouldn't we?

---

tlslite/handshakesettings.py line 97 at r8 (raw file):

    SignatureScheme.rsa_pss_rsae_sha512]

# if the value not set manualy, 7 days (in seconds) will be used as default

"maximum validity period of a delegated credential"?

---

tlslite/handshakesettings.py line 466 at r8 (raw file):

        # TLS 1.3 RFC9345
        self.delegated_credential = []
        self.dc_valid_time = DC_VALID_TIME

doc text for them missing, also the first one seems to me like it should be called dc_sig_algs or something similar...

---

tlslite/handshakesettings.py line 683 at r8 (raw file):

        if other.delegated_credential in DELEGETED_CREDENTIAL_FORBIDDEN_ALG:
            raise ValueError("The usage of the algorithm is forbidden "
                             "to use with delegated criterias")

s/criterias/credentials/

---

.github/workflows/ci.yml line 23 at r8 (raw file):

            python-version: 2.7
          - name: py3.6
            os: ubuntu-22.04

please place those changes to a separate PR, it's a general issue that doesn't need to be blocked conditional on this PR getting merged

[tomato42]

(review incomplete will continue tomorrow)

[tomato42]

Reviewed 2 of 6 files at r1, 2 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @gstarovo)

---

tlslite/tlsconnection.py line 780 at r1 (raw file):

                settings.minVersion <= (3, 4):
            if settings.delegated_credential:
                extensions.append(SignatureSchemeExtension()\

nit: \ not needed

---

tlslite/tlsconnection.py line 1462 at r1 (raw file):

                    for result in self._sendError(
                            AlertDescription.unexpected_message,
                            "The server provided delegated credential, "\

nit: \ not needed

---

tlslite/tlsconnection.py line 2044 at r1 (raw file):

            for result in self._sendError(
                    AlertDescription.illegal_parameter,
                    "The delegated credential is missing, though the peer "\

nit: \ not needed

---

tlslite/tlsconnection.py line 2057 at r1 (raw file):

        # "signature_algorithms" extension of the ClientHello message

        sig_list = self._sigHashesToList(settings=settings, version=(3, 4))

I think it would be safer to just pull this list from the sent ClientHello...

---

tlslite/tlsconnection.py line 2061 at r1 (raw file):

            for result in self._sendError(
                    AlertDescription.illegal_parameter,
                    "The algorithm of the delegated credential "\

nit: \ not needed

---

tlslite/tlsconnection.py line 2070 at r1 (raw file):

                    AlertDescription.illegal_parameter,
                    "The dc_cert_verify_algorithm of the delegated "\
                    "credential algorithm must be a type "\

nit: \ not needed

---

tlslite/tlsconnection.py line 2082 at r1 (raw file):

                yield result

        seven_days = float(7 * 24 * 60 * 60)

no need to convert it to float

---

tlslite/tlsconnection.py line 2086 at r1 (raw file):

            for result in self._sendError(
                    AlertDescription.illegal_parameter,
                    "The expiry time exceeds the current time "\

nit: \ not needed

---

tlslite/tlsconnection.py line 2094 at r1 (raw file):

                    AlertDescription.illegal_parameter,
                    "The dc_cert_verify_algorithm does not match "\
                    "the scheme indicated in the peer's "\

nit: \ not needed

---

tlslite/tlsconnection.py line 2119 at r1 (raw file):

        except (TLSIllegalParameterException, TLSDecryptionFailed) as alert:
            for result in self._sendError(
                    AlertDescription.illegal_parameter, str(alert)):

invalid signatures generally should emit decrypt_error, is it overridden by the DC RFC?

---

tlslite/tlsconnection.py line 2135 at r1 (raw file):

        yield verifyBytes

    def _pub_key_verify(self, sig_scheme, sig, sig_context, pub_key):

I don't want to force you to do it, but the code is kind of screaming to refactor it so that we have signature verification in just one place in tlsconnection... ☹️

---

.github/workflows/ci.yml line 318 at r8 (raw file):

          sudo apt-get install -y \
            python2.7 python2.7-dev
          curl https://bootstrap.pypa.io/pip/2.7/get-pip.py --output get-pip.py

that should be in separate PR

---

tlslite/x509.py line 207 at r1 (raw file):

def _ecdsa_pubkey_parsing(subject_public_key_info):

why the change of order of definitions?

---

tlslite/x509.py line 227 at r1 (raw file):

def _dsa_pubkey_parsing(subject_public_key_info):

why the change of order of definitions?

---

tlslite/tlsconnection.py line 2126 at r2 (raw file):

        yield deleg_cred.cred.pub_key

    def _compute_certificate_dc_sig(self, cert_bytes, cred_bytes, dc_alg):

it's calculating sig context, not the sig itself, the name is confusing

---

tlslite/tlsconnection.py line 3545 at r2 (raw file):

        return session

    def _create_del_cred(self, private_key, dc_key, pub_key, \

again, I think this should be asynchronous... shouldn't it?

[gstarovo]

tlslite/tlsconnection.py line 2046 at r8 (raw file):

                    AlertDescription.illegal_parameter,
                    "The delegated credential is missing, though the peer "\
                    "provided extension for delegated credential."):

nope, the server is not obliged to send a delegated credential if client advertises support for it

The check is for controlling of the case, when the server provides extension of DC in Certificate, but not the DC itself. The error message is misleading, will change.

scripts/tls.py line 144 at r8 (raw file):

  --groups - specify what key exchange groups should be supported
  --dc KEY - the private key of the delegated credential
  --dcpub KEY - the public key of the delegated credential

don't we need the whole delegated credential here? signature included (i.e. the DelegatedCredential struct)?

I did not found a way/tool how to make a DC and provide them, so I have chosen to create them.

scripts/tls.py line 592 at r8 (raw file):

    if (cert_chain and not privateKey) or (not cert_chain and privateKey):
        raise SyntaxError("Must specify CERT and KEY together")

with delegated credentials we still present the certificate, but we shouldn't need the private key... shouldn't we?

The client may not indicate the usage of DC, or not support those sig_algs of DC that server has, then the normal Certificate Verification with certificate's private key will be used.
As well the DC is constructed inside, so the key is needed, to sign the DC.

tlslite/tlsconnection.py line 2119 at r1 (raw file):

        except (TLSIllegalParameterException, TLSDecryptionFailed) as alert:
            for result in self._sendError(
                    AlertDescription.illegal_parameter, str(alert)):

invalid signatures generally should emit decrypt_error, is it overridden by the DC RFC?

RFC 9345:

 If one or more of these checks fail, then the delegated credential is
   deemed not valid.  Clients and servers that receive non-valid
   delegated credentials MUST terminate the connection with an
   "illegal_parameter" alert.

One of those checks is validating the signature:

 Use the public key in the peer's end-entity certificate to verify
       the signature of the credential using the algorithm indicated by
       DelegatedCredential.algorithm.

Should I keep or change it?

tlslite/x509.py line 207 at r1 (raw file):

def _ecdsa_pubkey_parsing(subject_public_key_info):

why the change of order of definitions?

tlslite/x509.py line 227 at r1 (raw file):

def _dsa_pubkey_parsing(subject_public_key_info):

why the change of order of definitions?

I have put them outside the X509 class to use it in the DC class, but will make them a class methods.

[tomato42]

Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @gstarovo)

---

scripts/tls.py line 144 at r8 (raw file):

Previously, gstarovo wrote…

I did not found a way/tool how to make a DC and provide them, so I have chosen to create them.

I think it would require an additional tool that would do that

---

scripts/tls.py line 592 at r8 (raw file):

Previously, gstarovo wrote…

The client may not indicate the usage of DC, or not support those sig_algs of DC that server has, then the normal Certificate Verification with certificate's private key will be used.
As well the DC is constructed inside, so the key is needed, to sign the DC.

yes, but having a server that support only DCs is valid too

---

tlslite/tlsconnection.py line 2119 at r1 (raw file):

Previously, gstarovo wrote…

RFC 9345:

 If one or more of these checks fail, then the delegated credential is
   deemed not valid.  Clients and servers that receive non-valid
   delegated credentials MUST terminate the connection with an
   "illegal_parameter" alert.

One of those checks is validating the signature:

 Use the public key in the peer's end-entity certificate to verify
       the signature of the credential using the algorithm indicated by
       DelegatedCredential.algorithm.

Should I keep or change it?

aah, ok, then illegal_parameter is correct

---

tlslite/tlsconnection.py line 2046 at r8 (raw file):

Previously, gstarovo wrote…

The check is for controlling of the case, when the server provides extension of DC in Certificate, but not the DC itself. The error message is misleading, will change.

ok

---

tlslite/x509.py line 227 at r1 (raw file):

Previously, gstarovo wrote…

I have put them outside the X509 class to use it in the DC class, but will make them a class methods.

no, what I'm referring to, is just the order in which the functions are placed in the file, there's no reason to move them like that (it creates churn which makes review harder)

[tomato42]

Reviewed 13 of 13 files at r9, all commit messages.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @gstarovo)

---

tests/serverX509DCPub.pem line 9 at r9 (raw file):

PLZj8bP529nzDYEBHfsI7yB07LPNoc7vV1Ew7fQgDlh8kQlIuQ/WunTEuKLHEaUI
jwIDAQAB
-----END PUBLIC KEY-----

that's just a SubjectPublicKeyInfo... it's not signed...

---

tlslite/messages.py line 1143 at r9 (raw file):

    def cert_chain(self, cert_chain):
        """Setter for the cert_chain property."""
        cert_chain, ext = cert_chain

why passing it as tuple?

---

tlslite/messages.py line 1159 at r9 (raw file):

    def create(self, cert_chain, context=b'', ext=None):
        """Initialise fields of the class."""
        self.cert_chain = (cert_chain, ext)

same here, why a tuple?

---

tlslite/extensions.py line 1393 at r9 (raw file):

        writer.addOne(self.delegated_credential.algorithm[1])
        writer.add_var_bytes(self.delegated_credential.signature, 2)
        return writer.bytes

this doesn't look right to me... extData() should be the inverse of parse()... why we're not using serialisation from the DelegatedCredential class if we're using parsing from it?

---

tlslite/handshakesettings.py line 92 at r9 (raw file):

# in TLS 1.3
# When using RSA in delegaterd credential,

nit: s/delegaterd/delegated/

---

tlslite/tlsconnection.py line 1471 at r9 (raw file):

                for result in self._sendError(
                        AlertDescription.illegal_parameter,
                        "The server sent multiple delegated credentials "\

nit: \ not needed

---

tlslite/tlsconnection.py line 1472 at r9 (raw file):

                        AlertDescription.illegal_parameter,
                        "The server sent multiple delegated credentials "\
                        "extensions in a single CertificateEntry "):

no need for the space at the end of the message

---

tlslite/tlsconnection.py line 1475 at r9 (raw file):

                    yield result

            if cert_ext is not None:

that can be written as if cert_ext:

---

tlslite/tlsconnection.py line 2154 at r9 (raw file):

                        hash_name,
                        salt_len):
            raise TLSDecryptionFailed("server {} signature verification "

needs {0} to work in Py2.6

---

tlslite/tlsconnection.py line 3247 at r9 (raw file):

                    cred_bytes,
                    cert_scheme
                    )

this signature should be created externally, and the whole Credential read from file...

---

tlslite/tlsrecordlayer.py line 978 at r9 (raw file):

            if len(msg.write()) == 0:
                return

why is this line dropped?

---

tlslite/tlsrecordlayer.py line 1005 at r9 (raw file):

        if self._buffer_content_type is None:
            self._buffer_content_type = msg.contentType

why is this line dropped?

---

scripts/tls.py line 113 at r9 (raw file):

    [--reqcert] [--param DHFILE] [--psk PSK] [--psk-ident IDENTITY]
    [--psk-sha384] [--ssl3] [--max-ver VER] [--tickets COUNT] [--cipherlist]
    [--request-pha] [--require-pha] [--echo] [--groups GROUPS] [--dc-key KEY]

--dc-pub missing

---

scripts/tls.py line 252 at r9 (raw file):

            if sys.version_info[0] >= 3:
                s = str(s, 'utf-8')
            dc_pub = dePem(s, "PUBLIC KEY")

how does that work...? AFAIK PUBLIC KEY is for unsigned public keys...

---

scripts/tls.py line 648 at r9 (raw file):

        settings.keyShares = []
    if dc_key and dc_pub:
        settings.delegated_credential = [SignatureScheme.ed25519]

and what if it's not an Ed25519 certificate/key?

---

tlslite/session.py line 101 at r9 (raw file):

        self.tls_1_0_tickets = None
        self.ec_point_format = 0
        self.delegated_credential = None

doc string missing for it

---

tlslite/x509.py line 183 at r2 (raw file):

        return self.bytes

top level elements in a module must be separated by two white lines

---

tlslite/x509.py line 178 at r9 (raw file):

        :returns: A hex-encoded fingerprint.
        """
        return b2a_hex(SHA1(self.bytes))

a "fingerprint" doesn't automatically mean "SHA-1"...

1. it should be using SHA-256 at least
2. it should document what hash it's using in the doc string
3. it probably should allow specifying the hash function for that...

---

tlslite/x509.py line 315 at r9 (raw file):

    This class represents a credential.

    :vartype valid_time: uint32

that's not a Python type

---

tlslite/x509.py line 319 at r9 (raw file):

      credential is no longer valid.

    :vartype dc_cert_verify_algorithm: tlslite.contstants.SignatureScheme

this is not a type we use... it's an enum, shouldn't it be tuple(int,int)?

---

tlslite/x509.py line 388 at r9 (raw file):

    :ivar cred: the credential structure

    :vartype algorithm: SignatureScheme

again, I think this should be a tuple(int,int)

---

tests/tlstest.py line 3692 at r9 (raw file):

    settings.dc_sig_algs = [SignatureScheme.rsa_pkcs1_sha256]
    connection.handshakeServer(certChain=x509Chain,
                               privateKey=x509Key,

it needs to work without privateKey being set...

[tomato42]

Reviewed 14 of 15 files at r10, all commit messages.
Reviewable status: 19 of 20 files reviewed, 20 unresolved discussions (waiting on @gstarovo)

---

scripts/tls.py line 125 at r10 (raw file):

    [--psk-sha384] [--ssl3] [--max-ver VER] [--tickets COUNT] [--cipherlist]
    [--request-pha] [--require-pha] [--echo] [--groups GROUPS] [--dc-key KEY]
    [--dc-pub KEY] [--sig-alg-cert SIG] [--dc-sig-scheme SIG]

why we need to specify the signature algorithms for the server?

---

scripts/tls.py line 126 at r10 (raw file):

    [--request-pha] [--require-pha] [--echo] [--groups GROUPS] [--dc-key KEY]
    [--dc-pub KEY] [--sig-alg-cert SIG] [--dc-sig-scheme SIG]
    [--d-credential DCFILE] [--dc-output DCFILE]

and why we need --dc-key --dc-pub --d-credential and --dc-output?!

---

scripts/tls.py line 136 at r10 (raw file):

  credential
    [-c CERT] [-k KEY] [--dc-key KEY] [--dc-pub KEY] [--sig-alg-cert SIG]

why we need the private key of the delegated credential to sign it? and why two different signature algorithms?

---

scripts/tls.py line 166 at r10 (raw file):

  --sig-alg-cert SIG - signature scheme for signing the delegated credential
  --dc-sig-scheme SIG - delegated credential'signature scheme for signing
  --d-credential DCFILE - the file to write form delegated credential

--dc-out ? to keep the --dc- prefix consistent?

also, "the file to write the delegate credential" or "file to store the delegated credential" ?

---

scripts/tls.py line 167 at r10 (raw file):

  --dc-sig-scheme SIG - delegated credential'signature scheme for signing
  --d-credential DCFILE - the file to write form delegated credential
  --dc-output DCFILE - the file to read form delegated credential

"...to read the delegated..." ?

---

scripts/tls.py line 918 at r10 (raw file):

                    sig_alg = SignatureScheme.ecdsa_brainpoolP512r1tls13_sha512
            else:
                sig_alg = SignatureScheme.ecdsa_secp256r1_sha256

and ECDSA curves in TLS 1.3 are also linked to hashes, you can't sign with SHA-256 using P-384 curve

---

unit_tests/test_tlslite_utils_tripledes_split.py line 33 at r10 (raw file):

                       .filter(lambda split_pts: split_pts[0] <= split_pts[1])
                       .map(lambda lengths: [i * 8 for i in lengths])))

this line shouldn't get removed...

[tomato42]

The nits pointed out by pep8 in codeclimate are valid

Reviewed 1 of 15 files at r10, 7 of 7 files at r11, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gstarovo)

---

scripts/tls.py line 125 at r10 (raw file):

Previously, tomato42 (Alicja Kario) wrote…

why we need to specify the signature algorithms for the server?

I still don't follow why we need to specify the --dc-sig-scheme...

---

scripts/tls.py line 165 at r11 (raw file):

  --dc-key KEY - the private key of the delegated credential
  --dc-pub KEY - the public key of the delegated credential
  --dc-sig-scheme SIG - delegated credential'signature scheme for signing

nit: space missing in "credential'signature"

---

scripts/tls.py line 898 at r11 (raw file):

                sig_alg = SignatureScheme.ecdsa_brainpoolP512r1tls13_sha512
        else:
            sig_alg = SignatureScheme.ecdsa_secp256r1_sha256

this is still an issue...

[gstarovo]

scripts/tls.py line 125 at r10 (raw file):
Previously, tomato42 (Alicja Kario) wrote…

I still don't follow why we need to specify the --dc-sig-scheme...

In case the user wants the delegated credential to use a specific signature algorithm. For example, if the public key is RSA, the default signature algorithm is rsa_pss_pss_sha256, but the user can specify rsa_pss_pss_sha384 or rsa_pss_pss_sha512.

[tomato42]

Reviewed 8 of 8 files at r12, all commit messages.
Dismissed @gstarovo and @tomato42 from a discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gstarovo)

---

scripts/tls.py line 125 at r10 (raw file):

Previously, gstarovo wrote…

In case the user wants the delegated credential to use a specific signature algorithm. For example, if the public key is RSA, the default signature algorithm is rsa_pss_pss_sha256, but the user can specify rsa_pss_pss_sha384 or rsa_pss_pss_sha512.

aah, ok! perfect, thank you!

[tomato42]

OK, looks good, thank you very much for the contribution!