Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sled-agent] NTP zone config set up via zone-setup CLI #5440

Merged
merged 20 commits into from
Apr 21, 2024
Merged

[sled-agent] NTP zone config set up via zone-setup CLI #5440

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
11e39aa
Rename zone-network-cli to zone-setup-cli
karencfv Mar 14, 2024
746f36a
Configure zone-setup cli for ntp zone
karencfv Mar 14, 2024
44e5a91
Generate chrony config
karencfv Apr 2, 2024
64c6c6b
Check if file has changed
karencfv Apr 2, 2024
218f9b4
Merge main into repurpose-zone-network-cli
karencfv Apr 2, 2024
17fc8cd
temp lines in templates
karencfv Apr 4, 2024
19d47b5
enable new start method
karencfv Apr 4, 2024
bb15ccc
ntp start command works except for starting daemon bit
karencfv Apr 5, 2024
8afbf7b
Add new chrony-setup service
karencfv Apr 9, 2024
3cbf2cc
Working
karencfv Apr 9, 2024
45deb92
clean up files
karencfv Apr 10, 2024
f83fc3f
clean up
karencfv Apr 10, 2024
a2abd3b
Simplify code
karencfv Apr 10, 2024
a285290
Update smf/ntp/manifest/manifest.xml
karencfv Apr 10, 2024
91732b7
Address comments
karencfv Apr 15, 2024
dd6ab17
fmt
karencfv Apr 15, 2024
f2ffd52
consolidate file generation templates
karencfv Apr 18, 2024
ed87a35
Use uzers crate to retrieve uid and gid
karencfv Apr 18, 2024
ff9cc6a
Merge branch 'main' into repurpose-zone-network-cli
karencfv Apr 18, 2024
94a50d8
address comment
karencfv Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ members = [
"wicket",
"wicketd",
"workspace-hack",
"zone-network-setup",
"zone-setup",
]

default-members = [
Expand Down Expand Up @@ -158,7 +158,7 @@ default-members = [
"wicket-dbg",
"wicket",
"wicketd",
"zone-network-setup",
"zone-setup",
]
resolver = "2"

Expand Down Expand Up @@ -442,6 +442,7 @@ update-common = { path = "update-common" }
update-engine = { path = "update-engine" }
usdt = "0.5.0"
uuid = { version = "1.8.0", features = ["serde", "v4"] }
uzers = "0.11"
walkdir = "2.5"
whoami = "1.5"
wicket = { path = "wicket" }
Expand Down
1 change: 1 addition & 0 deletions illumos-utils/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub mod route;
pub mod running_zone;
pub mod scf;
pub mod svc;
pub mod svcadm;
pub mod vmm_reservoir;
pub mod zfs;
pub mod zone;
Expand Down
21 changes: 21 additions & 0 deletions illumos-utils/src/svcadm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

//! Utilities for manipulating SMF services.

use crate::zone::SVCADM;
use crate::{execute, ExecutionError, PFEXEC};

/// Wraps commands for interacting with svcadm.
pub struct Svcadm {}

#[cfg_attr(any(test, feature = "testing"), mockall::automock)]
impl Svcadm {
pub fn refresh_logadm_upgrade() -> Result<(), ExecutionError> {
let mut cmd = std::process::Command::new(PFEXEC);
let cmd = cmd.args(&[SVCADM, "refresh", "logadm-upgrade"]);
execute(cmd)?;
Ok(())
}
}
1 change: 1 addition & 0 deletions illumos-utils/src/zone.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use crate::dladm::{EtherstubVnic, VNIC_PREFIX_BOOTSTRAP, VNIC_PREFIX_CONTROL};
use crate::{execute, PFEXEC};
use omicron_common::address::SLED_PREFIX;

pub const CHRONYD: &str = "/usr/sbin/chronyd";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this line any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! no, I'll update

const DLADM: &str = "/usr/sbin/dladm";
pub const IPADM: &str = "/usr/sbin/ipadm";
pub const SVCADM: &str = "/usr/sbin/svcadm";
Expand Down
62 changes: 30 additions & 32 deletions package-manifest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ only_for_targets.image = "standard"
source.type = "composite"
source.packages = [
"omicron-nexus.tar.gz",
"zone-network-setup.tar.gz",
"zone-setup.tar.gz",
"zone-network-install.tar.gz",
"opte-interface-setup.tar.gz",
]
Expand Down Expand Up @@ -130,11 +130,7 @@ output.intermediate_only = true
service_name = "oximeter"
only_for_targets.image = "standard"
source.type = "composite"
source.packages = [
"oximeter-collector.tar.gz",
"zone-network-setup.tar.gz",
"zone-network-install.tar.gz",
]
source.packages = [ "oximeter-collector.tar.gz", "zone-setup.tar.gz", "zone-network-install.tar.gz" ]
output.type = "zone"

[package.oximeter-collector]
Expand All @@ -157,8 +153,8 @@ source.type = "composite"
source.packages = [
"clickhouse_svc.tar.gz",
"internal-dns-cli.tar.gz",
"zone-network-setup.tar.gz",
"zone-network-install.tar.gz",
"zone-setup.tar.gz",
"zone-network-install.tar.gz"
]
output.type = "zone"

Expand All @@ -183,8 +179,8 @@ source.type = "composite"
source.packages = [
"clickhouse_keeper_svc.tar.gz",
"internal-dns-cli.tar.gz",
"zone-network-setup.tar.gz",
"zone-network-install.tar.gz",
"zone-setup.tar.gz",
"zone-network-install.tar.gz"
]
output.type = "zone"

Expand All @@ -209,8 +205,8 @@ source.type = "composite"
source.packages = [
"cockroachdb-service.tar.gz",
"internal-dns-cli.tar.gz",
"zone-network-setup.tar.gz",
"zone-network-install.tar.gz",
"zone-setup.tar.gz",
"zone-network-install.tar.gz"
]
output.type = "zone"

Expand Down Expand Up @@ -245,8 +241,8 @@ source.type = "composite"
source.packages = [
"dns-server.tar.gz",
"internal-dns-customizations.tar.gz",
"zone-network-setup.tar.gz",
"zone-network-install.tar.gz",
"zone-setup.tar.gz",
"zone-network-install.tar.gz"
]
output.type = "zone"

Expand All @@ -257,7 +253,7 @@ source.type = "composite"
source.packages = [
"dns-server.tar.gz",
"external-dns-customizations.tar.gz",
"zone-network-setup.tar.gz",
"zone-setup.tar.gz",
"zone-network-install.tar.gz",
"opte-interface-setup.tar.gz",
]
Expand Down Expand Up @@ -298,10 +294,11 @@ service_name = "ntp"
only_for_targets.image = "standard"
source.type = "composite"
source.packages = [
"chrony-setup.tar.gz",
"ntp-svc.tar.gz",
"opte-interface-setup.tar.gz",
"zone-network-setup.tar.gz",
"zone-network-install.tar.gz",
"zone-setup.tar.gz",
"zone-network-install.tar.gz"
]
output.type = "zone"

Expand All @@ -311,8 +308,17 @@ only_for_targets.image = "standard"
source.type = "local"
source.paths = [
{ from = "smf/ntp/manifest", to = "/var/svc/manifest/site/ntp" },
{ from = "smf/ntp/method", to = "/var/svc/method" },
{ from = "smf/ntp/etc", to = "/etc" },
]
output.intermediate_only = true
output.type = "zone"

[package.chrony-setup]
service_name = "chrony-setup"
only_for_targets.image = "standard"
source.type = "local"
source.paths = [
{ from = "smf/chrony-setup/manifest.xml", to = "/var/svc/manifest/site/chrony-setup/manifest.xml" },
{ from = "smf/chrony-setup/etc", to = "/etc" },
]
output.intermediate_only = true
output.type = "zone"
Expand Down Expand Up @@ -457,23 +463,15 @@ output.intermediate_only = true
service_name = "crucible"
only_for_targets.image = "standard"
source.type = "composite"
source.packages = [
"crucible.tar.gz",
"zone-network-setup.tar.gz",
"zone-network-install.tar.gz",
]
source.packages = [ "crucible.tar.gz", "zone-setup.tar.gz", "zone-network-install.tar.gz" ]
output.type = "zone"


[package.crucible-pantry-zone]
service_name = "crucible_pantry"
only_for_targets.image = "standard"
source.type = "composite"
source.packages = [
"crucible-pantry.tar.gz",
"zone-network-setup.tar.gz",
"zone-network-install.tar.gz",
]
source.packages = [ "crucible-pantry.tar.gz", "zone-setup.tar.gz", "zone-network-install.tar.gz" ]
output.type = "zone"

# Packages not built within Omicron, but which must be imported.
Expand Down Expand Up @@ -746,11 +744,11 @@ source.paths = [
output.type = "zone"
output.intermediate_only = true

[package.zone-network-setup]
service_name = "zone-network-cli"
[package.zone-setup]
service_name = "zone-setup-cli"
only_for_targets.image = "standard"
source.type = "local"
source.rust.binary_names = ["zone-networking"]
source.rust.binary_names = ["zone-setup"]
source.rust.release = true
output.type = "zone"
output.intermediate_only = true
Expand Down
12 changes: 8 additions & 4 deletions sled-agent/src/services.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2003,7 +2003,7 @@ impl ServiceManager {
Self::dns_install(info, Some(dns_servers.to_vec()), domain)
.await?;

let mut ntp_config = PropertyGroupBuilder::new("config")
let mut chrony_config = PropertyGroupBuilder::new("config")
.add_property("allow", "astring", &rack_net)
.add_property(
"boundary",
Expand All @@ -2012,7 +2012,7 @@ impl ServiceManager {
);

for s in ntp_servers {
ntp_config = ntp_config.add_property(
chrony_config = chrony_config.add_property(
"server",
"astring",
&s.to_string(),
Expand All @@ -2027,13 +2027,17 @@ impl ServiceManager {
}

let ntp_service = ServiceBuilder::new("oxide/ntp")
.add_instance(
.add_instance(ServiceInstanceBuilder::new("default"));

let chrony_setup_service =
ServiceBuilder::new("oxide/chrony-setup").add_instance(
ServiceInstanceBuilder::new("default")
.add_property_group(ntp_config),
.add_property_group(chrony_config),
);

let mut profile = ProfileBuilder::new("omicron")
.add_service(nw_setup_service)
.add_service(chrony_setup_service)
.add_service(disabled_ssh_service)
.add_service(dns_install_service)
.add_service(dns_client_service)
Expand Down
0 smf/ntp/etc/logadm.d/chrony.logadm.conf → ...ony-setup/etc/logadm.d/chrony.logadm.conf
View file
Open in desktop
File renamed without changes.
46 changes: 46 additions & 0 deletions smf/chrony-setup/manifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?xml version="1.0"?>
<!DOCTYPE service_bundle SYSTEM "/usr/share/lib/xml/dtd/service_bundle.dtd.1">

<service_bundle type='manifest' name='chrony-setup'>

<service name='oxide/chrony-setup' type='service' version='1'>
<create_default_instance enabled='true' />

<dependency name='multi_user' grouping='require_all' restart_on='none'
type='service'>
<service_fmri value='svc:/milestone/multi-user:default' />
</dependency>

<exec_method type='method' name='start'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but you could choose to reduce privileges here to just what is needed for the service. It's only writing out some config files so there are a lot it doesn't need. I'm happy to help with that if you want to consider it.

Copy link
Contributor Author

@karencfv karencfv Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Not sure which permissions these would be though. I'm guessing file_chown_self, file_dac_search and file_dac_write; any others?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ppriv utility has a handy debugging mode to show you which privileges are missing.

root@oxz_ntp_cdc8cbd0:~# ppriv -De chown root:sys /etc/logadm.d/chrony.logadm.conf
chown[641328]: missing privilege "file_chown" (euid = 0, syscall = 16) needed at secpolicy_vnode_chown+0x33
chown: /etc/logadm.d/chrony.logadm.conf: Not owner

So we need at least basic,file_chown. Since /etc/inet and /etc/inet/chrony* are owned by root, that might actually be all.

The _dac_ privileges allow access to things you don't own.

Basic expands out to a few too, so:

root@oxz_ntp_cdc8cbd0:~# ppriv $$
641141: -bash
flags = PRIV_AWARE
        E: basic,file_chown
        I: basic,file_chown

is actually:

root@oxz_ntp_cdc8cbd0:~# ppriv -v $$
641141: -bash
flags = PRIV_AWARE
        E: basic_test,file_chown,file_link_any,file_read,file_write,net_access,proc_exec,proc_fork,proc_info,proc_secflags,proc_session
        I: basic_test,file_chown,file_link_any,file_read,file_write,net_access,proc_exec,proc_fork,proc_info,proc_secflags,proc_session

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for doing this part!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As always, thanks for the detailed information. That utility is really handy!

exec='/opt/oxide/zone-setup-cli/bin/zone-setup chrony-setup -b %{config/boundary} -s %{config/server} -a %{config/allow}'
timeout_seconds='0'>
<method_context security_flags="aslr">
<method_credential user="root" group="root"
privileges="basic,file_chown" />
</method_context>
</exec_method>


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ties into my comment on the dependency's restart_on value below.
What is the eventual plan for the control plane dynamically reconfiguring this service? We will at some point allow an operator to set a new config/server at least through the API.

If, for example, the control plane will update the config/server property, and then issue a refresh of this service, the refresh method should also run the same command as start to regenerate the files. In this case, I would set the restart_on property of the chrony-setup dependency in the ntp:default service to refresh (see comment below), so that a refresh of the chrony-setup service triggers a restart of chrony.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll change the file

<property_group name='startd' type='framework'>
<propval name='duration' type='astring' value='transient' />
</property_group>

<property_group name="config" type="application">
<propval name="boundary" type="boolean" value="false" />
<propval name="server" type="astring" value="" />
<propval name="allow" type="astring" value="" />
</property_group>

<stability value='Unstable' />

<template>
<common_name>
<loctext xml:lang='C'>Oxide Chrony Setup</loctext>
</common_name>
<description>
<loctext xml:lang='C'>Configures chronyd for the NTP zone</loctext>
</description>
</template>
</service>

</service_bundle>
32 changes: 0 additions & 32 deletions smf/ntp/etc/inet/chrony.conf.boundary
View file
Open in desktop

This file was deleted.

31 changes: 0 additions & 31 deletions smf/ntp/etc/inet/chrony.conf.internal
View file
Open in desktop

This file was deleted.

Loading
Loading