Reviewed 1 of 10 files at r1.
Reviewable status: 1 of 10 files reviewed, 16 unresolved discussions (waiting on @karampok)

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

		opts infra.ChainOpts, client net.Addr) ([]byte, error)
	//GetASKey ...
	GetASKey(context.Context, string, *infra.ChainOpts) ([]byte, error)

should be addr.IA and not string.
Also, you are missing the certificate version

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

type verifier struct {
	//server net.Addr //TODO(karampok). discuss: who is adding the server.

the server is added on demand.
look at the invocation of infra.Verifier.WithServer in the code base.

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

	//server net.Addr //TODO(karampok). discuss: who is adding the server.

	GracePeriod time.Duration //timeskew allowed

AllowedSkew?

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

	GracePeriod time.Duration //timeskew allowed
	SRC         *ctrl.SignSrcDef

Maybe call id BoundSrc to make the intention more clear.
see comments below.

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

	SRC         *ctrl.SignSrcDef
	Store       CryptoProvider
	IA          addr.IA

Maybe call BoundIA

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

		return true
	case *cert_mgmt.ChainReq, *cert_mgmt.TRCReq:
		if sign == nil || sign.Type == proto.SignType_none {

can be simplified to return sign == nil || sign.Type.....

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

	}

	sign := spld.Sign

why?

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

	sign := spld.Sign

	if age := time.Now().Sub(sign.Time()); age < 10*time.Second { // check will go away

not just the check, the full method.

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

	sign := spld.Sign

	if age := time.Now().Sub(sign.Time()); age < 10*time.Second { // check will go away

The scion lab guys need to be able to configure this time, because they have some servers that have a lot of clock skew.

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

//TODO(karampok). discuss: who is using that?
func (v *verifier) Verify(ctx context.Context, msg common.RawBytes, sign *proto.SignS) error {

search for the usage of infra.Verifier.
this verifier should implement it https://godoc.org/github.com/scionproto/scion/go/lib/infra#Verifier
also it should adhere to the behavior defined by it.

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

	//Do we need anything else other than  IA?
	// remoteIA=ctrl.NewSignSrcDefFromRaw(sign.Src).IA
	f := func(s *ctrl.SignSrcDef, b common.RawBytes) (*ctrl.SignSrcDef, error) {

this is so much harder to read than:

src := v.src
if src == nil {
  src, err = ctrl.NewSignSrcDefFromRaw(..)
  if err != nil {
    return err
  }
}

or

var src ctrl.SignSrcDef
if v.SRC != nil {
  src = v.SRC
} else {
  src, err = ctrl.New....
}

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

		return err
	}
	v.SRC = t

no, don't modify the source of the verifier.
If the source is set, the verifier will always use that source to verify messages.
E.g. during beaconing, we want to bound the verifier to a specific source such that it will request exactly one certificate.

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

	v.SRC = t

	if !v.IA.Equal(v.SRC.IA) {

this should handle wildcards just like v1 trust store.
E.g. if IA is not set on the source, we should not enter this branch.

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

	}

	//opts := infra.ChainOpts{}

this needs to be uncommented

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

	//		TrustStoreOpts: infra.TrustStoreOpts{Server: v.server},
	//	}
	key, err := v.Store.GetASKey(ctx, v.SRC.IA.String(), nil)

this is missing the certificate version of the source.

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

	}

	return scrypto.VerifyED25519(key, msg, sign)

why would you hard-code this?

Reviewable status: 1 of 10 files reviewed, 17 unresolved discussions (waiting on @karampok)

go/lib/scrypto/asym.go, line 118 at r1 (raw file):

// VerifyED25519 reports whether sig is a valid signature of message by publicKey.
// It works only for Ed25519.
func VerifyED25519(publicKey, message []byte, sig *proto.SignS) error {

I don't see the need for this

Reviewable status: 1 of 10 files reviewed, 19 unresolved discussions (waiting on @karampok)

go/proto/sign.go, line 58 at r1 (raw file):

func (s *SignS) Valid(threshold time.Duration) error {
	if s == nil {
		return fmt.Errorf("SignS is unset")

use serrors.New

go/proto/sign.go, line 65 at r1 (raw file):

	age := time.Now().Sub(s.Time())
	if age < -threshold {

time.Now().Add(threshold).After(s.Time()) ?

Reviewable status: 1 of 10 files reviewed, 19 unresolved discussions (waiting on @karampok and @oncilla)

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

you wanted something like

 id chain.ID

i suppose this .ID will be addr.IA, version in a struct?
Does it exist or should I create it?

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

If the server is nil, do we actually allow Verify calls?

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

Can we list the unit-test cases? I do not get the bound source concept fully

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

apart from the Server value, what else can be part of this config, now or in the future?

Reviewable status: 1 of 10 files reviewed, 19 unresolved discussions (waiting on @karampok and @oncilla)

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

done

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

Done.

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

Done.

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

Done.

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

Can we somehow testdriven that part of the code? What confuses me is how the SignedPld can allow the code to skip validation?

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

mistake

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

Done.

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

Done.

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

Iho this is the minimum implementation, from high level usage this will just fail if not using ED25519, as now.
If I don't hardcode (either in the function name or as a static argument ), I will have to "force" more complicate interface
(key.Metada instead of []byte), something that we can currently avoid.

In the future also when we implement other algorithms, how we would do that is unclear.
For example I would like something that (no sure if it possible)

func verify(key,msg,sing) error{
  switch key.(type){
      case ED25519: return VerifyED25519(key,msg,sig)
      case RAS: return verifryRSA(key,msg,sig)
   }
}

My current approach will keep functionality and allow easier refactor in the future.

go/lib/scrypto/asym.go, line 118 at r1 (raw file):

above comment

Reviewable status: 1 of 10 files reviewed, 19 unresolved discussions (waiting on @karampok and @oncilla)

go/proto/sign.go, line 58 at r1 (raw file):

Done.

go/proto/sign.go, line 65 at r1 (raw file):

Done.

Reviewable status: 1 of 10 files reviewed, 19 unresolved discussions (waiting on @karampok and @oncilla)

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

refactored as discussed

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

refactored as discussed

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

Done.

Reviewable status: 1 of 8 files reviewed, 19 unresolved discussions (waiting on @oncilla)

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

Done.

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

Done.

Reviewed 1 of 10 files at r1, 2 of 6 files at r3.
Reviewable status: 4 of 8 files reviewed, 14 unresolved discussions (waiting on @karampok)

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

yes, The trust store will figure out where to send the request to.

In special cases, e.g. beaconing, we want to request from a specific server, because the trust store would resolve an incorrect location.

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

This is due to our weird message format.
When we refactor the structure of the messages, this part will be gone.

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

in this call location, or in general for GetASKey?

• this location: I don't see much that would need to be added. But future is always unknown.
• The GetASKey call in general: All options that can be set on infra.ChainOpts.

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

I agree that scrypto is a bit messy and we should refactor it to be more friendly to use.

go/lib/infra/modules/trust/v2/verifier.go, line 21 at r3 (raw file):

	AllowSkew time.Duration
	MaxAge    time.Duration
	BoundIA   addr.IA //TODO(karampok). discuss: can that be replaced with BoundSRC?

We can, but that means a lot more wildcarding in the source, which is additional complexity.
E.g. If you want to only bind the IA, but not the certificate/TRC version.

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

	MaxAge    time.Duration
	BoundIA   addr.IA //TODO(karampok). discuss: can that be replaced with BoundSRC?
	BoundSRC  *ctrl.SignSrcDef

SRC is not an initialism , should be BoundSrc

go/lib/infra/modules/trust/v2/verifier.go, line 83 at r3 (raw file):

	}

	if !v.BoundIA.IsZero() && !v.BoundIA.Equal(src.IA) {

this has not the same semantics as required by infra.Verifier interface.
But it is fine like this IMO, please adapt the infra.Verifier doc comment.

go/lib/infra/modules/trust/v2/verifier.go, line 88 at r3 (raw file):

	}

	if v.BoundSRC != nil && v.BoundSRC.String() != src.String() {

comparing the String() output is pretty dangerous.
Either implement an Equal function or rely on the == comparator

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

	if v.BoundSRC != nil && v.BoundSRC.String() != src.String() {
		//The entitiy that is the source of the RPC network request (BoundSRC)

comments should always start with a space

go/lib/infra/modules/trust/v2/verifier.go, line 91 at r3 (raw file):

		//The entitiy that is the source of the RPC network request (BoundSRC)
		//must be the same as the entitiy that signed the RPC (src).
		return serrors.New("SRc does not match bound SRC",

s/SRc/src

errors should not start with capital letters.

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

}

//TODO(karampok). discuss: given we have create setters for fields,

That does not work for the interface.
We want to pass around the interface, not the concrete implementation.

go/proto/sign.go, line 65 at r1 (raw file):

Oh, Should be time.Now().Add(threshold).Before(s.Time()), my bad

go/proto/sign.go, line 60 at r3 (raw file):

	}
	if len(s.Signature) == 0 {
		return fmt.Errorf("SignedPld is missing signature, %v", s.Type)

use our pattern serrors.New("missing signature", "type", s.Type)

go/proto/sign.go, line 63 at r3 (raw file):

	}
	if !time.Now().Add(threshold).After(s.Time()) {
		return serrors.New("Invalid timestamp, signature from future")

error messages should not start capital s/Invalid/invalid

Reviewable status: 1 of 10 files reviewed, 11 unresolved discussions (waiting on @oncilla)

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

Done.

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

Done.

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

Done.

go/lib/infra/modules/trust/v2/verifier.go, line 83 at r3 (raw file):

Done.

go/lib/infra/modules/trust/v2/verifier.go, line 88 at r3 (raw file):

Done.

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

Done.

go/lib/infra/modules/trust/v2/verifier.go, line 91 at r3 (raw file):

Done.

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

yes the discussion is more whether here it makes sense to have an interface.
I do not see why "we want to pass interface"

go/proto/sign.go, line 60 at r3 (raw file):

Done.

go/proto/sign.go, line 63 at r3 (raw file):

Done.

Reviewed 1 of 10 files at r1, 1 of 6 files at r3, 7 of 7 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

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

the same reason you want to have an interface for the CryptoProvider for example:

• easily mocking -> easy testing
• the usage site is in a package far from here, having it an interface allows modifying behavior.