DRKey ported from the old SCIONLab

[juagargi]

This is a port for the new SCIONLab of the old DRKey implementation. This old implementation was working previously, but had to be adapted to the new gRPC, among other things.

Pending:

• Document describing the limitations of this implementation. https://docs.google.com/document/d/1_YqE1euVw0CLlltyO_OIBOtwGnu1tCJrTzr5ZaQOJiE/edit
• Document describing the configuration for SCIONLab.
• Integration tests?
• Check that if not configured, cs will not use the DRKey code at all (no effect rule).

---

This change is 

[juagargi]

For reference. DRKey has no effect if not configured.
In CS:
DRKey is disabled if not configured through the use of cfg.DRKey.Enabled() in cs.go.
There are two module init functions: in piskes.go and scmp.go.
Both init functions just create a protocol instance and add it to the KnownDerivations map. "Almost" no effect.

In sciond:
If the configuration does't include a cfg.DRKeyDB.Connection, it's completely disabled.

Reviewable status: 0 of 149 files reviewed, all discussions resolved

[juagargi]

Sample client and gen directory with DRKey on https://gist.github.com/juagargi/508edaa77358a25867f06c7f21530f19

Reviewable status: 0 of 139 files reviewed, all discussions resolved

[matzf]

Reviewed 67 of 139 files at r1.
Reviewable status: 67 of 139 files reviewed, 5 unresolved discussions (waiting on @juagargi)

---

go/cs/cs.go, line 378 at r1 (raw file):

	//DRKey feature
	var drkeyServStore drkeystorage.ServiceStore
	var svFactory drkeystorage.SecretValueFactory

Nit: move into block below.

---

go/cs/cs.go, line 427 at r1 (raw file):

				libgrpc.LogIDServerInterceptor(),
			),
		)

It's not clear to me why this quicTLSServer (and e.g. the key/cert pair configuration) is required at all. Why can this not use the other infrastructure? Why are the key/cert pairs specific to DRKey?
If it is necessary, I'd argue that this code should be moved to e.g. go/lib/infra/infraenv/infraenv.go where the rest of the quic infrastructure "lives".

---

go/cs/config/drkey.go, line 47 at r1 (raw file):

	EpochDuration util.DurWrap `toml:"epoch_duration,omitempty"`
	// MaxReplyAge is the age limit for a level 1 reply to be accepted. Older are rejected.
	MaxReplyAge util.DurWrap `toml:"max_reply_age,omitempty"`

This config value appears to be unused.

---

go/cs/config/drkey.go, line 83 at r1 (raw file):

