protocol: update scion version and DRKey configuration #25

aaronbojarski · 2023-10-27T10:00:16Z

This change is

This PR updates the scion repository to the newest scionproto version.
As a consequence this includes updating a lot of configs and the DRKey derivation scheme.

Since AS-AS keys can no longer be fetched directly from the CS an option to configure shared keys in the LF config has been added. The fetching of HOST-AS and HOST-HOST keys will be added at a later point.
The DRKey timestamps and especially the SPAO header will be updated in a separate PR.

From the per peer preshared secret we derive a short term (3 day validity) AS-AS key. The derivation is done using AES-CBC MAC keyed with the shared secret and the input: (type | ISD_AS1 | ISD_AS2 | start timestamp) where

type is the 1 byte fixed constant 0.
ISD_AS1 is the ISD_AS number (8 byte) in network byte order of the fast side AS.
ISD_AS2 is the ISD_AS number (8 byte) in network byte order of the slow side AS.
start timestamp is the timestamp in ns (8 byte) of the start time of the validity period for the resulting key. This can be synchronized between peers since the initial start time is configured by both and consequent start times can be calculated by configured time + k * VALIDITY_PERIOD for some k such that the key is currently valid.

It is also possible to configure multiple shared secrets with different start time such that a shared secret can be replaced at some point.

…rong.

cs not starting. wrong static path in pyton file

tests still fail

aaronbojarski · 2023-11-17T13:48:25Z

This PR updates the scion repository to the newest scionproto version.
As a consequence this includes updating a lot of configs and the DRKey derivation scheme.

Since AS-AS keys can no longer be fetched directly from the CS an option to configure AS-AS keys in the LF config has been added.
The fetching of HOST-AS and HOST-HOST keys will be added at a later point.

The DRKey timestamps and especially the SPAO header will be updated in a separate PR.

aaronbojarski

Reviewable status: 26 of 40 files reviewed, 32 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/config.h line 53 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Sorry, I would say shared_secrets_configured_option here too.

Done.

src/config.c line 285 at r13 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Not being able to specify an empty list of shared secrets seems still a little inconsistent with, e.g., the peer list which can be empty as far as I can see. Would it make sense to set shared_secret_configured_option only if there is at least one entry in the list? This would then later serve as the flag on whether to use key fetching or the shared keys in the config.

If you think this clear enough then sure that works for me.
Done

src/config.c line 48 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

#define FIELD_SHARED_SECRETS "shared_secrets"

Done.

src/config.c line 258 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Empty line before this one.

Done.

src/keyfetcher.h line 52 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Wouldn't it be more consistent with the keymanager to have everywhere struct lf_keyfetcher *kf as the parameter name?

Sure. Done.

src/keyfetcher.h line 55 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Empty line before this one.

Done.

src/keyfetcher.c line 108 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment (redundant with following function name).

Done.

src/keyfetcher.c line 115 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment and formulate log message similar to keymanager messages:
LF_KEYFETCHER_LOG(ERR,
		"Fail to look up shared secret\n");

Done.

src/keyfetcher.c line 136 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove one empty line.

Done.

src/keyfetcher.c line 174 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove one empty line.

Done.

src/keyfetcher.c line 236 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Move this below variable decalrations, like in keymanager.

Done.

src/keyfetcher.c line 252 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove empty line.

Done.

