Reviewed 15 of 15 files at r1.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @oncilla)

a discussion (no related file):
New APIs look really nice 💯; I'm curious how they will plug together to build the full trust store implementation.

I've added quite a few comments, but most of them revolve around "Can we do without X?". Maybe a lot of the suggested simplifications do not make sense (because I'm not as familiar with what building blocks are needed later without seeing an implementation), but it's useful to strip out as much as possible. We can always add fields/methods later.

go/lib/infra/modules/trust/v2/db.go, line 37 at r1 (raw file):

)

// DB defines the interface an trust DB must implement.

s/an/a

go/lib/infra/modules/trust/v2/db.go, line 46 at r1 (raw file):

}

// Transaction represents a trust DB transaction with an ongoing transaction. To

First sentence is odd here.

go/lib/infra/modules/trust/v2/db.go, line 81 at r1 (raw file):

	// The ErrHashMismatch error is returned if the TRC is in the database and
	// the hash does not match.
	CompareTRCHash(ctx context.Context, isd addr.ISD,

What is the use case for this?

I'm asking here (and below, for chains), because I'm wondering if this is just an equality test, because if that is the case simpler APIs might exist.

go/lib/infra/modules/trust/v2/db.go, line 87 at r1 (raw file):

	// GetRawTRC returns the raw signed TRC bytes. If it is not found,
	// ErrNotFound is returned.
	GetRawTRC(ctx context.Context, isd addr.ISD, version scrypto.Version) ([]byte, error)

Is there a reason other than performance to support both GetTRC and GetRawTRC? Could GetRawTRC be replaced with GetTRC followed by a form of Serialize?

go/lib/infra/modules/trust/v2/db.go, line 90 at r1 (raw file):

	// GetTRCInfo returns the infos for the requested TRC. If it is not found,
	// ErrNotFound is returned.
	GetTRCInfo(ctx context.Context, isd addr.ISD, version scrypto.Version) (TRCInfo, error)

I think it's useful to try to minimize the surface area of the interface.

Could this be replaced with GetTRC, followed by direct field access for Version, GracePeriod, and Validity?

go/lib/infra/modules/trust/v2/db.go, line 97 at r1 (raw file):

	// InsertTRC inserts the TRCs. The call returns true if the TRC was
	// inserter, or false if it already existed and the Hash matches.
	InsertTRC(ctx context.Context, decoded DecodedTRC, hash []byte) (bool, error)

Is the hash only to protect against duplicates? If that is the case, it seems like a TRCWrite implementation detail (it should be the job of the implementation to implement the invariant), and not something that should be specified by callers.

go/lib/infra/modules/trust/v2/db.go, line 104 at r1 (raw file):

	// GetRawChain returns the raw signed certificate chain bytes. If it is not
	// found, ErrNotFound is returned.
	GetRawChain(ctx context.Context, ia addr.IA, version scrypto.Version) ([]byte, error)

I see only the raw call is present here (as oppose to TRCs where both Raw and Parsed are present). Is this on purpose?

go/lib/infra/modules/trust/v2/db.go, line 109 at r1 (raw file):

	// hash matches. The ErrHashMismatch error is returned if the AS certificate
	// is in the database and the hash does not match.
	CompareASHash(ctx context.Context, ia addr.IA, version scrypto.Version,

Same as above, what is the use case for this?

go/lib/infra/modules/trust/v2/db.go, line 115 at r1 (raw file):

	// database and the hash matches. The ErrHashMismatch error is returned if
	// the issuer certificate is in the database and the hash does not match.
	CompareIssuerHash(ctx context.Context, ia addr.IA,

What is the use case for this?

go/lib/infra/modules/trust/v2/db.go, line 122 at r1 (raw file):

type ChainWrite interface {
	// InsertChain inserts the certificate chain. The call returns true if the
	// certificate chain was inserter, or false if it already existed and the

s/inserter/inserted

go/lib/infra/modules/trust/v2/decode.go, line 25 at r1 (raw file):

// DecodedTRC is a container for the decoded TRC.
type DecodedTRC struct {

This struct contains the same information 3 times. Is it possible to reduce it to 1?

go/lib/infra/modules/trust/v2/decode.go, line 39 at r1 (raw file):

// DecodedChain is a container for the decoded certificate chain.
type DecodedChain struct {

Same question as above.

go/lib/infra/modules/trust/v2/inserter.go, line 44 at r1 (raw file):

}

// TRCProviderFunc provides TRCs. It is used to configure the TRC retrival

s/retrival/retrieval

go/lib/infra/modules/trust/v2/provider.go, line 28 at r1 (raw file):

// CryptoProvider provides crypto material. A crypto provider can spawn network
// request if necessary and permitted.

s/request/requests

go/lib/infra/modules/trust/v2/provider.go, line 33 at r1 (raw file):

	// unless inactive TRCs are specifically allowed. The optionally configured
	// server is queried over the network if the TRC is not available locally.
	// Otherwise, the default server is queried.

Expand whodefault server (or if callers should check the implementation godocs because it can vary and it's not a property all interface implementations must have).

go/lib/infra/modules/trust/v2/recurser.go, line 22 at r1 (raw file):

// Recurser decides whether a recursive request is permitted for a given peer.
// For infra services use either ASLocalRecurser or LocalOnlyRecurser.

Think these two types are not defined yet in this PR. Add a FIXME for them.

go/lib/infra/modules/trust/v2/resolvers.go, line 39 at r1 (raw file):

	ISD       addr.ISD
	Version   scrypto.Version
	CacheOnly bool

Would it be possible to remove this? It seems to me like it's always been something of an implementation detail, i.e., something the "server" side of the interaction would care about, and not something for the "client" side.

Why would a client care that a server has a cache? What if there's multiple levels of caching? It seems like it leaks internals clients should know nothing about (unless it's to build some duct tape hack around something that doesn't work).

go/lib/infra/modules/trust/v2/resolvers.go, line 46 at r1 (raw file):

	IA        addr.IA
	Version   scrypto.Version
	CacheOnly bool

Same as above.

go/lib/infra/modules/trust/v2/rpc.go, line 31 at r1 (raw file):

	SendTRC(context.Context, *cert_mgmt.TRC, net.Addr) error
	SendCertChain(context.Context, *cert_mgmt.Chain, net.Addr) error
	SetMsgr(msgr infra.Messenger)

Can this SetMsgr be removed? Looking at the rest of the interface, it doesn't feel like it's needed here.

Reviewable status: 11 of 15 files reviewed, 20 unresolved discussions (waiting on @scrye)

a discussion (no related file):

To get a full overview how this will plug together, you can take a look at:
https://github.com/Oncilla/scion/tree/pub-trust-store

You could also test it out locally:
https://github.com/Oncilla/scion/tree/pub-enable-trust-store

go/lib/infra/modules/trust/v2/db.go, line 37 at r1 (raw file):

Done.

go/lib/infra/modules/trust/v2/db.go, line 46 at r1 (raw file):

copy-pasted.
Done.

go/lib/infra/modules/trust/v2/db.go, line 81 at r1 (raw file):

This check is used when we get crypto material, whether they already exist and if the contents agree.

Otherwise, e.g. in case of a TRC, we have to do a lot of verification + DB interaction to verify the TRC.

go/lib/infra/modules/trust/v2/db.go, line 87 at r1 (raw file):

No, with the new design the actual raw data matters.
GetTRC will return the TRC payload. -> no signatures, no encoded payload

go/lib/infra/modules/trust/v2/db.go, line 90 at r1 (raw file):

I would prefer to do that in the database. Otherwise, we always have to load the full TRC and parse it just to get the newest version. Super expensive for no gain.

In my world, this values will be columns in the database.

go/lib/infra/modules/trust/v2/db.go, line 97 at r1 (raw file):

It's to check whether peers provide TRCs with different contents.
-> malicious behavior

go/lib/infra/modules/trust/v2/db.go, line 104 at r1 (raw file):

When we need a non-raw call, I will add it. In the current version, this is not necessary.

go/lib/infra/modules/trust/v2/db.go, line 109 at r1 (raw file):

Ditto

go/lib/infra/modules/trust/v2/db.go, line 115 at r1 (raw file):

Ditto

go/lib/infra/modules/trust/v2/db.go, line 122 at r1 (raw file):

Done.

go/lib/infra/modules/trust/v2/decode.go, line 25 at r1 (raw file):

I don't think so.
Otherwise we need to always pass Raw and decode it everywhere?

go/lib/infra/modules/trust/v2/decode.go, line 39 at r1 (raw file):

ditto

go/lib/infra/modules/trust/v2/inserter.go, line 44 at r1 (raw file):

Done.

go/lib/infra/modules/trust/v2/provider.go, line 28 at r1 (raw file):

Done.

go/lib/infra/modules/trust/v2/provider.go, line 33 at r1 (raw file):

Done.

go/lib/infra/modules/trust/v2/recurser.go, line 22 at r1 (raw file):

Defined the structs with todos.

go/lib/infra/modules/trust/v2/resolvers.go, line 39 at r1 (raw file):

This is just a wrapper around the capnp TRC request.

How it is used currently, it tells the server whether to recurse or not.
Which might be useful, IDK.

This simply mimics the current behavior.
If we want to change this, I would suggest doing together with the ctrl msg redesign.

go/lib/infra/modules/trust/v2/resolvers.go, line 46 at r1 (raw file):

ditto

go/lib/infra/modules/trust/v2/rpc.go, line 31 at r1 (raw file):

RPC essential abstracts the messenger.
Since we have a circular dependency between the trust store and the messenger, this is not really possible.

the workflow currently is:
1 init trust store
2. init messenger with trust store
3. set messenger on trust store

If you have a better way, I would be happy to adopt it.

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

go/lib/infra/modules/trust/v2/resolvers.go, line 39 at r1 (raw file):

Sounds good to me. I didn't realize this mimics the capnp object.

go/lib/infra/modules/trust/v2/rpc.go, line 31 at r1 (raw file):

But doesn't this interface abstract the messenger itself? Why would the messenger have a call to set itself as the messenger?

I might be overlooking something very basic here 😄.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scrye)

go/lib/infra/modules/trust/v2/rpc.go, line 31 at r1 (raw file):

It hides the need for messenger.NexId()

in fact, rpc will be an object on the store that either has a messenger or not and returns an error if no messenger is set.
RPC is not a sub-interface of infra.Messenger

Reviewable status: complete! all files reviewed, all discussions resolved

go/lib/infra/modules/trust/v2/rpc.go, line 31 at r1 (raw file):

Aaah, ok, I thought it's meant as a subinterface. Ack.