func (cfg *DRKeyConfig) Enabled() bool {
	// TODO(juagargi): check that disabled CSs can receive DRKey queries from sciond (mine crashes)
	return cfg.enabled

Nit: the way this is handled looks correct, but it would be clearer without the intermediate enabled state. Just return cfg.DRKeyDB.Connection != "" would do.

---

go/cs/config/drkey.go, line 88 at r1 (raw file):

// Validate validates that all values are parsable.
func (cfg *DRKeyConfig) Validate() error {
	if !cfg.Enabled() {

Nit: I think it's preferable to validate all the provided values, even this is disabled.

[matzf]

Reviewable status: 67 of 139 files reviewed, 5 unresolved discussions (waiting on @juagargi)

---

proto/drkey/mgmt/v1/mgmt.proto, line 83 at r1 (raw file):

    google.protobuf.Timestamp epoch_end = 4;
    // Additional information (optional)
    bytes misc = 5;

Drop this?

[JordiSubira]

Reviewable status: 67 of 139 files reviewed, 5 unresolved discussions (waiting on @juagargi and @matzf)

---

go/cs/cs.go, line 427 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

It's not clear to me why this quicTLSServer (and e.g. the key/cert pair configuration) is required at all. Why can this not use the other infrastructure? Why are the key/cert pairs specific to DRKey?
If it is necessary, I'd argue that this code should be moved to e.g. go/lib/infra/infraenv/infraenv.go where the rest of the quic infrastructure "lives".

Currently the messaging stack used looks like gRPC/HTTP2/QUIC/UDP/SCION and the go library doesn't support http3, thus we cannot access the quic tls connection underneath. This quicTLSServer enables the access to the certificates which we want to check in both server and client sides. I agree this might look a little bit awkward but I already discuss this with Dominik from Anapaya and that's a solution we came up with.

Some changes have been introduced in infraenv.go to enable this new messaging stack, but if you think anything else can be moved there, we can discuss that, were you thinking sth in concrete?

[JordiSubira]

Reviewable status: 67 of 139 files reviewed, 5 unresolved discussions (waiting on @juagargi and @matzf)

---

go/cs/cs.go, line 427 at r1 (raw file):

Previously, JordiSubira wrote…

Currently the messaging stack used looks like gRPC/HTTP2/QUIC/UDP/SCION and the go library doesn't support http3, thus we cannot access the quic tls connection underneath. This quicTLSServer enables the access to the certificates which we want to check in both server and client sides. I agree this might look a little bit awkward but I already discuss this with Dominik from Anapaya and that's a solution we came up with.

Some changes have been introduced in infraenv.go to enable this new messaging stack, but if you think anything else can be moved there, we can discuss that, were you thinking sth in concrete?

Actually , taking a second look what makes sense to me is moving out the fileLoader struct at the bottom, do you have some suggestion?

---

proto/drkey/mgmt/v1/mgmt.proto, line 83 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Drop this?

This would differ from what's specified here https://scion.docs.anapaya.net/en/latest/cryptography/DRKeyInfra.html#second-level, eventhough, I don't think this is a major issue. @juagargi might want to have a word about that.

[JordiSubira]

Reviewable status: 63 of 139 files reviewed, 2 unresolved discussions (waiting on @matzf)

---

go/cs/cs.go, line 378 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: move into block below.

Done

---

go/cs/config/drkey.go, line 47 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

This config value appears to be unused.

Removed

---

go/cs/config/drkey.go, line 83 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: the way this is handled looks correct, but it would be clearer without the intermediate enabled state. Just return cfg.DRKeyDB.Connection != "" would do.

Done

---

go/cs/config/drkey.go, line 88 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: I think it's preferable to validate all the provided values, even this is disabled.

Done

[matzf]

Reviewed 2 of 139 files at r1, 4 of 4 files at r2.
Reviewable status: 69 of 139 files reviewed, 1 unresolved discussion (waiting on @juagargi)

---

go/cs/cs.go, line 427 at r1 (raw file):

Previously, JordiSubira wrote…

Actually , taking a second look what make sense to me is moving out the fileLoader struct at the bottom, do you have some suggestion?

Ok, I got the main point now: for DRKey, you want to check client and server TLS certificates against the TRC/control plane PKI. The naming with "TLS" does not make that at all clear to be honest -- quic always uses TLS so it just seems odd.

This infrastructure for a CPPKI-backed-TLS (if that's the right way to put it) appears to be more generic than its use here for DRKey. I think it would make sense to try to upstream this separately and immediately.
Looking at this again now, it's not quite as much code to be moved out as I thought there was. But it would seem to make sense would be to move the getCredentials and the srvConfig and the related logic to create a "TLSQUICDialer" / Service to libgrpc.

What is not quite clear to me yet is what key/cert pair is to be used for this. Looking e.g. through the verification code, I get the impression that this is the AS key/cert. But then, why the separate configuration entry?

Not blocking as you're trying to move forward quickly, but it would be nice if this could be improved in a follow up.

---

go/cs/config/drkey.go, line 83 at r1 (raw file):

Previously, JordiSubira wrote…

Done

Nit: just return cfg.DRKeyDB.Connection != ""?

Can you also remove the outdated TODO above it, please?

---

proto/daemon/v1/BUILD.bazel, line 12 at r2 (raw file):

        "@com_google_protobuf//:duration_proto",
        "@com_google_protobuf//:timestamp_proto",
        "//proto/drkey/mgmt/v1:drkey",

Must be first in list, buildifier complains "reformat listsort unsafesort".

---

proto/drkey/mgmt/v1/mgmt.proto, line 83 at r1 (raw file):

Previously, JordiSubira wrote…

This would differ from what's specified here https://scion.docs.anapaya.net/en/latest/cryptography/DRKeyInfra.html#second-level, eventhough, I don't think this is a major issue. @juagargi might want to have a word about that.

@juagargi? You said this was a left-over.
Again, not blocking -- if we accept breaking changes in the future, this can be removed in a follow up.

[juagargi]

Reviewable status: 69 of 139 files reviewed, 1 unresolved discussion (waiting on @juagargi)

---

proto/drkey/mgmt/v1/mgmt.proto, line 83 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

@juagargi? You said this was a left-over.
Again, not blocking -- if we accept breaking changes in the future, this can be removed in a follow up.

It is part of a non implemented feature. It is part of the spec. though, but doable in a second / third iteration over DRKey.

[matzf]

Reviewable status: 69 of 139 files reviewed, 3 unresolved discussions (waiting on @juagargi)

---

proto/drkey/mgmt/v1/mgmt.proto, line 54 at r2 (raw file):

        // Host address
        bytes host = 2;
    }

In other proto definitions, host addresses are simply defined as strings that are then parsed (no type field necessary). This doesn't seem obviously better, but consistency probably makes sense. Otherwise, type should be an enum.
Relatedly (but not very important perhaps), couldn't drkey handle the host addresses as opaque strings anyway? There does not seem to be a real reason to parse them (other than to validate the configuration etc.)?

(Not blocking, can be improved in a (protocol-breaking) follow up change.)

---

proto/drkey/mgmt/v1/mgmt.proto, line 58 at r2 (raw file):

    string protocol = 1;
    // Requested DRKeyProtoKeyType
    uint32 req_type = 2;

Both protocol and req_type should preferably be an enum in protobuf.

(Not blocking, can be improved in a (protocol-breaking) follow up change.)

[JordiSubira]

Reviewable status: 66 of 140 files reviewed, 3 unresolved discussions (waiting on @matzf)

---

go/cs/cs.go, line 427 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Ok, I got the main point now: for DRKey, you want to check client and server TLS certificates against the TRC/control plane PKI. The naming with "TLS" does not make that at all clear to be honest -- quic always uses TLS so it just seems odd.

This infrastructure for a CPPKI-backed-TLS (if that's the right way to put it) appears to be more generic than its use here for DRKey. I think it would make sense to try to upstream this separately and immediately.
Looking at this again now, it's not quite as much code to be moved out as I thought there was. But it would seem to make sense would be to move the getCredentials and the srvConfig and the related logic to create a "TLSQUICDialer" / Service to libgrpc.

What is not quite clear to me yet is what key/cert pair is to be used for this. Looking e.g. through the verification code, I get the impression that this is the AS key/cert. But then, why the separate configuration entry?

Not blocking as you're trying to move forward quickly, but it would be nice if this could be improved in a follow up.

Yes the idea of the name was stating that this stack was used in TLS/gRPC, we can choose some other name of course.

I discussed with Dominik having some things merge in a separate PR, but he suggested that we could try it out first and then merge.

I've moved out the getCredentials and the fileLoader to the trustlib, srvConfig looks quite straightforward to me, but if you think it is worth it to move it out, I can do it.

It wasn't clear for me either, at a high level I believe it could be any certificate valid according to the CPPKI for that AS with Client/ServerAuthenticate, that's why I put it separately. I guess we can take a further look if necessary.

---

go/cs/config/drkey.go, line 83 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: just return cfg.DRKeyDB.Connection != ""?

Can you also remove the outdated TODO above it, please?

Done

---

proto/daemon/v1/BUILD.bazel, line 12 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Must be first in list, buildifier complains "reformat listsort unsafesort".

Done

---

proto/drkey/mgmt/v1/mgmt.proto, line 54 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

In other proto definitions, host addresses are simply defined as strings that are then parsed (no type field necessary). This doesn't seem obviously better, but consistency probably makes sense. Otherwise, type should be an enum.
Relatedly (but not very important perhaps), couldn't drkey handle the host addresses as opaque strings anyway? There does not seem to be a real reason to parse them (other than to validate the configuration etc.)?

(Not blocking, can be improved in a (protocol-breaking) follow up change.)

I will write it down for the next iteration

---

proto/drkey/mgmt/v1/mgmt.proto, line 58 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Both protocol and req_type should preferably be an enum in protobuf.

(Not blocking, can be improved in a (protocol-breaking) follow up change.)

I will write it down for the next iteration

[matzf]

Reviewed 1 of 139 files at r1, 1 of 5 files at r3.
Reviewable status: 68 of 140 files reviewed, all discussions resolved (waiting on @matzf)

[FR4NK-W]

Reviewable status: 68 of 140 files reviewed, 1 unresolved discussion (waiting on @juagargi and @matzf)

---

go/lib/drkey/protocol/protocol_test.go, line 188 at r3 (raw file):

four

Two? Three?
What are the other two for besides scmp and piskes?

[juagargi]

LGTM

Reviewable status: 68 of 140 files reviewed, 1 unresolved discussion (waiting on @FR4NK-W and @matzf)

---

go/lib/drkey/protocol/protocol_test.go, line 188 at r3 (raw file):

Previously, FR4NK-W wrote…

four

Two? Three?
What are the other two for besides scmp and piskes?

We used to have four names in the past, being "standard" and "delegated" aliases for "scmp" and "piskes".
Fixed comment now.

[juagargi]

Reviewable status: 68 of 140 files reviewed, all discussions resolved (waiting on @matzf)

Comment cleared

[matzf]

Reviewable status: 68 of 140 files reviewed, all discussions resolved (waiting on @matzf)

[juagargi]

Reviewed 133 of 139 files at r1, 2 of 4 files at r2, 5 of 5 files at r3.
Reviewable status: 139 of 140 files reviewed, all discussions resolved (waiting on @juagargi)

[juagargi]

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @juagargi)

[matzf]

Reviewed 67 of 139 files at r1, 4 of 5 files at r3, 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @juagargi)