src/keyfetcher.c line 268 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Just rte_free(, everywhere.

Done.

src/keyfetcher.c line 347 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Just rte_hash_free(.

Done.

src/keymanager.c line 117 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Why is + LF_DRKEY_PREFETCHING_PERIOD not needed anymore?

Sorry that got lost somehow. Added it again.

src/keymanager.c line 164 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

See question above.

Done.

src/keymanager.c line 399 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

I would move this up again, just to minimize the diff. I.e. put it just after
		if (res < 0) {
			dictionary_data->inbound_key.validity_not_after = 0;
		}
(A minimized diff will (hopefully) help tracking down potential errors, in case these changes here introduce new issues.)

Done.

src/keymanager.c line 400 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

And then remove the empty line before this statement.

Done.

src/keymanager.c line 480 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove to minimize diff.

Done.

src/mock/drkey_fetcher_mock.c line 45 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

{ 0 }

Done.

src/mock/drkey_fetcher_mock.c line 47 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

{ 0 }

Done.

src/mock/drkey_fetcher_mock.c line 99 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

{ 0 }

Done.

src/mock/drkey_fetcher_mock.c line 101 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

{ 0 }

Done.

src/test/keymanager_test.c line 429 at r13 (raw file):

Previously, marcfrei (Marc Frei) wrote…

I know, it's "just" test code, but this is still quite error prone. How about something along the following lines?
exit:
	if (config3 != NULL) {
		free(config3);
	}
	if (config1 != NULL) {
		free(config1);
	}
	if (km != NULL) {
		free_test_context(km)
	}
	return error_count;
}

Looks cleaner. Thanks. Done.

marcfrei

Reviewed 1 of 5 files at r17.
Reviewable status: 26 of 40 files reviewed, 42 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/keyfetcher.c line 68 at r18 (raw file):

	lf_crypto_drkey_derivation_step(drkey_ctx, &secret->key, buf,
			2 * LF_CRYPTO_CBC_BLOCK_SIZE, &key->key);

sizeof buf

Code quote:

	2 * LF_CRYPTO_CBC_BLOCK_SIZE

src/keyfetcher.c line 103 at r18 (raw file):

	} else {
		dict_key.as = src_ia;
	}

dict_key.as = src_ia == fetcher->src_ia ? dst_ia : src_ia;

Looks a little more idiomatic and matches better with the code in the other functions.

Code quote:

	if (src_ia == fetcher->src_ia) {
		dict_key.as = dst_ia;
	} else {
		dict_key.as = src_ia;
	}

src/keyfetcher.c line 115 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment and formulate log message similar to keymanager messages:
LF_KEYFETCHER_LOG(ERR,
		"Fail to look up shared secret\n");

It would also be good to include the src_ia and dst_ia info in the message, since this is most likely a configuration error that a human operator has to fix.

src/keyfetcher.c line 143 at r18 (raw file):

			(void **)&shared_secret_node);
	if (key_id >= 0) {
		// derive next key locally

Remove comment.

src/keyfetcher.c line 165 at r18 (raw file):

				rte_be_to_cpu_16(drkey_protocol), (int64_t)ms_valid,
				&validity_not_before_ms, &validity_not_after_ms, drkey_buf);
		key->validity_not_after =

Check and handle res here.

src/keyfetcher.c line 177 at r18 (raw file):

int
lf_keyfetcher_fetch_host_host_key(struct lf_keyfetcher *fetcher,

I like how this signature looks. Break up the above functions into lines like this one.

Code quote:

lf_keyfetcher_fetch_host_host_key(struct lf_keyfetcher *fetcher,

src/keyfetcher.c line 221 at r18 (raw file):

				rte_be_to_cpu_16(drkey_protocol), (int64_t)ms_valid,
				&validity_not_before_ms, &validity_not_after_ms, drkey_buf);
		key->validity_not_after =

Check and handle res here.

src/keyfetcher.c line 236 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Move this below variable decalrations, like in keymanager.

NOTICE instead of ERR

src/keyfetcher.c line 267 at r18 (raw file):

					rte_be_to_cpu_16(key_ptr->drkey_protocol));
			(void)rte_hash_del_key(fetcher->dict, key_ptr);
			// can be removed here since manager lock is beeing held

Looking at the keymanager code, I'm not sure about this. I think it would make more sense to pass the km's free_list to this function and let the km then later on free these entries.

src/keyfetcher.c line 288 at r18 (raw file):

							&shared_secret_data->secret_values[i].key);
				}
			}

Don't we have to remove the shared_secret entry from the dictionary, if it got removed from the config?

Code quote:

