DRKey version 2 #106
DRKey version 2 #106
Conversation
There was a problem hiding this comment.
Reviewed 17 of 101 files at r1.
Reviewable status: 17 of 101 files reviewed, 25 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/cs/main.go, line 495 at r1 (raw file):
epochDuration = config.DefaultEpochDuration } log.Debug("DRKey debug info", "epoch duration", epochDuration.String())
I'm not sure I understand the reason behind this change here; why not just keep the EpochDuration as an option in the toml configuration?
In case this is to discourage tweaking the value for anything other than testing (but why?), perhaps an environment variable (SCION_TESTING_DRKEY_EPOCH_DURATION) would be more appropriate. Generally, we don't really like reading files with fixed name relative to the config dir, it's annoying to work with.
go/cs/main.go, line 543 at r1 (raw file):
Fetcher: drkeyFetcher, Cache: cache, }
Maybe add a NewServiceStore function such that internals (the cache in this case) can be hidden.
go/cs/config/drkey.go, line 40 at r1 (raw file):
type DRKeyConfig struct { // enabled is set to true if we find all the required fields in the configuration. enabled bool
Remove this, left-over.
go/cs/config/drkey.go, line 42 at r1 (raw file):
enabled bool // DRKeyDB contains the DRKey DB configuration. DRKeyLV1DB storage.DBConfig `toml:"drkey_lvl1_db,omitempty"`
Can we shorten the name to Lvl1DB and lvl1_db (it's already in a DRKeyConfig)?
go/cs/config/drkey.go, line 53 at r1 (raw file):
// NewDRKeyConfig returns a pointer to a valid, empty configuration. func NewDRKeyConfig() *DRKeyConfig { c := DRKeyConfig{
Unused (except in test, see comment there).
go/cs/config/drkey.go, line 67 at r1 (raw file):
cfg.CacheEntries = DefaultCacheEntries } config.InitAll(&cfg.Delegation)
Shouldn't this also initialize DRKeyLV1DB, DRKeySVDB ?
go/cs/config/drkey.go, line 126 at r1 (raw file):
func (cfg *SVHostList) Validate() error { for proto, list := range *cfg { if _, ok := drkey.ProtocolStringToId(strings.ToUpper(proto)); !ok {
|| !proto.IsSpecific()? I don't think we should allow generic here.
go/cs/config/drkey.go, line 149 at r1 (raw file):
// ToMapPerHost will return map where there is a set of supported protocols per host. func (cfg *SVHostList) ToMapPerHost() map[[16]byte]map[string]struct{} {
Use inet.af/netaddr.IP for a better IP address that can be used in a map, instead of having to muck around with net.IP. Furthermore, we only need a flat set of allowed (Host, Proto) tuples. Putting this together:
type HostProto struct {
Host netaddr.IP
Proto string
}
func (cfg *SVHostList) ToAllowedSet() map[HostProto]struct{} {
...
}go/cs/config/drkey_test.go, line 24 at r1 (raw file):
"testing" "github.com/BurntSushi/toml"
Please change this to go-toml (I must have missed this in a previous merge -- it currently only builds with bazel).
go/cs/config/drkey_test.go, line 63 at r1 (raw file):
err = cfg.Validate() require.NoError(t, err) assert.EqualValues(t, 100, cfg.CacheEntries)
It's very hard to understand what this test is actually trying to verify, especially because there seem to be left-over assertions in there now and some seemingly assorted assertions. Better would as a table-driven test:
func TestDisable(t *testing.T) {
cases := []struct {
name string
prepareCfg func(cfg *DRKeyConfig)
expectEnabled bool
}{
{
name: "default",
expectEnabled: false,
},
{
name: "with CacheEntries",
prepareCfg: func(cfg *DRKeyConfig) {
cfg.CacheEntries = 100
},
expectEnabled: false,
},
{
name: "with Lvl1DB",
prepareCfg: func(cfg *DRKeyConfig) {
cfg.DRKeyLV1DB.Connection = "test"
},
expectEnabled: true,
},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
cfg := &DRKeyConfig{}
cfg.InitDefaults()
if c.prepareCfg != nil {
c.prepareCfg(cfg)
}
require.NoError(t, cfg.Validate())
assert.Equal(t, c.expectEnabled, cfg.Enabled())
})
}
}Same also for some other tests in this file -- whenever multiple independent test cases are checked, the table-driven test pattern is cleaner.
go/lib/daemon/daemon.go, line 103 at r1 (raw file):
// DRKeyGetSV sends a DRKey SV request to SCIOND DRKeyGetSV(ctx context.Context, proto drkey.DRKeyProto, valTime time.Time) (drkey.SV, error)
I would not add this. The sciond should not be in the loop to transport a sensitive secret like this. Requesting an SV is a special operation that's only done by special applications. For these use cases, directly talking to the CS to fetch the CS seems perfectly appropriate (it's the same grpc based API anyway). In fact, it seems plausible that sciond is not running along side of applications using an SV (e.g. sciond is not needed for the router).
Consequently I would also remove the client cache for SVs. Applications using SV will need to carefully manage these SVs anyway for performance, so they will not benefit from an additional level of caches.
go/lib/drkey/level2.go, line 39 at r1 (raw file):
m.Epoch.Equal(other.Epoch) && m.SrcIA.Equal(other.SrcIA) && m.DstIA.Equal(other.DstIA) && strings.Compare(m.SrcHost, other.SrcHost) == 0 && strings.Compare(m.DstHost, other.DstHost) == 0
Just == (?)
In fact, the whole struct can be compared with ==, all types are comparable.
go/lib/drkey/secret_value.go, line 28 at r1 (raw file):
) const drkeySalt = "Derive DRKey Key" // same as in Python
Nit, no more python.
go/lib/drkey/secret_value.go, line 39 at r1 (raw file):
type SV struct { SVMeta Key DRKey
No good place to comment on this, because you haven't touched it; I'm assuming that DRKeys are of a fixed size; perhaps consider changing the type DRKey to [16]byte.
This would make this SV into a value type (which is == comparable and hashable).
Side note; rename DRKey to Key?
go/pkg/cs/drkey/service_store.go, line 46 at r1 (raw file):
// Lvl1CacheKey is the key type for the Lvl1Cache. type Lvl1CacheKey struct { Addr addr.IA
IA not Addr.
go/pkg/cs/drkey/service_store.go, line 72 at r1 (raw file):
} s.Cache.Update(cacheKey) }
My understanding is this; the Level1 keys are stored in a (sqlite) DB. In order to remember which keys to prefetch, we maintain this set of recently used keys.
If we already have this cache, shouldn't we at least use it to cache the keys (and so avoid the DB lookup)?
If not, we should avoid calling this thing cache, it's confusing, rather something like "prefetch set" (also for the configuration option).
go/pkg/cs/drkey/arc/lvl1ARC.go, line 15 at r1 (raw file):
// limitations under the License. package arc
Does not feel like it should be a separate package. Just put it under cs/drkey/arc.go, and put interface and key type definition in the same place.
go/pkg/cs/drkey/arc/lvl1ARC.go, line 50 at r1 (raw file):
} else { c.cache.Add(keyPair, keyPair) }
As mentioned in service_store.go, it'd be nicer if this cache would actually cache the DRKeys too.
Anyway, I think this function does unnecessary work, just c.cache.Add(keyPair, keyPair) should work,.
go/pkg/cs/drkey/arc/lvl1ARC.go, line 55 at r1 (raw file):
// GetCachedASes returns the list of AS currently in cache func (c *Lvl1ARC) GetCachedASes() []interface{} { return c.cache.Keys()
This should return []drkey.Lvl1CacheKey so callers know what to expect. The name is also not great, it's not returning not only the IA but also the protocol.
go/lib/drkey/meta.go, line 32 at r1 (raw file):
// Equal returns true if both Epochs are identical. func (e Epoch) Equal(other Epoch) bool {
Should also not be necessary (== is enough).
go/lib/drkey/meta.go, line 52 at r1 (raw file):
} // type DRKeyProto is the 2-byte size protocol identifier
As always, doc strings should be a full sentence, starting with the name described (DRKeyProto is [,,,].)
go/lib/drkey/meta.go, line 53 at r1 (raw file):
// type DRKeyProto is the 2-byte size protocol identifier type DRKeyProto uint16
Just Proto (or Protocol), then it's used as drkey.Proto.
go/lib/drkey/meta.go, line 57 at r1 (raw file):
// DRKey protocol types. const ( GENERIC = DRKeyProto(pb.Protocol_PROTOCOL_GENERIC_UNSPECIFIED)
Generic
go/lib/drkey/meta.go, line 71 at r1 (raw file):
func (p DRKeyProto) IsSpecific() bool { _, ok := pb.Protocol_name[int32(p)]
What should this function really do? It will true for GENERIC, which seems a bit unexpected.
go/lib/drkey/meta.go, line 75 at r1 (raw file):
} func (p DRKeyProto) Equal(proto DRKeyProto) bool {
Should not be needed, can be compared with ==.
There was a problem hiding this comment.
Reviewed 1 of 37 files at r2.
Reviewable status: 12 of 104 files reviewed, 30 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/lib/daemon/daemon.go, line 102 at r2 (raw file):
valTime time.Time) (drkey.SV, error) DRKeyGetASHostKey(ctx context.Context, meta drkey.ASHostMeta, valTime time.Time) (drkey.ASHostKey, error)
The meta contains NotBefore and NotAfter field that is ignored here and the valTime needs to be passed separately. This seems a bit awkward. Wouldn't it be clearer to just pass all the parameters individually (and maybe get rid of the "Meta" types)?
DRKeyGetASHostKey(ctx context.Context, proto drkey.Protocol, srcIA, dstIA addr.IA, dstHost string, valTime time.Time) (drkey.ASHostKey, error)
and analogously for the similarly named functions, and the DB interface etc.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 34 at r2 (raw file):
// Lvl2Schema is the SQLite database layout. Lvl2Schema = ` CREATE TABLE ASHost (
What is the motivation to split this up into multiple tables? Instead of one table with optional src/dst IP fields, there are now three tables with defined, mandatory IP fields -- does this really change anything?
I'd keep the single table for simplicity; You can still keep the the multiple entry points so that the type distinction can remain throughout the rest of the codebase. Internally, GetHostASKey, GetHostHostKey, GetASHostKey would only need to check that the input has the correct IP fields set and then invoke the same query (and same for the Insert ... functions).
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 40 at r2 (raw file):
DstIsdID INTEGER NOT NULL, DstAsID INTEGER NOT NULL, DstHostIP TEXT,
Nit, mixed tabs/spaces
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 111 at r2 (raw file):
if err := base.prepareAll(stmts); err != nil { return nil, err }
Do you expect that these prepared statements significantly affect the performance (given that every request already involves at least one RPC)? None of the other systems use these, and there are some issues in prepareAll. For the sake of simplicity, I'd just drop these prepared statements.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 125 at r2 (raw file):
// at which it should be valid and returns the corresponding key. func (b *Lvl2Backend) GetASHostKey(ctx context.Context, meta drkey.ASHostMeta, valTime uint32) (*drkey.ASHostKey, error) {
Return small structs as value, not pointer. Just return an error if not found.
There was a problem hiding this comment.
Reviewable status: 5 of 107 files reviewed, 29 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/cs/main.go, line 495 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
I'm not sure I understand the reason behind this change here; why not just keep the EpochDuration as an option in the toml configuration?
In case this is to discourage tweaking the value for anything other than testing (but why?), perhaps an environment variable (SCION_TESTING_DRKEY_EPOCH_DURATION) would be more appropriate. Generally, we don't really like reading files with fixed name relative to the config dir, it's annoying to work with.
I changed it to an environment variable. It can be configure e.g. in supervisor/supervisor.conf (environment= SCION_TESTING_DRKEY_EPOCH_DURATION="1m"). The reason aside from this only being used in testing environments is that it does not require to change the config in every CS toml file. Otherwise we would have inconsistency because we are assuming global epoch duration at the moment.
go/cs/main.go, line 543 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Maybe add a
NewServiceStorefunction such that internals (thecachein this case) can be hidden.
Done.
go/cs/config/drkey.go, line 40 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Remove this, left-over.
Done.
go/cs/config/drkey.go, line 42 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Can we shorten the name to
Lvl1DBandlvl1_db(it's already in a DRKeyConfig)?
Done.
go/cs/config/drkey.go, line 53 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Unused (except in test, see comment there).
Done.
go/cs/config/drkey.go, line 67 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Shouldn't this also initialize
DRKeyLV1DB,DRKeySVDB?
At the moment the Enabled() function checks that Lvl1DB.Connection == "", if we init it to something else it would always enable drkey support in the CS. I'm not completely happy about this, it might be a better way to indicate that DRKey is enable or not in the config file.
go/cs/config/drkey.go, line 126 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
|| !proto.IsSpecific()? I don't think we should allow generic here.
Indeed, that's not the use case for the generic protocol.
go/cs/config/drkey.go, line 149 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Use
inet.af/netaddr.IPfor a better IP address that can be used in a map, instead of having to muck around withnet.IP. Furthermore, we only need a flat set of allowed (Host, Proto) tuples. Putting this together:type HostProto struct { Host netaddr.IP Proto string } func (cfg *SVHostList) ToAllowedSet() map[HostProto]struct{} { ... }
Done.
go/cs/config/drkey_test.go, line 24 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Please change this to go-toml (I must have missed this in a previous merge -- it currently only builds with bazel).
Done.
go/cs/config/drkey_test.go, line 63 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
It's very hard to understand what this test is actually trying to verify, especially because there seem to be left-over assertions in there now and some seemingly assorted assertions. Better would as a table-driven test:
func TestDisable(t *testing.T) { cases := []struct { name string prepareCfg func(cfg *DRKeyConfig) expectEnabled bool }{ { name: "default", expectEnabled: false, }, { name: "with CacheEntries", prepareCfg: func(cfg *DRKeyConfig) { cfg.CacheEntries = 100 }, expectEnabled: false, }, { name: "with Lvl1DB", prepareCfg: func(cfg *DRKeyConfig) { cfg.DRKeyLV1DB.Connection = "test" }, expectEnabled: true, }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { cfg := &DRKeyConfig{} cfg.InitDefaults() if c.prepareCfg != nil { c.prepareCfg(cfg) } require.NoError(t, cfg.Validate()) assert.Equal(t, c.expectEnabled, cfg.Enabled()) }) } }Same also for some other tests in this file -- whenever multiple independent test cases are checked, the table-driven test pattern is cleaner.
Done.
go/lib/daemon/daemon.go, line 103 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
I would not add this. The sciond should not be in the loop to transport a sensitive secret like this. Requesting an SV is a special operation that's only done by special applications. For these use cases, directly talking to the CS to fetch the CS seems perfectly appropriate (it's the same grpc based API anyway). In fact, it seems plausible that sciond is not running along side of applications using an SV (e.g. sciond is not needed for the router).
Consequently I would also remove the client cache for SVs. Applications using SV will need to carefully manage these SVs anyway for performance, so they will not benefit from an additional level of caches.
I agree that those special applications could contact directly the CS but I'm not completely sure that those applications cannot/must not use the scion daemon. e.g. the current LF implementation is using the daemon for fetching the DS (this pseudo-sv we had before), so it might become handy that the daemon exposes this feature as part of the API.
I completely agree, the cache should be carafully done by the application. The SV caching in the daemon is only an intermediate cache, similar to the Lvl2 cache. At the end both cache aren't completely necessary, they only avoid an extra call to the CS in case the SV or the Lvl2key have been requested previously.
go/lib/drkey/level2.go, line 39 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
==(?)
In fact, the whole struct can be compared with==, all types are comparable.
Done.
go/lib/drkey/secret_value.go, line 39 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
No good place to comment on this, because you haven't touched it; I'm assuming that DRKeys are of a fixed size; perhaps consider changing the type
DRKeyto[16]byte.
This would make thisSVinto a value type (which is==comparable and hashable).Side note; rename
DRKeytoKey?
I agree. I have changed Key to an array.
go/pkg/cs/drkey/service_store.go, line 46 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
IAnotAddr.
Done.
go/pkg/cs/drkey/service_store.go, line 72 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
My understanding is this; the Level1 keys are stored in a (sqlite) DB. In order to remember which keys to prefetch, we maintain this set of recently used keys.
If we already have this cache, shouldn't we at least use it to cache the keys (and so avoid the DB lookup)?
If not, we should avoid calling this thing cache, it's confusing, rather something like "prefetch set" (also for the configuration option).
Agree the naming was rather unfortunate.
As you said, the idea is maintaining a list of the LRU (AS, proto) tuples, so that we also limit the amount of prefetching calls to remote ASes. I do not intend to store actual keys because it would complicate key management. We might have different Lvl1 Keys with the same (AS, ProtoId) (e.g. prefetched key, grace period, past keys?). Since this prefetching happen in the slow side, it is not completely critical to do this lookup in exchange of keeping this list simple.
go/lib/drkey/meta.go, line 32 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Should also not be necessary (
==is enough).
Done.
go/lib/drkey/meta.go, line 52 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
As always, doc strings should be a full sentence, starting with the name described (
DRKeyProto is [,,,].)
Done.
go/lib/drkey/meta.go, line 53 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
Proto(orProtocol), then it's used asdrkey.Proto.
Done.
go/lib/drkey/meta.go, line 57 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Generic
Done.
go/lib/drkey/meta.go, line 71 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
What should this function really do? It will true for
GENERIC, which seems a bit unexpected.
IMO, Generic is a special case of specific protocol that allows this "niche" protocols. This function is used to check whether the Lvl1Request is a valid protocol (Generic and the rest of specific protocols). If you think that this is rather counter-intuitive I can change it, no problem.
go/lib/drkey/meta.go, line 75 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Should not be needed, can be compared with
==.
Done.
go/pkg/cs/drkey/arc/lvl1ARC.go, line 15 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Does not feel like it should be a separate package. Just put it under
cs/drkey/arc.go, and put interface and key type definition in the same place.
Done.
go/pkg/cs/drkey/arc/lvl1ARC.go, line 50 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
As mentioned in service_store.go, it'd be nicer if this cache would actually cache the DRKeys too.
Anyway, I think this function does unnecessary work, justc.cache.Add(keyPair, keyPair)should work,.
Done.
go/pkg/cs/drkey/arc/lvl1ARC.go, line 55 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
This should return
[]drkey.Lvl1CacheKeyso callers know what to expect. The name is also not great, it's not returning not only theIAbut also the protocol.
Done.
There was a problem hiding this comment.
Reviewed 6 of 101 files at r1, 1 of 37 files at r2, 15 of 54 files at r3.
Reviewable status: 27 of 107 files reviewed, 27 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
a discussion (no related file):
The organisation of the "DB" (Lvl1DB, Lvl2DB, SecretValueDB in go/lib/drkey) and "Store" interfaces (SecretValueStore, ServiceStore, ClientStore in go/lib/drkeystorage) as well as the implementations of these interfaces feels complicated. The code is hard to navigate here.
- I'm not sure "Store" is a good name for this abstraction.
- the "Store" interfaces do not need to be defined in a shared library
go/lib/drkeystorage; instead, these interfaces can be defined ad-hoc in the places where they are referenced (go/pkg/cs/drkey/grpcfor "ServiceStore" andgo/pkg/daemon/internal/servers/grpcfor "ClientStore").
We create mocks for these interfaces, but these mocks are not used. - The "ClientStore" interface can be replaced by / unified with the
go/pkg/daemon/drkey.Fetcherinterface
go/cs/main.go, line 495 at r1 (raw file):
Previously, JordiSubira wrote…
I changed it to an environment variable. It can be configure e.g. in supervisor/supervisor.conf (environment= SCION_TESTING_DRKEY_EPOCH_DURATION="1m"). The reason aside from this only being used in testing environments is that it does not require to change the config in every CS toml file. Otherwise we would have inconsistency because we are assuming global epoch duration at the moment.
👍
go/cs/main.go, line 790 at r3 (raw file):
return config.DefaultEpochDuration } err := duration.Set(s)
Just use util.ParseDuration here instead -- the DurWrap is only needed for use in commandline flags, json marshalling etc.
go/cs/main.go, line 793 at r3 (raw file):
if err != nil { log.Error("unmarshalling value", "err", err) return config.DefaultEpochDuration
I'd abort hard here (i.e. return an error from main).
go/cs/config/drkey.go, line 67 at r1 (raw file):
Previously, JordiSubira wrote…
At the moment the Enabled() function checks that Lvl1DB.Connection == "", if we init it to something else it would always enable drkey support in the CS. I'm not completely happy about this, it might be a better way to indicate that DRKey is enable or not in the config file.
We could use WithDefault("") to use list the configuration in InitAll but still keep DRKey is disabled by default. This would make this more explicit and clarify to a reader that this was not omitted by mistake.
config.InitAll(
cfg.Lvl1DB.WithDefault(""),
cfg,SVDB.WithDefault(""),
&cfg.Delegation,
)
go/lib/ctrl/drkey/lvl1_req.go, line 80 at r3 (raw file):
}, } copy(returningKey.Key[:], rep.Key)
Maybe check the key size? If the size is not 16, something will probably not work, so perhaps rather error immediately?
go/lib/daemon/daemon.go, line 103 at r1 (raw file):
Previously, JordiSubira wrote…
I agree that those special applications could contact directly the CS but I'm not completely sure that those applications cannot/must not use the scion daemon. e.g. the current LF implementation is using the daemon for fetching the DS (this pseudo-sv we had before), so it might become handy that the daemon exposes this feature as part of the API.
I completely agree, the cache should be carafully done by the application. The SV caching in the daemon is only an intermediate cache, similar to the Lvl2 cache. At the end both cache aren't completely necessary, they only avoid an extra call to the CS in case the SV or the Lvl2key have been requested previously.
Supporting this in the daemon adds complexity (more functions to the API, a separate cache DB) without clear benefits. The functionality will only be used rarely, by special hosts / application. Caching the secret does not help significantly as these applications will perform very few requests, on the order of one request per epoch (hours). Finally, the secret value is much more sensitive than Lvl2 keys (as it allows to derive the corresponding keys for any host, not only for the local host) and therefore caching them to disk appears to be a (somewhat unnecessary) security concern. (Note: the situation is a bit different for caching the Lvl1 keys in the CS, as this really appears to be necessary and the CS is needs to manage sensitive material anyway).
I see mostly downsides to this, and very few upsides. I'd really leave this away -- adding it later is easier than removing it.
go/lib/drkey/drkey.go, line 18 at r3 (raw file):
// DRKey represents a raw binary key type DRKey [16]byte
RenameKey (drkey.Key, instead of drkey.DRKey)?
(Previously mentioned this as a side note in previous, now resolved comment. Saying this this only to hopefully avoid disorientation)
go/lib/drkey/drkey.go, line 22 at r3 (raw file):
func (k DRKey) String() string { return "[redacted key]" }
Many small type definitions in this package are currently spread through various small files, making this harder to navigate than necessary, imho.
Could we move the following files into drkey.go (this file): level1.go, level2.go, meta.go and secret_value.go
go/lib/drkey/protocol/protocol.go, line 15 at r3 (raw file):
// limitations under the License. package protocol
The key derivations are an integral part of drkey. I don't think there is a good reason to split this off into a separate sub-package. Integrate this into go/lib/drkey.
Then, rename the protocol.Generic and protocol.Specific to drkey.GenericDeriver and drkey.SpecificDeriver (to avoid clashing with the identifiers for the drkey.Generic protocol identifier)
go/lib/drkey/protocol/protocol.go, line 39 at r3 (raw file):
AS_TO_HOST HOST_TO_AS HOST_TO_HOST
private, camelCase, not SCREAMING_SNAKE_CASE
go/lib/drkey/protocol/protocol.go, line 47 at r3 (raw file):
// HostAddr is the address representation of a host as defined in the SCION header. type HostAddr struct {
private
go/lib/drkey/protocol/protocol.go, line 54 at r3 (raw file):
// PacktoHostAddr returns a HostAddr parsing a given address in string format. func PacktoHostAddr(hostAddr string) (HostAddr, error) {
private
go/lib/drkey/protocol/protocol.go, line 139 at r3 (raw file):
// If the input buffer is reused in the future, the returned key buffer // will be overwritten. func DeriveKey(input []byte, inputLen int, key drkey.DRKey) ([]byte, error) {
Shouldn't this return a drkey.DRKey?
Btw. this would also get rid of the buffer reusal issue -- optimistically assuming that the compiler will do the escape analysis correctly, this does not require any heap allocations for the return value.
btw. as commented elsewhere, I think a nicer user-interface would be to make this function private and replace the InputDeriveXY functions with DeriveXY, directly.
go/lib/drkey/protocol/protocol.go, line 156 at r3 (raw file):
} func CBCMac(block cipher.BlockMode, b []byte) []byte {
private
go/lib/drkey/protocol/specific.go, line 29 at r3 (raw file):
// InputDeriveLvl1 populates the provided buffer with // the input for a lvl1 derivation, returning its length. func (p Specific) InputDeriveLvl1(buf []byte, meta drkey.Lvl1Meta) int {
All these InputDeriveXY functions could be replaced with simpler to use DeriveXY. The return type of this should be drkey.DRKey (or perhaps even the "typed" keys including the input metadata, drkey.Lvl1 etc).
If a buffer should be re-used, this can live in the reusable deriver object (currently called Specific / Generic now; as suggested elsewhere these could be moved and renamed to drkey.SpecificDeriver, drkey.GenericDeriver).
go/lib/drkey/protocol/specific.go, line 39 at r3 (raw file):
// Calculate a multiple of 16 such that the input fits in nrBlocks := int(math.Ceil((2 + float64(l)) / 16))
No need for floats; canonical ceiling integer division (dividend + divisor - 1) / divisor
go/lib/drkeystorage/BUILD.bazel, line 1 at r3 (raw file):
load("//lint:go.bzl", "go_library")
Why is this package not under go/lib/drkey?
Also, naming inconsistency, package is called storage as the interfaces in here are "Store" (see also related comment on the naming/organisation of these interfaces.
go/pkg/cs/drkey/grpc/drkey_fetcher.go, line 74 at r3 (raw file):
// DRKeyFetcher obtains Lvl1 DRKey from a remote CS. type DRKeyFetcher struct {
Just Fetcher?
Also, rename file to fetcher.go.
go/pkg/cs/drkey/grpc/drkey_fetcher.go, line 75 at r3 (raw file):
// DRKeyFetcher obtains Lvl1 DRKey from a remote CS. type DRKeyFetcher struct { Getter Lvl1KeyGetter
The implementation of Lvl1KeyFetcher / Lvl1KeyGetter can just be "subsumed" here into DRKeyFetcher, making this a bunch simpler.
go/pkg/cs/drkey/grpc/drkey_service.go, line 40 at r3 (raw file):
// DRKeyServer keeps track of the level 1 drkey keys. It is backed by a drkey.DB . type DRKeyServer struct {
Just Server?
go/pkg/cs/drkey/grpc/drkey_service.go, line 303 at r3 (raw file):
} meta := parsedReq.ToMeta() asHostKey, err := deriveASHost(meta, lvl1Key)
(somewhat arbitrary place for this comment)
Currently, the derivation for drkeys is implemented in two parts here; for the Lvl1 its in pkg/cs/drkey/service_store.go, and then the level 2 keys are derived here, in the grpc specific code.
This makes it hard to navigate the code and the responsibilities of the different pieces of code are mushed.
I'd reorganize this such that the "ServiceStore" is responsible for all the "business logic" of deriving keys etc, keeping only the interface stubs here in the grpc server code.
This means that the ServiceStore interface needs to be extended with the required methods to derive Lvl2 keys.
(see also top comment on the organisation of these interfaces)
go/lib/drkey/meta.go, line 71 at r1 (raw file):
Previously, JordiSubira wrote…
IMO, Generic is a special case of specific protocol that allows this "niche" protocols. This function is used to check whether the Lvl1Request is a valid protocol (Generic and the rest of specific protocols). If you think that this is rather counter-intuitive I can change it, no problem.
Ok. Maybe rename to IsValid?
There was a problem hiding this comment.
Reviewable status: 6 of 114 files reviewed, 26 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
a discussion (no related file):
Previously, matzf (Matthias Frei) wrote…
The organisation of the "DB" (
Lvl1DB,Lvl2DB,SecretValueDBingo/lib/drkey) and "Store" interfaces (SecretValueStore,ServiceStore,ClientStoreingo/lib/drkeystorage) as well as the implementations of these interfaces feels complicated. The code is hard to navigate here.
- I'm not sure "Store" is a good name for this abstraction.
- the "Store" interfaces do not need to be defined in a shared library
go/lib/drkeystorage; instead, these interfaces can be defined ad-hoc in the places where they are referenced (go/pkg/cs/drkey/grpcfor "ServiceStore" andgo/pkg/daemon/internal/servers/grpcfor "ClientStore").
We create mocks for these interfaces, but these mocks are not used.- The "ClientStore" interface can be replaced by / unified with the
go/pkg/daemon/drkey.Fetcherinterface
Done. I rename the interfaces/implementations, but feel free to suggest new ones. I decided to maintain the ClientEngine instead of merging it with the fetcher (similar to the ServiceEngine and its own fetcher). I reckon it makes sense to have this fetcher interface.
go/cs/main.go, line 790 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just use
util.ParseDurationhere instead -- theDurWrapis only needed for use in commandline flags, json marshalling etc.
Done.
go/cs/main.go, line 793 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
I'd abort hard here (i.e. return an error from main).
Done.
go/cs/config/drkey.go, line 67 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
We could use
WithDefault("")to use list the configuration inInitAllbut still keep DRKey is disabled by default. This would make this more explicit and clarify to a reader that this was not omitted by mistake.config.InitAll( cfg.Lvl1DB.WithDefault(""), cfg,SVDB.WithDefault(""), &cfg.Delegation, )
Done.
go/lib/ctrl/drkey/lvl1_req.go, line 80 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Maybe check the key size? If the size is not 16, something will probably not work, so perhaps rather error immediately?
Done.
go/lib/daemon/daemon.go, line 103 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Supporting this in the daemon adds complexity (more functions to the API, a separate cache DB) without clear benefits. The functionality will only be used rarely, by special hosts / application. Caching the secret does not help significantly as these applications will perform very few requests, on the order of one request per epoch (hours). Finally, the secret value is much more sensitive than Lvl2 keys (as it allows to derive the corresponding keys for any host, not only for the local host) and therefore caching them to disk appears to be a (somewhat unnecessary) security concern. (Note: the situation is a bit different for caching the Lvl1 keys in the CS, as this really appears to be necessary and the CS is needs to manage sensitive material anyway).
I see mostly downsides to this, and very few upsides. I'd really leave this away -- adding it later is easier than removing it.
Removed.
go/lib/daemon/daemon.go, line 102 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
The
metacontainsNotBeforeandNotAfterfield that is ignored here and thevalTimeneeds to be passed separately. This seems a bit awkward. Wouldn't it be clearer to just pass all the parameters individually (and maybe get rid of the "Meta" types)?
DRKeyGetASHostKey(ctx context.Context, proto drkey.Protocol, srcIA, dstIA addr.IA, dstHost string, valTime time.Time) (drkey.ASHostKey, error)
and analogously for the similarly named functions, and the DB interface etc.
Epoch is populated by the response, not in the request. I agree this is not very intuitive. However, having meta would avoid having to change the signature (also in many other functions) if the metadata associated to the key changes in the future. If that is not much of a benefit, I agree with passing individual parameters. An alternative to individual parameters would be, MetaReq and Meta (for the information after the response).
go/lib/drkey/drkey.go, line 18 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Rename
Key(drkey.Key, instead ofdrkey.DRKey)?(Previously mentioned this as a side note in previous, now resolved comment. Saying this this only to hopefully avoid disorientation)
Done.
go/lib/drkey/drkey.go, line 22 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Many small type definitions in this package are currently spread through various small files, making this harder to navigate than necessary, imho.
Could we move the following files intodrkey.go(this file):level1.go,level2.go,meta.goandsecret_value.go
Done.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 34 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
What is the motivation to split this up into multiple tables? Instead of one table with optional src/dst IP fields, there are now three tables with defined, mandatory IP fields -- does this really change anything?
I'd keep the single table for simplicity; You can still keep the the multiple entry points so that the type distinction can remain throughout the rest of the codebase. Internally,GetHostASKey,GetHostHostKey,GetASHostKeywould only need to check that the input has the correct IP fields set and then invoke the same query (and same for the Insert ... functions).
I decided to split into different tables to be consistent with the decision of splitting the end-host keys being independent in the pb messages. The reason was allowing them to grow independently, what I guess it means that new fields might be added for a specific one. Therefore, it didn't seem quite right to have the union of different fields in one database with optional values.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 111 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
Do you expect that these prepared statements significantly affect the performance (given that every request already involves at least one RPC)? None of the other systems use these, and there are some issues in
prepareAll. For the sake of simplicity, I'd just drop these prepared statements.
Done. I just realized that I forgot to do the same in the lvl1db, I will do it in the next pass.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 125 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
Return small structs as value, not pointer. Just return an error if not found.
Done.
go/lib/drkeystorage/BUILD.bazel, line 1 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Why is this package not under
go/lib/drkey?
Also, naming inconsistency, package is calledstorageas the interfaces in here are "Store" (see also related comment on the naming/organisation of these interfaces.
Removed
go/pkg/cs/drkey/grpc/drkey_fetcher.go, line 74 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
Fetcher?
Also, rename file tofetcher.go.
Done.
go/pkg/cs/drkey/grpc/drkey_fetcher.go, line 75 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
The implementation of
Lvl1KeyFetcher/Lvl1KeyGettercan just be "subsumed" here intoDRKeyFetcher, making this a bunch simpler.
Done.
go/pkg/cs/drkey/grpc/drkey_service.go, line 40 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
Server?
Done.
go/pkg/cs/drkey/grpc/drkey_service.go, line 303 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
(somewhat arbitrary place for this comment)
Currently, the derivation for drkeys is implemented in two parts here; for the Lvl1 its in
pkg/cs/drkey/service_store.go, and then the level 2 keys are derived here, in the grpc specific code.
This makes it hard to navigate the code and the responsibilities of the different pieces of code are mushed.I'd reorganize this such that the "ServiceStore" is responsible for all the "business logic" of deriving keys etc, keeping only the interface stubs here in the grpc server code.
This means that theServiceStoreinterface needs to be extended with the required methods to derive Lvl2 keys.(see also top comment on the organisation of these interfaces)
Done. I agree with keeping the business logic in the now called "ServiceEngine"
go/lib/drkey/meta.go, line 71 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ok. Maybe rename to
IsValid?
It is also used in some other places to check whether the lvl2 derivation must be done using the GenericDeriver or the SpecificDeriver. I can replace it with its negation, i.e. IsGeneric() and refactor accordingly if that helps with the semantics.
go/lib/drkey/protocol/protocol.go, line 15 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
The key derivations are an integral part of drkey. I don't think there is a good reason to split this off into a separate sub-package. Integrate this into
go/lib/drkey.Then, rename the
protocol.Genericandprotocol.Specifictodrkey.GenericDeriveranddrkey.SpecificDeriver(to avoid clashing with the identifiers for thedrkey.Genericprotocol identifier)
Done.
go/lib/drkey/protocol/protocol.go, line 39 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
private,
camelCase, notSCREAMING_SNAKE_CASE
Done.
go/lib/drkey/protocol/protocol.go, line 47 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
private
Done.
go/lib/drkey/protocol/protocol.go, line 54 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
private
Done.
go/lib/drkey/protocol/protocol.go, line 139 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Shouldn't this return a
drkey.DRKey?
Btw. this would also get rid of the buffer reusal issue -- optimistically assuming that the compiler will do the escape analysis correctly, this does not require any heap allocations for the return value.btw. as commented elsewhere, I think a nicer user-interface would be to make this function private and replace the
InputDeriveXYfunctions withDeriveXY, directly.
In order to return the drkey.Key I have allocated the buffer array (not in the heap but in the program stack I guess). I think Go treats arrays as value, so I guess the returned array is allocated/copied in the caller as well.Do you think this is okay or we should spare even this passing a pointer to a destination buffer?
go/lib/drkey/protocol/protocol.go, line 156 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
private
Done.
go/lib/drkey/protocol/specific.go, line 29 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
All these
InputDeriveXYfunctions could be replaced with simpler to useDeriveXY. The return type of this should bedrkey.DRKey(or perhaps even the "typed" keys including the input metadata,drkey.Lvl1etc).
If a buffer should be re-used, this can live in the reusable deriver object (currently calledSpecific/Genericnow; as suggested elsewhere these could be moved and renamed todrkey.SpecificDeriver,drkey.GenericDeriver).
At the moment, I stick to returning the drkey.Key. When we make a final decision about the parameter passing and returning types (somewhere else in the discussion) I can change it to return the "type" key if needed.
go/lib/drkey/protocol/specific.go, line 39 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
No need for floats; canonical ceiling integer division
(dividend + divisor - 1) / divisor
Done.
There was a problem hiding this comment.
Reviewed 5 of 101 files at r1, 1 of 54 files at r3, 23 of 71 files at r4.
Reviewable status: 35 of 114 files reviewed, 33 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
a discussion (no related file):
Previously, JordiSubira wrote…
Done. I rename the interfaces/implementations, but feel free to suggest new ones. I decided to maintain the ClientEngine instead of merging it with the fetcher (similar to the ServiceEngine and its own fetcher). I reckon it makes sense to have this fetcher interface.
Great! I think this helps a lot! Engine seems like a good choice for a name. I've made some further comments in the ServiceEngine, but this looks great now.
go/cs/main.go, line 792 at r4 (raw file):
if err != nil { log.Error("parsing value", "err", err) return 0, err
Don't log the error, it's returned and will be logged higher up.
Perhaps wrap the error though, return 0, serrors.WrapStr("parsing SCION_TESTING_DRKEY_EPOCH_DURATION", err).
go/cs/config/drkey.go, line 148 at r4 (raw file):
type HostProto struct { Host netaddr.IP Proto string
Store as drkey.Protocol, to avoid having to converting the protocol id to a string just to check membership in this set here.
go/lib/daemon/daemon.go, line 102 at r2 (raw file):
Previously, JordiSubira wrote…
Epoch is populated by the response, not in the request. I agree this is not very intuitive. However, having meta would avoid having to change the signature (also in many other functions) if the metadata associated to the key changes in the future. If that is not much of a benefit, I agree with passing individual parameters. An alternative to individual parameters would be, MetaReq and Meta (for the information after the response).
Gotcha. The MetaReq vs Meta sounds good in principle, albeit a bit complicated. What about making Meta just for the requests, and having the key types contain the metadata directly? So that would be, e.g.
type Lvl1Meta struct {
Validty time.Time
ProtocolId Protocol
SrcIA, DstIA addr.IA
}
type Lvl1Key struct {
Epoch Epoch // \
ProtocolId Protocol // > listed individually instead of including Lvl1Meta
SrcIA, DstIA addr.IA // /
Key Key
}
Maybe we could then add some helper functions to create the Lvl1Key from the Lvl1Meta + key + epoch, to avoid typing out the full struct initialization everywhere.
Just an idea, I'm not sure how much this helps and how much work it is to revisit these types. Maybe treat this with low priority :)
go/lib/drkey/db.go, line 28 at r4 (raw file):
io.Closer db.LimitSetter }
Remove this BaseDB interface, it's just clutter. Add the two interfaces to all the DB interfaces below explicitly.
go/lib/drkey/db.go, line 33 at r4 (raw file):
type SecretValueDB interface { BaseDB GetSV(ctx context.Context, proto Protocol, valTime uint32) (*SV, error)
Couldn't we pass all these valTime as time.Time and only internally convert to timestamps internally?
go/lib/drkey/db.go, line 35 at r4 (raw file):
GetSV(ctx context.Context, proto Protocol, valTime uint32) (*SV, error) InsertSV(ctx context.Context, key SV) error RemoveOutdatedSV(ctx context.Context, cutoff uint32) (int64, error)
Lets call these DeleteExpired for consistency (here and in the two other interfaces in this file).
Should also take a time.Time.
go/lib/drkey/drkeydbsqlite/lvl1db.go, line 15 at r4 (raw file):
// limitations under the License. package drkeydbsqlite
This package can be called just sqlite, it's already under drkey.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 111 at r2 (raw file):
Previously, JordiSubira wrote…
Done. I just realized that I forgot to do the same in the lvl1db, I will do it in the next pass.
Ok. Please also remove the prepareAll helper function and the corresponding const/var definitions when you get to it.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 121 at r4 (raw file):
return drkey.ASHostKey{}, db.NewReadError("getting ASHost key", err) } return drkey.ASHostKey{}, nil
This should return an error. The caller currently needs to check whether a zero key was returned, which is ugly because this technically a valid key. We can define a specific error for this, var ErrKeyNotFound = serrors.New("Key not found").
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 15 at r4 (raw file):
// limitations under the License. package drkeydbtest
Just dbtest, it's already under drkey.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 37 at r4 (raw file):
var ( asMasterPassword = []byte("0123456789012345")
Unused after using protocoltest.GetLvl1 to simplify initialization.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 39 at r4 (raw file):
asMasterPassword = []byte("0123456789012345") rawSrcIA = []byte{0xF0, 0x11, 0xF2, 0x33, 0x44, 0x55, 0x66, 0x77} rawDstIA = []byte{0xF0, 0x11, 0xF2, 0x33, 0x44, 0x55, 0x66, 0x88}
Use xtest.MustParseIA, like in drkey/protocol_test.go.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 88 at r4 (raw file):
} drkeyLvl1 := protocoltest.DeriveLvl1(t, lvl1Meta, sv)
Simplify initialization by using protocoltest.GetLvl1, remove the DeriveLvl1 helper function.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 113 at r4 (raw file):
} func ia(iaStr string) addr.IA {
Remove
go/lib/drkey/protocoltest/commons.go, line 31 at r4 (raw file):
DstIA: lvl1Meta.DstIA, ProtoId: sv.ProtoId, },
Initialize lvl1Meta with Epoch and ProtoId too, then we can just write Lvl1Meta: lvl1Meta here.
go/lib/drkey/protocoltest/commons.go, line 32 at r4 (raw file):
ProtoId: sv.ProtoId, }, Key: outKey,
Arrays are value types, they are copied by assignment without explicit copy call.
So here, we can just write Key: key and drop the var outKey.
Same applies for various functions below.
go/pkg/cs/drkey/service_engine.go, line 31 at r4 (raw file):
// Fetcher obtains a Lvl1 DRKey from a remote CS. type Fetcher interface { GetLvl1FromOtherCS(ctx context.Context, meta drkey.Lvl1Meta,
Just FetchLvl1?
go/pkg/cs/drkey/service_engine.go, line 40 at r4 (raw file):
// handed out secrets even after rebooting. It is not critical to the server // to derive secret values fast, so the lookup operation is acceptable. SecretValueManager
Why are these interfaces combined like this? This feels more tangled than necessary. Can we either
- keep them entirely separated (i.e. the
Backend(serviceEngine) has aSecretValueBackend, and the grpc server has a separateServiceEngineand aSecretValueManagermember), or - combine them entirely, so the methods of the SecretValueManager interface are defined here in
ServiceEngine, and the secret value backend is not exported from this package (created directly inNewBackend). Also mean combine the twoDeleteExpired...functions into a single one so that only a single cleaner is needed.
I think the second option is preferrable, my gut feeling is that nobody outside of this module needs to be concerned with a split between an engine for the secret values and the rest.
go/pkg/cs/drkey/service_engine.go, line 45 at r4 (raw file):
DeriveASHost(meta drkey.ASHostMeta, lvl1Key drkey.Lvl1Key) (drkey.ASHostKey, error) DeriveHostAS(meta drkey.HostASMeta, lvl1Key drkey.Lvl1Key) (drkey.HostASKey, error) DeriveHostHost(meta drkey.HostHostMeta, lvl1Key drkey.Lvl1Key) (drkey.HostHostKey, error)
Couldn't these functions include getting the level 1 key? This would move the rest of the "business logic" here, leaving only a single function to in the grpc server code.
They should then perhaps be called GetXYKey instead of DeriveXY (same names as in ClientEngine, yay!)
go/pkg/cs/drkey/service_engine.go, line 59 at r4 (raw file):
// NewServiceServiceEngineCleaner creates a Cleaner task that removes expired lvl1 keys. func NewServiceServiceEngineCleaner(s ServiceEngine) *cleaner.Cleaner {
Is this "ServiceService" intentional?
go/pkg/cs/drkey/service_engine.go, line 66 at r4 (raw file):
// ServiceBackend keeps track of the level 1 drkey keys. It is backed by a drkey.DB type Backend struct {
This can be private. Call it serviceEngine to avoid introducing a separate term for the same thing.
go/pkg/cs/drkey/service_engine.go, line 76 at r4 (raw file):
var _ ServiceEngine = (*Backend)(nil) func NewBackend(store SecretValueManager, localIA addr.IA,
NewServiceEngine
go/pkg/cs/drkey/grpc/drkey_service.go, line 303 at r3 (raw file):
Previously, JordiSubira wrote…
Done. I agree with keeping the business logic in the now called "ServiceEngine"
Nice, this looks better now! As commented on the ServiceEngine interface, I'd go further and include getting the L1 key in the Derive... functions.
go/pkg/daemon/drkey/client_store.go, line 1 at r4 (raw file):
// Copyright 2020 ETH Zurich
Rename file to client_engine.go
go/pkg/daemon/drkey/client_store.go, line 29 at r4 (raw file):
) // Client manages secret values and lvl2 keys and provides them to end hosts.
// ClientEngine manages...
go/pkg/daemon/drkey/client_store.go, line 37 at r4 (raw file):
GetHostHostKey(ctx context.Context, meta drkey.HostHostMeta, valTime time.Time) (drkey.HostHostKey, error) DeleteExpiredLvl2Keys(ctx context.Context) (int, error)
Just DeleteExpired?
go/pkg/daemon/drkey/client_store.go, line 58 at r4 (raw file):
// ClientStore is the DRKey store used in the client side. type Engine struct {
Private, clientEngine or engine (and adapt comment string)
go/pkg/daemon/drkey/client_store.go, line 68 at r4 (raw file):
// NewClientStore constructs a new client store without assigned messenger. func NewEngine(local addr.IA, svdb drkey.SecretValueDB,
NewClientEngine
go/pkg/daemon/drkey/client_store.go, line 93 at r4 (raw file):
logger.Debug("[DRKey ClientStore] AS-Host key not stored. Requesting it to CS") // if not, ask our CS for it
We should avoid excessive logging even at debug level. Probably the logging from the grpc would already suffice to figure out that it is making this request to the CS. Drop this log line.
go/pkg/daemon/drkey/client_store.go, line 100 at r4 (raw file):
} if err = s.db.InsertASHostKey(ctx, remoteKey); err != nil { logger.Error("[DRKey ClientStore] Could not insert AS-Host in DB", "error", err)
Don't log the error, it's returned and will be logged higher up anyway.
go/lib/drkey/generic.go, line 26 at r4 (raw file):
type GenericDeriver struct { buf [32]byte key Key
Is key just a shorthand to avoid typing Key{}? I wouldn't... :)
go/lib/drkey/generic.go, line 51 at r4 (raw file):
// derivation, returning its length. func (p GenericDeriver) DeriveASHost(meta ASHostMeta, key Key) (Key, error) {
All these methods need a pointer receiver, otherwise the buf member will not be reused between invocations (p escapes to the heap here).
go/lib/drkey/meta.go, line 71 at r1 (raw file):
Previously, JordiSubira wrote…
It is also used in some other places to check whether the lvl2 derivation must be done using the GenericDeriver or the SpecificDeriver. I can replace it with its negation, i.e. IsGeneric() and refactor accordingly if that helps with the semantics.
I see. I think IsGeneric would have the same issue, the same word meaning two slightly different things. Perhaps it would be clearer if we call these protocols "well-known" or "predefined" or "builtin"?
// IsPredefined checks whether this is a well-known, built-in protocol identifier, i.e. Generic, SCMP or DNS. Returns false for all other protocol identifiers ("niche protocols").
func (p Protocol) IsPredefined() bool {...}
Or alternatively, IsNiche for the logical complement?
go/lib/drkey/protocol/protocol.go, line 15 at r3 (raw file):
Previously, JordiSubira wrote…
Done.
💯
go/lib/drkey/protocol/protocol.go, line 139 at r3 (raw file):
Previously, JordiSubira wrote…
In order to return the drkey.Key I have allocated the buffer array (not in the heap but in the program stack I guess). I think Go treats arrays as value, so I guess the returned array is allocated/copied in the caller as well.Do you think this is okay or we should spare even this passing a pointer to a destination buffer?
Looks good. No, passing a pointer could easily have a worse result as it makes the escape analysis jumpy and might end up being allocated on the heap.
The comment is not accurate anymore. Just The input buffer is overwritten.
go/lib/drkey/protocol/specific.go, line 29 at r3 (raw file):
Previously, JordiSubira wrote…
At the moment, I stick to returning the drkey.Key. When we make a final decision about the parameter passing and returning types (somewhere else in the discussion) I can change it to return the "type" key if needed.
Excellent 👍
There was a problem hiding this comment.
Reviewed 13 of 101 files at r1, 9 of 37 files at r2, 5 of 54 files at r3, 46 of 71 files at r4, all commit messages.
Reviewable status: 108 of 114 files reviewed, 50 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go.mod, line 4 at r4 (raw file):
require ( github.com/BurntSushi/toml v0.3.1
Remove this (run go mod tidy)
go/daemon/main.go, line 164 at r4 (raw file):
if globalCfg.DRKeyLvl2DB.Connection != "" { ia := itopo.Get().IA() svDB, err := storage.NewDRKeySVStorage(globalCfg.DRKeySVDB)
After removing the SV interface from the daemon, this is no longer needed (remove here and in daemon/drkey.ClientEngine.
go/lib/ctrl/drkey/lvl2_req.go, line 30 at r4 (raw file):
// ASHostReq represents a ASHost key request from an endhost to a CS. type ASHostReq struct {
Do we really need all these Req types? It looks like these are always only used as intermediate steps when converting between the dkpb.XYRequest and the drkey.XYMeta types. Couldn't we avoid the intermediate types and remove a whole bunch of conversion code?
go/lib/ctrl/drkey/lvl2_req.go, line 314 at r4 (raw file):
} func HostHostreqToProtoRequest(req HostHostReq) (*dkpb.HostHostRequest, error) {
HostHostReq...
go/lib/daemon/grpc.go, line 456 at r4 (raw file):
} func svReqToProtoRequest(req dkctrl.SVReq) (*sdpb.SVRequest, error) {
Unused
go/lib/daemon/grpc.go, line 467 at r4 (raw file):
// getSVFromReply extracts the level 1 drkey from the reply. func getSVFromReply(rep *sdpb.SVResponse, proto drkey.Protocol) (drkey.SV, error) {
Unused
go/lib/drkey/db.go, line 33 at r4 (raw file):
type SecretValueDB interface { BaseDB GetSV(ctx context.Context, proto Protocol, valTime uint32) (*SV, error)
Should return SV as value, ErrKeyNotFound if not found.
go/lib/drkey/db.go, line 41 at r4 (raw file):
type Lvl1DB interface { BaseDB GetLvl1Key(ctx context.Context, key Lvl1Meta, valTime uint32) (*Lvl1Key, error)
Should return Lvl1Key as value, , ErrKeyNotFound if not found.
go/lib/drkey/drkeydbsqlite/db_test.go, line 46 at r4 (raw file):
func newSVDatabase(t *testing.T) (*SVBackend, func()) { file, err := ioutil.TempFile("", "db-test-")
As commented on the definition of Prepare in drkeydbtest, this can use t.TempDir() so that this will automatically be cleaned up after the test run. Then no close function needs to be returned.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 46 at r4 (raw file):
type TestableLvl1DB interface { drkey.Lvl1DB Prepare(t *testing.T, ctx context.Context) func()
This can be simplified; this returns a close function to clean up the temporary test file. By using testing.T.TempDir()in the Prepare function this will be cleaned up automatically and no separate cleanup function is needed.
Same for the other Testable...DB interfaces.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 64 at r4 (raw file):
} t.Run("DRKeyLvl1 tests the backend Lvl1Key methods", testWrapper(testDRKeyLvl1))
This wrapper helper function and calling Run is unnecessary. Just
func TestLvl1(t *testing.T, db TestableLvl1DB) {
prepareCtx, cancelF := context.WithTimeout(context.Background(), 2*timeout)
db.Prepare(t, prepareCtx) // see comment on interface, no `close` function returned here.
cancelF() // cancel here, no defer.
defer db.Close()
testDRKeyLvl1(t, db)
}
Same for the other Test... functions.
go/pkg/cs/drkey/grpc/fetcher.go, line 70 at r4 (raw file):
logger.Info("Resolving server", "srcIA", srcIA.String()) path, err := f.Router.Route(ctx, srcIA)
What if this path is broken?
For segment requests, we use a random path and retry on timeout. That's not been great either, it frequently causes problems as it stops working correctly with very high latency paths.
Note that, for segment requests this has been a problem even though we only need to contact the core ASes responsible for the destination AS, not the destination itself. Here, we do need this to contact the destination AS itself so the problem will be even worse (longer paths, more path options).
The "correct" solution for this would be to track health information for the paths, but I think this would be too much additional work for the current stage.
Maybe we can do the same terrible trick with random path choice and retry? At least the prefetcher should retry on error.
go/pkg/daemon/config/config.go, line 48 at r4 (raw file):
PathDB storage.DBConfig `toml:"path_db,omitempty"` DRKeyLvl2DB storage.DBConfig `toml:"drkey_lvl2_db,omitempty"` DRKeySVDB storage.DBConfig `toml:"drkey_sv_db,omitempty"`
Can be removed now that the SV has been removed from the daemon API.
go/pkg/daemon/drkey/client_store.go, line 41 at r4 (raw file):
// NewSVCleaner creates a Cleaner task that removes expired lvl2 keys. func NewClientStoreCleaner(s ClientEngine) *cleaner.Cleaner {
NewClientEngineCleaner (and adapt comment)
go/pkg/trust/key_provider.go, line 27 at r4 (raw file):
) type KeyProvider struct {
This seems to do almost exactly what LoadingRing from pkg/cs/trust/key_loader.go already does. Can we not use that? If necessary we can perhaps add an additional method to that LoadingRing, but we shouldn't need a completely separate implementation for this.
Btw, should we perhaps move these files related to transport credentials also to pkg/cs/trust?
go/pkg/trust/tls_handshake.go, line 137 at r4 (raw file):
// FileLoader loads key pair from file type FileLoader struct {
Unused, remove
go/pkg/trust/x509KeyPairProvider.go, line 88 at r4 (raw file):
} certPEM, keyPEM := PairToEncodedPEM(bestChain, bestKey) certTLS, err := tls.X509KeyPair(certPEM, keyPEM)
Coudln't we simplify this to
return &tls.Certificate{
Certificate: [][]byte{bestChain[0].Raw, bestChain[1].Raw}, // or maybe better collect these with a loop
PrivateKey: bestKey, // needs to be the crypto.Signer, not the bytes (parsed in bestKeyPair or by LoadingRing)
Leaf: bestChain[1],
}
This can also be packed in a helper function. In contrast to the round trip through PEM, this cannot error.
There was a problem hiding this comment.
Reviewable status: 70 of 117 files reviewed, 50 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/cs/main.go, line 792 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Don't log the error, it's returned and will be logged higher up.
Perhaps wrap the error though,return 0, serrors.WrapStr("parsing SCION_TESTING_DRKEY_EPOCH_DURATION", err).
Done.
go/cs/config/drkey.go, line 148 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Store as
drkey.Protocol, to avoid having to converting the protocol id to a string just to check membership in this set here.
Done.
go/lib/daemon/daemon.go, line 102 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
Gotcha. The
MetaReqvsMetasounds good in principle, albeit a bit complicated. What about makingMetajust for the requests, and having the key types contain the metadata directly? So that would be, e.g.type Lvl1Meta struct { Validty time.Time ProtocolId Protocol SrcIA, DstIA addr.IA } type Lvl1Key struct { Epoch Epoch // \ ProtocolId Protocol // > listed individually instead of including Lvl1Meta SrcIA, DstIA addr.IA // / Key Key }Maybe we could then add some helper functions to create the Lvl1Key from the Lvl1Meta + key + epoch, to avoid typing out the full struct initialization everywhere.
Just an idea, I'm not sure how much this helps and how much work it is to revisit these types. Maybe treat this with low priority :)
It seems a good idea to me. I will get back to it in the future and check whether this do not requires to many changes in the methods that use these objects.
go/lib/drkey/db.go, line 28 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Remove this
BaseDBinterface, it's just clutter. Add the two interfaces to all the DB interfaces below explicitly.
Done.
go/lib/drkey/db.go, line 33 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Couldn't we pass all these
valTimeastime.Timeand only internally convert to timestamps internally?
Done.
go/lib/drkey/db.go, line 33 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Should return
SVas value,ErrKeyNotFoundif not found.
Done.
go/lib/drkey/db.go, line 35 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Lets call these
DeleteExpiredfor consistency (here and in the two other interfaces in this file).
Should also take atime.Time.
Done.
go/lib/drkey/db.go, line 41 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Should return
Lvl1Keyas value, ,ErrKeyNotFoundif not found.
Done.
go/lib/drkey/protocoltest/commons.go, line 31 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Initialize
lvl1Metawith Epoch and ProtoId too, then we can just writeLvl1Meta: lvl1Metahere.
Done.
go/lib/drkey/protocoltest/commons.go, line 32 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Arrays are value types, they are copied by assignment without explicit
copycall.
So here, we can just writeKey: keyand drop thevar outKey.Same applies for various functions below.
Done.
go/pkg/cs/drkey/service_engine.go, line 31 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
FetchLvl1?
Done.
go/pkg/cs/drkey/service_engine.go, line 40 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Why are these interfaces combined like this? This feels more tangled than necessary. Can we either
- keep them entirely separated (i.e. the
Backend(serviceEngine) has aSecretValueBackend, and the grpc server has a separateServiceEngineand aSecretValueManagermember), or- combine them entirely, so the methods of the SecretValueManager interface are defined here in
ServiceEngine, and the secret value backend is not exported from this package (created directly inNewBackend). Also mean combine the twoDeleteExpired...functions into a single one so that only a single cleaner is needed.I think the second option is preferrable, my gut feeling is that nobody outside of this module needs to be concerned with a split between an engine for the secret values and the rest.
The reason may be that the SecretValueManager was common to the clientEngine before. The second option seems acceptable provided that we already remove the SV cache from the clientEngine
go/pkg/cs/drkey/service_engine.go, line 45 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Couldn't these functions include getting the level 1 key? This would move the rest of the "business logic" here, leaving only a single function to in the grpc server code.
They should then perhaps be calledGetXYKeyinstead ofDeriveXY(same names as in ClientEngine, yay!)
Done.
go/pkg/cs/drkey/service_engine.go, line 59 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Is this "ServiceService" intentional?
No, it wasn't
go/pkg/cs/drkey/service_engine.go, line 66 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
This can be private. Call it
serviceEngineto avoid introducing a separate term for the same thing.
Done.
go/pkg/cs/drkey/service_engine.go, line 76 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
NewServiceEngine
Done.
go/pkg/cs/drkey/grpc/drkey_service.go, line 303 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Nice, this looks better now! As commented on the
ServiceEngineinterface, I'd go further and include getting the L1 key in theDerive...functions.
Done.
go/lib/drkey/generic.go, line 26 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Is
keyjust a shorthand to avoid typingKey{}? I wouldn't... :)
It was old junk.
go/lib/drkey/generic.go, line 51 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
All these methods need a pointer receiver, otherwise the
bufmember will not be reused between invocations (p escapes to the heap here).
Done.
go/lib/drkey/meta.go, line 71 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
I see. I think
IsGenericwould have the same issue, the same word meaning two slightly different things. Perhaps it would be clearer if we call these protocols "well-known" or "predefined" or "builtin"?// IsPredefined checks whether this is a well-known, built-in protocol identifier, i.e. Generic, SCMP or DNS. Returns false for all other protocol identifiers ("niche protocols"). func (p Protocol) IsPredefined() bool {...}Or alternatively,
IsNichefor the logical complement?
IsPredefined seems acceptable to me.
go/lib/drkey/drkeydbsqlite/lvl1db.go, line 15 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
This package can be called just
sqlite, it's already underdrkey.
Done.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 111 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ok. Please also remove the
prepareAllhelper function and the corresponding const/var definitions when you get to it.
Done.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 15 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
dbtest, it's already underdrkey.
Done.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 37 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Unused after using
protocoltest.GetLvl1to simplify initialization.
Done.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 39 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Use
xtest.MustParseIA, like indrkey/protocol_test.go.
Done.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 88 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Simplify initialization by using
protocoltest.GetLvl1, remove theDeriveLvl1helper function.
Done.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 113 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Remove
Done.
go/lib/drkey/protocol/protocol.go, line 139 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Looks good. No, passing a pointer could easily have a worse result as it makes the escape analysis jumpy and might end up being allocated on the heap.
The comment is not accurate anymore. Just
The input buffer is overwritten.
Done.
go/lib/drkey/protocol/specific.go, line 29 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Excellent 👍
This related to another comment in this thread about changing the Meta and the "typed" Key object
go/pkg/daemon/drkey/client_store.go, line 1 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Rename file to
client_engine.go
Done.
go/pkg/daemon/drkey/client_store.go, line 29 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
// ClientEngine manages...
Done.
go/pkg/daemon/drkey/client_store.go, line 37 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
DeleteExpired?
I keep the Keys for consistency, but we can remove it everywhere else.
go/pkg/daemon/drkey/client_store.go, line 58 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Private,
clientEngineorengine(and adapt comment string)
Done.
go/pkg/daemon/drkey/client_store.go, line 68 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
NewClientEngine
Done.
go/pkg/daemon/drkey/client_store.go, line 93 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
We should avoid excessive logging even at debug level. Probably the logging from the grpc would already suffice to figure out that it is making this request to the CS. Drop this log line.
Done.
go/pkg/daemon/drkey/client_store.go, line 100 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Don't log the error, it's returned and will be logged higher up anyway.
Done.
There was a problem hiding this comment.
Reviewable status: 51 of 118 files reviewed, 50 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go.mod, line 4 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Remove this (run
go mod tidy)
Done.
go/daemon/main.go, line 164 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
After removing the SV interface from the daemon, this is no longer needed (remove here and in
daemon/drkey.ClientEngine.
Done.
go/lib/ctrl/drkey/lvl2_req.go, line 30 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Do we really need all these
Reqtypes? It looks like these are always only used as intermediate steps when converting between thedkpb.XYRequestand thedrkey.XYMetatypes. Couldn't we avoid the intermediate types and remove a whole bunch of conversion code?
I remove those intermediate objects. I have still keep some conversion code between protobuf and go objects here, since some code is reused in the code. We can move some if you think it helps with the organization.
go/lib/ctrl/drkey/lvl2_req.go, line 314 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
HostHostReq...
Done.
go/lib/daemon/daemon.go, line 102 at r2 (raw file):
Previously, JordiSubira wrote…
It seems a good idea to me. I will get back to it in the future and check whether this do not requires to many changes in the methods that use these objects.
I have done this as well. It made sense to tackle it at the same time as I was removing the intermediate golang objects for protobuf conversion.
go/lib/daemon/grpc.go, line 456 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Unused
Done.
go/lib/daemon/grpc.go, line 467 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Unused
Done.
go/pkg/cs/drkey/grpc/fetcher.go, line 70 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
What if this path is broken?
For segment requests, we use a random path and retry on timeout. That's not been great either, it frequently causes problems as it stops working correctly with very high latency paths.
Note that, for segment requests this has been a problem even though we only need to contact the core ASes responsible for the destination AS, not the destination itself. Here, we do need this to contact the destination AS itself so the problem will be even worse (longer paths, more path options).The "correct" solution for this would be to track health information for the paths, but I think this would be too much additional work for the current stage.
Maybe we can do the same terrible trick with random path choice and retry? At least the prefetcher should retry on error.
You're right, I had thought about this as well. I have applied this random path trick with a maximum retry attempts at the moment. For the prefetcher we could also adapt the Runner's periodicity. In any case, in future versions I hope we can adapt the prefetcher to comply with the design in the documentation site (once we have independent validity times for DRKey).
go/pkg/daemon/config/config.go, line 48 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Can be removed now that the SV has been removed from the daemon API.
Done.
go/pkg/trust/tls_handshake.go, line 137 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Unused, remove
Done.
go/pkg/trust/x509KeyPairProvider.go, line 88 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Coudln't we simplify this to
return &tls.Certificate{ Certificate: [][]byte{bestChain[0].Raw, bestChain[1].Raw}, // or maybe better collect these with a loop PrivateKey: bestKey, // needs to be the crypto.Signer, not the bytes (parsed in bestKeyPair or by LoadingRing) Leaf: bestChain[1], }This can also be packed in a helper function. In contrast to the round trip through PEM, this cannot error.
Done.
go/pkg/trust/key_provider.go, line 27 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
This seems to do almost exactly what
LoadingRingfrompkg/cs/trust/key_loader.goalready does. Can we not use that? If necessary we can perhaps add an additional method to thatLoadingRing, but we shouldn't need a completely separate implementation for this.Btw, should we perhaps move these files related to transport credentials also to
pkg/cs/trust?
I keep it in pkg/trust because at the moment the code is using some private functions in this package as activeTRCs or min. Yes, using the LoadingRing seems a good idea . I reckon we could move the LoadingRing to the pkg/trust if we finally decide that the code stays there. As a workaround I declared an interface with the PrivateKeys method. We can address that in next passes.
go/lib/drkey/drkeydbsqlite/db_test.go, line 46 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
As commented on the definition of Prepare in drkeydbtest, this can use
t.TempDir()so that this will automatically be cleaned up after the test run. Then noclosefunction needs to be returned.
Done.
go/lib/drkey/drkeydbsqlite/lvl2db.go, line 121 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
This should return an error. The caller currently needs to check whether a zero key was returned, which is ugly because this technically a valid key. We can define a specific error for this,
var ErrKeyNotFound = serrors.New("Key not found").
Done.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 46 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
This can be simplified; this returns a
closefunction to clean up the temporary test file. By usingtesting.T.TempDir()in the Prepare function this will be cleaned up automatically and no separate cleanup function is needed.Same for the other
Testable...DBinterfaces.
Done.
go/lib/drkey/drkeydbtest/drkeydbtest.go, line 64 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
This wrapper helper function and calling Run is unnecessary. Just
func TestLvl1(t *testing.T, db TestableLvl1DB) { prepareCtx, cancelF := context.WithTimeout(context.Background(), 2*timeout) db.Prepare(t, prepareCtx) // see comment on interface, no `close` function returned here. cancelF() // cancel here, no defer. defer db.Close() testDRKeyLvl1(t, db) }Same for the other
Test...functions.
Done.
go/pkg/daemon/drkey/client_store.go, line 41 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
NewClientEngineCleaner(and adapt comment)
Done.
There was a problem hiding this comment.
Reviewed 5 of 43 files at r5, 11 of 46 files at r6, 1 of 15 files at r7.
Reviewable status: 64 of 118 files reviewed, 7 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/lib/ctrl/drkey/lvl2_req.go, line 30 at r4 (raw file):
Previously, JordiSubira wrote…
I remove those intermediate objects. I have still keep some conversion code between protobuf and go objects here, since some code is reused in the code. We can move some if you think it helps with the organization.
Excellent. 👍
go/pkg/cs/drkey/grpc/drkey_service_test.go, line 57 at r6 (raw file):
tcpHost := net.TCPAddr{ IP: xtest.MustParseCIDR(t, "127.0.0.1/32").IP,
Just net.ParseIP("127.0.0.1"). Or go the other way around, starting with the (better) netaddr:
host := netaddr.MustParseIP(...)
tcpHost = host.WithPort(12345).TCPAddr()
go/pkg/cs/drkey/grpc/drkey_service_test.go, line 95 at r6 (raw file):
"no host": { peerAddr: &net.TCPAddr{ IP: xtest.MustParseCIDR(t, "127.0.0.1/32").IP,
Same here, just net.ParseIP, or (better) peerAddr: netaddr.MustParseIPPort("127.0.0.1:12345").TCPAddr()
go/pkg/cs/drkey/grpc/drkey_service_test.go, line 352 at r6 (raw file):
grpcServer := dk_grpc.Server{ LocalIA: xtest.MustParseIA("1-ff00:0:111"),
Use the ia111 variable defined above.
Perhaps even move this to a global (like lvl2Meta) and replace all the MustParseIA calls?
Similar thing could be done for the peer address and the various MustParseHexString-keys.
go/pkg/cs/trust/key_loader.go, line 66 at r6 (raw file):
} func (r LoadingRing) GetKeys(ctx context.Context) ([][]byte, error) {
Now unused, delete.
go/pkg/trust/tls_handshake.go, line 128 at r6 (raw file):
for _, extKeyUsage := range extKeyUsages { for _, certExtKeyUsage := range cert.ExtKeyUsage { if extKeyUsage == x509.ExtKeyUsageAny || extKeyUsage == certExtKeyUsage {
Should this be *cert*ExtKeyUsage == x509.ExtKeyUsageAny? Currently, this supports specifying "Any" in the expected key usages, which we don't need as we call this function only with exactly ServerAuth and ClientAuth. However, accepting a certificate that specifies "Any" may make sense (not sure).
Perhaps it would make sense to distinguish the variable names a bit clearer, e.g. expectedEKUs and certEKU or actualEKU.
Also, from the function signature I would have intuitively expected that it checks whether all the expected EKUs are present. However, it checks only if any is present. Currently we use this to check for a single EKU, so I'd suggest to reduce the scope of the function to check for a single expected EKU, so there is no possible confusion.
go/pkg/trust/x509KeyPairProvider.go, line 32 at r6 (raw file):
type KeyLoader interface { PrivateKeys(ctx context.Context) ([]crypto.Signer, error) }
This interface is identical with KeyRing (in the same package). Remove and replace usage with KeyRing.
go/pkg/trust/key_provider.go, line 27 at r4 (raw file):
Previously, JordiSubira wrote…
I keep it in
pkg/trustbecause at the moment the code is using some private functions in this package asactiveTRCsormin. Yes, using theLoadingRingseems a good idea . I reckon we could move theLoadingRingto thepkg/trustif we finally decide that the code stays there. As a workaround I declared an interface with thePrivateKeysmethod. We can address that in next passes.
All clear, seems good.
This new interface KeyLoader is identical with the existing interface KeyRing in the same package (commented there).
There was a problem hiding this comment.
Reviewed 5 of 43 files at r5, 9 of 46 files at r6, 3 of 15 files at r7, all commit messages.
Reviewable status: 81 of 118 files reviewed, 9 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/trust/x509KeyPairProvider.go, line 45 at r7 (raw file):
func (p X509KeyPairProvider) LoadX509KeyPair(extKeyUsages []x509.ExtKeyUsage) ( *tls.Certificate, error) { ctx, cancel := context.WithTimeout(context.Background(), p.Timeout)
This context should be passed as parameter to this function. Then we don't need the Timeout member. In TLSCryptoManager.GetCertificate/GetClientCertificate, we can pass the ClientHello.Context() or CertificateRequestInfo.Context(), respectively. This moves the definition of the timeout to the place where the connection is actually established. Also, this has the advantage that other events than timeout can potentially abort the context.
Btw; the VerifyPeerCertificate does not have a corresponding context, I'm not sure what the idea is there.
go/pkg/trust/x509KeyPairProvider_test.go, line 447 at r7 (raw file):
func loadSigner(t *testing.T, file string) crypto.Signer { raw, err := ioutil.ReadFile(file)
Deprecated, use os.ReadFile(file)
There was a problem hiding this comment.
Reviewed 1 of 101 files at r1, 7 of 43 files at r5, 15 of 46 files at r6, 11 of 15 files at r7.
Reviewable status: 115 of 118 files reviewed, 9 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/cs/drkey/export_test.go, line 24 at r7 (raw file):
) var NewSecretValueBackend = newSecretValueBackend
Unused
go/pkg/cs/drkey/grpc/fetcher.go, line 70 at r4 (raw file):
Previously, JordiSubira wrote…
You're right, I had thought about this as well. I have applied this random path trick with a maximum retry attempts at the moment. For the prefetcher we could also adapt the Runner's periodicity. In any case, in future versions I hope we can adapt the prefetcher to comply with the design in the documentation site (once we have independent validity times for DRKey).
💯
There was a problem hiding this comment.
Reviewable status: 108 of 118 files reviewed, 9 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/cs/drkey/export_test.go, line 24 at r7 (raw file):
Previously, matzf (Matthias Frei) wrote…
Unused
Done.
go/pkg/cs/drkey/grpc/drkey_service_test.go, line 57 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
net.ParseIP("127.0.0.1"). Or go the other way around, starting with the (better) netaddr:host := netaddr.MustParseIP(...) tcpHost = host.WithPort(12345).TCPAddr()
Done.
go/pkg/cs/drkey/grpc/drkey_service_test.go, line 95 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Same here, just
net.ParseIP, or (better)peerAddr: netaddr.MustParseIPPort("127.0.0.1:12345").TCPAddr()
Done.
go/pkg/cs/drkey/grpc/drkey_service_test.go, line 352 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Use the
ia111variable defined above.
Perhaps even move this to a global (like lvl2Meta) and replace all theMustParseIAcalls?
Similar thing could be done for the peer address and the variousMustParseHexString-keys.
Done.
go/pkg/trust/x509KeyPairProvider.go, line 32 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
This interface is identical with
KeyRing(in the same package). Remove and replace usage withKeyRing.
Done.
go/pkg/trust/x509KeyPairProvider.go, line 45 at r7 (raw file):
Previously, matzf (Matthias Frei) wrote…
This context should be passed as parameter to this function. Then we don't need the
Timeoutmember. InTLSCryptoManager.GetCertificate/GetClientCertificate, we can pass theClientHello.Context()orCertificateRequestInfo.Context(), respectively. This moves the definition of the timeout to the place where the connection is actually established. Also, this has the advantage that other events than timeout can potentially abort the context.Btw; the
VerifyPeerCertificatedoes not have a corresponding context, I'm not sure what the idea is there.
ClientHello.Context() or CertificateRequestInfo.Context() are added in go 1.17. Currently, I think we are using go 1.16.8 (if I'm not wrong and this line indicates the golang version that the project is compiled with). As you said even in the latest version VerifyPeerCertificate does not have a way to (easily at least) access the connection context.
go/pkg/trust/x509KeyPairProvider_test.go, line 447 at r7 (raw file):
Previously, matzf (Matthias Frei) wrote…
Deprecated, use
os.ReadFile(file)
Done.
go/pkg/cs/trust/key_loader.go, line 66 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Now unused, delete.
Done.
There was a problem hiding this comment.
Reviewable status: 108 of 118 files reviewed, 9 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/trust/tls_handshake.go, line 128 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Should this be
*cert*ExtKeyUsage == x509.ExtKeyUsageAny? Currently, this supports specifying "Any" in the expected key usages, which we don't need as we call this function only with exactlyServerAuthandClientAuth. However, accepting a certificate that specifies "Any" may make sense (not sure).Perhaps it would make sense to distinguish the variable names a bit clearer, e.g.
expectedEKUsandcertEKUoractualEKU.Also, from the function signature I would have intuitively expected that it checks whether all the expected EKUs are present. However, it checks only if any is present. Currently we use this to check for a single EKU, so I'd suggest to reduce the scope of the function to check for a single expected EKU, so there is no possible confusion.
It is not clear for me either, however looking at Anapaya's documentation: https://scion.docs.anapaya.net/en/latest/cryptography/certificates.html#id34, it seems the application may want to validate only certificates with the given flags set.
Indeed I agree, we should check only for a unique EKU for a given certificate
There was a problem hiding this comment.
Reviewed 1 of 101 files at r1, 8 of 9 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/trust/x509KeyPairProvider.go, line 45 at r7 (raw file):
Previously, JordiSubira wrote…
ClientHello.Context()orCertificateRequestInfo.Context()are added in go 1.17. Currently, I think we are using go 1.16.8 (if I'm not wrong and this line indicates the golang version that the project is compiled with). As you said even in the latest versionVerifyPeerCertificatedoes not have a way to (easily at least) access the connection context.
Ah, I see. Upstream has already updated to go 1.17 and this will be picked up for scionlab soon too (#110).
We'll have to update this PR before merging anyway, so we can quickly revisit this bit then.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/trust/x509KeyPairProvider.go, line 45 at r7 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ah, I see. Upstream has already updated to go 1.17 and this will be picked up for scionlab soon too (#110).
We'll have to update this PR before merging anyway, so we can quickly revisit this bit then.
👍
JordiSubira
force-pushed
the
scionlab_drkey_vs2
branch
from
cf42d11 to
df119bb
Compare
There was a problem hiding this comment.
Reviewed 15 of 15 files at r12, all commit messages.
Reviewable status: 86 of 140 files reviewed, 7 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/co/reservationstore/drkey.go, line 179 at r12 (raw file):
func (a *DRKeyAuthenticator) ComputeResponseMAC(ctx context.Context, res base.Response, path *base.TransparentPath, ts time.Time) error {
There is already a timestamp in AuthenticatedResponse. Shouldn't that be used here and for the sibling methods? Perhaps the timestamp needs to be exposed on the Response interface or something.
go/co/reservationstore/drkey.go, line 749 at r12 (raw file):
// XXX(JordiSubira): We could consider adding a Lvl1DB to avoid contacting multiple times the // CS for the same key.
Just use the same trivial in-memory cache as we have below for the secrets? This will be just as good in practice, I don't see strong upsides for caching this persistently here.
go/lib/colibri/drkey.go, line 156 at r12 (raw file):
// XXX(JordiSubira): We could avoid the sciond connection and directly request // the key to the CS. Even, we can derive from a cached Lvl1Key, if such a cache // is introduced at some point.
No. As discussed in the chat, lib/colibri is the application client library for applications running on any host in the AS. Here we cannot assume that we have access to the secret value or Lvl1 key for the colibri protocol.
proto/drkey/mgmt/v1/mgmt.proto, line 60 at r12 (raw file):
} message ASASRequest{
Everywhere else this was called Lvl1 key. I think we should keep the naming as consistent as we can, it's already confusing enough.
proto/drkey/mgmt/v1/mgmt.proto, line 76 at r12 (raw file):
// End of validity period of DRKey. google.protobuf.Timestamp epoch_end = 2; // Lvl2 key.
Suggestion:
Lvl1go/lib/drkey/protocol_test.go, line 396 at r12 (raw file):
Key: key, } assert.Equal(t, lvl1Target, lvl1)
Just compare only the key, in both instances here. The rest of the lvl1 and lvl1target structs is all defined in this function here and we're only testing the test code here.
There was a problem hiding this comment.
Reviewable status: 74 of 143 files reviewed, 7 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/co/reservationstore/drkey.go, line 179 at r12 (raw file):
Previously, matzf (Matthias Frei) wrote…
There is already a timestamp in
AuthenticatedResponse. Shouldn't that be used here and for the sibling methods? Perhaps the timestamp needs to be exposed on theResponseinterface or something.
Done.
go/co/reservationstore/drkey.go, line 749 at r12 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just use the same trivial in-memory cache as we have below for the secrets? This will be just as good in practice, I don't see strong upsides for caching this persistently here.
Still pending to do the cleaning (as in SV cache)
go/lib/colibri/drkey.go, line 156 at r12 (raw file):
Previously, matzf (Matthias Frei) wrote…
No. As discussed in the chat,
lib/colibriis the application client library for applications running on any host in the AS. Here we cannot assume that we have access to the secret value or Lvl1 key for the colibri protocol.
ack
proto/drkey/mgmt/v1/mgmt.proto, line 60 at r12 (raw file):
Previously, matzf (Matthias Frei) wrote…
Everywhere else this was called Lvl1 key. I think we should keep the naming as consistent as we can, it's already confusing enough.
As discussed, we might want to refactor the definitions in different packages for Intra and Inter services.
proto/drkey/mgmt/v1/mgmt.proto, line 76 at r12 (raw file):
// End of validity period of DRKey. google.protobuf.Timestamp epoch_end = 2; // Lvl2 key.
Done.
go/lib/drkey/protocol_test.go, line 396 at r12 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just compare only the
key, in both instances here. The rest of the lvl1 and lvl1target structs is all defined in this function here and we're only testing the test code here.
Done.
There was a problem hiding this comment.
Reviewed 35 of 51 files at r10, 19 of 26 files at r11, 15 of 15 files at r13, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/lib/scrypto/cppki/certs.go, line 147 at r13 (raw file):
TRC []*TRC CurrentTime time.Time // if zero, the current time is used ExtKeyUsage []x509.ExtKeyUsage
Unused
python/topology/go.py, line 138 at r13 (raw file):
#'delegation': { #'colibri': [str(sd_ip)], # local daemon must be able to get the colibri DS #},
This still needs to be fixed
go/lib/drkey/protocol_test.go, line 396 at r12 (raw file):
Previously, JordiSubira wrote…
Done.
Drop the definition for the lvl1struct also, and use assert.Equal in both cases.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/cs/drkey/grpc/drkey_service.go, line 211 at r13 (raw file):
peer, ok := peer.FromContext(ctx) if !ok { serrors.New("Cannot retrieve peer information from ctx")
Bug, missing return.
go/pkg/cs/drkey/grpc/drkey_service.go, line 262 at r13 (raw file):
peer, ok := peer.FromContext(ctx) if !ok { serrors.New("Cannot retrieve peer information from ctx")
Bug, missing return.
go/pkg/cs/drkey/grpc/drkey_service.go, line 332 at r13 (raw file):
peer, ok := peer.FromContext(ctx) if !ok { serrors.New("Cannot retrieve peer information from ctx")
Bug, missing return.
There was a problem hiding this comment.
Reviewable status: 122 of 144 files reviewed, 6 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/cs/drkey/grpc/drkey_service.go, line 211 at r13 (raw file):
Previously, matzf (Matthias Frei) wrote…
Bug, missing
return.
Done.
go/pkg/cs/drkey/grpc/drkey_service.go, line 262 at r13 (raw file):
Previously, matzf (Matthias Frei) wrote…
Bug, missing
return.
Done.
go/pkg/cs/drkey/grpc/drkey_service.go, line 332 at r13 (raw file):
Previously, matzf (Matthias Frei) wrote…
Bug, missing
return.
Done.
python/topology/go.py, line 138 at r13 (raw file):
Previously, matzf (Matthias Frei) wrote…
This still needs to be fixed
Changed to retrieve the list of ColServ network addresses.
go/lib/scrypto/cppki/certs.go, line 147 at r13 (raw file):
Previously, matzf (Matthias Frei) wrote…
Unused
Done.
There was a problem hiding this comment.
Reviewed 11 of 101 files at r1, 4 of 37 files at r2, 15 of 71 files at r4, 8 of 43 files at r5, 10 of 46 files at r6, 6 of 15 files at r7, 28 of 51 files at r10, 16 of 26 files at r11, 2 of 15 files at r12, 9 of 15 files at r13, 22 of 22 files at r14, all commit messages.
Reviewable status: all files reviewed, 26 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/co/reservationstore/drkey.go, line 742 at r14 (raw file):
mtx sync.Mutex localIA addr.IA Dialer libgrpc.Dialer
Private?
go/co/reservationstore/drkey.go, line 815 at r14 (raw file):
type cachingSVfetcher struct { Dialer libgrpc.Dialer
Private?
go/co/reservationstore/drkey.go, line 853 at r14 (raw file):
if err != nil { return drkey.SV{}, serrors.WrapStr("parsing AS-HOST request to protobuf", err)
Fix comment to SV
go/co/reservationstore/drkey_test.go, line 678 at r14 (raw file):
if meta.DstIA != f.localIA { panic(fmt.Sprintf("cannot fetch, DstIA != localIA, DstIA=%s, localIA=%s", f.localIA, meta.DstIA))
invert order
go/cs/config/drkey_test.go, line 132 at r14 (raw file):
require.Contains(t, m, HostProto{ Host: ip1111, Proto: drkey.DNS,
drkey.SCMP
go/daemon/main.go, line 196 at r14 (raw file):
drkeyDB, err := storage.NewDRKeyLvl2Storage(globalCfg.DRKeyLvl2DB) if err != nil { log.Error("Creating Lvl2 DRKey DB", "err", err)
Remove
go/lib/drkey/drkey.go, line 141 at r14 (raw file):
} // Lvl1Meta represents the information about a level 1 DRKey other than the key itself.
// Lvl1Meta contains metadata to obtain Lvl1Keys.
go/pkg/cs/drkey/prefetcher.go, line 95 at r14 (raw file):
_, err := engine.GetLvl1Key(pref_ctx, meta) if err != nil { log.Error("Failed to prefetch the level 1 key", "remote AS", srcIA.String(), "error", err)
Add proto to log
go/pkg/cs/drkey/service_engine.go, line 30 at r14 (raw file):
// Fetcher obtains a Lvl1 DRKey from a remote CS. type Fetcher interface { FetchLvl1(ctx context.Context, meta drkey.Lvl1Meta) (drkey.Lvl1Key, error)
Just Lvl1
go/pkg/cs/drkey/service_engine.go, line 161 at r14 (raw file):
} // DeleteExpiredKeys will remove any lvl1 expired keys.
Should go above the proper method.
go/pkg/cs/drkey/service_engine.go, line 176 at r14 (raw file):
} // GetCachedASes returns a list of ASes currently in the cache.
GetLvl1PrefetchInfo ...
go/pkg/cs/drkey/grpc/drkey_service.go, line 43 at r14 (raw file):
LocalIA addr.IA Engine cs_drkey.ServiceEngine // AllowedSVHost is a set of Host,Protocol pairs that represents the allowed
AllowedSVHostProto
go/pkg/cs/drkey/grpc/drkey_service.go, line 80 at r14 (raw file):
lvl1Key, err := d.Engine.DeriveLvl1(lvl1Meta) if err != nil { logger.Error("Error deriving level 1 key", "err", err)
replace log by wrapped error.
go/pkg/cs/drkey/grpc/drkey_service.go, line 352 at r14 (raw file):
} // validateSVReq checks that the requester is authorized to receive a SV
validateAllowedHost...
go/pkg/daemon/internal/servers/grpc.go, line 358 at r14 (raw file):
meta, err := ctrl_drkey.RequestToASHostMeta(req) if err != nil { logger.Error("[DRKey DeamonService] Invalid DRKey AS-Host request", "err", err)
Remove error logs
go/lib/drkey/specific.go, line 30 at r14 (raw file):
// DeriveLvl1 populates the provided buffer with // the input for a lvl1 derivation, returning its length.
Fix description; similarly below.
go/lib/drkey/sqlite/lvl1db.go, line 45 at r14 (raw file):
EpochEnd INTEGER NOT NULL, Key BLOB NOT NULL, PRIMARY KEY (SrcIsdID, SrcAsID, DstIsdID, DstAsID, EpochBegin)
Protocol should be part of the primary key.
go/pkg/cs/drkey/secret_value_mgr.go, line 73 at r14 (raw file):
err = s.DB.InsertSV(ctx, sv) if err != nil { log.FromCtx(ctx).Error("Cannot insert SV in persistence", "err", err)
return error
go/pkg/daemon/drkey/client_engine.go, line 44 at r14 (raw file):
// Fetcher obtains a end host keys from the local CS. type Fetcher interface { FetchASHostKey(ctx context.Context, meta drkey.ASHostMeta) (drkey.ASHostKey, error)
Just ASHostKey
go/pkg/daemon/drkey/grpc/fetcher.go, line 102 at r14 (raw file):
if err != nil { return drkey.HostHostKey{}, serrors.WrapStr("parsing HOST-AS request to protobuf", err)
Host-Host
There was a problem hiding this comment.
Reviewed 2 of 9 files at r8, 1 of 51 files at r10, 2 of 15 files at r13.
Reviewable status: all files reviewed, 26 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
There was a problem hiding this comment.
Reviewed 19 of 19 files at r15, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
There was a problem hiding this comment.
Reviewed 1 of 15 files at r13, 22 of 22 files at r14, 19 of 19 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
Squashed and rebased on scionlab. The changes in scionlab include updates to master, and DRKey integration for Colibri. Colibri's DRKey integration needs to be revisited entirely. This first pass breaks & comments out various related functionality and tests. - The co/ service does not currently build - The `drkeyutil` package should be moved closer to colibri. - Obtaining the AS-AS keys in colibri needs to be revisited, bypassing the daemon (SV and Lvl1 keys are not handled by the daemon). - The CS's drkey API lacks functionality to obtain a Lvl1 keys for authorized clients (analogous to SVs). Raw squash commit log below: ---- refactor protobuf lvl2 host to byte string only add X509KeyPairProvider add UT for key_provider + use in main add extendedKeyUsage deleted unnecessary exchange package remove unnecessary timestamp field in DRKey protobuf messages taking epoch from DS in lvl2 derivation functions rebase changes cleanly close DRKey DB add metrics for DRKey + refactor in drkeyDB remove unused DRKeyConfig TLS files add lvl1cache + ARC + refactoring service_store and prefetcher linting code add license golang-lru Halfway new derivation scheme commit: Done: - Implement new derivation scheme - Refactor code that uses new derivation scheme Missing: - Lint code - Update test cases - Integration tests added API for SV + refactor SVStore + tests + protobuf modify protobuf + refactoring rename AllowedDS and DelagationList rename gRPC services add metrics to SV_DB remove configuration Epoch duration naming+lint protobuf refactoring drkeystorage and drkey cleaners lint files protobuf comments modify host in Lvl2Req + add HostAddr for Lvl2/3 derivation add check for lvl1 key protocol + generic request for lvl2 keys linting add global epoch duration configuration option: - This feature is only intended for testing/debuging purposes. cleaning Lvl1DB add register for QUICTLSServer in main modify signature for GetLvl1Key, DeriveHostToHostKey and fixing some comments modify output metadata to inherit from upper secret when it applies improving some comments refactor derivation to use CBC MAC changes in derivation implementation + refactoring + testing separate lvl2/lvl3 into end-host keys fixing some comments replacing lvl2 req/res with end-host req/res added env variable epoch duration r1 lvl1 prefetching list r1 change prefetch list config name r1 partial pass fix assign drkeystore cs/main drkey byte slice to array remove prepared stmts lvl2db remove sv api in daemon + refactoring services renaming daemon fetcher methods change batch: - Rename DRKey to Key - Check 16 bytes key in ctrl parsing - Reorganize metadata and lvl1/2 key structs - InitDefault for lvl1db - Refactor drkey.Protocol refactor drkey fetcher move derivation to Engine r3 changes in lib and cs packages r3 in pkg cs r3 changes in clientEngine + protocol derivation fix comment client engine r4 updates r4 remove int request objects in lib/ctrl/drkey r4 retries fetcher + refactor test db update r5 addition to r5 update
Functionality somewhat specific (CMAC), not used elsewhere and kind of trivial.
Rebuild the drkey/fake (previously drkey/test) package from scratch, adapt all tests.
- add implementation slowKeyer - add ASAS endpoint on the CS - add COLIBRI id - bug fixes
JordiSubira
force-pushed
the
scionlab_drkey_vs2
branch
from
a267b38 to
1596fbf
Compare
There was a problem hiding this comment.
Reviewable status: 72 of 144 files reviewed, 1 unresolved discussion / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/trust/x509KeyPairProvider.go, line 45 at r7 (raw file):
Previously, JordiSubira wrote…
👍
I've changed the code as discussed above, i.e. removed the Timeout field in X509KeyPairProvider. The VerifyPeerCertificate still uses the Timeout in TLSCryptoManager
There was a problem hiding this comment.
Reviewed 1 of 63 files at r16, 14 of 25 files at r17, 57 of 57 files at r18, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
go/pkg/trust/x509KeyPairProvider.go, line 45 at r7 (raw file):
Previously, JordiSubira wrote…
I've changed the code as discussed above, i.e. removed the
Timeoutfield inX509KeyPairProvider. TheVerifyPeerCertificatestill uses theTimeoutinTLSCryptoManager
💯
Disclaimer: At the moment this is a draft; some changes/improvements are still to be incorporated.
This PR contains the following changes to DRKey (some of them breaking changes and not back-compatible):
This version does not include the following features:
This change is