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. |
|
Alright, this is finally ready for final review to merge! 🎉 Thanks everyone for your patience. All that remains now is documentation, which I'll open as a separate PR.
|
|
I will try to look at the GUI side of things sometime at the end of the week! At the moment, I would just like to ask one question about the name itself. "WebAuth" appears to be a shortened form of "Web Authentication" (see https://en.wikipedia.org/wiki/WebAuthn). Wouldn't it be preferable to use "Web Authentication" in full, at least in the user-facing HTML? This is because the form "WebAuthn" doesn't really mean anything to someone who doesn't already know what it is, especially if they don't speak English. What do you think? Also, would you be able to provide a few examples of wording some other major services or websites use when they offer WebAuthn login options? |
|
They do have error messages, just very well hidden - likely due to the concurrent test runs. Would still be nice if go at least indicated the failing tests at the end of the output. Windows one is in webauth stuff and I don't understand it at first glance:
macos looks like a plain boring timeout, probably due to concurrent execution and unrelated to your PR:
As for the linter: Clearly not an issue here given the state of |
I somewhat agree, but also therefore disagree. 🙂 "Web Authentication" is indeed the title of the spec, but it does a pretty bad job as a name. "Web Authentication" could mean any form of authentication on the web, but "WebAuthn" is now a pretty well-established name for this standard specifically. I would keep the "WebAuthn" term to distinguish from other forms of "authentication on the web", such as passwords and OTP codes.
Few or none use the term "WebAuthn" in user-facing UI, instead most use the terms "passkeys" and/or "security keys". GitHub, for example, uses both. I've considered changing to "passkeys" in the UI, but I hesitate because that's not quite technically accurate to how we use WebAuthn. A "passkey" is usually defined as a WebAuthn credential created with Another thing that somewhat sets Syncthing apart is that the end-user is also the service operator. This should hopefully not matter much with the default settings, but it does matter as soon as you want to host the web UI on some other address than |
|
Oh, I see, it does show up in the raw log view. All I saw in the job view was this, so I assumed the whole build was borked:
I'll take a look at the errors, then. I've seen "EOF" errors like these caused by the server crashing, so that might be what's happening in these
Alright, if you prefer only pointer receivers I can fix that for the |
Just a thought: A sensible way to handle this on language / compiler level would be to specify a non-pointer receiver argument, but skip the copying code if the compiler knows for sure that the method does not do any write access. Of course that would have to be conveyed through an attribute on each compiled function, in case the receiver is passed down to a nested method call. It would convey the inferred "this is not a pointer receiver, thus immutable / read-only" semantics, but still save the actual copying into a new object if not needed. Maybe the Golang compiler is already smart enough to do that? |
No need at all to change here. I just couldn't stop myself from commenting. As I said in the beginning, the fact that we already do value receivers for gui config (and many other) make it perfectly fine to do the same here. And I am not even certain on my point. As you correctly identified, with go it's quite often tradeoffs without a clear correct way. I totally sympathise with the idea of using semantic types (values if immutable), that definitely has value (badumm) too. As to what the compiler does and any further interesting digressions on value vs pointer (sorry for starting that) should probably go somehwere else/the forum. This PR is long enough without it :) |
|
I'll need some help troubleshooting the Windows/Mac failures. It is consistently the They do consistently print logs like this: where the All this points to some kind of race conditions or timing issues, but I can't figure out what. I tried to model Might anyone be able to help troubleshoot this? |
TestWebauthnConfigChanges seems to need a longer shutdown timeout when running on GitHub Actions. The problem manifests in errors like this: ``` 2024-05-05T17:00:45.0503602Z api_test.go:2919: TestWebauthnConfigChanges/Can_edit_GUIConfiguration.WebauthnUserId Put "http://127.0.0.1:37585/rest/config/gui": EOF [] 2024-05-05T17:00:45.0566336Z --- FAIL: TestWebauthnConfigChanges/Can_edit_GUIConfiguration.WebauthnUserId (0.52s) ``` indicating that the server was forcefully shut down (or panicked, but that was ruled out in these cases) before the request was fully written.
|
Alright, I think I found out why the tests were failing: the server shutdown grace period on config changes was a bit too aggressive at 100 ms. 🙂 I made that grace period configurable in tests (7ed2db8) and increased it to 1 second for As mentioned above, most DeepSource warnings do not originate in this PR, and I was told to ignore those that do (#9175 (comment)). All that's left now is a PR to the docs repo, which I'm currently working on. |
|
Hold that thought - the WebAuthn login flow seems to be broken for some reason 🤦 even though it does successfully return a session cookie which is sent in subsequent requests. Might be caused by PR #9425. Investigating... |
|
Yeah, the WebAuthn credential state update is causing a server restart because it's changing config, which causes the I think someone mentioned at some point before that these state variables really should be stored in DB rather than config - this settles it. |
Fixes #8409. Replaces PR #8417.
Remaining things to do
Purpose
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
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.