This PR adds cross signatures to TRCs.

[sezergueler]

When generating TRC in generator.py the cross signatures from neighboring ISDs
(1 CA, 1 root ASes, RAINS) are added to the TRC. Functionality for checking
those cross-signatures and verification of trust chains from one TRC to a
different TRC was added.

Also:

• Synchronizes CA and RAINS part of TRC with the book (Adds keys for RAINS and CAs).
• Removes logging from library and replaces them with exceptions.

---

This change is 

[sezergueler]

@pszalach Can you review, please?

[pszal]

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

lib/crypto/trc.py, line 27 at r1 (raw file):

# External
import lz4
from OpenSSL import crypto

as we decided to use cryptography.io (see #1003), I'm wondering whether this lib is needed...

---

lib/crypto/trc.py, line 315 at r1 (raw file):

        return ases

    def _parse_subject_str(self, subject):

it makes sense to describe formats here

---

lib/crypto/trc.py, line 317 at r1 (raw file):

    def _parse_subject_str(self, subject):
        sub = subject.split(',', 1)
        # We have a CA or rains as subject

local signatures are not captured here?

---

lib/crypto/trc.py, line 322 at r1 (raw file):

            if sub[1].strip() == "RAINS":
                return "RAINS", isd, ""
            else:

check for CA

elif ... == "CA":
...
else
    logging.error(...)

---

lib/crypto/trc.py, line 325 at r1 (raw file):

                ca = sub[1].split(':')[1].strip()
                return "CA", isd, ca
        # We have an AS

local, remote, or any AS/

---

lib/crypto/trc.py, line 388 at r1 (raw file):

        return True
    # Try to verify with verified remote TRCs
    for trc in verified_rem_trcs:

would it verify TRC chains longer than 2?

---

lib/crypto/trc.py, line 429 at r1 (raw file):

        if remote_trc.verify_signature(signature, pub_key):
            return True
    logging.warning("Remote TRC(ISD %s) is not(correctly) signed by a core"

error

---

lib/crypto/trc.py, line 473 at r1 (raw file):

            crypto.verify(cert, signature, remote_trc.sig_input(), "sha256")
            return True
        except crypto.Error:

logging.error/warning?

---

lib/crypto/trc.py, line 475 at r1 (raw file):

        except crypto.Error:
            continue
    logging.warning("Remote TRC(ISD %s) is not(correctly) signed by a CA"

error

---

topology/generator.py, line 418 at r1 (raw file):

        neighbor_isds = defaultdict(set)
        for link in self.topo_config["links"]:
            a = ISD_AS(link["a"])

assign a = ISD_AS(link["a"])[0] (b analogically ). Then the second if is much more readable.

---

Comments from Reviewable

[pszal]

@sezergueler a few comments after the first round.

---

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[sezergueler]

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

lib/crypto/trc.py, line 27 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

as we decided to use cryptography.io (see #1003), I'm wondering whether this lib is needed...

I am using the verify function from OpenSSL here. It looks like cryptography.io does not support this yet: pyca/cryptography#2381. They have a PR for this opened almost 2 years ago, but looks like it will take some time until it will be supported: pyca/cryptography#2460.
I am not sure, but I think we can use cryptography.io everywhere else(also for CA creation) and use the verification method from OpenSSL when needed.

---

lib/crypto/trc.py, line 315 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

it makes sense to describe formats here

Done.

---

lib/crypto/trc.py, line 317 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

local signatures are not captured here?

All signatures are captured, this function only parses the 3 different subject types(CA, RAINS, AS). I added some additional checks such that it does not fail if a subject string can not be parsed.

---

lib/crypto/trc.py, line 322 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

check for CA

elif ... == "CA":
...
else
    logging.error(...)

Done.

---

lib/crypto/trc.py, line 325 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

local, remote, or any AS/

Done.

---

lib/crypto/trc.py, line 388 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

would it verify TRC chains longer than 2?

This is a good point. The idea is the following: All TRCs that are already verified would be provided by the caller of verify_trc_chain within the list verified_rem_trcs. If we have ISDs connected like this: 1-2-3-4, and ISD 1 wants to check if there is a trust chain to 4, it will provide TRC 2 and 3 in the list and call the function. This works if TRCs 2 and 3 arrived before TRC 4.
Thus, if TRC 4 arrives before TRCs 2 and 3, ISD 1 will try to verify based on the TRCs it already received(in this case none). If it can not be verified, TRC 4 will appended to a list(unverified_trcs) and after some time it will be tried again.

The problem here is, that we do not have topology information. We do not know, which intermediate TRCs we need to verify TRC 4. I think this is the best we can do here.

---

lib/crypto/trc.py, line 429 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

error

Done.

---

lib/crypto/trc.py, line 473 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

logging.error/warning?

Done.

---

lib/crypto/trc.py, line 475 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

error

Done.

---

topology/generator.py, line 418 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

assign a = ISD_AS(link["a"])[0] (b analogically ). Then the second if is much more readable.

Done.

---

Comments from Reviewable

[sezergueler]

@pszalach Thanks for review! I addressed the comments.

[sezergueler]

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

lib/crypto/trc.py, line 388 at r1 (raw file):

Previously, sezergueler wrote…

This is a good point. The idea is the following: All TRCs that are already verified would be provided by the caller of verify_trc_chain within the list verified_rem_trcs. If we have ISDs connected like this: 1-2-3-4, and ISD 1 wants to check if there is a trust chain to 4, it will provide TRC 2 and 3 in the list and call the function. This works if TRCs 2 and 3 arrived before TRC 4.
Thus, if TRC 4 arrives before TRCs 2 and 3, ISD 1 will try to verify based on the TRCs it already received(in this case none). If it can not be verified, TRC 4 will appended to a list(unverified_trcs) and after some time it will be tried again.

The problem here is, that we do not have topology information. We do not know, which intermediate TRCs we need to verify TRC 4. I think this is the best we can do here.

@PSZ I changed this a little bit. It is not necessary to construct the complete tree, but one can of course take only the neighbors of the remote TRC and try to verify only based on those instead of my previous brute-force approach where I tried to verify with every TRC in the list.

---

Comments from Reviewable

[sezergueler]

@pszalach Online keys added.

[pszal]

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

lib/crypto/trc.py, line 322 at r1 (raw file):

Previously, sezergueler wrote…

Done.

tmp = sub[1].strip() ? Also if you think that these xsignature strings are annoying/inconsistent feel free to propose something better.

---

lib/crypto/trc.py, line 386 at r3 (raw file):

                return "CA", isd, ca
            else:
                logging.error("Subject parse failed! %s" % subject)

maybe: "Cannot parse the subject: %s" % subject

---

lib/crypto/trc.py, line 394 at r3 (raw file):

                return "AS", isd_as, ""
            except:
                logging.error("Subject parse failed! %s" % subject)

ditto

---

lib/crypto/trc.py, line 446 at r3 (raw file):

def verify_trc_chain(local_trc, verified_rem_trcs, remote_trc):

I'd be consistent with remote and rem, especially when used in the same method

---

lib/crypto/trc.py, line 461 at r3 (raw file):

    if local_trc.isd in rem_nbs:
        # Try to verify with local TRC
        if verify_trc_xsigs(local_trc, remote_trc) \

parentheses instead of \
btw, we updated text width to ~100 characters, so maybe it fits a single line...

---

sub/web, line 1 at r3 (raw file):

Subproject commit d18e88e110ee57cde790edcf4839f13ef526d2ca

update submodules

---

topology/generator.py, line 178 at r3 (raw file):

        Generate all needed files.
        """
        ca_private_key_files, ca_cert_files, ca_certs, ca_online_key_pairs = \

single line

---

topology/generator.py, line 471 at r3 (raw file):

                isd_as = random.choice(isd_ases[neighbor])
                trc = self.trcs[isd]
                trc.sign(str(isd_as),

single line

---

Comments from Reviewable

[sezergueler]

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

lib/crypto/trc.py, line 322 at r1 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

tmp = sub[1].strip() ? Also if you think that these xsignature strings are annoying/inconsistent feel free to propose something better.

Yes, there is a whitespace in the subject string before "CA".
One could use a single separator "," or ":" and not have whitespaces, but that would affect TRCs readability and parsing is not beautiful anyway. I think we can let it as it is.

---

lib/crypto/trc.py, line 386 at r3 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

maybe: "Cannot parse the subject: %s" % subject

Done.

---

lib/crypto/trc.py, line 394 at r3 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

ditto

Done.

---

lib/crypto/trc.py, line 446 at r3 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

I'd be consistent with remote and rem, especially when used in the same method

Done.

---

lib/crypto/trc.py, line 461 at r3 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

parentheses instead of \
btw, we updated text width to ~100 characters, so maybe it fits a single line...

Done.

---

sub/web, line 1 at r3 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

update submodules

I will update when rebasing.

---

topology/generator.py, line 178 at r3 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

single line

Done.

---

topology/generator.py, line 471 at r3 (raw file):

Previously, pszalach (Paweł Szałachowski) wrote…

single line

Done.

---

Comments from Reviewable

[shitz]

Review status: 0 of 3 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

lib/crypto/trc.py, line 55 at r4 (raw file):

class TRC(object):

Look at how Certificate(Chain) handles errors: they don't log anything but raise an exception with an appropriate error string. We should avoid logging in library code if possible and let the application decide what to do. Please refactor the entire file.

---

lib/crypto/trc.py, line 292 at r4 (raw file):

        return trc_str

    def get_ca_sigs(self):

get_*_sigs can all be merged in one method get_sigs(self, sig_type). All you have to do is change one line: if type_ == sig_type and have them all return a list of triples (isd, ca_name, signature) which is good for consistency anyway.

---

Comments from Reviewable

[sezergueler]

Review status: 0 of 3 files reviewed at latest revision, 10 unresolved discussions.

---

lib/crypto/trc.py, line 55 at r4 (raw file):

Previously, shitz wrote…

Look at how Certificate(Chain) handles errors: they don't log anything but raise an exception with an appropriate error string. We should avoid logging in library code if possible and let the application decide what to do. Please refactor the entire file.

Done.

---

lib/crypto/trc.py, line 292 at r4 (raw file):

Previously, shitz wrote…

get_*_sigs can all be merged in one method get_sigs(self, sig_type). All you have to do is change one line: if type_ == sig_type and have them all return a list of triples (isd, ca_name, signature) which is good for consistency anyway.

Done.

---

Comments from Reviewable

[shitz]

I'm going to close this, since it's highly unlikely this will ever get merged.