Rework of the revocation authentication #1513

worxli · 2018-04-12T09:29:25Z

This PR includes changes in:

Border Router
SCIOND
Path Server
Beacon Server

The hashtree based revocation has been replaced by signed messages.

This change is

Benchmarks

Hashtree
Valid proof: ConnectedHashTree.verify(proof, root) - 24.05us/op
Invalid proof: ConnectedHashTree.verify(proof, root) - 27.50us/op

Signature
Valid signature: srev_info.verify(vk) - 104.28us/op
Valid signature: try: srev_info.verify(vk) except: pass - 126.79us/op
Invalid signature: try: srev_info.verify(vk) except: pass - 131.24us/op

kormat · 2018-04-12T10:24:35Z

Reviewed 25 of 47 files at r1.
Review status: 25 of 47 files reviewed at latest revision, all discussions resolved, some commit checks failed.

proto/rev_info.capnp, line 11 at r1 (raw file):

    linkType @2 :LinkType;  # Link type of the revoked interface
    timestamp @3 :UInt32;  # Creation timestamp, seconds since Unix Epoch
    revTTL @4 :UInt32;  # The validity period of the revocation in seconds.

I think ttl is sufficient, the rev part is already implied by the struct its in.

proto/rev_info.capnp, line 14 at r1 (raw file):

}

enum LinkType {

I'm not thrilled about having this in the rev_info.capnp file. While currently it's only used by RevInfo, it's a very general concept. Let's make a common.capnp which contains things like this.

proto/rev_info.capnp, line 15 at r1 (raw file):


enum LinkType {
    core @0;

It's useful to have an unset @0; entry. It prevents buggy code that forgets to set the type from marking everything as core, for example.

python/lib/rev_cache.py, line 86 at r1 (raw file):

    def values(self):
        """
        Return all validated values

s/validated values/active revocations/

python/lib/rev_cache.py, line 90 at r1 (raw file):

        """
        with self._lock:
            rev_infos = self._cache.values()

I think this can be simplified:

ret = []
for v in list(self._cache.values()):
  if self._check_active(v):
    ret.append(v)
return ret

python/lib/rev_cache.py, line 107 at r1 (raw file):

        """
        if not rev_info.active():
            return False

rev_info.validate() should be called first.

python/lib/rev_cache.py, line 135 at r1 (raw file):

            return False

    def _validate_entry(self, rev_info):  # pragma: no cover

This should be renamed. Maybe _check_active()?

python/lib/types.py, line 57 at r1 (raw file):

class ProtoLinkType(TypeBase):

It would be better to merge this with the existing LinkType class. Also, add a comment like XXX(worxli): these values must be kept in sync with the capnp LinkType enum.

python/lib/packet/pcb.py, line 48 at r1 (raw file):

    @classmethod
    def from_values(cls, in_ia, remote_in_ifid, in_mtu, out_ia,
                    remote_out_ifid, hof):  # pragma: no cover

Why this change?

python/lib/packet/proto_sign.py, line 55 at r1 (raw file):

        if self.p.type == ProtoSignType.ED25519:
            self.p.signature = sign(self._sig_input(msg), key)
            self.p.timestamp = int(time.time())

To make this more flexible, i suggest:

def sign(self, key, msg, ts=None):
   if ts is None:
      ts = time.time()
   ...
      self.p.timestamp = int(ts)

python/lib/packet/path_mgmt/rev_info.py, line 55 at r1 (raw file):

        :param int revTTL: Revocation validity period in seconds
        """
        assert revTTL >= MIN_REVOCATION_TTL

When using assertions, you should also provide the actual value for the exception:

assert revTTL >= MIN_REVOCATION_TTL, revTTL

Otherwise you'll just get an assertion error without any idea what the bad value was.

python/lib/packet/path_mgmt/rev_info.py, line 66 at r1 (raw file):

            raise RevInfoValidationError("Timestamp in the future: %s" % self.p.timestamp)
        if self.p.revTTL < MIN_REVOCATION_TTL:
            raise RevInfoValidationError("TTL is too small: %s" % self.p.revTTL)

Mention the min ttl as well.

python/lib/packet/path_mgmt/rev_info.py, line 74 at r1 (raw file):

        now = int(time.time())
        # Make sure the revocation timestamp is within the validity window
        assert self.p.timestamp <= now + 1

, self.p.timestamp at the end.

python/lib/packet/path_mgmt/rev_info.py, line 81 at r1 (raw file):

