This is an extract from PR #9175, which can be reviewed in isolation to
reduce the volume of changes to review all at once in #9175. There are
about to be several services and API handlers that read and set cookies
and session state, so this abstraction will prove helpful.
In particular a motivating cause for this is that with the current
architecture in PR #9175, in `api.go` the [`webauthnService` needs to
access the
session](https://github.com/syncthing/syncthing/pull/9175/files#diff-e2e14f22d818b8e635572ef0ee7718dee875c365e07225d760a6faae8be7772dR309-R310)
for authentication purposes but needs to be instantiated before the
`configMuxBuilder` for config purposes, because the WebAuthn additions
to config management need to perform WebAuthn registration ceremonies,
but currently the session management is embedded in the
`basicAuthAndSessionMiddleware` which is [instantiated much
later](https://github.com/syncthing/syncthing/pull/9175/files#diff-e2e14f22d818b8e635572ef0ee7718dee875c365e07225d760a6faae8be7772dL371-R380)
and only if authentication is enabled in `guiCfg`. This refactorization
extracts the session management out from `basicAuthAndSessionMiddleware`
so that `basicAuthAndSessionMiddleware` and `webauthnService` can both
use the same shared session management service to perform session
management logic.
### Testing
This is a refactorization intended to not change any externally
observable behaviour, so existing tests (e.g., `api_auth_test.go`)
should cover this where appropriate. I have manually verified that:
- Appending `+ "foo"` to the cookie name in `createSession` causes
`TestHtmlFormLogin/invalid_URL_returns_403_before_auth_and_404_after_auth`
and `TestHtmlFormLogin/UTF-8_auth_works` to fail
- Inverting the return value of `hasValidSession` cases a whole bunch of
tests in `TestHTTPLogin` and `TestHtmlFormLogin` to fail
- (Fixed) Changing the cookie to `MaxAge: 1000` in `destroySession` does
NOT cause any tests to fail!
- Added tests `TestHtmlFormLogin/Logout_removes_the_session_cookie`,
`TestHTTPLogin/*/Logout_removes_the_session_cookie`,
`TestHtmlFormLogin/Session_cookie_is_invalid_after_logout` and
`TestHTTPLogin/200_path#01/Session_cookie_is_invalid_after_logout` to
cover this.
- Manually verified that these tests pass both before and after the
changes in this PR, and that changing the cookie to `MaxAge: 1000` or
not calling `m.tokens.Delete(cookie.Value)` in `destroySession` makes
the respective pair of tests fail.
Somewhere along the way, the non-parallel test became parallel, and at
that point, timeouts occurred. Parallel is better, so increase the
timeout on the offending call a bit...
This makes a couple of small improvements to the folder summary
mechanism:
- The folder summary includes the local and remote sequence numbers in
clear text, rather than some odd sum that I'm not sure what it was
intended to represent.
- The folder summary event is generated when appropriate, regardless of
whether there is an event listener. We did this before because
generating it was expensive, and we wanted to avoid doing it
unnecessarily. Nowadays, however, it's mostly just reading out
pre-calculated metadata, and anyway, it's nice if it shows up reliably
when running with -verbose.
The point of all this is to make it easier to use these events to judge
when devices are, in fact, in sync. As-is, if I'm looking at two
devices, it's very difficult to reliably determine if they are in sync
or not. The reason is that while we can ask device A if it thinks it's
in sync, we can't see if the answer is "yes" because it has processed
all changes from B, or if it just doesn't know about the changes from B
yet. With proper sequence numbers in the event we can compare the two
and determine the truth. This makes testing a lot easier.
This adds a "token manager" which handles storing and checking expired
tokens, used for both sessions and CSRF tokens. It removes the old,
corresponding functionality for CSRFs which saved things in a file. The
result is less crap in the state directory, and active login sessions
now survive a Syncthing restart (this really annoyed me).
It also adds a boolean on login to create a longer-lived session cookie,
which is now possible and useful. Thus we can remain logged in over
browser restarts, which was also annoying... :)
<img width="1001" alt="Screenshot 2023-12-12 at 09 56 34"
src="https://github.com/syncthing/syncthing/assets/125426/55cb20c8-78fc-453e-825d-655b94c8623b">
Best viewed with whitespace-insensitive diff, as a bunch of the auth
functions became methods instead of closures which changed indentation.
This adds our short device ID to the basic auth realm. This has at least
two consequences:
- It is different from what's presented by another device on the same
address (e.g., if I use SSH forwards to different dives on the same
local address), preventing credentials for one from being sent to
another.
- It is different from what we did previously, meaning we avoid cached
credentials from old versions interfering with the new login flow.
I don't *think* there should be things that depend on our precise realm
string, so this shouldn't break any existing setups...
Sneakily this also changes the session cookie and CSRF name, because I
think `id.Short().String()` is nicer than `id.String()[:5]` and the
short ID is two characters longer. That's also not a problem...
This is motivated by the Android app:
https://github.com/syncthing/syncthing-android/pull/1982#issuecomment-1752042554
The planned fix in response to basic auth behaviour changing in #8757
was to add the `Authorization` header when opening the WebView, but it
turns out the function used only applies the header to the initial page
load, not any subsequent script loads or AJAX calls. The
`basicAuthAndSessionMiddleware` checks for no-auth exceptions before
checking the `Authorization` header, so the header has no effect on the
initial page load since the `/` path is a no-auth exception. Thus the
Android app fails to log in when opening the WebView.
This changes the order of checks in `basicAuthAndSessionMiddleware` so
that the `Authorization` header is always checked if present, and a
session cookie is set if it is valid. Only after that does the
middleware fall back to checking for no-auth exceptions.
`api_test.go` has been expanded with additional checks:
- Check that a session cookie is set whenever correct basic auth is
provided.
- Check that a session cookie is not set when basic auth is incorrect.
- Check that a session cookie is not set when authenticating with an API
token (either via `X-Api-Key` or `Authorization: Bearer`).
And an additional test case:
- Check that requests to `/` always succeed, but receive a session
cookie when correct basic auth is provided.
I have manually verified that
- The new assertions fail if the `createSession` call is removed in
`basicAuthAndSessionMiddleware`.
- The new test cases in e6e4df4d7034302b729ada6d91cff6e2b29678da fail
before the change in 0e47d37e738d4c15736c496e01cd949afb372e71 is
applied.
This adds the ability to have multiple concurrent connections to a single device. This is primarily useful when the network has multiple physical links for aggregated bandwidth. A single connection will never see a higher rate than a single link can give, but multiple connections are load-balanced over multiple links.
It is also incidentally useful for older multi-core CPUs, where bandwidth could be limited by the TLS performance of a single CPU core -- using multiple connections achieves concurrency in the required crypto calculations...
Co-authored-by: Simon Frei <freisim93@gmail.com>
Co-authored-by: tomasz1986 <twilczynski@naver.com>
Co-authored-by: bt90 <btom1990@googlemail.com>
Currently, historically, we look for the `X-API-Key` header to
authenticate with an API key. There's nothing wrong with this, but in
some scenarios it's easier to produce an `Authorization` header with a
`Bearer $token` content, which is nowadays more common. This change adds
support for both, so that we will accept an API key either in our custom
header or as a bearer token.
This adds an environment variable STVERSIONEXTRA that, when set, gets
added to the version information in the API and GUI.
The purpose of all this is to be able to communicate something about the
bundling or packaging, through the log & GUI and the end user, to the
potential person supporting it -- i.e., us. :) A wrapper can set this
variable to indicate that Syncthing is being run via `SyncTrayzor`,
`Syncthing-macOS`, etc., and thus indicate to the end user that the GUI
they are looking at is perhaps not the only source of truth and
management for this instance.
This fixes various test issues with Go 1.20.
- Most tests rewritten to use fakefs where possible
- Some tests that were already skipped, or dubious (invasive,
unmaintainable, unclear what they even tested) have been removed
- Some actual code rewritten to better support testing in fakefs
Co-authored-by: Eric P <eric@kastelo.net>
This adds the BlocksHash field from the FileInfo to our API output. It
can be useful for debugging, or for external tools. I'm intentionally
leaving it as an opaque base64 string because no meaning should be
derived from it: it's just a string.
This adds a word to the version string when running containerized. The
purpose is mostly to facilitate troubleshooting via screenshot by
"leaking" this rather important aspect of the setup. Additionally, the
version row gets "no-overflow-ellipsis" treatment so that the whole
thing is actually visible in the GUI and the (now useless) tooltip is
removed. In production releases this won't make a difference as the
whole thing will typically fit, but in odd setups it provides more info
up front.
There are some situations where an upgrade wouldn't be supported, even though the noUpgrade bool isn't set. So when handling the errors that are caused by this, when attempting an upgrade, it shouldn't lead to some sort of offline-message/restart/warning/etc...
I added some checks on specific errors related to this and return a 501 (Not Implemented) response instead, in case of an "UpgradeUnsupported"-error. Additionally, on the GUI-side, the 501-response is now not to be considered an error to act upon.
This adds support for syncing extended attributes on supported
filesystem on Linux, macOS, FreeBSD and NetBSD. Windows is currently
excluded because the APIs seem onerous and annoying and frankly the uses
cases seem few and far between. On Unixes this also covers ACLs as those
are stored as extended attributes.
Similar to ownership syncing this will optional & opt-in, which two
settings controlling the main behavior: one to "sync" xattrs (read &
write) and another one to "scan" xattrs (only read them so other devices
can "sync" them, but not apply any locally).
Co-authored-by: Tomasz Wilczyński <twilczynski@naver.com>
* lib/locations: Fix enum values camelCase.
* lib/locations: Remove unused FailuresFile.
* cmd/syncthing: Turn around role of locations storage.
Previously the locations package was used to provide default paths,
possibly with an overridden home directory. Extra paths supplied on
the command line were handled and passed around in the options object.
To make the changed paths available to any other interested package,
override the location setting from the option if supplied, instead of
vice versa when not supplied. Adapt code using this to read from the
locations package instead of passing through the options object.
* lib/locations: Refactor showPaths to locations package.
Generate a reusable string in locations.PrettyPrintPaths().
Enumerating all possible locations in different packages is error
prone, so add a new public function to generate the listing as a
string in the locations package. Adapt cmd/syncthing --paths to use
that instead of its own console output.
* lib/locations: Include CSRF token in pretty printed paths.
* lib/api: New endpoint /rest/system/paths.
The paths should be available for troubleshooting from a running
instance. Using the --paths CLI option is not easy in some
environments, so expose the locations mapping to a JSON endpoint.
Add utility function ListExpandedPaths() that also filters out any
entries which still contain variable placeholders.
* gui: List runtime paths in separate log viewer tab.
* Wrap paths.
* lib/syncthing: Utilize locations.Get() instead of passing an arg.
* Include base directories, move label to table caption.
* gui: Switch to hard-coded paths instead of iterating over all.
* gui: Break aboutModalView into tabs.
Use tabs to separate authors from included third-party software.
* gui: Move paths from log viewer to about modal.
* lib/locations: Adjust pretty print output order to match GUI.
* gui, authors: Remove additional bot names and fix indent.
The indentation changed because of the tabbed about dialog, fix the
authors script to respect that.
Skip Syncthing*Automation in authors list as well.
* Update AUTHORS list to remove bot names.
* Revert AUTHORS email order change.
* Do not emphasize DB and log file locations.
* Review line wrapping.
* review part 1: strings.Builder, naming
* Rename and extend locations.Set() with error handling.
Remodel the Override() function along the existing SetBaseDir() and
rename it to simply Set(). Make sure to use absolute paths when given
log file or GUI assets override options. Add proper error reporting
if that goes wrong.
* Remove obsolete comment about empty logfile option.
* Don't filter out unexpanded baseDir placeholders, only ${timestamp}.
* Restore behavior regarding special "-" logfile argument.
If the option is given, but with empty value, assume the no log
file (same as "-"). Don't try to convert the special value to an
absolute path though and document this fact in a comment for the Set()
function.
* Use template to check for location key validity.
* Don't filter out timestamp placeholders.
* lib/api: Remove paths from /rest/system/status.
* lib/ur: Properly initialize map in failure data (fixes#8479)
Co-authored-by: Jakob Borg <jakob@kastelo.net>
all: Add package runtimeos for runtime.GOOS comparisons
I grew tired of hand written string comparisons. This adds generated
constants for the GOOS values, and predefined Is$OS constants that can
be iffed on. In a couple of places I rewrote trivial switch:es to if:s,
and added Illumos where we checked for Solaris (because they are
effectively the same, and if we're going to target one of them that
would be Illumos...).
An off-by-one error could cause tokens to be forgotten. Suppose
tokens := []string{"foo", "bar", "baz", "quux"}
i := 2
token := tokens[i] // token == "baz"
Then, after
copy(tokens[1:], tokens[:i+1])
tokens[0] = token
we have
tokens == []string{"baz", "foo", "bar", "baz"}
The short test actually relied on this bug.
This commit replaces `os.MkdirTemp` with `t.TempDir` in tests. The
directory created by `t.TempDir` is automatically removed when the test
and all its subtests complete.
Prior to this commit, temporary directory created using `os.MkdirTemp`
needs to be removed manually by calling `os.RemoveAll`, which is omitted
in some tests. The error handling boilerplate e.g.
defer func() {
if err := os.RemoveAll(dir); err != nil {
t.Fatal(err)
}
}
is also tedious, but `t.TempDir` handles this for us nicely.
Reference: https://pkg.go.dev/testing#T.TempDir
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
* lib/api: Note ItemStarted and ItemFinished for default filtering.
The reasoning why LocalChangeDetected and RemoteChangeDetected events
are not included in the event stream by default (without explicit
filter mask requested) also holds for the ItemStarted and ItemFinished
events. They should be excluded as well when we start to break the
API compatibility for some reason.
* gui: Enumerate unused event types in the eventService.
Define constants for the unused event types as well, for completeness'
sake. They are intentionally not handled in the GUI currently.
* cmd/syncthing: Harmonize uppercase CLI argument placeholders.
Use ALL-UPPERCASE and connecting dashes to distinguish argument
placeholders from literal argument options (e.g. "cpu" or "heap" for
profiling). The dash makes it clear which words form a single
argument and where a new argument starts.
This style is already used for the "syncthing cli debug file" command.
* lib/model: Simplify event data structure.
Using map[string]interface{} is not necessary when all values are
known to be strings.
What hash is used to store the password should ideally be an
implementation detail, so that every user of the GUIConfiguration
object automatically agrees on how to handle it. That is currently
distribututed over the confighandler.go and api_auth.go files, plus
tests.
Add the SetHasedPassword() / CompareHashedPassword() API to keep the
hashing method encapsulated. Add a separate test for it and adjust
other users and tests. Remove all deprecated imports of the bcrypt
package.
LoadOrGenerateCertificate() takes two file path arguments, but then
uses the locations package to determine the actual path. Fix that
with a minimally invasive change, by using the arguments instead.
Factor out GenerateCertificate().
The only caller of this function is cmd/syncthing, which passes the
same values, so this is technically a no-op.
* lib/tlsutil: Make storing generated certificate optional. Avoid
temporary cert and key files in tests, keep cert in memory.
Jakob Borg
Emil Lundberg
Jakob Borg
Eng Zer Jun
Will Rouesnel
Eric P
luzpaz
André Colomb
greatroar
Simon Frei