Since the incubator parses the UID in base 0 via flags.IntVar()
and isRootUser() uses base 10, it was possible to get inconsistent
results between the two using non-base 10 numbers.
Updates tailscale/corp#50380
Change-Id: I9b2fb16bc37153df7b695533cc73a3499cb774eb
Signed-off-by: Francois Marier <francois@tailscale.com>
Re-evaluate authenticated connections on policy updates and at session
and forwarding gates. Close connections whose grants are revoked or
narrowed, including forwarding permissions, user identity, duration,
AcceptEnv, and effective recording requirements. Recheck rule-derived
AcceptEnv and recording configuration for delegated connections too.
Compare recorder lists with duplicate counts preserved so duplicate
entries cannot hide a change of recording destination.
Treat recording-failure changes in either direction as material,
including fail-open to fail-closed transitions. An inconclusive user
lookup retains the previous identity but does not skip other policy
checks, including for `"*": "="` mappings. An absent account or a changed
username, UID, or primary GID revokes the connection.
Preserve explicit getent failures when the os/user fallback cannot find
the account, instead of treating every lookup failure as account deletion.
Numeric UID mappings use LookupId in the fallback, not Lookup by name.
Known limitation: NSS can report a directory-only account as not found
(getent exit status 2) during an sssd/LDAP outage. If the fallback also
reports the account missing, this is indistinguishable from deletion and
still revokes the connection. This availability tradeoff remains accepted
for now; explicit command failures, timeouts, and malformed output do not
by themselves establish that an account was deleted.
Reject forwarding requests using the latched Allow bit before doing a
policy recheck. Forwarding gates still re-evaluate policy, but use the
latched account identity rather than spawning getent for every forwarded
connection (including each SSH -D request). Account identity is rechecked
at authentication completion, policy updates, rule expiry, and session
gates.
Re-arm rule-expiry timers under the policy recheck lock and prevent
re-arming after teardown. Add connection-level login grace, extending
it during delegation and stopping it only on SSH authentication
success, not on unsigned key queries or partial authentication.
Close the raw transport and cancel delegated authentication on teardown.
Add regression tests for these transitions, wait for the auth-completion
recheck before test policy flips, and release stalled recorder handlers
even on test failure.
Updates tailscale/corp#50380
Signed-off-by: James Tucker <james@tailscale.com>
sftpSubsystemSession waited for the stdout copier's done channel but not
the stderr copier's. The stderr copier carries the recording-policy
refusal notices. The deferred channel close could cut it off mid-drain.
The refusal text never reached the test's buffer. Both copiers are
now waited on before the channel closes.
Updates tailscale/corp#50380
Signed-off-by: James Tucker <james@tailscale.com>
Remove the blanket sftp-subsystem exemption from session recording:
in a recording-mandatory tailnet, sftp sessions (and scp -s, which
rides the sftp subsystem) now go through recording setup with the same
OnRecordingFailure semantics — an unreachable recorder refuses the
session (exit 254) instead of letting file access proceed silently
unrecorded.
SFTP casts are metadata-only: the CastHeader identifies the peer, local
user, connection, and records Command = "sftp" (subsystem requests
carry no command string), but no I/O events are captured — the SFTP
wire protocol is binary, so pass-through capture would not be
human-meaningful and would balloon cast sizes. The upload/cleanup
lifecycle is unchanged. With no recorders configured, sftp is
unchanged.
Updates tailscale/corp#50380
Signed-off-by: James Tucker <james@tailscale.com>
GLIBC_TUNABLES is parsed by glibc in root su/login environments with
AT_SECURE=0 (CVE-2023-4911 class); add it to the dangerous-env-var
denylist next to its GCONV_PATH/LOCPATH siblings.
The TERM/LANG/LC_* pairs accepted without policy (OpenSSH's default
AcceptEnv) had unvalidated values that reach root-exec'd login/su
environments, where glibc treats path-like locale names as locale-data
paths and leading dashes as its own options. Reject path- and
option-like values (and NUL, which would truncate on exec) instead of
trying to sanitize them.
Also cap accepted env var names at 256 bytes before any wildcard
matching, bounding the matcher input and the child's "su -w" allowlist;
real names are short (LOGIN_NAME_MAX).
Updates tailscale/corp#50380
Signed-off-by: James Tucker <james@tailscale.com>
A policy rejection names the requested sshUser: that name is the
client's own request and reflects only tailnet policy (the client's
own ACL view), never a local-host fact, so naming it leaks nothing.
What a client-visible rejection must never depend on is server-side
local-user information: local-user lookup failures send one fixed
message for every failure cause, with the underlying error logged
server-side only, so the client cannot tell "no such local user"
from any other lookup failure and no banner is a
username-existence oracle.
Pin both properties in TestSSHAuthFlow: the user-mismatch cases,
including a rule whose "=" mapping forces a local lookup of a user
that does not exist, all name the requested user identically, and an
accept rule whose mapped local user fails lookup gets the fixed
lookup-failure banner.
Updates tailscale/corp#50380
Signed-off-by: James Tucker <james@tailscale.com>
An NSS uid like "00" passed the string != "0" check but parses to 0 in
the incubator, so a session could bypass non-root checks and run as
root. Compare the uid numerically instead; non-numeric uids keep the
previous behavior.
Updates tailscale/corp#50380
Signed-off-by: James Tucker <james@tailscale.com>
The CastHeader recorded a shell-parsed reconstruction of the session
command, but the command is passed to the user's shell verbatim, and the
shell lexer silently drops unbalanced quotes. A spoofed command could
therefore make the recording differ from what the shell actually ran.
Record ss.RawCommand() instead.
Updates tailscale/corp#50380
Signed-off-by: James Tucker <james@tailscale.com>
On non-PTY exec sessions stdout was wrapped in the recording writer but
stderr was copied to the client raw, so any command output written to
stderr never appeared in the session recording. PTY sessions were
unaffected because they fold stdout and stderr into a single stream that
already flows through the recording writer.
Wrap stderr in rec.writer too, recording it under the "o" (output)
direction, matching how PTY sessions record combined output. The
asciinema cast format has no separate stderr channel, so "o" is the
correct code for players and tsrecorder to render it.
Strengthen TestSSHRecordingNonInteractive to run a command that writes to
both stdout and stderr and assert both appear in the recorded cast lines.
Updates tailscale/corp#48187
Change-Id: I1dc36073722677a85f2b23b331fd55b8df4d342e
Reported-by: Ben Carman <benthecarman@live.com>
Signed-off-by: Patrick O'Doherty <patrick@tailscale.com>
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>
Send exit-status before EOF before CHANNEL_CLOSE per RFC 4254 §6.10.
The old code raced CloseWrite against Exit; macOS OpenSSH often saw
EOF first and dropped the exit code.
Replace atomic.Bool/atomic.Int32/channel sync with a sync.WaitGroup
over the output goroutines and emit:
cmd.Wait -> ss.Exit -> closeAll(childPipes) -> wg.Wait -> ss.Close
The stdin copier stays out of wg: it blocks reading from the SSH
channel until the client half-closes, and many clients (go-scp,
plain `ssh host cmd`, scp, sftp) don't half-close before they
receive CHANNEL_CLOSE from us. Including it deadlocks. It self-
cleans via deferred ss.Close.
Align exit codes with shell convention:
255 SSH-internal error (OpenSSH ssh.c:1693)
254 recording-infrastructure failure
127 command-not-found (POSIX)
Switch session termination from SIGKILL to SIGHUP, matching
OpenSSH PTY-master-close (session.c:2246). Process-group delivery
follows in a later commit.
Relies on the gliderssh wire-order change (tailscale/gliderssh#8),
which go.mod already pins since 3f5eb3199.
Based on tailscale/tailscale#18331.
Updates #18256
Signed-off-by: James Tucker <james@tailscale.com>
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
Extract the in-process SSH server harness out of tailssh_test.go into
tailssh_exitcodes_test.go so the exit-status tests can share it, and add
the integrationtest-tagged exit-code suite (Go and OpenSSH clients).
Updates #18256
Signed-off-by: Kristoffer Dalby <kristoffer@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 test is still flaking on macOS, so mark it as such so we can track
and investigate further.
Updates #7707
Change-Id: I640da3c1068a90a9815caab2df9431bceb01f846
Signed-off-by: Alex Chan <alexc@tailscale.com>
When a recording upload fails mid-session, the recording goroutine
cancels the session context. This triggers two concurrent paths:
exec.CommandContext kills the process (causing cmd.Wait to return),
and killProcessOnContextDone tries to write the termination message
via exitOnce.Do. If cmd.Wait returns first, the main goroutine's
exitOnce.Do(func(){}) steals the once, and the termination message
is never written to the client.
Fix by waiting for killProcessOnContextDone to finish writing the
termination message (via <-ss.exitHandled) before claiming exitOnce,
when the context is already done.
Also fix the fallback path when launchProcess itself fails due to
context cancellation: use SSHTerminationMessage() with the correct
"\r\n\r\n" framing instead of fmt.Fprintf with the internal error
string.
Deflakes TestSSHRecordingCancelsSessionsOnUploadFailure, which was
failing consistently at a low rate due to the exitOnce race. After
this fix, flakestress passes with 8,668 runs, 0 failures.
Fixes#7707 (again. hopefully for good.)
Change-Id: I5ab911c71574db8d3f9d979fb839f273be51ecf9
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Brings in a newer version of Gliderlabs SSH with added socket forwarding support.
Fixes#12409Fixes#5295
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
This makes tsnet apps not depend on x/crypto/ssh and locks that in with a test.
It also paves the wave for tsnet apps to opt-in to SSH support via a
blank feature import in the future.
Updates #12614
Change-Id: Ica85628f89c8f015413b074f5001b82b27c953a9
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
When a recording upload fails mid-session, killProcessOnContextDone
writes the termination message to ss.Stderr() and kills the process.
Meanwhile, run() takes the ss.ctx.Done() path and proceeds to
ss.Exit(), which tears down the SSH channel. The termination message
write races with the channel teardown, so the client sometimes never
receives it.
Fix by adding an exitHandled channel that killProcessOnContextDone
closes when done. run() now waits on this channel after ctx.Done()
fires, ensuring the termination message is fully written before
the SSH channel is torn down.
Fixes#7707
Change-Id: Ib60116c928d3af46d553a4186a72963c2c731e3e
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
I omitted a lot of the min/max modernizers because they didn't
result in more clear code.
Some of it's older "for x := range 123".
Also: errors.AsType, any, fmt.Appendf, etc.
Updates #18682
Change-Id: I83a451577f33877f962766a5b65ce86f7696471c
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
OnPolicyChange can observe a conn in activeConns before authentication
completes. The previous `c.info == nil` guard was itself a data race
against clientAuth writing c.info, and even when c.info appeared
non-nil, c.localUser could still be nil, causing a nil pointer
dereference at c.localUser.Username.
Add an authCompleted atomic.Bool to conn, stored true after all auth
fields are written in clientAuth. OnPolicyChange checks this atomic
instead of c.info, which provides the memory barrier guaranteeing all
prior writes are visible to the concurrent reader.
Updates tailscale/corp#36268 (fixes, but we might want to cherry-pick)
Co-authored-by: Gesa Stupperich <gesa@tailscale.com>
Change-Id: I4c69843541f5f9f04add9bf431e320c65a203a39
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>
The Tracker was using direct callbacks to ipnlocal. This PR moves those
to be triggered via the eventbus.
Additionally, the eventbus is now closed on exit from tailscaled
explicitly, and health is now a SubSystem in tsd.
Updates #15160
Signed-off-by: Claus Lensbøl <claus@tailscale.com>
Also add a trailing newline to error banners so that SSH client messages don't print on the same line.
Updates tailscale/corp#29138
Signed-off-by: Percy Wegmann <percy@tailscale.com>
In tailssh.go:1284, (*sshSession).startNewRecording starts a fire-and-forget goroutine that can
outlive the test that triggered its creation. Among other things, it uses ss.logf, and may call it
after the test has already returned. Since we typically use (*testing.T).Logf as the logger,
this results in a data race and causes flaky tests.
Ideally, we should fix the root cause and/or use a goroutines.Tracker to wait for the goroutine
to complete. But with the release approaching, it's too risky to make such changes now.
As a workaround, we update the tests to use tstest.WhileTestRunningLogger, which logs to t.Logf
while the test is running and disables logging once the test finishes, avoiding the race.
While there, we also fix TestSSHAuthFlow not to use log.Printf.
Updates #15568
Updates #7707 (probably related)
Signed-off-by: Nick Khyl <nickk@tailscale.com>
Although, at the moment, we do not yet require an event bus to be present, as
we start to add more pieces we will want to ensure it is always available. Add
a new constructor and replace existing uses of new(tsd.System) throughout.
Update generated files for import changes.
Updates #15160
Change-Id: Ie5460985571ade87b8eac8b416948c7f49f0f64b
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
Some clients don't request 'none' authentication. Instead, they immediately supply
a password or public key. This change allows them to do so, but ignores the supplied
credentials and authenticates using Tailscale instead.
Updates #14922
Signed-off-by: Percy Wegmann <percy@tailscale.com>
This fork golang.org/x/crypto/ssh (at upstream x/crypto git rev e47973b1c1)
into tailscale.com/tempfork/sshtest/ssh so we can hack up the client in weird
ways to simulate other SSH clients seen in the wild.
Two changes were made to the files when they were copied from x/crypto:
* internal/poly1305 imports were replaced by the non-internal version;
no code changes otherwise. It didn't need the internal one.
* all decode-with-passphrase funcs were deleted, to avoid
using the internal package x/crypto/ssh/internal/bcrypt_pbkdf
Then the tests passed.
Updates #14969
Change-Id: Ibf1abebfe608c75fef4da0255314f65e54ce5077
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
The upstream crypto package now supports sending banners at any time during
authentication, so the Tailscale fork of crypto/ssh is no longer necessary.
github.com/tailscale/golang-x-crypto is still needed for some custom ACME
autocert functionality.
tempfork/gliderlabs is still necessary because of a few other customizations,
mostly related to TTY handling.
Originally implemented in 46fd4e58a2,
which was reverted in b60f6b849a to
keep the change out of v1.80.
Updates #8593
Signed-off-by: Percy Wegmann <percy@tailscale.com>
This reverts commit 46fd4e58a2.
We don't want to include this in 1.80 yet, but can add it back post 1.80.
Updates #8593
Signed-off-by: Percy Wegmann <percy@tailscale.com>
The upstream crypto package now supports sending banners at any time during
authentication, so the Tailscale fork of crypto/ssh is no longer necessary.
github.com/tailscale/golang-x-crypto is still needed for some custom ACME
autocert functionality.
tempfork/gliderlabs is still necessary because of a few other customizations,
mostly related to TTY handling.
Updates #8593
Signed-off-by: Percy Wegmann <percy@tailscale.com>
When we first made Tailscale SSH, we assumed people would want public
key support soon after. Turns out that hasn't been the case; people
love the Tailscale identity authentication and check mode.
In light of CVE-2024-45337, just remove all our public key code to not
distract people, and to make the code smaller. We can always get it
back from git if needed.
Updates tailscale/corp#25131
Updates golang/go#70779
Co-authored-by: Percy Wegmann <percy@tailscale.com>
Change-Id: I87a6e79c2215158766a81942227a18b247333c22
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
The v2 endpoint supports HTTP/2 bidirectional streaming and acks for
received bytes. This is used to detect when a recorder disappears to
more quickly terminate the session.
Updates https://github.com/tailscale/corp/issues/24023
Signed-off-by: Andrew Lytvynov <awly@tailscale.com>
This adds a new generic result type (motivated by golang/go#70084) to
try it out, and uses it in the new lineutil package (replacing the old
lineread package), changing that package to return iterators:
sometimes over []byte (when the input is all in memory), but sometimes
iterators over results of []byte, if errors might happen at runtime.
Updates #12912
Updates golang/go#70084
Change-Id: Iacdc1070e661b5fb163907b1e8b07ac7d51d3f83
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
this commit changes usermetrics to be non-global, this is a building
block for correct metrics if a go process runs multiple tsnets or
in tests.
Updates #13420
Updates tailscale/corp#22075
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
cmd/k8s-operator,k8s-operator/sessionrecording,sessionrecording,ssh/tailssh: refactor session recording functionality
Refactor SSH session recording functionality (mostly the bits related to
Kubernetes API server proxy 'kubectl exec' session recording):
- move the session recording bits used by both Tailscale SSH
and the Kubernetes API server proxy into a shared sessionrecording package,
to avoid having the operator to import ssh/tailssh
- move the Kubernetes API server proxy session recording functionality
into a k8s-operator/sessionrecording package, add some abstractions
in preparation for adding support for a second streaming protocol (WebSockets)
Updates tailscale/corp#19821
Signed-off-by: Irbe Krumina <irbe@tailscale.com>
Previously, we were registering TCP and UDP connections in the same map,
which could result in erroneously removing a mapping if one of the two
connections completes while the other one is still active.
Add a "proto string" argument to these functions to avoid this.
Additionally, take the "proto" argument in LocalAPI, and plumb that
through from the CLI and add a new LocalClient method.
Updates tailscale/corp#20600
Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I35d5efaefdfbf4721e315b8ca123f0c8af9125fb
In prep for it being required in more places.
Updates #11874
Change-Id: Ib743205fc2a6c6ff3d2c4ed3a2b28cac79156539
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Francois Marier