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.
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 |
|
@emlun Are you still up to finishing this, with the required SQL-fu skill level? I haven't looked much at how SQL is handled in Syncthing-v2 in general. But what I've seen seemed pretty easy to work with. There is precedence already for adding new tables in a migration, keeping some tables in a separate DB file (which would make sense here IMHO), etc. I'm sad we didn't get this thing over the finish line before the SQL migration landed. But OTOH it now allows doing a clean design, instead of living with a blob-based storage shoehorned into a catch-all SQL table full of random blobs. If you want to continue and have the capacity to work on this further, I'm happy to help. |
|
Just tried to fix the merge conflicts from the main branch, but only partial so far. The logging framework switch still needs some adjustments in added modules. |
|
I am, but I need to find the time and energy to get to it... sorry 😅 I was also concurrently resolving the merge conflicts. Looks like we did it mostly the same, but I'll push my additions shortly. |
|
Thanks @acolomb, I've pushed the remaining merge resolutions now (including the log statements referencing nonexistent methods, but not a comprehensive migration). You're very welcome to take a swing at this if you like. I'll let y'all know when I get back on this, to avoid double-work. |
|
Cool, thanks for picking it up. I just ran out of time and committed the first state without conflicts. Will let you know how far I got when I found some time to take a deeper look at the needed adjustments. |
| @@ -0,0 +1,22 @@ | |||
| Copyright (c) 2019 GitHub, Inc. | |||
There was a problem hiding this comment.
According to (the now archived) https://github.com/github/webauthn-json/blob/main/README.md :
As of March 2025, stable versions of all major browsers now support the following methods:
PublicKeyCredential.parseCreationOptionsFromJSON(…)
PublicKeyCredential.parseRequestOptionsFromJSON(…)
PublicKeyCredential → .toJSON()
These should be used instead of @github/webauthn-json.
Should we consider this change prior to release?
There was a problem hiding this comment.
Definitely sounds like a simplification worth adopting.
There was a problem hiding this comment.
@emlun Do you think this is worth trying to tackle now? If so, I can look into it. It should simplify things a smidge, yes?
|
Any chance we move this into beta release now that V2 has stabilized? |
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.