-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TrustStore: Provider handles certificate chains #3496
Conversation
e96ded5
to
d13bcea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @oncilla)
go/lib/infra/modules/trust/v2/provider.go, line 184 at r1 (raw file):
return nil, serrors.WrapStr("unable to get requested certificate chain", err) } if !opts.AllowInactive {
hm I think the code would be cleaner if we would return early here if opts.AllowInactive
is true:
if opts.AllowInactive {
return chain.Raw, nil
}
err = p.issuerActive(ctx, chain, opts.TrustStoreOpts, client)
switch {
case err == nil:
return chain.Raw, nil
case opts.LocalOnly, !version.IsLatest(), !xerrors.Is(err, ErrInactive):
return nil, err
default:
// There might exist a more recent certificate chain that is not
// available locally yet.
fetched, err := p.fetchChain(ctx, ia, scrypto.LatestVer, opts, client)
if err != nil {
return nil, serrors.WrapStr("unable to fetch latest certificate chain from network",
err)
}
if err := p.issuerActive(ctx, fetched, opts.TrustStoreOpts, client); err != nil {
return nil, serrors.WrapStr("latest certificate chain from network not active", err)
}
return fetched.Raw, nil
}
go/lib/infra/modules/trust/v2/provider.go, line 188 at r1 (raw file):
switch { case err == nil: case opts.LocalOnly, !version.IsLatest(), !xerrors.Is(err, ErrInactive):
this is a bit hard to read. Maybe add a comment explaining the condition.
TBH for me it already helped to just split it up:
case opts.LocalOnly:
return nil, err
case !xerrors.Is(err, ErrInactive):
return nil, err
case !version.IsLatest():
return nil, err
default:
// There might exist a more
even though this is the same it kind of reads better IMO.
You can go through the cases and add to the knowledge about the state and in the end the explanation in the default case makes sense.
go/lib/infra/modules/trust/v2/provider.go, line 207 at r1 (raw file):
} func (p *cryptoProvider) issuerActive(ctx context.Context, chain decoded.Chain,
I think this should come after getChain
no strong opinion though.
go/lib/infra/modules/trust/v2/provider.go, line 229 at r1 (raw file):
return serrors.WrapStr("unable to get issuing key info for latest TRC", err) } if iss.Version != latest.Version {
I think we can also invert this.
go/lib/infra/modules/trust/v2/provider.go, line 230 at r1 (raw file):
latest.TRC.Base()
this can never be true IMO, if it would be Base it must beiss.Version == latest.Version
go/lib/infra/modules/trust/v2/provider.go, line 273 at r1 (raw file):
} // In case the server is provided, cache-only should be set. cacheOnly := server != nil || p.alwaysCacheOnly
Yeah let's already get rid off this here.
go/lib/infra/modules/trust/v2/provider.go, line 277 at r1 (raw file):
IA: ia, Version: version, CacheOnly: cacheOnly,
remove
go/lib/infra/modules/trust/v2/provider_test.go, line 472 at r1 (raw file):
ChainDesc: chain110v1, Opts: infra.ChainOpts{AllowInactive: true}, ExpectedRaw: func(t *testing.T) []byte { return loadChain(t, chain110v1).Raw },
Wouldn't it be easier to load this once and store it in a variable at the top and then have here a field instead of a function?
go/lib/infra/modules/trust/v2/provider_test.go, line 744 at r1 (raw file):
TRC: trust.TRCInfo{ Validity: info.Validity, GracePeriod: time.Hour,
Applies to all tests:
Shouldn't the GracePeriod == 0
for all TRCInfo
s where Version: 1
is set?
d13bcea
to
224ca35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 8 unresolved discussions (waiting on @lukedirtwalker and @oncilla)
go/lib/infra/modules/trust/v2/provider.go, line 188 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
this is a bit hard to read. Maybe add a comment explaining the condition.
TBH for me it already helped to just split it up:case opts.LocalOnly: return nil, err case !xerrors.Is(err, ErrInactive): return nil, err case !version.IsLatest(): return nil, err default: // There might exist a moreeven though this is the same it kind of reads better IMO.
You can go through the cases and add to the knowledge about the state and in the end the explanation in the default case makes sense.
Re-ordered to reflect the reasoning.
Done.
go/lib/infra/modules/trust/v2/provider.go, line 229 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
I think we can also invert this.
Done.
go/lib/infra/modules/trust/v2/provider.go, line 230 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
latest.TRC.Base()
this can never be true IMO, if it would be Base it must be
iss.Version == latest.Version
That is not true.
You could have iss.Version = 5 and then there is a trust reset at v6.
The latest.Version will be 6, base and != iss.Version.
go/lib/infra/modules/trust/v2/provider.go, line 273 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Yeah let's already get rid off this here.
Done.
go/lib/infra/modules/trust/v2/provider.go, line 277 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
remove
Done.
go/lib/infra/modules/trust/v2/provider_test.go, line 472 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Wouldn't it be easier to load this once and store it in a variable at the top and then have here a field instead of a function?
Done.
go/lib/infra/modules/trust/v2/provider_test.go, line 744 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Applies to all tests:
Shouldn't theGracePeriod == 0
for allTRCInfo
s whereVersion: 1
is set?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 8 unresolved discussions (waiting on @lukedirtwalker)
go/lib/infra/modules/trust/v2/provider.go, line 184 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
hm I think the code would be cleaner if we would return early here if
opts.AllowInactive
is true:if opts.AllowInactive { return chain.Raw, nil } err = p.issuerActive(ctx, chain, opts.TrustStoreOpts, client) switch { case err == nil: return chain.Raw, nil case opts.LocalOnly, !version.IsLatest(), !xerrors.Is(err, ErrInactive): return nil, err default: // There might exist a more recent certificate chain that is not // available locally yet. fetched, err := p.fetchChain(ctx, ia, scrypto.LatestVer, opts, client) if err != nil { return nil, serrors.WrapStr("unable to fetch latest certificate chain from network", err) } if err := p.issuerActive(ctx, fetched, opts.TrustStoreOpts, client); err != nil { return nil, serrors.WrapStr("latest certificate chain from network not active", err) } return fetched.Raw, nil }
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
go/lib/infra/modules/trust/v2/provider_test.go, line 744 at r1 (raw file):
Previously, Oncilla wrote…
Done.
Well here it is also the case? since we load v1 into dec
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved
go/lib/infra/modules/trust/v2/provider_test.go, line 744 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Well here it is also the case? since we load v1 into
dec
right?
🙈 so many indirections.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
This PR adds support for certificate chains to the CryptoProvider in the trust store. fixes scionproto#3470
49ad292
to
b5b7ffd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r5.
Reviewable status:complete! all files reviewed, all discussions resolved
This PR adds support for certificate chains to the CryptoProvider in the
trust store.
fixes #3470
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)