feat: add WebAuthn support to GUI #9175
Conversation
|
By the way: to test this, you don't need a YubiKey or other dedicated security key hardware. There are a few other options:
|
|
Emlun -- Kudos to you and all the contributors for getting the HTML login form merged in preparation for WebAuthn support! Running v1.26.0-rc.1 here under the various browsers on Windows Server and client and its great finally being able to use password managers. Huge modernization and improvement! Comments on the WebAuthn GUI design you posted: Would it make sense to fill in the relaying party ID field with a default value (if blank) derived from the Syncthing Device Name? For example if the Syncthing device name was "Ranger" under settings, the RP ID field default could be "Syncthing Ranger". I'm guessing here that credential prompts will show the RP ID to identify the entity asking for credentials? If that assumption above is correct, I think this would make it easier for a non-technical user to get started with WebAuthn. This way a user could simply hit +Add and not have to understand RP ID until and unless the user wanted to choose another RP ID value. |
No, but also yes, the implementation already does. The WebAuthn spec has two identifiers for the RP: the RP ID, which is the "real" one which defines credential scope, and the "RP name", which is purely for display. The RP name is a free parameter; the implementation here sets it to "Syncthing" by default and "Syncthing @ <device name>" if the device configuration is readable and has a nonempty value. The idea for the RP name is indeed that clients (browsers) would display it in credential pickers, but in practice all current client implementations just display the RP ID. Therefore the RP name is currently not configurable in the GUI, since it's fairly useless in practice. The RP ID, however, must equal or be a parent domain of the webpage that performs the WebAuthn request - so if the GUI is hosted at The implementation here sets the RP ID to The other configuration field, "WebAuthn Origin", also relates somewhat to the RP ID. This is the address (including scheme and port, but not path) where the GUI is expected to be hosted; the client encodes this address into the signed assertion and the WebAuthn verification steps check that it equals the "WebAuthn Origin" value configured here. The default value of "WebAuthn Origin" is
Yes - in theory the RP name would also be displayed, but in practice all current client implementations show only the RP ID. But as explained above, the RP ID is severely restricted by the credential scoping rules.
This is already the case if the GUI is hosted at Does that answer your questions? |
|
That all sounds pretty complex, and also mostly static. Does that need to be exposed to the user in the main config interface? Seems like having reasonable default values and then keeping it advanced only would be enough and cause the least confusion (users tend to mess with anything that's being presented to them, regardless of if they have a need/understand it). |
|
Emlun -- Thanks for the detailed explanation for us non-WebAuthn developers. I concur with imsodin about moving these fields under Advanced options. Given those fields are likely to cause confusion for non-technical Syncthing users and will be fine for these users at default values. |
|
Yep, that all sounds fair to me. We can still keep the "RP ID should be equal to or a parent domain of the current webpage domain" warning for the case when the GUI is accessed at an address that doesn't match the RP ID (in which case neither credential registration nor login would work), but rewrite it a bit and link out to the docs for the advanced options. |
|
I have removed the RP ID and Origin settings from the GUI and set explicit defaults for them instead of deriving suitable defaults dynamically, so that the values shown in advanced settings are the values actually in effect. Docs for these are not yet written, but I intend to write it as part of this. I have not yet updated the top post screenshots to reflect these changes. |
### Purpose Discovered while working on the WebAuthn credentials table in #9175: there's a style on `td input[type="checkbox"]` that modifies margins for all checkboxes in `<table>`s. It looks like this style is specially tailored to the particular table that added it (PR #8734), so it should have a correspondingly special-purpose class to not accidentally apply it to other tables. As best as I could tell there are only 2 instances of `<input type="checkbox">` in `<td>`s, shown in the screenshots below. ### Testing - Open "Actions > Logging > Debugging Facilities" and observe the vertical spacing of the checkboxes. - Open "Edit Folder > Advanced", check "Sync Extended Attributes" or "Send Extended Attributes", click "Add filter entry" and observe the vertical spacing of the checkbox that appears. ### Screenshots #### Before   #### After Logs > Debugging Facilities now more compact:  Add filter entry unchanged: 
|
I've updated the settings GUI a bit to better reflect the RP ID and origin settings being moved to advanced settings:
I've updated the top post and the screenshots. I believe this is now feature-complete. 😄 Please take another look, and if this looks good I'll proceed with adding tests and documentation. |
|
@emlun What's the state of this PR, resp. what input are you looking for at the moment? Do you still need to do more work to get high-level review or are you waiting for that before continuing further work on the PR? |
|
@emlun Thank you for all the hard work on the upcoming WebAuthn support. Very much looking forward to seeing this merged. It's been a long road getting the required login page changes completed in preparation. Standing by to offer our assistance testing once an RC is released. |
|
Hi! As I mentioned in the last message, I was waiting for confirmation of the overall design before I start working on tests and documentation. But I'll take the previous two comments and my recent invitation to the Syncthing org as an okay to go ahead. 😄 I'll probably get started on the tests and docs sometime in the next two weeks.. Maybe we can finish this in January! |
That makes total sense, I just really am not very attentive to such written "details" lately... Nothing design wise/fundamental that would stand in the way of this as far as I see. I have some generic WebAuthn confusion: Looks like a WebAuthn method might be anything, also something pretty weak that's usually only used for 2FA like "having a device" - then again PWs can also be super weak. So yeah nothing problematic, I just don't really have a good grasp of that "ecosystem"/the mechanics. |
|
Yeah, a WebAuthn assertion is at minimum a proof of possession of a cryptographic private key - i.e., a "something you have" authentication factor. The authenticator that holds that key could be built into the client platform (Windows, Mac, iOS and Android all support this today) or could be an external device like a USB security key. The ones built into iOS and Android can even be used as an external authenticator on a different machine using the "cross-device authentication" (AKA "hybrid") flow. But in addition to the possession factor, a WebAuthn assertion also includes a flag indicating whether a second factor was also checked. This is called user verification (UV) and is usually a PIN, screen lock or a biometric such as a fingerprint or face print. The PIN or biometric itself is not sent to the server - rather the authenticator performs the second factor check locally and just sets the flag to indicate that some second factor was used. All the server sees is the single bit flag. This flag is of course also signed by the assertion signature so it can't be tampered with. Whether you trust the authenticator to be honest about the flag is up to the server to decide, but there's no reason to not trust it if you trust legitimate users to not lie to the server. The implementation here by default does not require UV, because single-factor WebAuthn is probably at least as secure as a single-factor password in most cases, which is what Syncthing currently supports. But the user can configure in the GUI settings, for each credential, whether UV should be required with that credential. So if you want your Syncthing instance to require multi-factor authentication, you can just check that checkbox and you'll be required to enter a PIN, unlock your phone screen, present some biometric or something like that in addition to proving possession of the authenticator. I haven't implemented a login flow using WebAuthn with a password as the second factor, because honestly, to me, WebAuthn in Syncthing is primarily a convenience feature. 😄 For me, using WebAuthn is far easier than retrieving my password from my password vault (and both are kept on the same YubiKey anyway). If I want 2FA, I'll use WebAuthn with UV instead - the FIDO PIN on a YubiKey is the same for all websites, because it's not sent to the server, so I have that memorized and can easily type it instead of having to fetch it from the vault. There's just no reason for me to want a password involved, because it would make my user experience much worse for no benefit. |
|
Any updates or approvals from anyone? Going to be automatically closed soon :( |
|
There is a huge amount of work on this just sitting here waiting to be merged. Who are we waiting on? Thanks |
|
Waiting on me :/ Sorry, I'll try to finish this soon. |
This will make it easier to merge main into the `webauthn` branch (PR syncthing#9175), as there are about to be several services and API handlers that read and set cookies and session state.
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.
lib/api/api_test.go
Outdated
| func startHTTP(cfg config.Wrapper) (string, context.CancelFunc, error) { | ||
| func startHTTP(cfg config.Wrapper) (string, context.CancelFunc, *webauthnService, error) { |
There was a problem hiding this comment.
I'm really annoyed by this extra return value, as it affects many many more calling sites where the reference is not needed at all. And all for mocking the timeNow function of this specific service? There must be a less intrusive way to test the timing behavior...
And what complicates things are the various similarly named functions: startHTTP, startServer, startHttpServer... Could we please at least not proliferate those and minimize changes to the mess which are not strictly necessary for this PR?
Correction: The latter two names are locally assigned function references. Some consistency in naming wouldn't hurt though.
There was a problem hiding this comment.
Yeah, fair. One easy fix we could do is to have startHTTP wrap an "inner" variant of itself and omit the extra return value from its own signature. For example:
func startHTTP(cfg config.Wrapper) (string, context.CancelFunc, error) {
return startHTTPWithShutdownTimeout(cfg, 0)
}
func startHTTPWithShutdownTimeout(cfg config.Wrapper, shutdownTimeout time.Duration) (string, context.CancelFunc, error) {
baseURL, cancel, _, err := startHTTPWebauthnWithShutdownTimeout(cfg, shutdownTimeout)
return baseURL, cancel, err
}
func startHTTPWebauthn(cfg config.Wrapper) (string, context.CancelFunc, *webauthnService, error) {
return startHTTPWebauthnWithShutdownTimeout(cfg, 0)
}
func startHTTPWebauthnWithShutdownTimeout(cfg config.Wrapper, shutdownTimeout time.Duration) (string, context.CancelFunc, *webauthnService, error) {
// <Function body here>
return baseURL, cancel, webauthnService, nil
}That would help with not cluttering all call sites, but would instead introduce two new overloads which I suppose would qualify as "adding to the mess"? What do you think?
And for the names: yes, we can definitely at least rename startHttpServer to startServer for consistency with the others. But that still leaves both startServer and startHTTP which have somewhat different roles (the latter starts the server, the former assembles server config and packages the server with HTTP methods preconfigured with the right baseURL etc). Say we're only considering renames for now, would renaming startServer to maybe setupHttpEnvironment or setupTestEnvironment help in any way?
There was a problem hiding this comment.
I'd be fine with just avoiding another new name and using startServer. You shouldn't rename other stuff though, only try to blend in as much as possible with the needed additions for this PR. See the other comment for an idea how we might avoid the extra return completely.
There was a problem hiding this comment.
Ok, d885afb renames startHttpServer to startServer at least.
| if ok && deviceCfg.Name != "" { | ||
| deviceName = deviceCfg.Name | ||
| } | ||
| webauthnService, err := newWebauthnService(guiCfg, deviceName, s.evLogger, s.miscDB, "webauthn") |
There was a problem hiding this comment.
Why not create the service as part of the outer API service creation? If we don't delay it to Serve(), then passing the handle for test adjustments wouldn't need intrusive changes either, maybe?
There was a problem hiding this comment.
It was like that in 4a14a21, but then reverted in 038627e (emlun#4). Looks like that was prompted by this review comment: #9175 (comment). Honestly I don't fully understand all the nuances around these service lifecycles, but maybe @imsodin could weigh in on whether the webauthnService could be moved out from being instantiated inside service.Serve()?
There was a problem hiding this comment.
Well to me it feels weird to construct a new sub-service when the "owner" is about to start serving its purpose. And as for the API, adding an error return value to api.New() is enough for being able to use newWebauthnService() from there and be able to pass out the possible error return value.
However, I didn't get much of that discussion around service lifetimes. And I haven't wrapped (!) my head around the whole deal with config and config wrappers. It does make sense that services need to be notified of config changes, and that's certainly true for the webauthnService as well. So maybe it needs a call to suture.Service.Add() as well, I don't know.
Anyway, your changes involving the startedTestMsg channel for sneaking out a service reference to the tests point toward the current approach being messy and not the correct solution within the nested service architecture. Sorry I can't be of much more help on this topic, we need @imsodin to explain the design and lifecycle considerations again.
There was a problem hiding this comment.
But the least we could do is store a pointer to the webauthnService inside the service type and let it be nil until Serve() has instantiated it? That would at least avoid all the ugly changes related to passing out the pointer when starting.
| // State that changes often but also is not security-critical, | ||
| // and therefore can be wiped and overwritten without much consequence if needed. | ||
| message WebauthnVolatileState { | ||
| map<string, WebauthnCredentialVolatileState> credentials = 1; // Keys are base64.RawURLEncoding.EncodeToString(credential ID) |
There was a problem hiding this comment.
In previous PRs, I remember getting feedback that associative arrays (maps) are undesirable in the API. Because some languages do not cope well with the concept. We might want to switch to a simple enumeration with the credential ID being just another field. Which of course could lead to duplicates, but that's easily avoided in the backend.
Considering the effort toward an SQL-based database backend, this might even better be stored as individual entries rather than a single blob containing all serialized states? I think I mentioned that in an earlier comment, as it will make manipulations easier as well (removing a single DB entry instead of a read-modify-update transaction).
There was a problem hiding this comment.
The SQL initiative being #9965? It doesn't seem wise to try integrating towards that until it's more stable, does it?
The map/array concern is fair I suppose, but I can't really imagine what language would be fine with "record"-style JSON but not "map"-style JSON since the representations are identical. Do you know of any concrete examples?
There was a problem hiding this comment.
I couldn't find that old remark, but it's basically a question of having an object with dynamically named fields:
{
"abcd": { "x": 1, "y": 2 },
"defg": { "x": 3, "y": 4 }
}versus
[
{ "id": "abcd", "x": 1, "y": 2 },
{ "id": "defg", "x": 3, "y": 4 }
]The latter can always be represented by defining a static type with the attributes / fields id, x and y. The outer object in the first example cannot, as it has arbitrary keys which cannot be enumerated in advance.
Yes, I'm talking about #9965. For a table-based database, keeping individual records is natural. But it's also possible with the simple key-value store, which makes for an easier migration path and avoids the need for read-modify-update cycles on a single blob entry.
There was a problem hiding this comment.
Yeah, I understand the difference, just not what language or serialization library would have trouble specifically with dynamic maps but not with JSON in general. But fair enough, both topics are addressed in meta-PR emlun#16.
There was a problem hiding this comment.
I don't object to maps in general but, assuming the comment above about b64 and stuff is correct, I agree that a list of things is better since you don't need to force the credential into some sort of string then, just leave it as []byte in the entry. That still applies in the sqlite world.
) ### Purpose Since syncthing#8757, the Syncthing GUI now has an unauthenticated state. One consequence of this is that `$scope.versionBase()` is not initialized while unauthenticated, which causes the `docsURL` function to truncate links to just `https://docs.syncthing.net`/, discarding the section path. This currently affects at least the "Help > Introduction" link reachable both while logged in and not. The issue is exacerbated in syncthing#9175 where we sometimes want to show additional contextual help links from the login page to particular sections of the docs. I don't think it's any worse to try to preserve the section path even without an explicit version tag, than to fall back to just the host and lose all context the link was attempting to provide. ### Testing - On commit b1ed280 (before): - Open the GUI, set a username and log out. - Open the "Help" drop-down. The "Introduction" item links to: https://docs.syncthing.net/ - Log in. - Open the "Help" drop-down. The "Introduction" item links to: https://docs.syncthing.net/v1.27.10/intro/gui - On commit 44fef31 (after): - Open the GUI, set a username and log out. - Open the "Help" drop-down. The "Introduction" item links to: https://docs.syncthing.net/intro/gui - Log in. - Open the "Help" drop-down. The "Introduction" item links to: https://docs.syncthing.net/v1.27.10/intro/gui ### Screenshots This is a GUI change, but affecting only URLs in the markup with no visual changes. ### Drawbacks If a `docsURL` call generates a versionless link to a docs page that doesn't exist on https://docs.syncthing.net - presumably because Syncthing is not the latest version and links to a deleted page? - then this will lead to GitHub's generic 404 page with no link to the Syncthing docs root. Before, any versionless link would also be a pathless link, leading to the Syncthing docs root instead of a 404 page.
Per review suggestion: #14 (comment)
Per review suggestion: #14 (comment)
|
Hi Team - I've been running the build with WebAuthn from June after emlun's last updates and merge and it's been wonderful! Looks like the Syncthing V2 RC's do not include WebAuthn which I'm really surprised at? What is the plan for completing this as it's been a very long time? Any chance we can get this merged into the Nightly builds to foster continued testing? Thank You |
|
Mostly waiting on review, which I didn't find enough time for lately. Yes, it's been quite a while and now the SQLite migration has come in the way and took some more effort for porting. We should reconsider now whether putting this info in a |
|
👍 for dedicated tables |
Fixes #8409. Replaces PR #8417.
Remaining things to do
Add explicit "require auth" option: Add explicit "Require authentication" option emlun/syncthing#5file separate from DBconfig: Move WebAuthn state back into GUIConfiguration, except SignCount and LastUseTime emlun/syncthing#7Purpose
This adds support for WebAuthn authentication in the GUI. WebAuthn is a W3C standard that provides an API for strong, privacy-conscious public-key authentication, which can enable a both more pleasant and more secure user experience than traditional password authentication. WebAuthn both works with external security keys (typically via USB or NFC) and is built into many modern browsers and operating systems.
For users who already use WebAuthn, this both cuts out a detour through a password manager and improves security by making it possible to not need a password at all.
I'm opening this as a draft PR so people can try it out while I work on finishing the last few things listed above.
Testing
Manual tests performed:
With no password set and no (eligible) WebAuthn credentials enrolled:
With password set OR at least one (eligible) WebAuthn credential enrolled:
WebAuthn settings/credential management:
Warning appears when WebAuthn origin does not match RP IDWarning appears when changing RP IDPUT /rest/configignores WebAuthn credentials not already registered (identified by ID)excludeCredentialsis used to prevent registering the same authenticator twiceuserVerification = "discouraged"fails).I intend to add automated tests as well, but would first like a review of the overall design and how it fits into the existing architecture.
Screenshots
WebAuthn button added to login form
New GUI settings
Registering a new credential
Renaming a credential
Separate table of ineligible credentials may appear if "Webauthn Rp Id" is changed in advanced settings
Documentation
Opened:
Probably a new section should be added alongside "LDAP Authentication". WebAuthn normally doesn't require much technical knowledge from the user since you only interact with the end-user side, but in the Syncthing case you are both end-user and server administrator. WebAuthn requires some domain settings to line up, so the "RP ID" and "WebAuthn Origin" settings and their impact need to be explained here. I volunteer to write this documentation once I have the green light to finish the feature.