proto: enhance ntp configuration #70

christoph-zededa · 2024-10-11T13:19:06Z

allow to set more than one NTP server
allow to make NTP servers from cloud controller exclusively used, i.e. the NTP servers that got announced via DHCP are not used (in certain cases this an attacker might send DHCP responses with a different NTP server set and therefore can control time on EVE)

milan-zededa · 2024-10-11T17:02:22Z

proto/config/devconfig.proto

+// DhcpVendorExtensionsOverride is used to override or add dhcp vendor extensions like f.e.
+// the NTP servers - currently no other options are supported, but DNS servers might be a
+// good candidate in the future
+message DhcpVendorExtensionsOverride {


I think that terminology 'vendor extensions' is more used with BOOTP. For DHCP the RFC just refers to them as Options.
So what about calling this DhcpOptionsOverride?

I changed it to DhcpOptionsIgnore https://github.com/lf-edge/eve-api/pull/70/files#diff-35ea4ed74707f6ad6d17be169425a7acf4e25e09e9de81a0f7300ed80c5ec656R101

milan-zededa · 2024-10-11T17:04:37Z

proto/config/devconfig.proto

@@ -169,6 +169,14 @@ message EdgeDevConfig {

  // cluster configuration
  EdgeNodeCluster cluster = 43;
+  DhcpVendorExtensionsOverride dhcp_extensions_override = 44;


I think this should be part of NetworkConfig instead. Either inside ipspec or probaly better as another field.

You can have several NetworkConfigs - what is the meaning if you set it in one to true and in another one to false?
That means that for network device eth0 you can override the dhcp option and for another network device eth1 you don't override the dhcp option.
Is my interpretation correct?

Yes, that would be the preferred behavior.

milan-zededa · 2024-10-11T17:06:02Z

proto/config/devconfig.proto

+// the NTP servers - currently no other options are supported, but DNS servers might be a
+// good candidate in the future
+message DhcpVendorExtensionsOverride {
+  bool ntpServerExclusively = 1; // use exclusively specified NTP servers


Should this contain a list of NTP servers to configure or where do you plan to take the "specified NTP servers" from?

the NTP servers come from here: https://github.com/lf-edge/eve-api/pull/70/files#diff-b6af1b4801076bb7a7c0b5f919ecab887a34b218f6a5f2bfdffa40845a5e618dR769-R770

Currently they are only used if there is a static network configuration - I want to change it, to even use it if the network configuration is done via dhcp.

That link points to info.proto, i.e. status reported by device to zedcloud (we need to report these additional NTP servers, so that is correct).
But we will need such field also in this override structure or in ipspec which is used by NetworkConfig.

Thx. I added it here: https://github.com/lf-edge/eve-api/pull/70/files#diff-35ea4ed74707f6ad6d17be169425a7acf4e25e09e9de81a0f7300ed80c5ec656R86

eriknordmark · 2024-10-15T20:18:34Z

proto/config/netcmn.proto

@@ -84,11 +84,21 @@ message ipspec {
  string gateway = 5;
  string domain = 6;
  string ntp = 7;
+  repeated string moreNtp = 10;


How abot calling this more_ntp_servers and adding a comment that ntp+more_ntp_servers is used to specify a set of NTP servers. Also, can these be hostnames or IP addresses? Makes sense to state that in a comment.

Done - https://github.com/lf-edge/eve-api/pull/70/files#diff-35ea4ed74707f6ad6d17be169425a7acf4e25e09e9de81a0f7300ed80c5ec656R86

eriknordmark · 2024-10-15T20:19:26Z

proto/config/netcmn.proto

+// the NTP servers - currently no other options are supported, but DNS servers might be a
+// good candidate in the future
+message DhcpOptionsOverride {
+  bool ntpServerExclusively = 1; // use exclusively specified NTP servers


For these new fields it makes sense to follow the buflint about lower_snake_case.

eriknordmark · 2024-10-15T20:22:09Z

proto/config/netcmn.proto

+  DhcpOptionsOverride dhcp_options_override = 11;
+}
+
+// DhcpOptionsOverride is used to override or add dhcp options like f.e.


Are the semantics to override or to ignore? Stated differently, if there are no ntp servers specified but ntp_server_exclusively is true, will the result be that no NTP server configured? Or will the ntp_server_exclusively be ignored if no NTP server is configured in the API?

Makes sense to be explicit and make the name match the intended semantics.

I changed it to DhcpOptionsIgnore https://github.com/lf-edge/eve-api/pull/70/files#diff-35ea4ed74707f6ad6d17be169425a7acf4e25e09e9de81a0f7300ed80c5ec656R101

And I added a comment regarding what should happen if no ntp servers are specified: https://github.com/lf-edge/eve-api/pull/70/files#diff-35ea4ed74707f6ad6d17be169425a7acf4e25e09e9de81a0f7300ed80c5ec656R103

milan-zededa · 2024-11-11T16:43:16Z

proto/config/netcmn.proto

  repeated string dns = 8;

  // for IPAM management when dhcp is turned on.
  // If none provided, system will default pool.
  ipRange dhcpRange = 9;
+
+  DhcpOptionsIgnore dhcp_options_ignore = 11;


If I understand it correctly, there are 3 cases:

dhcp_options_ignore is not defined (nil in Go): use NTP from DHCP

dhcp_options_ignore is defined and ntp_server_exclusively is false: use NTP from both DHCP and the static config

dhcp_options_ignore is defined and ntp_server_exclusively is true: use NTP only from the static config

Correct?

I am not sure if we should put meaning in it if it is nil (because I don't know if other programming languages support this). Therefore if it is nil it should be handled as if the default value is set, i.e. false.
Therefore if dhcp_options_ignore is not defined, "use NTP from both DHCP and the static config".

If you want to use NTP from DHCP only, set ntp and more_ntp to empty strings.

Do we do this somewhere else? If yes, then it is perhaps better to do it the way you suggest.

I see. No, I'm not aware of any place where we would check if struct (proto message) is set or unset.
Your approach makes sense to me now. I would maybe just suggest to add a comment (next to dhcp_options_ignore ?) that even if DhcpType is Client, EVE still accepts static IP config and will merge it with the DHCP-provided one by default (and merge can be changed to override in dhcp_options_ignore).

In the protobuf wire encoding the fields with default values are not sent. Thus e.g., a boolean which is false, an integer of value zero, is not sent. As a result the receiver can not tell whether the sender explicitly set it to false or zero in those examples. I imagine this applies to a struct/embedded message as well; if all fields are zero it might not be sent at all.
I don't know if this is useful.

eriknordmark · 2024-11-18T06:37:35Z

proto/config/netcmn.proto

@@ -83,12 +83,28 @@ message ipspec {
  string subnet = 3;
  string gateway = 5;
  string domain = 6;
+  // ntp and more_ntp are used to specify several NTP servers


Presumably with ntp being the first and more_ntp being the rest aka the concatenation of ntp+more_ntp specifies all the NTP servers.

Hmm, I would not call it "all NTP servers" because there are possibly also NTP servers configured via DHCP.

Correct. But it would be helpful to have a comment that it uses the union of ntp and more_ntp as the set of statically/manually configured NTP servers. And it wouldn't hurt to point out that this is due to originally only having a field for a single server.

proto/config/netcmn.proto

1. allow to set more than one NTP server 2. allow to make NTP servers from cloud controller exclusively used, i.e. the NTP servers that got announced via DHCP are not used (in certain cases this an attacker might send DHCP responses with a different NTP server set and therefore can control time on EVE) Signed-off-by: Christoph Ostarek <christoph@zededa.com>

Signed-off-by: Christoph Ostarek <christoph@zededa.com>

eriknordmark

LGTM

Need to bump eve-api after following PRs lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

Need to bump eve-api after following PR lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa requested review from rouming and milan-zededa October 11, 2024 13:21

milan-zededa reviewed Oct 11, 2024

View reviewed changes

christoph-zededa force-pushed the ntp_configuration_enhancements branch 4 times, most recently from 883651e to 75711f7 Compare October 14, 2024 11:50

eriknordmark reviewed Oct 15, 2024

View reviewed changes

christoph-zededa force-pushed the ntp_configuration_enhancements branch 3 times, most recently from 38ad25c to 946ef0f Compare November 11, 2024 13:11

christoph-zededa changed the title ~~WIP: proto: enhance ntp configuration~~ proto: enhance ntp configuration Nov 11, 2024

christoph-zededa marked this pull request as ready for review November 11, 2024 13:34

milan-zededa reviewed Nov 11, 2024

View reviewed changes

christoph-zededa force-pushed the ntp_configuration_enhancements branch 2 times, most recently from 7c710c3 to 1ee75f8 Compare November 14, 2024 13:17

eriknordmark reviewed Nov 18, 2024

View reviewed changes

proto/config/netcmn.proto Show resolved Hide resolved

christoph-zededa added 2 commits November 25, 2024 11:27

Generated go,image,python files

cb98e8b

Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa force-pushed the ntp_configuration_enhancements branch from 1ee75f8 to cb98e8b Compare November 25, 2024 10:28

eriknordmark approved these changes Nov 25, 2024

View reviewed changes

eriknordmark merged commit b026f21 into lf-edge:main Nov 25, 2024
4 checks passed

christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Nov 26, 2024

Bump eve-api

eb484d9

Need to bump eve-api after following PRs lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Nov 26, 2024

Bump eve-api

564d0bc

Need to bump eve-api after following PRs lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Nov 26, 2024

Bump eve-api

06cbdea

Need to bump eve-api after following PR lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa mentioned this pull request Nov 26, 2024

Bump eve-api lf-edge/eve#4447

Merged

christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Nov 26, 2024

Bump eve-api

00dd7c4

Need to bump eve-api after following PR lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Nov 26, 2024

Bump eve-api

d396f91

Need to bump eve-api after following PR lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Nov 26, 2024

Bump eve-api

2ec1fb0

Need to bump eve-api after following PR lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Nov 26, 2024

Bump eve-api

fb2db68

Need to bump eve-api after following PR lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

eriknordmark pushed a commit to lf-edge/eve that referenced this pull request Nov 26, 2024

Bump eve-api

1b0e5af

Need to bump eve-api after following PR lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa added a commit to christoph-zededa/eve that referenced this pull request Nov 27, 2024

Bump eve-api

407bda5

Need to bump eve-api after following PR lf-edge/eve-api#70 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto: enhance ntp configuration #70

proto: enhance ntp configuration #70

christoph-zededa commented Oct 11, 2024

milan-zededa Oct 11, 2024

christoph-zededa Nov 11, 2024

milan-zededa Oct 11, 2024

christoph-zededa Oct 11, 2024

milan-zededa Oct 11, 2024

milan-zededa Oct 11, 2024

christoph-zededa Oct 11, 2024

milan-zededa Oct 11, 2024

christoph-zededa Nov 11, 2024

eriknordmark Oct 15, 2024

christoph-zededa Nov 11, 2024

eriknordmark Oct 15, 2024

christoph-zededa Nov 11, 2024

eriknordmark Oct 15, 2024

christoph-zededa Nov 11, 2024

milan-zededa Nov 11, 2024

christoph-zededa Nov 11, 2024

milan-zededa Nov 13, 2024 •

edited

Loading

eriknordmark Nov 18, 2024

eriknordmark Nov 18, 2024

christoph-zededa Nov 19, 2024

eriknordmark Nov 19, 2024

eriknordmark left a comment

proto: enhance ntp configuration #70

proto: enhance ntp configuration #70

Conversation

christoph-zededa commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milan-zededa Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknordmark left a comment

Choose a reason for hiding this comment

milan-zededa Nov 13, 2024 •

edited

Loading