src/keyfetcher.c line 305 at r18 (raw file):

			// populate secret data and add to dict
			for (int i = 0; i < LF_CONFIG_SV_MAX; i++) {
				if (peer->shared_secrets[i].not_before == 0) {

The entire array peer->shared_secrets is correctly zero'ed out, correct? Then I would remove this optimization like above.

Code quote:

peer->shared_secrets

src/keyfetcher.c line 316 at r18 (raw file):

			res = rte_hash_add_key_data(fetcher->dict, &key,
					(void *)shared_secret_data);

All these casts to void * aren't really necessary but I guess we keep them for consistency, for now.

Code quote:

	(void *)

marcfrei

Reviewed 1 of 5 files at r18.
Reviewable status: 26 of 40 files reviewed, 44 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/drkey.h line 69 at r18 (raw file):

static inline void
lf_drkey_derive_host_as_from_as_as(struct lf_crypto_drkey_ctx *drkey_ctx,
		const struct lf_crypto_drkey *drkey_asas,

drkey_as_as

Code quote:

drkey_asas

src/drkey.h line 150 at r18 (raw file):

static inline void
lf_drkey_derive_host_host_from_as_as(struct lf_crypto_drkey_ctx *drkey_ctx,
		const struct lf_crypto_drkey *drkey_asas,

drkey_as_as

Code quote:

drkey_asas

marcfrei · 2024-01-24T12:13:07Z

src/config.c line 285 at r13 (raw file):

Previously, aaronbojarski (Aaron) wrote…

If you think this clear enough then sure that works for me.
Done

Ah, I'm not sure about the return 1 either.

I guess we have to better define what it means to have a "shared_secrets" field in the config. Does it mean that only the set of shared_secrets should be used, without ever attempting to fetch from the CS? If yes, just going zero times through the loop and returning 0 would be more consistent, in my opinion.

On the other hand, "shared_secrets" could also just provide some secrets and if none of them matches, LF should try to fetch from the CS.

marcfrei

Reviewable status: 26 of 40 files reviewed, 24 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/keyfetcher.c line 109 at r18 (raw file):

	if (key_id >= 0) {
		// derive next key locally
		res = lf_keyfetcher_derive_shared_key(&fetcher->drkey_ctx,

res < 0 means that we didn't find a shared secret, right? So we should probably log this too.

Code quote:

res

src/keyfetcher.c line 148 at r18 (raw file):

				&as_as_key);
		if (res < 0) {
			return res;

Should we try and fetch from the CS here too? Not sure.

src/keyfetcher.c line 201 at r18 (raw file):

				shared_secret_node, src_ia, dst_ia, drkey_protocol, ns_valid,
				&as_as_key);
		if (res < 0) {

Should we try and fetch from the CS here too? Not sure.

src/test/keymanager_test.c line 429 at r13 (raw file):

Previously, aaronbojarski (Aaron) wrote…

Looks cleaner. Thanks. Done.

All three variables have to be declared and initialized to NULL before the first goto. Otherwise, they are potentially uninitialized and we free up random pointers.

marcfrei

Reviewed 14 of 14 files at r19, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/keyfetcher.c line 148 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Should we try and fetch from the CS here too? Not sure.

See also comment in config.c

marcfrei

Reviewable status: all files reviewed, 24 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/config.c line 237 at r19 (raw file):

				error_count++;
			}
			ts_flag = true;

We should check here that not_before != 0. Otherwise we could miss later secrets in the list because of the not_before == 0 optimization in the keyfetcher.c. I think removing the not_before == 0 optimization would be more elegant, but not sure.

aaronbojarski

Reviewable status: 36 of 40 files reviewed, 23 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/drkey.h line 69 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

drkey_as_as

Done.

src/drkey.h line 150 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

drkey_as_as

Done.

src/keyfetcher.c line 68 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

sizeof buf

Done.

src/keyfetcher.c line 103 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

dict_key.as = src_ia == fetcher->src_ia ? dst_ia : src_ia;
Looks a little more idiomatic and matches better with the code in the other functions.

Done.

src/keyfetcher.c line 109 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

res < 0 means that we didn't find a shared secret, right? So we should probably log this too.

Done. Logging is in the deriving function.

src/keyfetcher.c line 115 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

It would also be good to include the src_ia and dst_ia info in the message, since this is most likely a configuration error that a human operator has to fix.

Done.

src/keyfetcher.c line 143 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment.

Done.

src/keyfetcher.c line 165 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Check and handle res here.

Done.

src/keyfetcher.c line 177 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

I like how this signature looks. Break up the above functions into lines like this one.

This is the result of the clang-format formatting. How important is this to you?

src/keyfetcher.c line 221 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Check and handle res here.

Done.

src/keyfetcher.c line 236 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

NOTICE instead of ERR

Done.

src/keyfetcher.c line 267 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Looking at the keymanager code, I'm not sure about this. I think it would make more sense to pass the km's free_list to this function and let the km then later on free these entries.

The fetching and config update functions of the keyfetcher are only called, when the manager lock is being held. Therefore the free list is not needed. Added additional comments to point that out.

src/keyfetcher.c line 288 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Don't we have to remove the shared_secret entry from the dictionary, if it got removed from the config?

If you mean removing when a peer is removed, this happens before.

If you mean removing one of the shared_secrets from an existing peer, then we do not need to remove it from the dictionary since we overwrite the old values with the new values.

src/keyfetcher.c line 305 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

The entire array peer->shared_secrets is correctly zero'ed out, correct? Then I would remove this optimization like above.

Yes it is zero'ed out. Okay. Done.

src/keymanager.c line 506 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

+1 for move to lf_keyfetcher_init.

Done.

src/test/keymanager_test.c line 429 at r13 (raw file):

Previously, marcfrei (Marc Frei) wrote…

All three variables have to be declared and initialized to NULL before the first goto. Otherwise, they are potentially uninitialized and we free up random pointers.

Done.

marcfrei · 2024-01-24T15:36:27Z

src/keyfetcher.c line 177 at r18 (raw file):

Previously, aaronbojarski (Aaron) wrote…

This is the result of the clang-format formatting. How important is this to you?

Ah, no. The formatter is always right, ofc.

marcfrei

Reviewed 3 of 4 files at r20, all commit messages.
Reviewable status: 39 of 40 files reviewed, 11 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/keyfetcher.c line 288 at r18 (raw file):

Previously, aaronbojarski (Aaron) wrote…

If you mean removing when a peer is removed, this happens before.

If you mean removing one of the shared_secrets from an existing peer, then we do not need to remove it from the dictionary since we overwrite the old values with the new values.

Nor completeley sure, but what happens if peer->shared_secrets_configured_option chnages from true to false in a config update?

fstreun

Reviewed 2 of 2 files at r2, 2 of 10 files at r15.
Reviewable status: 39 of 40 files reviewed, 8 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

aaronbojarski

Reviewable status: 37 of 40 files reviewed, 8 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/config.c line 285 at r13 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Ah, I'm not sure about the return 1 either.

I guess we have to better define what it means to have a "shared_secrets" field in the config. Does it mean that only the set of shared_secrets should be used, without ever attempting to fetch from the CS? If yes, just going zero times through the loop and returning 0 would be more consistent, in my opinion.

On the other hand, "shared_secrets" could also just provide some secrets and if none of them matches, LF should try to fetch from the CS.

I see. I assumed that whenever (at least one) shared_secrets are defined, then only those should be used.
I therefore changed the return statement as suggested.

If you disagree with this assumption we can discuss it again.

src/config.c line 237 at r19 (raw file):

Previously, marcfrei (Marc Frei) wrote…

We should check here that not_before != 0. Otherwise we could miss later secrets in the list because of the not_before == 0 optimization in the keyfetcher.c. I think removing the not_before == 0 optimization would be more elegant, but not sure.

I decided to remove the optimisation. I also switched the identifying of correctly set keys by checking if the value is different from the all zero key. I think this is anyways more reasonable since a value not_before == 0 would be possible, but a key value { 0 } does not make much sense for security reasons.

I therefore added a check here for secret_value != { 0 }.

src/keyfetcher.h line 52 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Ok, this minimizes the diff. At the same time, this interface will have to change again, once we are going to implement it also based on fetching keys from the CS instead of deriving them from shared secrets. Is this intended or should we already now offer only the future keyfetcher interface (lf_keyfetcher_fetch_host_as_key and lf_keyfetcher_fetch_host_host_key)? If no, add this task as an GitHub issue.

We discussed that to keep this PR smaller we will leave this task for a future PR.
I will open an Issue for it.

src/keyfetcher.h line 66 at r18 (raw file):

Previously, fstreun wrote…

Are all those functions used?

No not yet. Currently only the lf_keyfetcher_fetch_as_as_key is used.
I know that this is dead code right now. It should however change soon in a next PR.

src/keyfetcher.c line 148 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

See also comment in config.c

Depends on the same discussion as in config.c.

src/keyfetcher.c line 201 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

See also comment in config.c

See above

src/keyfetcher.c line 288 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Nor completeley sure, but what happens if peer->shared_secrets_configured_option chnages from true to false in a config update?

Ah yes I missed that case. Done.

src/lib/json-parser/lf_json_util.h line 282 at r18 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Replace atoi by lf_parse_unum or one of the strtoX functions with proper error handling?

Done.

aaronbojarski

Reviewable status: 34 of 40 files reviewed, 8 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/keyfetcher.h line at r18 (raw file):

Previously, fstreun wrote…

Could you please elaborate on why you added the keyfetcher.h and keyfetcher.c.
I am all good with dividing the keymanager into keymanager and keyfetcher, as long as the functional separation is clear.

Just from the name, I'd assume that keymanager manages the set of keys used by LF (i.e., used by the workers). The manager checks from time to time, which keys are outdated (or not there yet) and requests them through a keyfetcher.
The fetcher could get the key from the control service, or from the config.
Whether the keymanager or the fetcher implements some caching does not matter to me (just make it clear and don't do it at both to reduce complexity).

Please add the clarification as a comment to the header.

I added the keyfetcher since the keymanager got a bit complicated when adding the option of configuring keys in the config. The functional seperation is pretty much what you mentioned.
I envision some caching to be done by the keymanager.

I have also added clarification to the header.

marcfrei

Reviewed 2 of 3 files at r21, 4 of 4 files at r22, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/config.c line 360 at r22 (raw file):

		} else if (strcmp(field_name, FIELD_SHARED_SECRETS) == 0) {
			res = parse_shared_secret_list(field_value, peer->shared_secrets);
			if (res > 0) {

	if (res >= 0) {

For me it would be more consistent this way. The semantic would then be:

If there is an array value provided for the shared_secrets key in the config, we should use what's provided there and not fetch keys from CS. An empty array value would then be an edge case but still not lead to fetching the keys from CS, allowing it to locally overwrite keys that would be provided by CS with an empty set of keys.

If we (at some later point in time) want to support the possibility of having a shared_secrets key in the config and still having the keys be fetched from CS, we could express this as "shared_secrets": null. I.e., make a distinction between "shared_secrets": [] and "shared_secrets": null.

Code quote:

	if (res > 0) {

src/lib/json-parser/lf_json_util.h line 277 at r22 (raw file):

	}

	uint64_t pared_num;

parsed_num? I would probably call it just v (for val)

Code quote:

pared_num

aaronbojarski

Reviewable status: 38 of 40 files reviewed, 4 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

src/config.c line 360 at r22 (raw file):

Previously, marcfrei (Marc Frei) wrote…

	if (res >= 0) {
For me it would be more consistent this way. The semantic would then be:

If there is an array value provided for the shared_secrets key in the config, we should use what's provided there and not fetch keys from CS. An empty array value would then be an edge case but still not lead to fetching the keys from CS, allowing it to locally overwrite keys that would be provided by CS with an empty set of keys.

If we (at some later point in time) want to support the possibility of having a shared_secrets key in the config and still having the keys be fetched from CS, we could express this as "shared_secrets": null. I.e., make a distinction between "shared_secrets": [] and "shared_secrets": null.

Ah okay. Then I misunderstood you before. This is now consistent with what I had in mind.
Done.

src/lib/json-parser/lf_json_util.h line 277 at r22 (raw file):

Previously, marcfrei (Marc Frei) wrote…

parsed_num? I would probably call it just v (for val)

Done.

marcfrei

Reviewed 2 of 2 files at r23, all commit messages.
Reviewable status: complete! 1 of 1 LGTMs obtained

fstreun

Reviewed 1 of 4 files at r22.
Reviewable status: complete! 1 of 1 LGTMs obtained

src/keyfetcher.h line at r18 (raw file):

Previously, aaronbojarski (Aaron) wrote…

I added the keyfetcher since the keymanager got a bit complicated when adding the option of configuring keys in the config. The functional seperation is pretty much what you mentioned.
I envision some caching to be done by the keymanager.

I have also added clarification to the header.

Alright.

NIT:

How does this work together with the mock keyfetcher?

Any reason why you did not implement the keyfetcher for keys in the config separately? I'd imagine that you'd have multiple different keyfetchers (one that returns the keys from the discovery server, one that derives the keys from the config, one that just makes up keys, i.e., the mock keyfetcher) and then have the keymanatcher to decide which keyfetcher to use to for which AS?

marcfrei · 2024-02-02T19:00:55Z

src/keyfetcher.h line at r18 (raw file):

Previously, fstreun wrote…

Alright.

NIT:

How does this work together with the mock keyfetcher?

Any reason why you did not implement the keyfetcher for keys in the config separately? I'd imagine that you'd have multiple different keyfetchers (one that returns the keys from the discovery server, one that derives the keys from the config, one that just makes up keys, i.e., the mock keyfetcher) and then have the keymanatcher to decide which keyfetcher to use to for which AS?

As I see it, drkey_fetcher_mock.c will keep its role in the new design and the upcoming iterations (#39).

In the design proposed in this PR, key fetching happens in two steps:

First we check whether, there are any preconfigured secret values in the config for a given peer. If yes, these secret values are used to locally derive the DRKeys. If no, we proceed with step 2.
If there are no preconfigured secret values for a given peer, we fetch either the HOST-AS or the HOST-HOST key from the CS (supported with a yet to be designed local cache). This step will be implemented in the context of issue feature: add key fetching from new CS #39 by calling the corresponding Go library in drkey_fetcher_scion.c. To unit test this second step, we will still also need drkey_fetcher_mock.c which implements the same interface (drkey_fetcher.h) as drkey_fetcher_scion.c.

This two step approach is encapsulated in keyfetcher.c which is used by keymanager.c. At the moment, only step 1 is actually implemented. Step 2 will follow as soon as work on issue #39 starts.

marcfrei

Reviewable status: complete! all files reviewed, all discussions resolved

We agreed offline to merge the current state and re-evaluate the general approach and design again after work on issue #39 is further along.

Aaron Bojarski added 26 commits October 6, 2023 09:31

change scion to scionproto in go mod and update imports. Not working yet

62ba829

add dummy key to be able to compile

cc736aa

change topo gen script path

f2701d2

add functions to derive keys in new format. Not tested and probably w…

d1b0e7e

…rong.

update dockerfile

687216a

update go version

7d16a49

modify config for new scion.

ed5250e

cs not starting. wrong static path in pyton file

got control services starting

566953e

tests still fail

fix buffer overflow mistake in keymanager

2b8e920

format

0d94738

use relative path

c4ce846

add temporary logging to figure out why drkey derivation is wrong

3446827

improve logging

c680261

unpack IPv4 from IPv6 if needed

1c5a182

parametrize drkey protocol ID

310d39d

move logging

3bf609c

do explicit casting

bfe6cb6

format

3d6505c

add preconfigured AS_AS keys option

3f3b6de

skip update behaviour when using preconfigured keys

b0e6f2a

update config design

c437184

update doc

e73543d

Merge branch 'open-source' into scionproto

09d3bf0

deactivate key fetching

871dda9

cleanup

837cf70

remove logging

509b780

aaronbojarski marked this pull request as ready for review November 17, 2023 08:46

aaronbojarski changed the title ~~draft: update scion version and DRKey usage~~ update scion version and DRKey configuration Nov 17, 2023

aaronbojarski changed the title ~~update scion version and DRKey configuration~~ protocol: update scion version and DRKey configuration Nov 17, 2023

reduce diff

a8e96d3

aaronbojarski commented Jan 24, 2024

View reviewed changes

marcfrei requested changes Jan 24, 2024

View reviewed changes

abojarski added 2 commits January 24, 2024 13:49

keyfetcher improvements

78073da

move init call

8eb2e2c

aaronbojarski commented Jan 24, 2024

View reviewed changes

marcfrei requested changes Jan 24, 2024

View reviewed changes

fstreun reviewed Jan 28, 2024

View reviewed changes

abojarski added 3 commits January 31, 2024 09:03

improve config parsing

34bdd20

fix config update error

1d414e0

improve timestamp parsing

059a9a3

aaronbojarski commented Jan 31, 2024

View reviewed changes

add clarification

0edbcca

aaronbojarski commented Jan 31, 2024

View reviewed changes

marcfrei requested changes Feb 1, 2024

View reviewed changes

minor improvements

ce5a926

aaronbojarski commented Feb 2, 2024

View reviewed changes

marcfrei approved these changes Feb 2, 2024

View reviewed changes

fstreun reviewed Feb 2, 2024

View reviewed changes

marcfrei approved these changes Feb 7, 2024

View reviewed changes

marcfrei merged commit 04d9345 into netsec-ethz:open-source Feb 7, 2024
4 checks passed

aaronbojarski deleted the scionproto branch February 9, 2024 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protocol: update scion version and DRKey configuration #25

protocol: update scion version and DRKey configuration #25

aaronbojarski commented Oct 27, 2023 •

edited

Loading

aaronbojarski commented Nov 17, 2023 •

edited

Loading

aaronbojarski left a comment

marcfrei left a comment

marcfrei left a comment

marcfrei commented Jan 24, 2024

marcfrei left a comment

marcfrei left a comment

marcfrei left a comment

aaronbojarski left a comment

marcfrei commented Jan 24, 2024

marcfrei left a comment

fstreun left a comment

aaronbojarski left a comment

aaronbojarski left a comment

marcfrei left a comment

aaronbojarski left a comment

marcfrei left a comment

fstreun left a comment

marcfrei commented Feb 2, 2024

marcfrei left a comment

protocol: update scion version and DRKey configuration #25

protocol: update scion version and DRKey configuration #25

Conversation

aaronbojarski commented Oct 27, 2023 • edited Loading

aaronbojarski commented Nov 17, 2023 • edited Loading

aaronbojarski left a comment

Choose a reason for hiding this comment

marcfrei left a comment

Choose a reason for hiding this comment

marcfrei left a comment

Choose a reason for hiding this comment

marcfrei commented Jan 24, 2024

marcfrei left a comment

Choose a reason for hiding this comment

marcfrei left a comment

Choose a reason for hiding this comment

marcfrei left a comment

Choose a reason for hiding this comment

aaronbojarski left a comment

Choose a reason for hiding this comment

marcfrei commented Jan 24, 2024

marcfrei left a comment

Choose a reason for hiding this comment

fstreun left a comment

Choose a reason for hiding this comment

aaronbojarski left a comment

Choose a reason for hiding this comment

aaronbojarski left a comment

Choose a reason for hiding this comment

marcfrei left a comment

Choose a reason for hiding this comment

aaronbojarski left a comment

Choose a reason for hiding this comment

marcfrei left a comment

Choose a reason for hiding this comment

fstreun left a comment

Choose a reason for hiding this comment

marcfrei commented Feb 2, 2024

marcfrei left a comment

Choose a reason for hiding this comment

aaronbojarski commented Oct 27, 2023 •

edited

Loading

aaronbojarski commented Nov 17, 2023 •

edited

Loading