        b.append(self.p.isdas.to_bytes(8, 'big'))
        b.append(self.p.ifID.to_bytes(8, 'big'))
        b.append(self.p.linkType.raw.to_bytes(8, 'big'))

linkType is effectively a uint16, so this should be 2. (https://capnproto.org/encoding.html#enums)

python/lib/packet/path_mgmt/rev_info.py, line 83 at r1 (raw file):

        b.append(self.p.linkType.raw.to_bytes(8, 'big'))
        b.append(self.p.timestamp.to_bytes(8, 'big'))
        b.append(self.p.revTTL.to_bytes(8, 'big'))

Both of these are uint32, so 4.

python/lib/packet/path_mgmt/rev_info.py, line 96 at r1 (raw file):

    def short_desc(self):
        return "RevInfo: %s IF: %s Link type: %s Timestamp: %s TTL: %s" % (

TTL: %ss - so it's obvious that it's seconds.

python/lib/packet/path_mgmt/rev_info.py, line 97 at r1 (raw file):

    def short_desc(self):
        return "RevInfo: %s IF: %s Link type: %s Timestamp: %s TTL: %s" % (
            self.isd_as(), self.p.ifID, self.p.linkType,

Link type should be formatted using lib.types.LinkType.to_str().

python/lib/packet/path_mgmt/seg_recs.py, line 64 at r1 (raw file):

    def rev_info(self, idx):
        return ProtoSignedBlob(self.p.revInfos[idx])

There should be a wrapper type for this. You'd end up with return SignedRevInfo(self.p.revInfos[idx]), and callers can call rev_info() on the returned object.

python/scion_elem/scion_elem.py, line 1180 at r1 (raw file):

        return results

    def _verify_revocation_for_asm(self, rev_info, as_marking, verify_all=True):

This no longer verifies - where is that done?

topology/Tiny.topo, line 15 at r1 (raw file):

    cert_issuer: 1-4_295_001_010
links:
  - {a: 1-4_295_001_010, b: 1-4_295_001_011, ltype: PARENT, mtu: 1280}

Why this change?

Comments from Reviewable

kormat · 2018-04-12T11:14:55Z

Reviewed 2 of 47 files at r1.
Review status: 27 of 47 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

python/path_server/base.py, line 114 at r1 (raw file):

        self.waiting_targets = defaultdict(list)
        self.revocations = RevCache(labels=self._labels)
        self.seglock = Lock()

Add a comment saying that this is used to serialise removal of revoked segments.

python/path_server/base.py, line 305 at r1 (raw file):

        if_id = rev_info.p.ifID

        def _handle_one_seg(seg, db):

Suggestion: add a method on pcb.PathSegment that takes a revinfo, and looks a bit like:

def rev_match(self, rev_info):
   for asm in self.iter_asms():
     if rev_info.isd_as() != asm.isd_as():
         continue
     for i, pcbm in asm.iter_pcbms():
        hof = pcbm.hof()
        if rev_info.ifid == hof.ingress_if:
            return True, LinkType.PARENT if i == 0 else LinkType.PEER
        if rev_info.ifid == hof.egress_if:
             return True, LinkType.CHILD
    return False, None

This function then simplifies to:

def _handle_one_sec(seg, db):
    rm, ltype = seg.rev_match(rev_info)
    if rm and ltype in [LinkType.Parent, LinkType.Child] and db.delete(seg.get_hops_hash()) == DBResult.ENTRY_DELETED:
      return 1
   return 0

python/path_server/base.py, line 328 at r1 (raw file):

                core_segs_removed += _handle_one_seg(core_segment, self.core_segments)

            logging.debug("Removed segments revoked by [%s]: UP: %d DOWN: %d CORE: %d" %

This doesn't need to be inside the lock.

python/sciond/sciond.py, line 199 at r1 (raw file):

            if not signed_rev_info.verify(cert.as_cert.subject_sig_key_raw):
                logging.error("Failed to verify signature!")
            self.rev_cache.add(rev_info)

This doesn't look right. handle_revocation does similar stuff, as well as calling .validate(), and removing revoked segments. That should also be done for revocations received via this path. I suggest making a new method that:

takes a signed rev info
calls .validate() and .active() on it
verifies the signature
puts it in rev cache
flushes revoked segments

That can then be called by handle_path_reply, handle_scmp_revocation and handle_revocation.

python/sciond/sciond.py, line 419 at r1 (raw file):

        self.handle_revocation(CtrlPayload(PathMgmt(signed_rev_info)), meta)

    def handle_revocation(self, cpld, meta):

I think that we should probably re-examine the SCIONDRevReplyStatus types. These are the categories that come to mind:

Revocation is too old (stale)
Revocation ttl is too low, ifid is 0, or revocation from the future (illegal)
IA's cert can't be fetched, or some other failure (unknown)
Revocation signature verification failed (the worst possible failure)
None of the above.

I think it's ok to do these checks in order (meaning that a forged and expired rev info will be rejected early just on timestamp, and the forgery won't be noted), but we should be able to distinguish between them.

So, my suggestions:

Verified (case 5)
Stale (case 1)
Invalid (case 2)
Unknown (case 3)
SigFailed (case 4)

python/sciond/sciond.py, line 436 at r1 (raw file):

                "Failed to verify validity: timestamp %s, current timestamp %s, TTL %d."
                % (rev_info.p.timestamp, str(time.time()), rev_info.p.revTTL))
            return SCIONDRevReplyStatus.INVALID

This depends on whether the timestamp is too old, or too new. If it's expired, then this should be returning .STALE. If it's too new, then .INVALID with an error logged.

python/sciond/sciond.py, line 440 at r1 (raw file):

        cert = self.trust_store.get_cert(rev_info.isd_as())
        if not cert:
            logging.warning("Failed to fetch cert for ISD-AS: %s", rev_info.isd_as())

return SCIONDRevReplyStatus.UNKNOWN

python/sciond/sciond.py, line 451 at r1 (raw file):

            removed_core = self._remove_revoked_pcbs(self.core_segments, rev_info)
        elif rev_info.p.linkType == ProtoLinkType.PARENT or \
                rev_info.p.linkType == ProtoLinkType.CHILD:

if rev_info.p.linkType in [LinkType.PARENT, LinkType.CHILD]:

python/sciond/sciond.py, line 461 at r1 (raw file):

        total = removed_up + removed_core + removed_down
        if total > 0:
            return SCIONDRevReplyStatus.VALID

We no longer depend on having removed segments to know if a rev info is legit or not, which also means the below comment is obsolete too.

return SCIONDRevReplyStatus.VALID

Comments from Reviewable

kormat · 2018-04-12T11:28:51Z

Reviewed 3 of 47 files at r1.
Review status: 30 of 47 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

python/beacon_server/base.py, line 528 at r1 (raw file):

            for raw in rev_infos:
                try:
                    rev_info = RevocationInfo.from_raw(raw)

The zk entries should be SignedRevInfos.

python/beacon_server/base.py, line 556 at r1 (raw file):

            link_type = ProtoLinkType.from_string(br.interfaces[if_id].link_type)
            rev_info = RevocationInfo.from_values(
                self.addr.isd_as, if_id, link_type, int(time.time()), MIN_REVOCATION_TTL)

This should use self.REVOCATION_TTL.

python/beacon_server/base.py, line 616 at r1 (raw file):

        cert = self.trust_store.get_cert(rev_info.isd_as())
        if not cert:
            logging.warning("Failed to fetch cert for ISD-AS: %s", rev_info.isd_as())

This should stop processing.

python/beacon_server/base.py, line 643 at r1 (raw file):

        with self._rev_seg_lock:
            self.local_rev_cache.add(rev_info.copy())
        rev_info_packed = rev_info.copy().pack()

Rev infos should always be wrapped inside a SignedRevInfo, otherwise they can't be verified.

python/beacon_server/base.py, line 715 at r1 (raw file):

                        # Interface hasn't timed out
                        continue
                    if if_state.is_revoked() and \

Use ()'s for handling line-breaks.

python/beacon_server/base.py, line 746 at r1 (raw file):

                    rev_info = RevocationInfo.from_values(
                        self.addr.isd_as, ifid, link_type, int(time.time()),
                        MIN_REVOCATION_TTL)

self.REVOCATION_TTL

python/beacon_server/base.py, line 751 at r1 (raw file):

                    signed_rev_info.sign(self.signing_key)
                    info = IFStateInfo.from_values(ifid, state.is_active(), signed_rev_info)
                else:

signed_rev_info = None
if state.is_revoked():
   ...
   signed_rev_info = ...
infos.append(IFStateInfo.from_values(ifid, state.is_active(), signed_rev_info)

python/path_server/base.py, line 113 at r1 (raw file):

        # Used when l/cPS doesn't have up/dw-path.
        self.waiting_targets = defaultdict(list)
        self.revocations = RevCache(labels=self._labels)

This can't just store RevInfo objects. The PS has to include the SignedRevInfo objects in path replies. I.e. RevCache needs to store both SignedRevInfo and RevInfo objects.

Comments from Reviewable

kormat · 2018-04-12T11:36:31Z

Reviewed 3 of 47 files at r1.
Review status: 33 of 47 files reviewed at latest revision, 37 unresolved discussions, some commit checks failed.

go/lib/ctrl/path_mgmt/rev_info.go, line 34 at r1 (raw file):

const MinRevTTL = 10 * time.Second // Revocation MinRevTTL

var _ proto.Cerealizable = (*RevInfo)(nil)

This should be before type RevInfo struct.

go/lib/ctrl/path_mgmt/rev_info.go, line 82 at r1 (raw file):

func (r *RevInfo) Valid() bool {
	assert.Must(r.RevTTL >= uint32(MinRevTTL.Seconds()), "RevTTL must not be smaller than MinRevTTL")

Line length.

go/lib/ctrl/path_mgmt/rev_info.go, line 82 at r1 (raw file):

func (r *RevInfo) Valid() bool {
	assert.Must(r.RevTTL >= uint32(MinRevTTL.Seconds()), "RevTTL must not be smaller than MinRevTTL")

This shouldn't be an assertion. It's run on revinfos that are received from other services. Receiving a malformed revinfo shouldn't cause this process to crash. I would instead change the return type to be error, and use a custom error type that implements common.Timeout for use when the revinfo is expired.

go/lib/ctrl/path_mgmt/rev_info.go, line 85 at r1 (raw file):

	now := uint32(time.Now().Unix())
	// Revocation is not valid if its timestamp is not within the MinRevTTL
	if r.Timestamp > now || r.Timestamp < now-r.RevTTL {

if r.Timestamp > now+1 || r.Timestamp+r.RevTTL > now {

go/lib/ctrl/path_mgmt/rev_info.go, line 96 at r1 (raw file):

func (r *RevInfo) String() string {
	return fmt.Sprintf("IA: %s IfID: %d Link type: %s Timestamp: %s TTL: %d",

TTL: %ds

go/lib/ctrl/path_mgmt/rev_info.go, line 97 at r1 (raw file):

func (r *RevInfo) String() string {
	return fmt.Sprintf("IA: %s IfID: %d Link type: %s Timestamp: %s TTL: %d",
		r.IA(), r.IfID, r.LinkType, util.USecsToTime(uint64(r.Timestamp)), r.RevTTL)

Add a util.TimeToString() around util.USecsToTime.

Comments from Reviewable

kormat · 2018-04-12T11:41:41Z

Reviewed 6 of 47 files at r1.
Review status: 39 of 47 files reviewed at latest revision, 43 unresolved discussions, some commit checks failed.

go/lib/ctrl/path_mgmt/ifstate_infos.go, line 47 at r1 (raw file):

	IfID    uint64
	Active  bool
	RevInfo *SignedRevInfo

Suggestion:

SRevInfo *SignedRevInfo `capnp:"revInfo"`

to indicate that it's the signed form.

go/lib/ctrl/path_mgmt/path_mgmt.go, line 34 at r1 (raw file):

	SegReg       *SegReg
	SegSync      *SegSync
	RevInfo      *SignedRevInfo

Ditto re: SRevInfo

go/lib/pathmgr/pathmgr.go, line 246 at r1 (raw file):

		return
	}
	parsedRev, err := parsedSignedRev.RevInfo()

This doesn't need to be parsed until after sciond replies.

Comments from Reviewable

kormat · 2018-04-12T11:47:23Z

Reviewed 7 of 47 files at r1.
Review status: 46 of 47 files reviewed at latest revision, 46 unresolved discussions, some commit checks failed.

go/border/ifstate/ifstate.go, line 71 at r1 (raw file):

	IfID         common.IFIDType
	Active       bool
	RevInfo      *path_mgmt.SignedRevInfo

SRevInfo.

go/border/rpkt/path.go, line 105 at r1 (raw file):

	// Interface is revoked.
	signedRevInfo := state.RevInfo
	revInfo, err := signedRevInfo.RevInfo()

This gets a bit expensive. It would be better to cache both the signed and plain revinfos.

go/border/rpkt/process.go, line 336 at r1 (raw file):

			"expected", "*scmp.InfoRevocation", "actual", common.TypeOf(pld.Info))
	}
	if args.SignedRevInfo, err = path_mgmt.NewSignedRevInfoFromRaw(infoRev.RevToken); err != nil {

Hmm. We should really stop calling it RevToken.

python/test/lib/rev_cache_test.py, line 68 at r1 (raw file):

timestamp=int(time.time())

Nooo, you can't do that. Default arguments are evaluated once in python, the first time the function is called (i believe). To work around this, the convention is to do timestamp=None in the signature, and then timestamp = int(timetstamp or time.time()) at the top of the method.

Comments from Reviewable

worxli · 2018-04-13T15:38:29Z

Review status: 21 of 56 files reviewed at latest revision, 50 unresolved discussions, some commit checks failed.

go/border/ifstate/ifstate.go, line 71 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

SRevInfo.

Done.

go/border/rpkt/path.go, line 105 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This gets a bit expensive. It would be better to cache both the signed and plain revinfos.

Done.

go/border/rpkt/process.go, line 336 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Hmm. We should really stop calling it RevToken.

Renamed it to RawRev, was sometimes referred to it as that.

go/lib/ctrl/path_mgmt/ifstate_infos.go, line 47 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Suggestion:
SRevInfo *SignedRevInfo `capnp:"revInfo"`
to indicate that it's the signed form.

Done.

go/lib/ctrl/path_mgmt/path_mgmt.go, line 34 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Ditto re: SRevInfo

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 34 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should be before type RevInfo struct.

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 82 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Line length.

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 82 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This shouldn't be an assertion. It's run on revinfos that are received from other services. Receiving a malformed revinfo shouldn't cause this process to crash. I would instead change the return type to be error, and use a custom error type that implements common.Timeout for use when the revinfo is expired.

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 85 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

if r.Timestamp > now+1 || r.Timestamp+r.RevTTL > now {

Shouldn't it be ...+r.RevTTL < now ?

go/lib/ctrl/path_mgmt/rev_info.go, line 96 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

TTL: %ds

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 97 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Add a util.TimeToString() around util.USecsToTime.

Done.

go/lib/pathmgr/pathmgr.go, line 246 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This doesn't need to be parsed until after sciond replies.

Done.

proto/rev_info.capnp, line 11 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I think ttl is sufficient, the rev part is already implied by the struct its in.

Done.

proto/rev_info.capnp, line 14 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'm not thrilled about having this in the rev_info.capnp file. While currently it's only used by RevInfo, it's a very general concept. Let's make a common.capnp which contains things like this.

Done.

proto/rev_info.capnp, line 15 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

It's useful to have an unset @0; entry. It prevents buggy code that forgets to set the type from marking everything as core, for example.

Done.

python/beacon_server/base.py, line 528 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

The zk entries should be SignedRevInfos.

Done.

python/beacon_server/base.py, line 556 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should use self.REVOCATION_TTL.

Done.

python/beacon_server/base.py, line 616 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should stop processing.

Done.

python/beacon_server/base.py, line 643 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Rev infos should always be wrapped inside a SignedRevInfo, otherwise they can't be verified.

Done.

python/beacon_server/base.py, line 715 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Use ()'s for handling line-breaks.

Done.

python/beacon_server/base.py, line 746 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

self.REVOCATION_TTL

Done.

python/beacon_server/base.py, line 751 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

signed_rev_info = None
if state.is_revoked():
   ...
   signed_rev_info = ...
infos.append(IFStateInfo.from_values(ifid, state.is_active(), signed_rev_info)

Done.

python/lib/rev_cache.py, line 86 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

s/validated values/active revocations/

Done.

python/lib/rev_cache.py, line 90 at r1 (raw file):

ret = []
for v in list(self._cache.values()):
if self._check_active(v):
ret.append(v)
return ret

Like that you need to copy all the values instead of just the keys. But if memory if not the issue this works.

python/lib/rev_cache.py, line 107 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

rev_info.validate() should be called first.

I think this should not be part of the RevCache's responsibilities. Validation should be done by whom ever receives or creates the RevInfo before inserting it into the cache.

python/lib/rev_cache.py, line 135 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should be renamed. Maybe _check_active()?

Done.

python/lib/types.py, line 57 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

It would be better to merge this with the existing LinkType class. Also, add a comment like XXX(worxli): these values must be kept in sync with the capnp LinkType enum.

Done.

python/lib/packet/pcb.py, line 48 at r1 (raw file):

XXX(worxli): these values must be kept in sync with the capnp

Imho it improves readability.

python/lib/packet/proto_sign.py, line 55 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

To make this more flexible, i suggest:

def sign(self, key, msg, ts=None):
   if ts is None:
      ts = time.time()
   ...
      self.p.timestamp = int(ts)

Done.

This is just something I noticed. The timestamp was never set...

python/lib/packet/path_mgmt/rev_info.py, line 55 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

When using assertions, you should also provide the actual value for the exception:
assert revTTL >= MIN_REVOCATION_TTL, revTTL
Otherwise you'll just get an assertion error without any idea what the bad value was.

Done.

python/lib/packet/path_mgmt/rev_info.py, line 66 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Mention the min ttl as well.

Done.

python/lib/packet/path_mgmt/rev_info.py, line 74 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

, self.p.timestamp at the end.

Done.

python/lib/packet/path_mgmt/rev_info.py, line 81 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

linkType is effectively a uint16, so this should be 2. (https://capnproto.org/encoding.html#enums)

Done.

python/lib/packet/path_mgmt/rev_info.py, line 83 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Both of these are uint32, so 4.

Done.

python/lib/packet/path_mgmt/rev_info.py, line 96 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

TTL: %ss - so it's obvious that it's seconds.

Done.

python/lib/packet/path_mgmt/rev_info.py, line 97 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Link type should be formatted using lib.types.LinkType.to_str().

Done.

python/lib/packet/path_mgmt/seg_recs.py, line 64 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

There should be a wrapper type for this. You'd end up with return SignedRevInfo(self.p.revInfos[idx]), and callers can call rev_info() on the returned object.

Done.

python/path_server/base.py, line 113 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This can't just store RevInfo objects. The PS has to include the SignedRevInfo objects in path replies. I.e. RevCache needs to store both SignedRevInfo and RevInfo objects.

Done.

python/path_server/base.py, line 114 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Add a comment saying that this is used to serialise removal of revoked segments.

Done.

python/path_server/base.py, line 305 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Suggestion: add a method on pcb.PathSegment that takes a revinfo, and looks a bit like:

def rev_match(self, rev_info):
   for asm in self.iter_asms():
     if rev_info.isd_as() != asm.isd_as():
         continue
     for i, pcbm in asm.iter_pcbms():
        hof = pcbm.hof()
        if rev_info.ifid == hof.ingress_if:
            return True, LinkType.PARENT if i == 0 else LinkType.PEER
        if rev_info.ifid == hof.egress_if:
             return True, LinkType.CHILD
    return False, None

This function then simplifies to:

def _handle_one_sec(seg, db):
    rm, ltype = seg.rev_match(rev_info)
    if rm and ltype in [LinkType.Parent, LinkType.Child] and db.delete(seg.get_hops_hash()) == DBResult.ENTRY_DELETED:
      return 1
   return 0

Done.

python/path_server/base.py, line 328 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This doesn't need to be inside the lock.

Done.

python/scion_elem/scion_elem.py, line 1180 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This no longer verifies - where is that done?

The revocation is always verified beforehand. Renamed this fct to reflect that.

python/sciond/sciond.py, line 199 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This doesn't look right. handle_revocation does similar stuff, as well as calling .validate(), and removing revoked segments. That should also be done for revocations received via this path. I suggest making a new method that:

takes a signed rev info

calls .validate() and .active() on it

verifies the signature

puts it in rev cache

flushes revoked segments

That can then be called by handle_path_reply, handle_scmp_revocation and handle_revocation.

I don't think this works because in handle_revocation we need the results from the different steps.

python/sciond/sciond.py, line 419 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I think that we should probably re-examine the SCIONDRevReplyStatus types. These are the categories that come to mind:

Revocation is too old (stale)

Revocation ttl is too low, ifid is 0, or revocation from the future (illegal)

IA's cert can't be fetched, or some other failure (unknown)

Revocation signature verification failed (the worst possible failure)

None of the above.

I think it's ok to do these checks in order (meaning that a forged and expired rev info will be rejected early just on timestamp, and the forgery won't be noted), but we should be able to distinguish between them.

So, my suggestions:

Verified (case 5)

Stale (case 1)

Invalid (case 2)

Unknown (case 3)

SigFailed (case 4)

Done.

python/sciond/sciond.py, line 436 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This depends on whether the timestamp is too old, or too new. If it's expired, then this should be returning .STALE. If it's too new, then .INVALID with an error logged.

Yes, but a too new timestamp should already be catched by validate().

python/sciond/sciond.py, line 440 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

return SCIONDRevReplyStatus.UNKNOWN

Done.

python/sciond/sciond.py, line 451 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

if rev_info.p.linkType in [LinkType.PARENT, LinkType.CHILD]:

Done.

python/sciond/sciond.py, line 461 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

We no longer depend on having removed segments to know if a rev info is legit or not, which also means the below comment is obsolete too.
return SCIONDRevReplyStatus.VALID

Done.

python/test/lib/rev_cache_test.py, line 68 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

timestamp=int(time.time())

Nooo, you can't do that. Default arguments are evaluated once in python, the first time the function is called (i believe). To work around this, the convention is to do timestamp=None in the signature, and then timestamp = int(timetstamp or time.time()) at the top of the method.

Done.

topology/Tiny.topo, line 15 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Why this change?

The mentioned AS's do not exist. I think that was a bug when renaming.

Comments from Reviewable

shitz · 2018-04-16T10:40:47Z

Reviewed 16 of 47 files at r1, 33 of 35 files at r2, 3 of 5 files at r3.
Review status: 56 of 58 files reviewed at latest revision, 50 unresolved discussions, some commit checks failed.

go/border/ifstate/ifstate.go, line 72 at r2 (raw file):

	Active       bool
	RevInfo      *path_mgmt.RevInfo
	SRevInfo     *path_mgmt.SignedRevInfo

Why does this contain a SignedRevInfo and a RevInfo if SignedRevInfo already contains the RevInfo?

go/border/rpkt/path.go, line 105 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Done.

Hmm, but RevInfo is already being cached in SignedRevInfo so in most cases this is just returning the parsed object or am I missing something?

go/lib/ctrl/path_mgmt/rev_info.go, line 31 at r2 (raw file):

)

const MinRevTTL = 10 * time.Second // Revocation MinRevTTL

This needs a better docstring.
// MinRevTTL is the minimum lifetime of a revocation

go/lib/ctrl/path_mgmt/rev_info.go, line 33 at r2 (raw file):

const MinRevTTL = 10 * time.Second // Revocation MinRevTTL

type RevTimeError struct {

Would be good to make it obvious that this implements the common.Timeout interface. var _ common.Timeout = (*RevTimeError)(nil)

go/lib/ctrl/path_mgmt/rev_info.go, line 55 at r2 (raw file):

	IfID      uint64
	RawIsdas  addr.IAInt     `capnp:"isdas"`
	LinkType  proto.LinkType // Link type of revocation

In Go, we put docstring comments above the declaration. This way it will be properly picked up be godoc. Also, the convention is for exported fields that the docstring always starts with the name of the field, e.g., // LinkType is the type of the revoked link

proto/rev_info.capnp, line 12 at r2 (raw file):

    isdas @1 :UInt64;  # ISD-AS of the revocation issuer.
    linkType @2 :Common.LinkType;  # Link type of the revoked interface
    timestamp @3 :UInt32;  # Creation timestamp, seconds since Unix Epoch

This should be 64bit. We want this to work after 2038.

python/beacon_server/base.py, line 746 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Done.

So this means that we move away from having clearly defined epochs during which an interface is globally revoked (since you are always calling time.time()). This will lead to different entities in the network having different views of how long an interface is revoked. I think that is what we decided in the design doc, but I just want to make sure that we are on the same page here, @kormat.

python/beacon_server/base.py, line 113 at r2 (raw file):

    ZK_REVOCATIONS_PATH = "rev_cache"
    # Time revocation objects are cached in memory (in seconds).
    ZK_REV_OBJ_MAX_AGE = MIN_REVOCATION_TTL

This will remove revocations that are valid for longer than 10s, however, it's probably not a big problem. We might need to revisit this in the future when the BS gets rewritten.

python/beacon_server/base.py, line 692 at r3 (raw file):

                        continue
                    if (if_state.is_revoked() and if_id_last_revoked[if_id] +
                       self.REVOCATION_TTL > int(time.time())):

You should use start_time instead of continuously calling time.time().

python/lib/defines.py, line 135 at r2 (raw file):

MAX_HOST_ADDR_LEN = 16

# Revocation TTL in seconds

Minimum

python/lib/rev_cache.py, line 107 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

I think this should not be part of the RevCache's responsibilities. Validation should be done by whom ever receives or creates the RevInfo before inserting it into the cache.

I agree with @worxli. This could become very costly and if we have the invariant that only verified revocations get added to the RevCache we should be fine.

python/lib/types.py, line 171 at r2 (raw file):

############################
class LinkType(TypeBase):
    # XXX(worxli): these values must be kept in sync with the capnp

capnp Linktype enum

python/lib/packet/pcb.py, line 310 at r2 (raw file):

            if rev_info.isd_as() != asm.isd_as():
                continue
            for i, pcbm in asm.iter_pcbms():

don't you need enumerate(asm.iter_pcbms()) here?

python/lib/packet/proto_sign.py, line 59 at r2 (raw file):

        if ts is None:
            ts = time.time()
        self.p.timestamp = int(ts)

Wait, shouldn't the timestamp be part of the signature?

python/lib/packet/path_mgmt/rev_info.py, line 56 at r3 (raw file):

        return self._rev_info

    def verify(self, trust_store):

This should just take the verifying key as an input.

python/scion_elem/scion_elem.py, line 1256 at r3 (raw file):

        """
        Checks if the revocation is valid and processing should continue
        :returns: boolean if all checks were successful

Where does this return a boolean? return returns None, which is then interpreted as False. I don't see where this returns True. Also, please return explicit True or False

Comments from Reviewable

worxli · 2018-04-17T07:30:25Z

Review status: 45 of 58 files reviewed at latest revision, 62 unresolved discussions.

go/border/ifstate/ifstate.go, line 72 at r2 (raw file):

Previously, shitz wrote…

Why does this contain a SignedRevInfo and a RevInfo if SignedRevInfo already contains the RevInfo?

Done.

go/border/rpkt/path.go, line 105 at r1 (raw file):

Previously, shitz wrote…

Hmm, but RevInfo is already being cached in SignedRevInfo so in most cases this is just returning the parsed object or am I missing something?

Done. Yeah true and it's just used here.

go/lib/ctrl/path_mgmt/rev_info.go, line 31 at r2 (raw file):

Previously, shitz wrote…

This needs a better docstring.
// MinRevTTL is the minimum lifetime of a revocation

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 33 at r2 (raw file):

Previously, shitz wrote…

Would be good to make it obvious that this implements the common.Timeout interface. var _ common.Timeout = (*RevTimeError)(nil)

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 55 at r2 (raw file):

Previously, shitz wrote…

In Go, we put docstring comments above the declaration. This way it will be properly picked up be godoc. Also, the convention is for exported fields that the docstring always starts with the name of the field, e.g., // LinkType is the type of the revoked link

Done.

proto/rev_info.capnp, line 12 at r2 (raw file):

Previously, shitz wrote…

This should be 64bit. We want this to work after 2038.

Done.

python/beacon_server/base.py, line 746 at r1 (raw file):

Previously, shitz wrote…

So this means that we move away from having clearly defined epochs during which an interface is globally revoked (since you are always calling time.time()). This will lead to different entities in the network having different views of how long an interface is revoked. I think that is what we decided in the design doc, but I just want to make sure that we are on the same page here, @kormat.

From my side I think what you said is correct. But the interface should be revoked the same amount of time for everyone (minus time sync differences)?

python/beacon_server/base.py, line 113 at r2 (raw file):

Previously, shitz wrote…

This will remove revocations that are valid for longer than 10s, however, it's probably not a big problem. We might need to revisit this in the future when the BS gets rewritten.

Agreed, but as mentioned I didn't want to change even more without needing it in the future.

python/beacon_server/base.py, line 692 at r3 (raw file):

Previously, shitz wrote…

You should use start_time instead of continuously calling time.time().

Done.

python/lib/defines.py, line 135 at r2 (raw file):

Previously, shitz wrote…

Minimum

Done.

python/lib/types.py, line 171 at r2 (raw file):

Previously, shitz wrote…

capnp Linktype enum

Done.

python/lib/packet/pcb.py, line 310 at r2 (raw file):

Previously, shitz wrote…

don't you need enumerate(asm.iter_pcbms()) here?

Done.

python/lib/packet/proto_sign.py, line 59 at r2 (raw file):

Previously, shitz wrote…

Wait, shouldn't the timestamp be part of the signature?

Probably a good idea, @kormat any reason this wasn't done?

python/lib/packet/path_mgmt/rev_info.py, line 56 at r3 (raw file):

Previously, shitz wrote…

This should just take the verifying key as an input.

Done.

python/scion_elem/scion_elem.py, line 1256 at r3 (raw file):

Previously, shitz wrote…

Where does this return a boolean? return returns None, which is then interpreted as False. I don't see where this returns True. Also, please return explicit True or False

Done.

Comments from Reviewable

kormat · 2018-04-17T12:06:14Z

Reviewed 10 of 35 files at r2, 5 of 12 files at r4.
Review status: 47 of 58 files reviewed at latest revision, 51 unresolved discussions, some commit checks failed.

go/lib/sciond/sciond.go, line 112 at r1 (raw file):

	// RevNotification sends a raw revocation to SCIOND, as contained in an
	// SCMP message.
	RevNotificationFromRaw(revInfo []byte) (*RevReply, error)

revInfo isn't a great name here (and gets confusing in the implementation below). We normally just use b for a raw byte slice.

go/lib/sciond/sciond.go, line 114 at r1 (raw file):

	RevNotificationFromRaw(revInfo []byte) (*RevReply, error)
	// RevNotification sends a RevocationInfo message to SCIOND.
	RevNotification(signedRevInfo *path_mgmt.SignedRevInfo) (*RevReply, error)

sRevInfo is a bit shorter, and more consistent with naming elsewhere.

Also in connector.RevNotifcation() and MockConn.RevNofication().

go/lib/topology/testdata/basic.json, line 33 at r4 (raw file):

                    "Bandwidth": 1000,
                    "ISD_AS": "1-4_294_967_312",
                    "LinkType": "parent",

This change is not required from what i can see (and i prefer fixed values to be in caps to make it more obvious that it's not just free-form text).

proto/rev_info.capnp, line 12 at r2 (raw file):
@shitz : note that it was unsigned:

>>> time.ctime((1<<32)-1)
'Sun Feb  7 07:28:15 2106'

We use uint32 for this elsewhere, InfoField in particular. I think uint32 is fine for this, as we only need 1s resolution. We use 64-bit timestamps when we want ųs resolution.

proto/sciond.capnp, line 82 at r1 (raw file):


struct RevNotification {
    revInfo @0 :Sign.SignedBlob;

I think we should probably rename the capnp fields to sRevInfo here and elsewhere.

python/lib/rev_cache.py, line 107 at r1 (raw file):

This could become very costly

Costly? Validate checks that the values are within acceptable ranges, that's all.

only verified revocations

validation != verification. The current verification doesn't actually look at the contents of the RevInfo at all (which is actually a problem, because there's no check that the signer AS and the revinfo AS match).

I'd be satisfied if we change SignedRevInfo.verify to do something like:

rev_info = self.rev_info()
if rev_info.isd_as() != get_signer_from_proto_sign(self.p.sign):
   raise SignedRevInfoVerificationError("SignedRevInfo signer (%s) does not match contents (%s)" ...)
rev_info.validate()
if not super().verify(key):
  ...

python/lib/packet/proto_sign.py, line 59 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Probably a good idea, @kormat any reason this wasn't done?

Eh, cough, i think the TS was a later addition to the design, and i forgot? That's the best "reason" i have :)

python/lib/packet/proto_sign.py, line 80 at r4 (raw file):

        return b"".join(b)

    def _sig_input(self, msg, ts):

ts should be included in sig_pack, not passed in here.

b = [str(self.p.type).encode("utf-8"), self.p.src, self.p.timestamp.to_bytes(4, 'big')]

Comments from Reviewable

worxli · 2018-04-17T15:29:58Z

Review status: 31 of 60 files reviewed at latest revision, 56 unresolved discussions.

go/lib/sciond/sciond.go, line 112 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

revInfo isn't a great name here (and gets confusing in the implementation below). We normally just use b for a raw byte slice.

Done.

go/lib/sciond/sciond.go, line 114 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

sRevInfo is a bit shorter, and more consistent with naming elsewhere.

Also in connector.RevNotifcation() and MockConn.RevNofication().

Done.

proto/rev_info.capnp, line 12 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

@shitz : note that it was unsigned:
>>> time.ctime((1<<32)-1)
'Sun Feb  7 07:28:15 2106'
We use uint32 for this elsewhere, InfoField in particular. I think uint32 is fine for this, as we only need 1s resolution. We use 64-bit timestamps when we want ųs resolution.

Though, I checked other timestamps in capnp and they also seem to be seconds and UInt64.

proto/sciond.capnp, line 82 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I think we should probably rename the capnp fields to sRevInfo here and elsewhere.

Done.

python/lib/rev_cache.py, line 107 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This could become very costly

Costly? Validate checks that the values are within acceptable ranges, that's all.

only verified revocations

validation != verification. The current verification doesn't actually look at the contents of the RevInfo at all (which is actually a problem, because there's no check that the signer AS and the revinfo AS match).

I'd be satisfied if we change SignedRevInfo.verify to do something like:
rev_info = self.rev_info()
if rev_info.isd_as() != get_signer_from_proto_sign(self.p.sign):
   raise SignedRevInfoVerificationError("SignedRevInfo signer (%s) does not match contents (%s)" ...)
rev_info.validate()
if not super().verify(key):
  ...

As discussed the order is:validation, active(), verification. At the moment this is done in SCIONElement.check_revocation. That is called whenever a verification is needed, therefore a validation is always done before a verification. I don't think this should be put into RevocationInfo, as depending on the context failures need to be handled differently.

python/lib/packet/proto_sign.py, line 80 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

ts should be included in sig_pack, not passed in here.
b = [str(self.p.type).encode("utf-8"), self.p.src, self.p.timestamp.to_bytes(4, 'big')]

Done.

go/lib/topology/testdata/basic.json, line 33 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This change is not required from what i can see (and i prefer fixed values to be in caps to make it more obvious that it's not just free-form text).

Done.

Comments from Reviewable

kormat · 2018-05-02T11:46:40Z

Reviewed 1 of 5 files at r3, 1 of 5 files at r5, 15 of 44 files at r6.
Review status: 32 of 62 files reviewed at latest revision, 45 unresolved discussions.

python/integration/base_cli_srv.py, line 135 at r6 (raw file):

        """Request path via SCIOND API."""
        path_entries = self._try_sciond_api(flush)
        logging.info("Path entries (%s) from SCIOND:\n%s", len(path_entries),

This should be at the DEBUG level.

python/integration/base_cli_srv.py, line 136 at r6 (raw file):

        path_entries = self._try_sciond_api(flush)
        logging.info("Path entries (%s) from SCIOND:\n%s", len(path_entries),
                     "\n    ".join([str(entry) for entry in path_entries]))

This formats badly. "\n".join(...) will work well, each entry self-indents.

python/lib/packet/path_mgmt/rev_info.py, line 46 at r6 (raw file):

    """
    Wrapper for signed revocation information.
    """

NAME = "SignedRevInfo, for consistency.

python/lib/packet/path_mgmt/rev_info.py, line 67 at r6 (raw file):

            logging.error("Other SignedRevInfo object is None.")
            return False
        if not isinstance(other, SignedRevInfo):

Whenever i see this, i always wonder where this is getting called from that the identity of other isn't already guaranteed. A simpler version is just to do:

assert isinstance(other, SignedRevInfo), type(SignedRevInfo)

python/lib/packet/path_mgmt/rev_info.py, line 76 at r6 (raw file):
This is not a useful format:

SRevInfo Blob: b'\x10\x05\x10\x04\x01Oc\x11\x02\xff\x02\x11\x03\n\x0f~\x9d\xe9Z' Sign: ( type = ed25519,
>   src = "DEFAULT: IA: 2-ff00:0:211 CHAIN: 1 TRC: 1",
>   signature = "\x97bDLo,\x96r\xca\b\xd5\xf0O\x1f\xe7\xcb\xb8z{\xf6\x97f\x9eUTv\xfb._\x90z\xd1{H\x99\xc4\xc8\x8f\xb7\x84r\x9f\xb8\xb4\xb2Be^\xacK\x1d\xaf\x1b\xe7h(@\x85\xd7\xdb\xec\xda\x19\a",
>   timestamp = 1525259646 )

Suggestion:

"%s:\n%s\n%s" % (self.NAME, self.rev_info(), self.psign)
Add a __str__ method to ProtoSign, but exclude the signature from the output:

return "%s: type: %s src: %s ts: %s" % (self.NAME, self.p.type, self.p.src, iso_timestamp(self.p.timestamp))

python/lib/packet/path_mgmt/rev_info.py, line 118 at r6 (raw file):

        if self.p.ifID == 0:
            raise RevInfoValidationError("Invalid ifID: %s" % self.p.ifID)
        if self.isd_as().is_zero():

This isn't a great check. It will only fire if both ISD and AS are zero.

self.isd_as()
if self._isd_as[0] == 0 or self._isd_as[1] == 0:

python/lib/packet/path_mgmt/rev_info.py, line 125 at r6 (raw file):

        # Make sure the revocation timestamp is within the validity window
        assert self.p.timestamp <= now + 1, self.p.timestamp
        return now < (self.p.timestamp + self.p.ttl)

This should probably be <=.

Comments from Reviewable

kormat · 2018-05-02T11:55:24Z

Reviewed 4 of 44 files at r6.
Review status: 36 of 62 files reviewed at latest revision, 51 unresolved discussions.

python/lib/packet/proto_sign.py, line 59 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Eh, cough, i think the TS was a later addition to the design, and i forgot? That's the best "reason" i have :)

Tracked by #1524

python/lib/packet/proto_sign.py, line 76 at r6 (raw file):

    def sig_pack(self, incl_sig=True):
        # XXX(worxli) add ts to signature but in sep. PR as it also needs changes in BR
        # b = [str(self.p.type).encode("utf-8"), self.p.src, self.p.timestamp.to_bytes(4, 'big')]

As timestamp is field @3, it should come after the signature.

python/lib/sciond_api/revocation.py, line 48 at r6 (raw file):

    def short_desc(self):
        return self.srev_info().rev_info().short_desc()

With the updated SignedRevInfo.short_desc(), you can just use that instead.

Comments from Reviewable

kormat · 2018-05-02T12:19:06Z

Reviewed 1 of 44 files at r6.
Review status: 37 of 62 files reviewed at latest revision, 52 unresolved discussions.

python/scion_elem/scion_elem.py, line 1267 at r6 (raw file):

        if not rev_info.active():
            return False
        cert = self.trust_store.get_cert(rev_info.isd_as())

As discussed offline, this will work for now as we don't actually have updating certs yet, but i've filed #1545 to track doing this properly. Add a FIXME for now.

Comments from Reviewable

kormat · 2018-05-02T12:30:46Z

Reviewed 1 of 44 files at r6.
Review status: 38 of 62 files reviewed at latest revision, 48 unresolved discussions.

python/sciond/sciond.py, line 199 at r1 (raw file):

I don't think this works because in handle_revocation we need the results from the different steps.

The simple answer there is to refactor check_revocation:

Define 5 different exceptions in rev_info.py, one for each of the results.
Have the rev_info.validate() and rev_info.active() methods raise the appropriate exceptions, and don't catch them in check_revocation.
Raise the remaining exceptions in check_revocation

That way most callers can just catch the base rev info exception and stop processing it, and sciond can look at the exception type to determine what result to return to the client.

Unrelatedly, when are revoked segments removed based on revocations received in handle_path_reply?

Comments from Reviewable

kormat · 2018-05-02T13:05:45Z

Reviewed 2 of 44 files at r6.
Review status: 40 of 62 files reviewed at latest revision, 43 unresolved discussions.

python/lib/packet/pcb.py, line 313 at r6 (raw file):

                hof = pcbm.hof()
                if rev_info.p.ifID == hof.ingress_if:
                    return True, LinkType.PARENT if i == 0 else LinkType.PEER

I've just realised - as a path segment doesn't know if it's a core segment or not, you need to pass in another parameter to this method. It could be as simple as core=False. If core is true, then all link types should be LinkType.CORE.

python/path_server/base.py, line 188 at r6 (raw file):

            rev_info = srev_info.rev_info()
            if not self.check_revocation(srev_info, "zk"):
                return

continue

python/path_server/base.py, line 237 at r6 (raw file):

        rev_info = srev_info.rev_info()
        try:
            rev_info.validate()

_handle_revocation already does this - is this needed?

python/path_server/base.py, line 256 at r6 (raw file):

        # failure if the rev_info is invalid.
        try:
            rev_info.validate()

This is done in self.check_revocation - maybe drop this and just use check_revocation before checking for presence in self.revocations.

python/path_server/base.py, line 290 at r6 (raw file):

        def _handle_one_seg(seg, db):
            rm, ltype = seg.rev_match(rev_info)
            if rm and (ltype in [LinkType.PARENT, LinkType.CHILD] and

This should also include LinkType.CORE.

Comments from Reviewable

kormat · 2018-05-02T13:10:35Z

Reviewed 4 of 44 files at r6.
Review status: 44 of 62 files reviewed at latest revision, 48 unresolved discussions.

python/lib/packet/path_mgmt/rev_info.py, line 59 at r6 (raw file):

        """
        Verfiy the signature
        """

There needs to be a check that the signing AS is the same as the revoking AS.

Comments from Reviewable

kormat · 2018-05-02T13:43:03Z

Reviewed 2 of 44 files at r6.
Review status: 46 of 62 files reviewed at latest revision, 43 unresolved discussions.

python/beacon_server/base.py, line 715 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Done.

In this case it would be better to do the line-break after and, so that the second condition is on one line.

python/beacon_server/base.py, line 746 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

From my side I think what you said is correct. But the interface should be revoked the same amount of time for everyone (minus time sync differences)?

Oh, excellent point @shitz. We should be caching revocations when we create them, and using those when responding to ifstate requests. Having two places issue revocations is not desirable from an understandability point of view.

We could do this by replacing if_id_last_revoked with a self.if_revocationdict that just stores the last revocation for that interface.

python/beacon_server/base.py, line 536 at r6 (raw file):

                if not self.check_revocation(srev_info, "zk"):
                    continue
                self.local_rev_cache.add(srev_info.copy())

Ah. This doesn't actually need to be a copy, as the ownership is being transferred.

python/beacon_server/base.py, line 588 at r6 (raw file):

        logging.debug("Received revocation via SCMP: %s (from %s)", rev_info.short_desc(), meta)
        try:
            rev_info.validate()

Same applies here - this is done by check_revocation in _handle_revocation.

python/beacon_server/base.py, line 698 at r6 (raw file):

                        continue
                    if (if_state.is_revoked() and if_id_last_revoked[if_id] +
                       self.REVOCATION_TTL > start_time):

As discussed offline, let's do over-lapping revocations. If the existing revocation has <=2s left, issue a new revocation. This prevents a whole class of awkward race-conditions.

python/lib/rev_cache.py, line 107 at r1 (raw file):

depending on the context failures need to be handled differently.

Using the per-failure-type exceptions would solve that issue.

In any case, i'm a lot happier now that check_revocation exists, it makes it a lot easier to handle revocations consistently.

My point about AS mismatch appears to have been missed, so i opened a separate comment thread about it on SignedRevInfo.verify.

Comments from Reviewable

kormat · 2018-05-02T14:08:32Z

Reviewed 1 of 12 files at r4, 4 of 44 files at r6.
Review status: 51 of 62 files reviewed at latest revision, 40 unresolved discussions.

go/lib/ctrl/path_mgmt/rev_info.go, line 85 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Shouldn't it be ...+r.RevTTL < now ?

Oops, yep :)

I've just realised though that r.Timestamp > now+1 should raise a different, non-timeout, error.

go/lib/ctrl/path_mgmt/rev_info.go, line 35 at r6 (raw file):

var _ common.Timeout = (*RevTimeError)(nil)

type RevTimeError struct {

This can be simpler: type RevTimeError string

go/lib/ctrl/path_mgmt/rev_info.go, line 41 at r6 (raw file):

func NewRevTimeError(ts uint64, ttl uint32) RevTimeError {
	return RevTimeError{Msg: fmt.Sprintf(
		"Revocation is not valid in window, timestamp: %d, TTL %ds.", ts, ttl)}

"Revocation is not in valid window"?

timestamp: %s

util.TimeToString(util.USecsToTime(ts))

go/lib/pathmgr/pathmgr.go, line 234 at r6 (raw file):

}

func (r *PR) revoke(revInfo common.RawBytes) {

b, or rawRevInfo.

go/lib/pathmgr/pathmgr.go, line 235 at r6 (raw file):

func (r *PR) revoke(revInfo common.RawBytes) {
	parsedSRev, err := path_mgmt.NewSignedRevInfoFromRaw(revInfo)

sRevInfo

go/lib/pathmgr/pathmgr.go, line 256 at r6 (raw file):

		// Continue with revocation
	}
	parsedRev, err := parsedSRev.RevInfo()

revInfo

go/lib/sciond/sciond.go, line 309 at r6 (raw file):

func (c *connector) RevNotificationFromRaw(b []byte) (*RevReply, error) {
	// Extract information from notification
	ri, err := path_mgmt.NewSignedRevInfoFromRaw(b)

sRevInfo?

go/lib/sciond/types.go, line 237 at r6 (raw file):

type RevNotification struct {
	RevInfo *path_mgmt.SignedRevInfo

SRevInfo

Comments from Reviewable

kormat · 2018-05-02T14:09:38Z

Review status: 51 of 62 files reviewed at latest revision, 47 unresolved discussions.

go/lib/ctrl/path_mgmt/rev_info.go, line 41 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

"Revocation is not in valid window"?

timestamp: %s

util.TimeToString(util.USecsToTime(ts))

Actually, based on my comment below, the string can be changed to say revocation expired.

Comments from Reviewable

kormat · 2018-05-02T14:18:48Z

Reviewed 1 of 35 files at r2, 1 of 12 files at r4, 9 of 44 files at r6.
Review status: 61 of 62 files reviewed at latest revision, 44 unresolved discussions.

go/border/ifstate/ifstate.go, line 72 at r6 (raw file):

	Active       bool
	SRevInfo     *path_mgmt.SignedRevInfo
	RawRev       common.RawBytes

RawSRev

go/border/rpkt/path.go, line 106 at r6 (raw file):

	revInfo, err := state.SRevInfo.RevInfo()
	if err != nil {
		rp.Warn("Could not parse RevInfo for interface", "ifid", ifid)

The two changes on this line don't look right. The interface is revoked, ifid is a pointer.

go/border/rpkt/payload_scmp.go, line 26 at r6 (raw file):

)

type RawRevCallbackArgs struct {

RawSRevCallbackArgs

go/border/rpkt/process.go, line 351 at r6 (raw file):

	}
	if args.SignedRevInfo, err = path_mgmt.NewSignedRevInfoFromRaw(infoRev.RawRev); err != nil {
		return common.NewBasicError("Unable to decode rawRev", err)

"Unable to decode SignedRevInfo from SCMP InfoRevocation payload"

go/border/rpkt/rpkt.go, line 48 at r6 (raw file):

// for various processing tasks.
var callbacks struct {
	rawRevF func(RawRevCallbackArgs)

rawSRevF

go/lib/ctrl/path_mgmt/ifstate_infos.go, line 53 at r6 (raw file):

	desc := fmt.Sprintf("IfID: %v, Active: %v", i.IfID, i.Active)
	if i.SRevInfo != nil {
		desc += fmt.Sprintf(", SRevInfo: %v", i.SRevInfo)

%s, as SignedRevInfo implements Stringer.

go/lib/scmp/info.go, line 173 at r6 (raw file):

type InfoRevocation struct {
	*InfoPathOffsets
	RawRev common.RawBytes

RawSRev

Comments from Reviewable

kormat · 2018-05-02T14:36:36Z

Review status: 61 of 62 files reviewed at latest revision, 51 unresolved discussions.

python/lib/rev_cache.py, line 64 at r6 (raw file):

    def __contains__(self, rev_info):  # pragma: no cover
        return self.contains_key(_mk_key(rev_info))

This is a trap - overlapping revocations will cause the new one to be discarded. Proposal:

stored_info = self.get(_mk_key(srev_info))
if not stored_info:
  return None
return stored_info == srev_info

(assuming SignedRevInfo implements eq appropriately).

Comments from Reviewable

kormat · 2018-05-02T14:41:04Z

Reviewed 1 of 44 files at r6.
Review status: all files reviewed at latest revision, 52 unresolved discussions.

python/lib/rev_cache.py, line 94 at r6 (raw file):

                if self._check_active(v):
                    ret.append(v)
            return ret

This should be outside the lock.

Comments from Reviewable

worxli · 2018-05-03T10:53:25Z

Review status: all files reviewed at latest revision, 53 unresolved discussions.

go/border/ifstate/ifstate.go, line 72 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

RawSRev

Done.

go/border/rpkt/path.go, line 106 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

The two changes on this line don't look right. The interface is revoked, ifid is a pointer.

What aside from the missing pointer does not look right?

go/border/rpkt/payload_scmp.go, line 26 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

RawSRevCallbackArgs

Done.

go/border/rpkt/process.go, line 351 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

"Unable to decode SignedRevInfo from SCMP InfoRevocation payload"

Done.

go/border/rpkt/rpkt.go, line 48 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

rawSRevF

Done.

go/lib/ctrl/path_mgmt/ifstate_infos.go, line 53 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

%s, as SignedRevInfo implements Stringer.

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 35 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This can be simpler: type RevTimeError string

Done.

go/lib/ctrl/path_mgmt/rev_info.go, line 41 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Actually, based on my comment below, the string can be changed to say revocation expired.

Done.

go/lib/pathmgr/pathmgr.go, line 234 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

b, or rawRevInfo.

Done.

go/lib/pathmgr/pathmgr.go, line 235 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

sRevInfo

Done.

go/lib/pathmgr/pathmgr.go, line 256 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

revInfo

Done.

go/lib/sciond/sciond.go, line 309 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

sRevInfo?

Done.

go/lib/sciond/types.go, line 237 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

SRevInfo

Done.

go/lib/scmp/info.go, line 173 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

RawSRev

Done.

python/integration/base_cli_srv.py, line 135 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should be at the DEBUG level.

Done.

python/integration/base_cli_srv.py, line 136 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This formats badly. "\n".join(...) will work well, each entry self-indents.

Done.

python/lib/packet/path_mgmt/rev_info.py, line 46 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

NAME = "SignedRevInfo, for consistency.

Done.

python/lib/packet/path_mgmt/rev_info.py, line 67 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Whenever i see this, i always wonder where this is getting called from that the identity of other isn't already guaranteed. A simpler version is just to do:
assert isinstance(other, SignedRevInfo), type(SignedRevInfo)

Done.

Comments from Reviewable

worxli · 2018-05-03T15:17:48Z

Review status: 36 of 63 files reviewed at latest revision, 53 unresolved discussions.

python/beacon_server/base.py, line 715 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

In this case it would be better to do the line-break after and, so that the second condition is on one line.

Done.

python/beacon_server/base.py, line 746 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Oh, excellent point @shitz. We should be caching revocations when we create them, and using those when responding to ifstate requests. Having two places issue revocations is not desirable from an understandability point of view.

We could do this by replacing if_id_last_revoked with a self.if_revocationdict that just stores the last revocation for that interface.

Done.

python/beacon_server/base.py, line 536 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Ah. This doesn't actually need to be a copy, as the ownership is being transferred.

Done.

python/beacon_server/base.py, line 588 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Same applies here - this is done by check_revocation in _handle_revocation.

Done.

python/beacon_server/base.py, line 698 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

As discussed offline, let's do over-lapping revocations. If the existing revocation has <=2s left, issue a new revocation. This prevents a whole class of awkward race-conditions.

Done.

python/lib/rev_cache.py, line 64 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This is a trap - overlapping revocations will cause the new one to be discarded. Proposal:
stored_info = self.get(_mk_key(srev_info))
if not stored_info:
  return None
return stored_info == srev_info
(assuming SignedRevInfo implements eq appropriately).

Done.

python/lib/rev_cache.py, line 94 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should be outside the lock.

Done.

python/lib/packet/pcb.py, line 313 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I've just realised - as a path segment doesn't know if it's a core segment or not, you need to pass in another parameter to this method. It could be as simple as core=False. If core is true, then all link types should be LinkType.CORE.

Done.

python/lib/packet/proto_sign.py, line 76 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

As timestamp is field @3, it should come after the signature.

I don't get that? But it needs to be used for the signature?

python/lib/packet/path_mgmt/rev_info.py, line 59 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

There needs to be a check that the signing AS is the same as the revoking AS.

Done.

python/lib/packet/path_mgmt/rev_info.py, line 76 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This is not a useful format:

SRevInfo Blob: b'\x10\x05\x10\x04\x01Oc\x11\x02\xff\x02\x11\x03\n\x0f~\x9d\xe9Z' Sign: ( type = ed25519,
>   src = "DEFAULT: IA: 2-ff00:0:211 CHAIN: 1 TRC: 1",
>   signature = "\x97bDLo,\x96r\xca\b\xd5\xf0O\x1f\xe7\xcb\xb8z{\xf6\x97f\x9eUTv\xfb._\x90z\xd1{H\x99\xc4\xc8\x8f\xb7\x84r\x9f\xb8\xb4\xb2Be^\xacK\x1d\xaf\x1b\xe7h(@\x85\xd7\xdb\xec\xda\x19\a",
>   timestamp = 1525259646 )

Suggestion:

"%s:\n%s\n%s" % (self.NAME, self.rev_info(), self.psign)
Add a __str__ method to ProtoSign, but exclude the signature from the output:

return "%s: type: %s src: %s ts: %s" % (self.NAME, self.p.type, self.p.src, iso_timestamp(self.p.timestamp))

Done.

python/lib/packet/path_mgmt/rev_info.py, line 118 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This isn't a great check. It will only fire if both ISD and AS are zero.
self.isd_as()
if self._isd_as[0] == 0 or self._isd_as[1] == 0:

Done.

python/lib/packet/path_mgmt/rev_info.py, line 125 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should probably be <=.

Done.

python/lib/sciond_api/revocation.py, line 48 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

With the updated SignedRevInfo.short_desc(), you can just use that instead.

Done.

python/path_server/base.py, line 188 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

continue

Done.

python/path_server/base.py, line 237 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

_handle_revocation already does this - is this needed?

Done.

python/path_server/base.py, line 256 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This is done in self.check_revocation - maybe drop this and just use check_revocation before checking for presence in self.revocations.

As discussed, validation before checking presence in self.revocations.

python/path_server/base.py, line 290 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should also include LinkType.CORE.

Done.

python/scion_elem/scion_elem.py, line 1267 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

As discussed offline, this will work for now as we don't actually have updating certs yet, but i've filed #1545 to track doing this properly. Add a FIXME for now.

Done.

python/sciond/sciond.py, line 199 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I don't think this works because in handle_revocation we need the results from the different steps.

The simple answer there is to refactor check_revocation:

Define 5 different exceptions in rev_info.py, one for each of the results.

Have the rev_info.validate() and rev_info.active() methods raise the appropriate exceptions, and don't catch them in check_revocation.

Raise the remaining exceptions in check_revocation

That way most callers can just catch the base rev info exception and stop processing it, and sciond can look at the exception type to determine what result to return to the client.

Unrelatedly, when are revoked segments removed based on revocations received in handle_path_reply?

Seems like they are not. Added a helper fct which does the removal.

Comments from Reviewable

kormat · 2018-05-04T10:12:16Z

Reviewed 27 of 27 files at r7.
Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

go/border/rpkt/path.go, line 106 at r6 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

What aside from the missing pointer does not look right?

A previous version had: "RevInfo for revoked interface"

go/lib/pathmgr/pathmgr.go, line 238 at r7 (raw file):

	if err != nil {
		log.Error("Revocation failed, unable to parse signed revocation info",
			"sRevInfo", b, "err", err)

s/sRevInfo/raw/

go/lib/pathmgr/pathmgr.go, line 258 at r7 (raw file):

	revInfo, err := sRevInfo.RevInfo()
	if err != nil {
		log.Error("Revocation failed, unable to parse revocation info",

If this fails, then revInfo will be a zero-value. It'd be better to log the sRevInfo instead.

python/beacon_server/base.py, line 145 at r7 (raw file):

            self.ifid_state[ifid] = InterfaceState()
        self.ifid_state_lock = RLock()
        self.if_revocations = defaultdict(None)

Wait, why a defaultdict? It's just a simple dict, no?

python/beacon_server/base.py, line 708 at r7 (raw file):

                        # we want to issue a new revocation REVOCATION_OVERLAP seconds
                        # before it is expired
                        revoked_until = srev_info.rev_info().p.timestamp + self.REVOCATION_TTL

It would be cleaner to use the TTL in the revinfo itself.

rev_info = srev_info.rev_info()
if rev_info.p.timestamp + rev_info.p.ttl - self.REVOCATION_OVERLAP > start_time:
   contine

python/lib/packet/pcb.py, line 313 at r7 (raw file):

                hof = pcbm.hof()
                if rev_info.p.ifID == hof.ingress_if:
                    return True, LinkType.CORE if core else (LinkType.PARENT if i == 0

This is getting hard to follow :) It would be cleaner to just handle it with normal if/else statements rather than trinary.

python/lib/packet/proto_sign.py, line 32 at r7 (raw file):

class DefaultSignSrc(Serializable):

This should be at the end of the file, as it's less fundamental than ProtoSign, ProtoSignedBlob.

python/lib/packet/proto_sign.py, line 171 at r7 (raw file):

        return self.psign.verify(key, self.p.blob)

    def get_signer_from_proto_sign(self):

This shouldn't hard-code the signer src type. ProtoSignedBlob can't know the type of the src, and in some contexts the source won't even be an AS, it can be an element (e.g. for the proposal to sign intra-AS infra messages).

python/lib/packet/path_mgmt/rev_info.py, line 39 at r7 (raw file):

class CertFetchError(SCIONBaseError):

This name is too generic given how we do python imports. It ends up as a bare "CertFetchError" in other python code, which gives no hints as where it comes from. SignedRevInfoCertFetchError is much clearer (despite being saddeningly long).

python/lib/packet/path_mgmt/rev_info.py, line 44 at r7 (raw file):

class RevInfoValidationError(SCIONBaseError):
    """Active check on RevInfo failed"""

The docstring doesn't really match the error name. Same for RevInfoExpiredError.

python/path_server/base.py, line 216 at r7 (raw file):

        return False

    def handle_ifstate_infos(self, cpld, meta):

I'm preeeetty sure this method shouldn't exist. Waiting on @shitz to Explain.

python/scion_elem/scion_elem.py, line 1268 at r7 (raw file):

        cert = self.trust_store.get_cert(rev_info.isd_as())
        if not cert:
            raise CertFetchError("Failed to fetch cert for ISD-AS: %s" % rev_info.isd_as())

"Failed to fetch cert for SRevInfo"

python/sciond/sciond.py, line 440 at r7 (raw file):

            return SCIONDRevReplyStatus.INVALID
        except RevInfoExpiredError as e:
            logging.warning("Failed to verify RevInfo validity, %s from %s: %s",

The wording here is a bit confusing, it probably also doesn't warrant a warning level:

logging.info("Ignoring expired Revinfo, %s from %s", srev_info.short_desc(), meta)

python/sciond/sciond.py, line 454 at r7 (raw file):

            logging.error("Revocation check failed for %s from %s:\n%s",
                          srev_info.short_desc(), meta, e)
            return

This should probably also be .UNKNOWN.

Comments from Reviewable

worxli · 2018-05-04T13:06:19Z

Review status: 55 of 63 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

go/border/rpkt/path.go, line 106 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

A previous version had: "RevInfo for revoked interface"

Which is still there, but I think the order was wrong.

go/lib/pathmgr/pathmgr.go, line 238 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

s/sRevInfo/raw/

Done.

go/lib/pathmgr/pathmgr.go, line 258 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

If this fails, then revInfo will be a zero-value. It'd be better to log the sRevInfo instead.

Done.

python/beacon_server/base.py, line 145 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Wait, why a defaultdict? It's just a simple dict, no?

Done.

python/beacon_server/base.py, line 708 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

It would be cleaner to use the TTL in the revinfo itself.
rev_info = srev_info.rev_info()
if rev_info.p.timestamp + rev_info.p.ttl - self.REVOCATION_OVERLAP > start_time:
   contine

Done.

python/lib/packet/pcb.py, line 313 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This is getting hard to follow :) It would be cleaner to just handle it with normal if/else statements rather than trinary.

Done.

python/lib/packet/proto_sign.py, line 32 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should be at the end of the file, as it's less fundamental than ProtoSign, ProtoSignedBlob.

Done.

python/lib/packet/proto_sign.py, line 171 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This shouldn't hard-code the signer src type. ProtoSignedBlob can't know the type of the src, and in some contexts the source won't even be an AS, it can be an element (e.g. for the proposal to sign intra-AS infra messages).

Done.

python/lib/packet/path_mgmt/rev_info.py, line 39 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This name is too generic given how we do python imports. It ends up as a bare "CertFetchError" in other python code, which gives no hints as where it comes from. SignedRevInfoCertFetchError is much clearer (despite being saddeningly long).

Done.

python/lib/packet/path_mgmt/rev_info.py, line 44 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

The docstring doesn't really match the error name. Same for RevInfoExpiredError.

Yeah, they were exchanged.

python/scion_elem/scion_elem.py, line 1268 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

"Failed to fetch cert for SRevInfo"

Done.

python/sciond/sciond.py, line 440 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

The wording here is a bit confusing, it probably also doesn't warrant a warning level:
logging.info("Ignoring expired Revinfo, %s from %s", srev_info.short_desc(), meta)

Done.

python/sciond/sciond.py, line 454 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should probably also be .UNKNOWN.

Done.

Comments from Reviewable

- Border Router - SCIOND - Path Server - Beacon Server The hashtree based revocation has been replaced by signed messages.

worxli · 2018-05-07T06:44:59Z

Review status: 55 of 63 files reviewed at latest revision, 29 unresolved discussions.

python/path_server/base.py, line 216 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'm preeeetty sure this method shouldn't exist. Waiting on @shitz to Explain.

According to #1216 it should probably :)

Comments from Reviewable

shitz · 2018-05-07T06:52:39Z

Review status: 55 of 63 files reviewed at latest revision, 29 unresolved discussions.

python/path_server/base.py, line 216 at r7 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

According to #1216 it should probably :)

It seems that revocations that are issued from the BS to the PS are sent as part of an IFStatePayload object, instead of just a raw RevInfo. I don't see any reason of using IFStatePayload over RevInfo except simpler code in the BS, since it already creates the IFStatePayloads for the border routers. If needed/desired, this can be changed to a BS issuing a revocation towards the PS in form of a RevInfo

Comments from Reviewable

kormat · 2018-05-07T08:58:57Z

Reviewed 8 of 8 files at r8.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

go/border/rpkt/path.go, line 114 at r8 (raw file):

		return nil
	}
	// Check that the revocation timestamp is within the TTL.

That's an odd phrasing. I'm not sure that a comment is needed in any case, as revInfo.Active() is fairly self-explanatory.

proto/rev_info.capnp, line 12 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Though, I checked other timestamps in capnp and they also seem to be seconds and UInt64.

@worxli : i had a quick look, and that's fair, we're pretty inconsistent on this. We should really fix that. I'll file an issue, we can leave this as UInt64 for now.

(Filed #1548)

python/beacon_server/base.py, line 145 at r7 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Done.

= {} is more common.

python/path_server/base.py, line 216 at r7 (raw file):

Previously, shitz wrote…

It seems that revocations that are issued from the BS to the PS are sent as part of an IFStatePayload object, instead of just a raw RevInfo. I don't see any reason of using IFStatePayload over RevInfo except simpler code in the BS, since it already creates the IFStatePayloads for the border routers. If needed/desired, this can be changed to a BS issuing a revocation towards the PS in form of a RevInfo

Filed #1549

Comments from Reviewable

worxli · 2018-05-07T09:18:23Z

Review status: 61 of 63 files reviewed at latest revision, 18 unresolved discussions.

go/border/rpkt/path.go, line 114 at r8 (raw file):

Previously, kormat (Stephen Shirley) wrote…

That's an odd phrasing. I'm not sure that a comment is needed in any case, as revInfo.Active() is fairly self-explanatory.

Done.

python/beacon_server/base.py, line 145 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

= {} is more common.

Done.

Comments from Reviewable

kormat · 2018-05-07T10:25:14Z

Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 16 unresolved discussions.

Comments from Reviewable

kormat · 2018-05-07T10:25:32Z

Please make sure you update the relevant design doc.

Review status: all files reviewed at latest revision, 16 unresolved discussions.

Comments from Reviewable

shitz · 2018-05-07T11:53:56Z

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

worxli requested a review from kormat April 12, 2018 09:29

worxli changed the title ~~Rework of the revocation authentication, this includes changes in:~~ Rework of the revocation authentication Apr 12, 2018

kormat added the i/breaking change PR that breaks forwards or backwards compatibility label Apr 12, 2018

worxli force-pushed the feature-revocation-rework branch from e14e91a to 3d1c3a7 Compare April 17, 2018 11:49

worxli force-pushed the feature-revocation-rework branch from f06aa98 to 0cbad19 Compare April 18, 2018 14:33

worxli self-assigned this Apr 19, 2018

worxli force-pushed the feature-revocation-rework branch from 0cbad19 to 75abecf Compare April 26, 2018 07:25

shitz modified the milestone: Test Sprint Apr 27, 2018

worxli force-pushed the feature-revocation-rework branch from 30ee513 to 01fecee Compare May 2, 2018 06:10

worxli force-pushed the feature-revocation-rework branch from eba80fc to 66988dc Compare May 3, 2018 15:30

worxli added 9 commits May 4, 2018 15:11

Rework of the revocation authentication, this includes changes in:

a18c308

- Border Router - SCIOND - Path Server - Beacon Server The hashtree based revocation has been replaced by signed messages.

Comments

00f32df

Further renaming

51c4680

Fix PS looping and add __eq__

e7d00e6

fix topologies and generator

94c88ff

fix pathdb full issue

bb51ea0

fix signer and linting

aa25964

Fix review comments

553eabe

fix comments

5dac9b5

worxli force-pushed the feature-revocation-rework branch from a1bf6a8 to 5dac9b5 Compare May 4, 2018 13:11

kormat mentioned this pull request May 7, 2018

Use consistent units for timestamps #1548

Closed

comments

d87a9a8

worxli merged commit 2b9bbf4 into scionproto:master May 7, 2018

worxli deleted the feature-revocation-rework branch May 7, 2018 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework of the revocation authentication #1513

Rework of the revocation authentication #1513

worxli commented Apr 12, 2018 •

edited

Loading

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

worxli commented Apr 13, 2018

shitz commented Apr 16, 2018

worxli commented Apr 17, 2018

kormat commented Apr 17, 2018

worxli commented Apr 17, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

worxli commented May 3, 2018

worxli commented May 3, 2018

kormat commented May 4, 2018

worxli commented May 4, 2018

worxli commented May 7, 2018

shitz commented May 7, 2018

kormat commented May 7, 2018

worxli commented May 7, 2018

kormat commented May 7, 2018

kormat commented May 7, 2018

shitz commented May 7, 2018

Rework of the revocation authentication #1513

Rework of the revocation authentication #1513

Conversation

worxli commented Apr 12, 2018 • edited Loading

Benchmarks

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

kormat commented Apr 12, 2018

worxli commented Apr 13, 2018

shitz commented Apr 16, 2018

worxli commented Apr 17, 2018

kormat commented Apr 17, 2018

worxli commented Apr 17, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

kormat commented May 2, 2018

worxli commented May 3, 2018

worxli commented May 3, 2018

kormat commented May 4, 2018

worxli commented May 4, 2018

worxli commented May 7, 2018

shitz commented May 7, 2018

kormat commented May 7, 2018

worxli commented May 7, 2018

kormat commented May 7, 2018

kormat commented May 7, 2018

shitz commented May 7, 2018

worxli commented Apr 12, 2018 •

edited

Loading