The main server file mixed portable session handling with Unix details:
sending SIGHUP to end a session, decoding exec.ExitError, the euid check
for whether the process can switch users, agent forwarding's chown of the
socket, and reading /etc/ssh host keys as root. Those now sit behind small
functions (hangupProcess, waitProcess, canSwitchToLocalUser,
handleSSHAgentForwarding, systemHostKeyFile, isRootUser) in the new
process_unix.go, along with the incubator's forwarded-environment pipe
helpers, and the session's *exec.Cmd moves into an embedded osSessionState
struct defined there, so that the portable code no longer refers to the
process representation at all.
user.go keeps only the portable userMeta and userLookup; the login shell
and default PATH logic moves to user_unix.go. The SFTP child entrypoint and
its stdio adapter, which incubator.go and incubator_plan9.go each had a
copy of, move to sftp.go. The c2n usernames handler gains a hook for
platforms that list users some other way than /etc/passwd.
The agent socket's uid and gid are parsed as 31-bit rather than 32-bit
unsigned values so that the conversion to int for os.Chown cannot
overflow on 32-bit platforms, which is the pattern CodeQL flags.
Updates #cleanup
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Change-Id: I4c7e2b9a0d3f5e1c8b6a4d2f0e9c7b5a3d1f8e6c
The ts_omit_<name> build tags omit a feature at build time; there has
been no way to do the same at runtime. Some users (either proactively
or in response to a security announcement) might like a way to disable
a feature that's linked-in in their binaries that they're not using.
Then a mitigation announcement can say "set this env var" without
asking users to rebuild or wait for a new release.
This adds env var TS_DISABLE_FEATURE, a comma-separated list of
feature names to disable, and the listed set is reported by the
debug-optional-features LocalAPI endpoint next to the registered set.
The legacy per-feature knobs such as TS_DISABLE_SSH_SERVER and
TS_DISABLE_TAILDROP keep working independently.
A disabled feature behaves as if it had not been linked: it is absent
from feature.IsRegistered, its hooks are unset, and its extensions and
handlers are not registered. Three pieces make that happen:
* feature.Register now returns bool, false when disabled, and
feature packages gate their registration init on it. It was added
to the feature packages that never called it (including taildrop
and ssh), which also completes the picture reported by
debug-optional-features. taildrop, routecheck, favorites, and
serviceclientprefs had registration split across several inits and
now register from one gated init.
* ipnext.RegisterExtension ignores a disabled feature's extension.
* feature.Hook.Set and feature.Hooks.Add walk the call stack and
silently skip when the calling package under feature/<name> is
disabled. This covers sub-packages such as
feature/captiveportal/netcheckhook, which cannot call Register
themselves without colliding with their parent, and future
packages whose authors forget the gate.
ssh/tailssh's registrations moved from its inits into tailssh.Register,
called from feature/ssh's gated init. The aws and kube state stores and
syspolicy's Windows store registration are gated too.
feature/register_disable_test.go runs this test binary as a child
process (it links condregister, as tailscaled does) with
TS_DISABLE_FEATURE set to every registered feature at once, and fails
if any of them register anyway, so a feature that ignores the variable
cannot land.
Updates #12614
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Change-Id: I720af6ccab844ae060a9dfd1539fee577fd483e3
Previously the acceptEnv variables forwarded to the incubator child were
JSON-encoded onto its command line (--encoded-env), so their values were
visible in /proc/<pid>/cmdline to any other local user and were logged in
the session-start argv (locally and to log.tailscale.com except where
--no-logs-no-support was specified).
This change now carries those variables through an os.Pipe file
descriptor as a json encoded payload. Added end-to-end testing
helps validate secrets reach the session but are not in flags or logged.
Fixestailscale/corp#44903
Change-Id: I5b137b20e9c06feec6b70aaf4e6925e6db74017e
Signed-off-by: Mike Jensen <mikej@tailscale.com>
Co-authored-by: Mike Jensen <mikej@tailscale.com>
Add two narrower accessors alongside the existing
[LocalBackend.NetMap], with docs that distinguish their semantics:
- NetMapNoPeers: cheap (returns the cached *netmap.NetworkMap with
a possibly-stale Peers slice). For callers that only read non-Peers
fields like SelfNode, DNS, PacketFilter, capabilities.
- NetMapWithPeers: documented as returning an up-to-date Peers slice.
For callers that genuinely need to iterate Peers or call
PeerByXxx.
Mark the existing NetMap deprecated and point readers at the two new
accessors. NetMap, NetMapNoPeers, and NetMapWithPeers all currently
return the same value (b.currentNode().NetMap()): this commit is a
no-op behaviorally, just a renaming and migration of in-tree callers.
A subsequent change in the same series will switch
NetMapWithPeers to actually rebuild the Peers slice from the live
per-node-backend peers map (O(N) per call), at which point the
distinction between the two new accessors becomes load-bearing.
Migrate in-tree callers to the appropriate accessor based on what
fields they read:
- NetMapNoPeers (most common): localapi handlers, peerapi accept,
GetCertPEMWithValidity, web client noise request, doctor DNS
resolver check, tsnet CertDomains/TailscaleIPs, ssh/tailssh
SSH-policy/cap reads, several LocalBackend internals
(isLocalIP, allowExitNodeDNSProxyToServeName, pauseForNetwork
nil-check, serve config).
- NetMapWithPeers: writeNetmapToDiskLocked (persist full netmap to
disk for fast restart), PeerByTailscaleIP lookup.
Tests still call the legacy NetMap; they'll see the deprecation
warning but otherwise behave identically.
Also add two pieces of plumbing the next change in this series will
need, but which are already useful on their own:
- [client/local.GetDebugResultJSON]: a generic [Client.DebugResultJSON]
that decodes directly into a target type T, avoiding the
marshal/unmarshal roundtrip callers otherwise need.
- localapi "current-netmap" debug action: returns the current
netmap (with peers) as JSON. Documented as debug-only — the
netmap.NetworkMap shape is internal and may change without notice.
This commit is part of a series breaking up a larger change for
review; on its own it is a no-op refactor.
Updates #12542
Change-Id: Idbb30707414f8da3149c44ca0273262708375b02
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This file was never truly necessary and has never actually been used in
the history of Tailscale's open source releases.
A Brief History of AUTHORS files
---
The AUTHORS file was a pattern developed at Google, originally for
Chromium, then adopted by Go and a bunch of other projects. The problem
was that Chromium originally had a copyright line only recognizing
Google as the copyright holder. Because Google (and most open source
projects) do not require copyright assignemnt for contributions, each
contributor maintains their copyright. Some large corporate contributors
then tried to add their own name to the copyright line in the LICENSE
file or in file headers. This quickly becomes unwieldy, and puts a
tremendous burden on anyone building on top of Chromium, since the
license requires that they keep all copyright lines intact.
The compromise was to create an AUTHORS file that would list all of the
copyright holders. The LICENSE file and source file headers would then
include that list by reference, listing the copyright holder as "The
Chromium Authors".
This also become cumbersome to simply keep the file up to date with a
high rate of new contributors. Plus it's not always obvious who the
copyright holder is. Sometimes it is the individual making the
contribution, but many times it may be their employer. There is no way
for the proejct maintainer to know.
Eventually, Google changed their policy to no longer recommend trying to
keep the AUTHORS file up to date proactively, and instead to only add to
it when requested: https://opensource.google/docs/releasing/authors.
They are also clear that:
> Adding contributors to the AUTHORS file is entirely within the
> project's discretion and has no implications for copyright ownership.
It was primarily added to appease a small number of large contributors
that insisted that they be recognized as copyright holders (which was
entirely their right to do). But it's not truly necessary, and not even
the most accurate way of identifying contributors and/or copyright
holders.
In practice, we've never added anyone to our AUTHORS file. It only lists
Tailscale, so it's not really serving any purpose. It also causes
confusion because Tailscalars put the "Tailscale Inc & AUTHORS" header
in other open source repos which don't actually have an AUTHORS file, so
it's ambiguous what that means.
Instead, we just acknowledge that the contributors to Tailscale (whoever
they are) are copyright holders for their individual contributions. We
also have the benefit of using the DCO (developercertificate.org) which
provides some additional certification of their right to make the
contribution.
The source file changes were purely mechanical with:
git ls-files | xargs sed -i -e 's/\(Tailscale Inc &\) AUTHORS/\1 contributors/g'
Updates #cleanup
Change-Id: Ia101a4a3005adb9118051b3416f5a64a4a45987d
Signed-off-by: Will Norris <will@tailscale.com>
Brad Fitzpatrick