tailscale icon indicating copy to clipboard operation
tailscale copied to clipboard

util/syspolicy: implement a syspolicy store that reads settings from environment variables

Open nickkhyl opened this issue 1 year ago • 3 comments

In this PR, we implement (but do not use yet, pending #13727 review) a syspolicy/source.Store that reads policy settings from environment variables. It converts a CamelCase setting.Key, such as AuthKey or ExitNodeID, to a SCREAMING_SNAKE_CASE, TS_-prefixed environment variable name, such as TS_AUTH_KEY and TS_EXIT_NODE_ID. It then looks up the variable and attempts to parse it according to the expected value type. If the environment variable is not set, the policy setting is considered not configured in this store (the syspolicy package will still read it from other sources). Similarly, if the environment variable has an invalid value for the setting type, it won't be used (though the reported/logged error will differ).

Updates #13193 Updates #12687

nickkhyl avatar Oct 18 '24 15:10 nickkhyl

Can you hold on merging this until next week? One topic I wanted to talk to you about in Mexico was configfile-vs-MDM-env-vs-tailscale set --global or whatever.

It seems you're doing stuff that overlaps with all that, but I don't know what the overall plan is yet. Is there a design doc saying where this is all going?

bradfitz avatar Oct 18 '24 15:10 bradfitz

@bradfitz: merging this won't automatically make syspolicy use it on all platforms.

The primary driver for this change was to allow configuring syspolicy in integration tests. For example, see https://github.com/tailscale/tailscale/issues/13193, which @agottardo filed in response to https://github.com/tailscale/tailscale/pull/13061#pullrequestreview-2246415062

Any possibility we can get a test for this?

Like something in tstest/integration/... ?

I also think it would be beneficial for those using Linux, where we haven't had syspolicy support at all, as well as on other platforms, and would love to include it in 1.78 if possible. But then again, merging it != enabling it.

nickkhyl avatar Oct 18 '24 16:10 nickkhyl

Re: overall plan:

  • Make syspolicy the single source of truth for the enforced configuration (aka policy settings). This means we can likely deprecate/discontinue at least some methods of configuring Tailscale, especially as we expose more preferences via policy settings. This should be easier with strongly-typed policies (see below);
  • Enable support for multiple underlying policy sources and provide access to a merged/effective policy. The main driver for this is to support user-side policy settings on Windows (for settings that make sense in the user context, such as GUI visibility, access rights, etc). But this can also be useful in other cases and on all platforms, allowing us to use policy settings from environment variables, config files, and potentially ACL in the future (though this is out of scope and would require a design doc if/when we pursue it).
  • I don't have a strong opinion on which sources we should support for each platform. But I see value in supporting both environment variables and, say, a syspolicy.hujson file on Linux, as long as we can show the user where the policy setting originates. I also think there should be significant overlap between policy settings and preferences, as most of the items in ipn.Prefs should also be configurable via syspolicy. However, certain fields in ipn.Prefs represent states rather than user-configurable preferences, so they won't have corresponding policy settings. At the same time, certain policy settings, like permissions, visibility settings, and the list of allowed exit nodes, cannot (or shouldn't) be represented in ipn.Prefs.
  • Reload policy settings automatically as they change, without needing to restart tailscaled and the GUI. Reading them just once per process leads to issues (policy settings not taking effect as customers expect) and confusion (the GUI and tailscaled may become out of sync with both the actual policy settings and each other);
  • Provide visibility into the configured policy settings via CLI;
  • Push policy settings via notifications from LocalBackend to the GUI, where the received JSON with a merged/effective policy will serve as a policy source for the GUI once it's connected to the LocalAPI;
  • Make policy settings strongly typed and accessible via a Go struct, rather than as a collection of keys;
  • Use the metadata to generate/update ADMX/ADML files (policy definition files for Group Policy and MDM), docs, etc.

nickkhyl avatar Oct 18 '24 16:10 nickkhyl

I didn't think we were merging this.

I don't want all syspolicy settings to pollute the TS_FOO environment variable namespace on any platform, ever.

Please at least make them prefixed with TS_DEBUGSYSPOLICY_FOO_BAR instead of TS_FOO_BAR if you absolutely need environment variables for testing.

bradfitz avatar Oct 30 '24 16:10 bradfitz

This PR does not register the environment variable policy source on any platform, therefore I believe it does not pollute anything. We can decide when to use it (and enable/register it explicitly), and I don’t think there were any disagreements about using it in tests?

nickkhyl avatar Oct 30 '24 16:10 nickkhyl

This PR does not register the environment variable policy source on any platform

Yet. :)

I sent https://github.com/tailscale/tailscale/pull/13967 to make me feel more comfortable.

bradfitz avatar Oct 30 '24 16:10 bradfitz