TrustStore: Implement crypto provider

[oncilla]

The trust store is split into multiple sub-components.

The crypto provider handles crypto material requests. If the
requested crypto material is in the database, it is returned.
Otherwise, the crypto provider decides based on the recurser
whether it can issue a network request. If it is allowed to,
it selects the path and remote using the router and calls
resolver to resolve all the missing material.

This PR is the start of a series of PRs that make the new trust store
operational. The individual components are then glued together into
the trust store. To the client package, the trust store appears as
a single unit.

Contributes to #3115

---

This change is 

[scrye]

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

---

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

package trust

var NewCryptoProvider = newTestCryptoProvider

If this stays, it needs a docstring.

---

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

// newTestCryptoProvider returns a new crypto provider for testing.
func newTestCryptoProvider(db DBRead, recurser Recurser, resolver Resolver, router Router,

This sets all existing fields, so structure initialization might look better. It would also get rid of the Test helper, because the test would instantiate it with mocks.

---

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

	opts infra.TRCOpts, client net.Addr) (*trc.TRC, []byte, error) {

	dec, err := p.getTRC(ctx, isd, version, opts, nil)

Rename dec, it refers to an adjective which makes it a bit confusing.

---

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

	}
	if !opts.AllowInactive {
		info, err := p.db.GetTRCInfo(ctx, isd, scrypto.Version(scrypto.LatestVer))

Can scrypto.Version and scrypto.LatestVer have the same type? (They have the both underlying type right now anyway). The cast also makes the condition in which it appears (a few lines below) much harder to read.

---

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

			return nil, nil, serrors.WrapStr("unable to get latest TRC info", err)
		}
		endGracePeriod := info.Validity.NotBefore.Add(info.GracePeriod)

Move this under the only case for which it is relevant. You'll need to define a helper function for the grace period condition to keep the full condition on that case readable, something like:

case info.Version == dec.TRC.Version+1 && graceExpired(info.Validity, info.GracePeriod)

---

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

					"validity", dec.TRC.Validity)
			}
			fetched, err := p.fetchTRC(ctx, isd, version, opts, client)

Change version to explicit latest version.

---

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

					"version", fetched.TRC.Version, "validity", fetched.TRC.Version)
			}
			return dec.TRC, dec.Raw, nil

Delete. It makes it clearer that !opts.AllowInactive is just a sequence of checks.

---

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

		return decoded.TRC{}, serrors.WrapStr("localOnly requested", err)
	}
	return p.fetchTRC(ctx, isd, version, opts, client)

Move this under default in switch.

---

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

	opts infra.TRCOpts, client net.Addr) (decoded.TRC, error) {

	if err := p.recurser.AllowRecursion(client); err != nil {

Add a mention to the AllowRecursion docstring that a return value of nil means that recursion is allowed.

Alternatively, AllowRecursion can return (bool, error) so the error/reason for failing is always out of band.

---

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

	if opts.Server == nil {
		var err error
		if opts.Server, err = p.router.ChooseServer(ctx, isd); err != nil {

Nit: this relies on opts getting passed by value s.t. it doesn't clobber fetchTRC's caller's value. It's not a bug, but it can lead to a very subtle bug if a programmer were to say "oh, opts is pretty big nowadays, I'll refactor to pass by pointer", because the compiler will never complain.

It's safer to do:

server := opts.Server

at the start of this function, and then just use server throughout.

---

go/lib/infra/modules/trust/v2/testdata/gen_crypto_tar.sh, line 4 at r1 (raw file):

# usage: gen_crypto_tar.sh <scion-pki> <output-file>
# 

trailing whitespace

---

go/lib/infra/modules/trust/v2/testdata/gen_crypto_tar.sh, line 5 at r1 (raw file):

# usage: gen_crypto_tar.sh <scion-pki> <output-file>
# 
# CRYPTO_PATH="./go/lib/infra/modules/trust/v2/testdata"

Are these leftovers, or are they an example? If the latter, they should be marked as such.

[oncilla]

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

---

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

Previously, scrye (Sergiu Costea) wrote…

If this stays, it needs a docstring.

Done.

---

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

Previously, scrye (Sergiu Costea) wrote…

This sets all existing fields, so structure initialization might look better. It would also get rid of the Test helper, because the test would instantiate it with mocks.

That does not work unfortunately.
This is here in the export_test.go file such that the tests in the _test package can instantiate the private cryptoProvider

---

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

Previously, scrye (Sergiu Costea) wrote…

Rename dec, it refers to an adjective which makes it a bit confusing.

what would you suggest.

dec was chosen as the short form of decodedTRC

---

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

Previously, scrye (Sergiu Costea) wrote…

Can scrypto.Version and scrypto.LatestVer have the same type? (They have the both underlying type right now anyway). The cast also makes the condition in which it appears (a few lines below) much harder to read.

Yes, working on a PR to fix it.

---

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

Previously, scrye (Sergiu Costea) wrote…

Move this under the only case for which it is relevant. You'll need to define a helper function for the grace period condition to keep the full condition on that case readable, something like:

case info.Version == dec.TRC.Version+1 && graceExpired(info.Validity, info.GracePeriod)

Done.

---

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

Previously, scrye (Sergiu Costea) wrote…

Change version to explicit latest version.

Done.

---

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

Previously, scrye (Sergiu Costea) wrote…

Delete. It makes it clearer that !opts.AllowInactive is just a sequence of checks.

argh, that is actually a bug. It should return the fetched and not the expired.
Added unit test that catches this bug.

---

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

Previously, scrye (Sergiu Costea) wrote…

Move this under default in switch.

Done.

---

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

Previously, scrye (Sergiu Costea) wrote…

Add a mention to the AllowRecursion docstring that a return value of nil means that recursion is allowed.

Alternatively, AllowRecursion can return (bool, error) so the error/reason for failing is always out of band.

Done.

---

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

Previously, scrye (Sergiu Costea) wrote…

Nit: this relies on opts getting passed by value s.t. it doesn't clobber fetchTRC's caller's value. It's not a bug, but it can lead to a very subtle bug if a programmer were to say "oh, opts is pretty big nowadays, I'll refactor to pass by pointer", because the compiler will never complain.

It's safer to do:

server := opts.Server

at the start of this function, and then just use server throughout.

Done.

---

go/lib/infra/modules/trust/v2/testdata/gen_crypto_tar.sh, line 4 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

trailing whitespace

Done.

---

go/lib/infra/modules/trust/v2/testdata/gen_crypto_tar.sh, line 5 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Are these leftovers, or are they an example? If the latter, they should be marked as such.

This makes go test ./... work from root dir.

[scrye]

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

---

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

Previously, Oncilla wrote…

what would you suggest.

dec was chosen as the short form of decodedTRC

decTRC would work.

---

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

Previously, Oncilla wrote…

argh, that is actually a bug. It should return the fetched and not the expired.
Added unit test that catches this bug.

Oh, ouch, didn't even realize it. Got lucky here.

[oncilla]

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @scrye)

---

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

Previously, scrye (Sergiu Costea) wrote…

decTRC would work.

Done.

[scrye]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[scrye]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved