-
Notifications
You must be signed in to change notification settings - Fork 566
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.
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
interfaces/builtin: support time and date settings via 'org.freedesktop.timedate1 #1832
Conversation
member="Set{Time,Timezone,LocalRTC,NTP}" | ||
peer=(label=unconfined), | ||
|
||
dbus (receive, send) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why send and receive? // my knowledge of apparmor is rusty here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. receive is needed for property change signals (and only for them) and it was a little bit too lenient. I've split this block into two and added comments - property change signals need 'receive', Get & GetAll need 'send'.
Please document this interface so that we know what it allows to do (in I have one question about apparmor, please ask @jdstrand for a review after you know the answer to my question. |
@zyga I've added a short doc and made it auto-connect. I'm not sure however about the making it 'reserved', is it about marking it reservedForOs (which it is already)? |
Needs conflict solving, and a review from @jdstrand. LGTM other than that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR should be renamed as "support time and date settings via 'org.freedesktop.timedate1" and you should add the timedate-control interface (please rename from time-date-control to timedate-control) and update the timezone-control and timeserver-control as described (each should work without the others, so add the seccomp and dbus introspection rules to all 3).
connectedPlugAppArmor: timeDateControlConnectedPlugAppArmor, | ||
connectedPlugSecComp: timeDateControlConnectedPlugSecComp, | ||
reservedForOS: true, | ||
autoConnect: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusting the time on a system is a privileged action and this should not autoconnect.
path=/org/freedesktop/timedate1 | ||
interface=org.freedesktop.DBus.Properties | ||
member=PropertiesChanged | ||
peer=(label=unconfined), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we can't mediate on method data and systemd only provides Get methods for properties, so you'll need to have these two rules in timezone-control and timeserver-control. This means that all the time* interfaces will be able to read each others properties, but at least they won't be able to Set them.
bus=system | ||
path=/org/freedesktop/timedate1 | ||
interface=org.freedesktop.timedate1 | ||
member="Set{Time,Timezone,LocalRTC,NTP}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the timeserver-control interface for NTP. NTP calls should be moved there.
We already have the timezone-control interface for adjusting TZ. TZ calls should be moved there.
SetTime and SetLocalRTC can remain here.
'time-control' is probably a better name than timedate-control. Will want @niemeyer to weigh in on that. |
@jdstrand Thanks for the review, I have followed your suggestions and moved some permissions to the other interfaces. All interfaces have permissions to read the properties as well as to introspect timedate1 object. |
PanicMatches, `plug is not of interface "timedate-control"`) | ||
} | ||
|
||
func (s *TimeDateControlTestInterfaceSuite) TestUnusedSecuritySystems(c *C) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's drop this test please. We just concluded a large simplification where those tests were axed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on security policy changes. Please adjust docs/interfaces.md and implicit.go
I think that 'time-control' would be a nicer name for this interface. @niemeyer, can you comment on if you prefer 'timedate-control' or 'time-control' (or something else)?
### timedate-control | ||
|
||
Can modify time/date exposed via D-Bus by systemd-timedated and read all | ||
all properties of /org/freedesktop/timedate1 D-Bus object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rephrase this to be:
Can set system time and date and query systemd-timedated for time information.
It's possible we may allow setting the time or the date in other ways (eg, like we do with timezone-control and timeserver-control).
all properties of /org/freedesktop/timedate1 D-Bus object. | ||
|
||
* Auto-Connect: yes | ||
|
||
### timeserver-control | ||
|
||
Can manage timeservers directly separate from ``config core``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update this to be:
Can manage timeservers via systemd-timedated and directly separate from ``config core``
### timezone-control
Can manage timezone via systemd-timedated and directly separate from ``config core``
* Auto-Connect: no
In other words, update timeserver-control to reference systemd-timedated and add timezone-control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdstrand apologies for forgetting about this change, I'll address it in a separete PR.
@@ -54,6 +54,7 @@ var implicitSlots = []string{ | |||
"timezone-control", | |||
"tpm", | |||
"kernel-module-control", | |||
"timedate-control", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put this in alphabetical order?
Note that I approved the changes you made, but still request further changes (using github's 'comment' mechanism instead of 'request changes' to avoid blocking on me). |
+1 on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once @jdstrand is happy.
"+1 on time-control if we intend this interface to remain intermediating the ability to set the time even if we change the backend." Yes this is the intent. In this manner it is similar to timezone-control and timeserver-control. @stolowski - we now have agreement. Please rename to 'time-control' and I can do the final review. |
FYI, the request to add |
Imported using git-ubuntu import. Changelog parent: d92891f New changelog entries: * New upstream release, LP: #1637215: - release: os-release on core has changed - tests: /dev/ptmx does not work on powerpc, skip here - docs: moved to github.com/snapcore/snapd/wiki (snapcore#2258) - debian: golang is not installable on powerpc, use golang-any * New upstream release, LP: #1637215: - overlord/ifacestate: add unit tests for undo of setup-snap- security (snapcore#2243) - daemon,overlord,snap,tests: download to .partial in final dir (snapcore#2237) - overlord/state: marshaling tests for lanes (snapcore#2245) - overlord/state: introduce state lanes (snapcore#2241) - overlord/snapstate: fix revert+refresh (snapcore#2224) - interfaces/sytemd: enable/disable generated service units (snapcore#2229) - many: fix incorrect security files generation on undo - overlord/snapstate: add dynamic snapdX.Y assumes (snapcore#2227) - interfaces: network-manager: give slot full read-write access to /run/NetworkManager - docs: update the name of the command for the cross-build - overlord/snapstate: fix missing argument to Noticef - snapstate: ensure gadget/core/kernel can not be disabled (snapcore#2218) - asserts: limit to 1y only if len(models) == 0 (snapcore#2219) - debian: only install share/locale if available (missing on powerpc) - overlrod/snapstate: fix revert followed by refresh to old-current (snapcore#2214) - interfaces/builtin: network-manager and bluez can change hostname (snapcore#2204) - snap: switch the auto-import dir to /run/snapd/auto-import - docs: less details about cloud.cfg as requested in trello (snapcore#2206) - spread.yaml: Ensure ubuntu user has passwordless sudo for autopkgtests (snapcore#2201) - interfaces/builtin: add dcdbas-control interface - boot: do not set boot to try mode if the revision is unchanged - interfaces: add shutdown interface (snapcore#2162) - interfaces: add system-power-control interface - many: use the new systemd backend for configuring GPIOs - overlord/ifacestate: setup security for slots before plugs - snap: spool assertion candidates if snapd is not up yet - store,daemon,overlord: download things to a partials dir - asserts,daemon: implement system-user-authority header/concept - interfaces/builtin: home base declaration rule using on-classic for its policy - interfaces/builtin: finish decl based checks - asserts: bump snap-declaration to allow signing with new-style plugs and slots - overlord: checks for kernel installation/refresh based on model assertion and previous kernel - tests/lib/fakestore: fix logic to distinguish assertion not found errors - client: add a few explicit error types (around the request cycle) - tests/lib/fakestore/cmd/fakestore: make it log, and fix a typo - overlord/snapstate: two bugs for one - snappy: disable auto-import of assertions on classic (snapcore#2122) - overlord/snapstate: move trash cleanup to a cleanup handler (snapcore#2173) - daemon: make create-user --known fail on classic without --force- managed (snapcore#2123) - asserts,interfaces/policy: implement on-classic plug/slot constraints - overlord: check that the first installed gadget matches the model assertion - tests: use the snapd-control-consumer snap from the store - cmd/snap: make snap run not talk to snapd for finding the revision - snap/squashfs: try to hard link instead of copying. Also, switch to osutil.CopyFile for cp invocation. - store: send supported max-format when retrieving assertions - snapstate, devicestate: do not remove seed - boot,image,overlord,partition: read/write boot variables in single operation - tests: reenable ubuntu-core tests on qemu - asserts,interfaces/policy: allow OR-ing of subrule constraints in plug/slot rules - many: move from flags as ints to flags as structs-of-bools (snapcore#2156) - many: add supports for keeping and finding assertions with different format iterations - snap: stop using ubuntu-core-launcher, use snap-confine - many: introduce an assertion format iteration concept, refuse to add unsupported assertion - interfaces: tweak wording and comment - spread.yaml: dump apparmor denials on spread failure - tests: unflake ubuntu-core-reboot (snapcore#2150) - cmd/snap: tweak unknown command error message (snapcore#2139) - client,daemon,cmd: add payment-declined error kind (snapcore#2107) - cmd/snap: update remove command help (snapcore#2145) - many: removed frameworks target and fixed service files (snapcore#2138) - asserts,snap: validate attributes to a JSON-compatible type subset (snapcore#2140) - asserts: remove unused serial-proof type - tests: skip auto-import tests on systems without test keys (snapcore#2142) - overlord/devicestate: don't spam the debug log on classic (snapcore#2141) - cmd/snap: simplify auto-import mountinfo parsing (snapcore#2135) - tests: run ubuntu-core upgrades on isolated machine (snapcore#2137) - overlord/devicestate: recover seeding from old external approach (snapcore#2134) - overlord: merge overlord/boot pkg into overlord/devicestate (snapcore#2118) - daemon: add postCreateUserSuite test suite (snapcore#2124) - tests: abort tests if an update process is scheduled (snapcore#2119) - snapstate: avoid reboots if nothing in the boot setup has changed (snapcore#2117) - cmd/snap: do not auto-import from loop or non-dev devices (snapcore#2121) - tests: add spread test for `snap auto-import` (snapcore#2126) - tests: add test for auto-mount assertion import (snapcore#2127) - osutil: add missing unit tests for IsMounted (snapcore#2133) - tests: check for failure creating user on managed ubuntu-core systems (snapcore#2096) - snap: ignore /dev/loop addings from udev (snapcore#2111) - tests: remove snapd.boot-ok reference (snapcore#2109) - tests: enable tests related to the home interface in all-snaps (snapcore#2106) - snapstate: only import defaults from gadget on install (snapcore#2105) - many: move firstboot code into the snapd daemon (snapcore#2033) - store: send correct JSON type of string for expected payment amount (snapcore#2103) - cmd/snap: rename is-managed to managed and tune (snapcore#2102) - interfaces,overlord/ifacestate: initial cleaning up of no arg AutoConnect related bits (snapcore#2090) - client, cmd: prompt for password when buying (snapcore#2086) - snapstate: fix hanging `snap remove` if snap is no longer mounted - image: support gadget specific cloud.conf file (snapcore#2101) - cmd/snap,ctlcmd: fix behavior of snap(ctl) get (snapcore#2093) - store: local users download from the anonymous url (snapcore#2100) - docs/hooks.md: fix typos (snapcore#2099) - many: check installation of slots and plugs against declarations - docs: fix missing "=" in the systemd-active docs - store: do not set store auth for local users (snapcore#2092) - interfaces,overlord/ifacestate: use declaration-based checking for auto-connect (snapcore#2071) - overlord, daemon, snap: support gadget config defaults (snapcore#2082)The main semantic changes are: - tests: fix snap-disconnect tests after core rename (snapcore#2088) - client,daemon,overlord,cmd: add /v2/users and create-user on auto- import (snapcore#2074) - many: abbreviated forms of disconnect (snapcore#2066) - asserts: require lowercase model until insensitive matching is ready (snapcore#2076) - cmd/snap: add version command, same as --version (snapcore#2075) - all: use "core" by default but allow "ubuntu-core" still (snapcore#2070) - overlord/devicestate, docs/hooks.md: nest prepare-device configuration options - daemon: fix login API to return local macaroons (snapcore#2078) - daemon: do not hardcode UID in userLookup (snapcore#2080) - client, cmd: connect fixes (snapcore#2026) - many: preparations for switching most of autoconnect to use the declarationsfor now: - overlord/auth: update CheckMacaroon to verify local snapd macaroons (snapcore#2069) - cmd/snap: trivial auto-import and download tweaks (snapcore#2067) - interfaces: add repo.ResolveConnect that handles name resolution - interfaces/policy: introduce InstallCandidate and its checks - interfaces/policy,overlord: check connection requests against the declarations in ifacestate - many: setup snapd macaroon for local users (snapcore#2051)Next step: do snapd macaroons verification. - interfaces/policy: implement snap-id/publisher-id checks - many: change Connect to take ConnRef instead of strings (snapcore#2060) - snap: auto mount block devices and import assertions (snapcore#2047) - daemon: add `snap create-user --force-managed` support (snapcore#2041) - docs: remove references to removed buying features (snapcore#2057) - interfaces,docs: allow sharing SNAP{,_DATA,_COMMON} via content iface (snapcore#2063) - interfaces: add Plug/Slot/Connection reference helpers (snapcore#2056) - client,daemon,cmd/snap: improve create-user APIs (snapcore#2054) - many: introduce snap refresh --ignore-validation <snap> to override refresh validation (snapcore#2052) - daemon: add support for `snap create-user --known` (snapcore#2040) - interfaces/policy: start of interface policy checking code based on declarations (snapcore#2050) - overlord/configstate: support nested configuration (snapcore#2039) - asserts,interfaces/builtin,overlord/assertstate: introduce base- declaration (snapcore#2037) - interfaces: builtin: Allow writing DHCP lease files to /run/NetworkManager/dhcp (snapcore#2049) - many: remove all traces of the /v2/buy/methods endpoint (snapcore#2045) - tests: add external spread backend (snapcore#1918) - asserts: parse the slot rules in snap-declarations (snapcore#2035) - interfaces: allow read of /etc/ld.so.preload by default for armhf on series 16 (snapcore#2048) - store: change purchase to order and store clean up first pass (snapcore#2043) - daemon, store: switch to new store APIs in snapd (snapcore#2036) - many: add email to UserState (snapcore#2038) - asserts: support parsing the plugs stanza i.e. plug rules in snap- declarations (snapcore#2027) - store: apply deltas if explicitly enabled (snapcore#2031) - tests: fix create-key/snap-sign test isolation (snapcore#2032) - snap/implicit: don't restrict the camera iface to clasic (snapcore#2025) - client, cmd: change buy command to match UX document (snapcore#2011) - coreconfig: nuke it. Also, ignore po/snappy.pot. (snapcore#2030) - store: download deltas if explicitly enabled (snapcore#2017) - many: allow use of the system user assertion with create-user (snapcore#1990) - asserts,overlord,snap: add prepare-device hook for device registration (snapcore#2005) - debian: adjust packaging for trusty/deputy systemd (snapcore#2003) - asserts: introduce AttributeConstraints (snapcore#2015) - interface/builtin: access system bus on screen-inhibit-control - tests: add firewall-control interface test (snapcore#2009) - snapstate: pass errors from ListRefresh in updateInfo (snapcore#2018) - README: add links to IRC, mailing list and social media (snapcore#2022) - docs: add `configure` hook to hooks list (snapcore#2024)LP: #1596629 - cmd/snap,configstate: rename apply-config variables to configure. (snapcore#2023) - store: retry download on 500 (snapcore#2019) - interfaces/builtin: support time and date settings via 'org.freedesktop.timedate1 (snapcore#1832)
Added new timedate-control interface and updated two existing interfaces: timeserver-control and timezone-control to allow access to respective parts of systemd' org.freedesktop.timedate1 interface (lp #1616052).
The test shamelessly stolen from unity7_test.go.
Tested introspection and a property getter manually with:
$ timedatetest.gdbus introspect --system --dest org.freedesktop.timedate1 --object-path /org/freedesktop/timedate1
$ timedatetest.gdbus call --system --dest org.freedesktop.timedate1 --object-path /org/freedesktop/timedate1 --method org.freedesktop.DBus.Properties.Get "org.freedesktop.timedate1" "Timezone"
with the following simple snap:
name: timedatetest
version: 1.0
summary: A test
description: A test
apps:
gdbus:
command: usr/bin/gdbus
plugs: [timedate-control]
parts:
gdbus:
plugin: nil
stage-packages:
- libglib2.0-bin