There’s (*LocalBackend).ResetForClientDisconnect, and there’s also (*LocalBackend).resetForProfileChangeLockedOnEntry.
Both methods essentially did the same thing but in slightly different ways. For example, resetForProfileChangeLockedOnEntry didn’t reset the control client until (*LocalBackend).Start() was called at the very end and didn’t reset the keyExpired flag, while ResetForClientDisconnect didn’t reinitialize TKA.
Since SetCurrentUser can be called with a nil argument to reset the currently connected user and internally calls resetForProfileChangeLockedOnEntry, we can remove ResetForClientDisconnect and let SetCurrentUser and resetForProfileChangeLockedOnEntry handle it.
Updates #14823
Signed-off-by: Nick Khyl <nickk@tailscale.com>
Currently, profileManager filters profiles based on their creator/owner and the "current user"'s UID.
This causes DefaultUserProfileID(uid) to work incorrectly when the UID doesn't match the current user.
While we plan to remove the concept of the "current user" completely, we're not there yet.
In this PR, we fix DefaultUserProfileID by updating profileManager to allow checking profile access
for a given UID and modifying helper methods to accept UID as a parameter when returning
matching profiles.
Updates #14823
Signed-off-by: Nick Khyl <nickk@tailscale.com>
updates tailscale/corp#25687
The darwin appstore and standalone clients now support XPC and the keychain for passing user credentials securely between the gui process and an NEVPNExtension hosted tailscaled. Clients that can communicate directly with the network extension, via XPC or the keychain, are now expected to call SetCredentials and supply credentials explicitly, fixing issues with the cli breaking if the current user cannot read the contents of /Library/Tailscale due to group membership restrictions. This matches how those clients source and supply credentials to the localAPI http client.
Non-platform-specific code that has traditionally been in the client is moved to safesocket.
/Libraray/Tailscaled/sameuserproof has its permissions changed to that it's readably only by users in the admin group. This restricts standalone CLI access for and direct use of localAPI to admins.
Signed-off-by: Jonathan Nobels <jonathan@tailscale.com>
It was moved in f57fa3cbc3.
Updates tailscale/corp#22748
Change-Id: I19f965e6bded1d4c919310aa5b864f2de0cd6220
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
With #14843 merged, (*localapi.Handler).servePrefs() now requires a non-nil actor,
and other places may soon require it as well.
In this PR, we update localapi.NewHandler with a new required parameter for the actor.
We then update tsnet to use ipnauth.Self.
We also rearrange the code in (*ipnserver.Server).serveHTTP() to pass the actor via Handler's
constructor instead of the field.
Updates #14823
Signed-off-by: Nick Khyl <nickk@tailscale.com>
In this PR, we move the code that checks the AlwaysOn policy from ipnserver.actor to ipnauth.
It is intended to be used by ipnauth.Actor implementations, and we temporarily make it exported
while these implementations reside in ipnserver and in corp. We'll unexport it later.
We also update [ipnauth.Actor.CheckProfileAccess] to accept an auditLogger, which is called
to write details about the action to the audit log when required by the policy, and update
LocalBackend.EditPrefsAs to use an auditLogger that writes to the regular backend log.
Updates tailscale/corp#26146
Signed-off-by: Nick Khyl <nickk@tailscale.com>
Without adding this, the packet filter rejects traffic to VIP service
addresses before checking the filters sent in the netmap.
Fixestailscale/corp#26241
Change-Id: Idd54448048e9b786cf4873fd33b3b21e03d3ad4c
Signed-off-by: Adrian Dewhurst <adrian@tailscale.com>
Many places that need to work with node/peer capabilities end up with a
something-View and need to either reimplement the helper code or make an
expensive copy. We have the machinery to easily handle this now.
Updates #cleanup
Change-Id: Ic3f55be329f0fc6c178de26b34359d0e8c6ca5fc
Signed-off-by: Adrian Dewhurst <adrian@tailscale.com>
Observed on some airlines (British Airways, WestJet), Squid is
configured to cache and transform these results, which is disruptive.
The server and client should both actively request that this is not done
by setting Cache-Control headers.
Send a timestamp parameter to further work against caches that do not
respect the cache-control headers.
Updates #14856
Signed-off-by: James Tucker <james@tailscale.com>
In this PR, we update client/tailscale.LocalClient to allow sending requests with an optional X-Tailscale-Reason
header. We then update ipn/ipnserver.{actor,Server} to retrieve this reason, if specified, and use it to determine
whether ipnauth.Disconnect is allowed when the AlwaysOn.OverrideWithReason policy setting is enabled.
For now, we log the reason, along with the profile and OS username, to the backend log.
Finally, we update LocalBackend to remember when a disconnect was permitted and do not reconnect automatically
unless the policy changes.
Updates tailscale/corp#26146
Signed-off-by: Nick Khyl <nickk@tailscale.com>
The AlwaysOn policy can be applied by (*LocalBackend).applySysPolicy, flipping WantRunning from false to true
before (*LocalBackend).Start() has been called for the first time and set a control client in b.cc. This results in a nil
pointer dereference and a panic when setPrefsLockedOnEntry applies the change and calls controlclient.Client.Login().
In this PR, we fix it by only doing a login if b.cc has been set.
Updates #14823
Signed-off-by: Nick Khyl <nickk@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>
In this PR, we update LocalBackend to set WantRunning=true when applying policy settings
to the current profile's prefs, if the "always-on" mode is enabled.
We also implement a new (*LocalBackend).EditPrefsAs() method, which is like EditPrefs
but accepts an actor (e.g., a LocalAPI client's identity) that initiated the change.
If WantRunning is being set to false, the new EditPrefsAs method checks whether the actor
has ipnauth.Disconnect access to the profile and propagates an error if they do not.
Finally, we update (*ipnserver.actor).CheckProfileAccess to allow a disconnect
only if the "always-on" mode is not enabled by the AlwaysOn policy setting.
This is not a comprehensive solution to the "always-on" mode across platforms,
as instead of disconnecting a user could achieve the same effect by creating
a new empty profile, initiating a reauth, or by deleting the profile.
These are the things we should address in future PRs.
Updates #14823
Signed-off-by: Nick Khyl <nickk@tailscale.com>
The implementations define it to verify whether the actor has the requested access to a login profile.
Updates #14823
Signed-off-by: Nick Khyl <nickk@tailscale.com>
Conventionally, we use views (e.g., ipn.PrefsView, tailcfg.NodeView, etc.) when
dealing with structs that shouldn't be mutated. However, ipn.LoginProfile has been
an exception so far, with a mix of passing and returning LoginProfile by reference
(allowing accidental mutations) and by value (which is wasteful, given its
current size of 192 bytes).
In this PR, we generate an ipn.LoginProfileView and use it instead of passing/returning
LoginProfiles by mutable reference or copying them when passing/returning by value.
Now, LoginProfiles can only be mutated by (*profileManager).setProfilePrefs.
Updates #14823
Signed-off-by: Nick Khyl <nickk@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>
It was a temporary migration over four years ago. It's no longer
relevant.
Updates #610
Change-Id: I1f00c9485fab13ede6f77603f7d4235222c2a481
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
We've been maintaining temporary dev forks of golang.org/x/crypto/{acme,ssh}
in https://github.com/tailscale/golang-x-crypto instead of using
this repo's tempfork directory as we do with other packages. The reason we were
doing that was because x/crypto/ssh depended on x/crypto/ssh/internal/poly1305
and I hadn't noticed there are forwarding wrappers already available
in x/crypto/poly1305. It also depended internal/bcrypt_pbkdf but we don't use that
so it's easy to just delete that calling code in our tempfork/ssh.
Now that our SSH changes have been upstreamed, we can soon unfork from SSH.
That leaves ACME remaining.
This change copies our tailscale/golang-x-crypto/acme code to
tempfork/acme but adds a test that our vendored copied still matches
our tailscale/golang-x-crypto repo, where we can continue to do
development work and rebases with upstream. A comment on the new test
describes the expected workflow.
While we could continue to just import & use
tailscale/golang-x-crypto/acme, it seems a bit nicer to not have that
entire-fork-of-x-crypto visible at all in our transitive deps and the
questions that invites. Showing just a fork of an ACME client is much
less scary. It does add a step to the process of hacking on the ACME
client code, but we do that approximately never anyway, and the extra
step is very incremental compared to the existing tedious steps.
Updates #8593
Updates #10238
Change-Id: I8af4378c04c1f82e63d31bf4d16dba9f510f9199
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
We had the debug packet capture code + Lua dissector in the CLI + the
iOS app. Now we don't, with tests to lock it in.
As a bonus, tailscale.com/net/packet and tailscale.com/net/flowtrack
no longer appear in the CLI's binary either.
A new build tag ts_omit_capture disables the packet capture code and
was added to build_dist.sh's --extra-small mode.
Updates #12614
Change-Id: I79b0628c0d59911bd4d510c732284d97b0160f10
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Adds an envknob setting for changing the client's ACME directory URL.
This allows testing cert issuing against LE's staging environment, as
well as enabling local-only test environments, which is useful for
avoiding the production rate limits in test and development scenarios.
Fixes#14761
Change-Id: I191c840c0ca143a20e4fa54ea3b2f9b7cbfc889f
Signed-off-by: Tom Proctor <tomhjp@users.noreply.github.com>
The CN field is technically deprecated; set the requested name in a DNS SAN
extension in addition to maximise compatibility with RFC 8555.
Fixes#14762
Change-Id: If5d27f1e7abc519ec86489bf034ac98b2e613043
Signed-off-by: Tom Proctor <tomhjp@users.noreply.github.com>
If we fail to parse the upstream DNS response in an app connector, we
might miss new IPs for the target domain. Log parsing errors to be able
to diagnose that.
Updates #14606
Signed-off-by: Andrew Lytvynov <awly@tailscale.com>
This pulls out the Wake-on-LAN (WoL) code out into its own package
(feature/wakeonlan) that registers itself with various new hooks
around tailscaled.
Then a new build tag (ts_omit_wakeonlan) causes the package to not
even be linked in the binary.
Ohter new packages include:
* feature: to just record which features are loaded. Future:
dependencies between features.
* feature/condregister: the package with all the build tags
that tailscaled, tsnet, and the Tailscale Xcode project
extension can empty (underscore) import to load features
as a function of the defined build tags.
Future commits will move of our "ts_omit_foo" build tags into this
style.
Updates #12614
Change-Id: I9c5378dafb1113b62b816aabef02714db3fc9c4a
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
* Reapply "ipn/ipnlocal: re-advertise appc routes on startup (#14609)"
This reverts commit 51adaec35a.
Signed-off-by: Andrew Lytvynov <awly@tailscale.com>
* ipn/ipnlocal: fix a deadlock in readvertiseAppConnectorRoutes
Don't hold LocalBackend.mu while calling the methods of
appc.AppConnector. Those methods could call back into LocalBackend and
try to acquire it's mutex.
Fixes https://github.com/tailscale/corp/issues/25965Fixes#14606
Signed-off-by: Andrew Lytvynov <awly@tailscale.com>
---------
Signed-off-by: Andrew Lytvynov <awly@tailscale.com>
Rather than using a string everywhere and needing to clarify that the
string should have the svc: prefix, create a separate type for Service
names.
Updates tailscale/corp#24607
Change-Id: I720e022f61a7221644bb60955b72cacf42f59960
Signed-off-by: Adrian Dewhurst <adrian@tailscale.com>
This commit intend to provide support for TCP and Web VIP services and also allow user to use Tun
for VIP services if they want to.
The commit includes:
1.Setting TCP intercept function for VIP Services.
2.Update netstack to send packet written from WG to netStack handler for VIP service.
3.Return correct TCP hander for VIP services when netstack acceptTCP.
This commit also includes unit tests for if the local backend setServeConfig would set correct TCP intercept
function and test if a hander gets returned when getting TCPHandlerForDst. The shouldProcessInbound
check is not unit tested since the test result just depends on mocked functions. There should be an integration
test to cover shouldProcessInbound and if the returned TCP handler actually does what the serveConfig says.
Updates tailscale/corp#24604
Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com>
We previously baked in the LetsEncrypt x509 root CA for our tlsdial
package.
This moves that out into a new "bakedroots" package and is now also
shared by ipn/ipnlocal's cert validation code (validCertPEM) that
decides whether it's time to fetch a new cert.
Otherwise, a machine without LetsEncrypt roots locally in its system
roots is unable to use tailscale cert/serve and fetch certs.
Fixes#14690
Change-Id: Ic88b3bdaabe25d56b9ff07ada56a27e3f11d7159
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Adds a new Hostinfo.IngressEnabled bool field that holds whether
funnel is currently enabled for the node. Triggers control update
when this value changes.
Bumps capver so that control can distinguish the new field being false
vs non-existant in previous clients.
This is part of a fix for an issue where nodes with any AllowFunnel
block set in their serve config are being displayed as if actively
routing funnel traffic in the admin panel.
Updates tailscale/tailscale#11572
Updates tailscale/corp#25931
Signed-off-by: Irbe Krumina <irbe@tailscale.com>
We throw error early with a warning if users attempt to enable background funnel
for a node that does not allow incoming connections
(shields up), but if it done in foreground mode, we just silently fail
(the funnel command succeeds, but the connections are not allowed).
This change makes sure that we also error early in foreground mode.
Updates tailscale/tailscale#11049
Signed-off-by: Irbe Krumina <irbe@tailscale.com>
Updates tailscale/corp#25936
This defines a new syspolicy 'Hostname' and allows an IT administrator to override the value we normally read from os.Hostname(). This is particularly useful on Android and iOS devices, where the hostname we get from the OS is really just the device model (a platform restriction to prevent fingerprinting).
If we don't implement this, all devices on the customer's side will look like `google-pixel-7a-1`, `google-pixel-7a-2`, `google-pixel-7a-3`, etc. and it is not feasible for the customer to use the API or worse the admin console to manually fix these names.
Apply code review comment by @nickkhyl
Signed-off-by: Andrea Gottardo <andrea@gottardo.me>
Co-authored-by: Nick Khyl <1761190+nickkhyl@users.noreply.github.com>
If the server was in use at the time of the initial check, but disconnected and was removed
from the activeReqs map by the time we registered a waiter, the ready channel will never
be closed, resulting in a deadlock. To avoid this, we check whether the server is still busy
after registering the wait.
Fixes#14655
Signed-off-by: Nick Khyl <nickk@tailscale.com>
I made a last-minute change in #14626 to split a single loop that created 1_000 concurrent
connections into an inner and outer loop that create 100 concurrent connections 10 times.
This introduced a race because the last user's connection may still be active (from the server's
perspective) when a new outer iteration begins. Since every new client gets a unique ClientID,
but we reuse usernames and UIDs, the server may let a user in (as the UID matches, which is fine),
but the test might then fail due to a ClientID mismatch:
server_test.go:232: CurrentUser(Initial): got &{S-1-5-21-1-0-0-1001 User-4 <nil> Client-2 false false};
want &{S-1-5-21-1-0-0-1001 User-4 <nil> Client-114 false false}
In this PR, we update (*testIPNServer).blockWhileInUse to check whether the server is currently busy
and wait until it frees up. We then call blockWhileInUse at the end of each outer iteration so that the server
is always in a known idle state at the beginning of the inner loop. We also check that the current user
is not set when the server is idle.
Updates tailscale/corp#25804
Updates #14655 (found when working on it)
Signed-off-by: Nick Khyl <nickk@tailscale.com>
There's at least one example of stored routes and advertised routes
getting out of sync. I don't know how they got there yet, but this would
backfill missing advertised routes on startup from stored routes.
Also add logging in LocalBackend.AdvertiseRoute to record when new
routes actually get put into prefs.
Updates #14606
Signed-off-by: Andrew Lytvynov <awly@tailscale.com>
In this commit, we add a failing test to verify that ipn/ipnserver.Server correctly
sets and unsets the current user when two different clients send requests concurrently
(A sends request, B sends request, A's request completes, B's request completes).
The expectation is that the user who wins the race becomes the current user
from the LocalBackend's perspective, remaining in this state until they disconnect,
after which a different user should be able to connect and use the LocalBackend.
We then fix the second of two bugs in (*Server).addActiveHTTPRequest, where a race
condition causes the LocalBackend's state to be reset after a new client connects,
instead of after the last active request of the previous client completes and the server
becomes idle.
Fixestailscale/corp#25804
Signed-off-by: Nick Khyl <nickk@tailscale.com>
In this commit, we add a failing test to verify that ipn/ipnserver.Server correctly
sets and unsets the current user when two different users connect sequentially
(A connects, A disconnects, B connects, B disconnects).
We then fix the test by updating (*ipn/ipnserver.Server).addActiveHTTPRequest
to avoid calling (*LocalBackend).ResetForClientDisconnect again after a new user
has connected and been set as the current user with (*LocalBackend).SetCurrentUser().
Since ipn/ipnserver.Server does not allow simultaneous connections from different
Windows users and relies on the LocalBackend's current user, and since we already
reset the LocalBackend's state by calling ResetForClientDisconnect when the last
active request completes (indicating the server is idle and can accept connections
from any Windows user), it is unnecessary to track the last connected user on the
ipnserver.Server side or call ResetForClientDisconnect again when the user changes.
Additionally, the second call to ResetForClientDisconnect occurs after the new user
has been set as the current user, resetting the correct state for the new user
instead of the old state of the now-disconnected user, causing issues.
Updates tailscale/corp#25804
Signed-off-by: Nick Khyl <nickk@tailscale.com>
We update client/tailscale.LocalClient to allow specifying an optional Transport
(http.RoundTripper) for LocalAPI HTTP requests, and implement one that injects
an ipnauth.TestActor via request headers. We also add several functions and types
to make testing an ipn/ipnserver.Server possible (or at least easier).
We then use these updates to write basic tests for ipnserver.Server,
ensuring it works on non-Windows platforms and correctly sets and unsets
the LocalBackend's current user when a Windows user connects and disconnects.
We intentionally omit tests for switching between different OS users
and will add them in follow-up commits.
Updates tailscale/corp#25804
Signed-off-by: Nick Khyl <nickk@tailscale.com>
In preparation for adding test coverage for ipn/ipnserver.Server, we update it
to use ipnauth.Actor instead of its concrete implementation where possible.
Updates tailscale/corp#25804
Signed-off-by: Nick Khyl <nickk@tailscale.com>
This deprecates the old "DERP string" packing a DERP region ID into an
IP:port of 127.3.3.40:$REGION_ID and just uses an integer, like
PeerChange.DERPRegion does.
We still support servers sending the old form; they're converted to
the new form internally right when they're read off the network.
Updates #14636
Change-Id: I9427ec071f02a2c6d75ccb0fcbf0ecff9f19f26f
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This doesn't seem to have any immediate impact, but not allowing access via the IPv6 masquerade
address when an IPv4 masquerade address is also set seems like a bug.
Updates #cleanup
Updates #14570 (found when working on it)
Signed-off-by: Nick Khyl <nickk@tailscale.com>
In this PR, we add a generic views.ValuePointer type that can be used as a view for pointers
to basic types and struct types that do not require deep cloning and do not have corresponding
view types. Its Get/GetOk methods return stack-allocated shallow copies of the underlying value.
We then update the cmd/viewer codegen to produce getters that return either concrete views
when available or ValuePointer views when not, for pointer fields in generated view types.
This allows us to avoid unnecessary allocations compared to returning pointers to newly
allocated shallow copies.
Updates #14570
Signed-off-by: Nick Khyl <nickk@tailscale.com>
sync.OnceValue and slices.Compact were both added in Go 1.21.
cmp.Or was added in Go 1.22.
Updates #8632
Updates #11058
Change-Id: I89ba4c404f40188e1f8a9566c8aaa049be377754
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This commit updates the return body of c2n endpoint /vip-services to keep hash generation logic on client side.
Updates tailscale/corp#24510
Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com>
Nick Khyl
Jonathan Nobels
Brad Fitzpatrick
Adrian Dewhurst
James Tucker
Percy Wegmann
Tom Proctor
Andrew Lytvynov
KevinLiang10
Irbe Krumina
Andrea Gottardo
Nahum Shalman