Add WebAuthn support to GUI (fixes #8409) #8417
Conversation
|
Translations are handled by Transifex, so you shouldn't add anything manually, except for the English strings, which are optional also (as the script will add them later anyway). Another question is whether this should be added to the "main" GUI (as the commit currently does) or rather stay in the Advanced Configuration only (as it is the case with LDAP). |
| createSession(s.cookieName, guiCfg.User, guiCfg, s.evLogger, w, r) | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
It feels a bit weird that these {start|finish}WebauthnAuthentication are in a different file than the {start|finish}WebauthnRegistration methods in confighandler.go, but they do belong to different sections of the API. But perhaps they should both be moved out into one shared separate file and imported from there?
lib/config/guiconfiguration.pb.go
Outdated
| Nickname string `protobuf:"bytes,2,opt,name=nickname,proto3" json:"nickname" xml:"nickname,attr"` | ||
| PublicKeyCose string `protobuf:"bytes,3,opt,name=publicKeyCose,proto3" json:"publicKeyCose" xml:"publicKeyCose,attr" restart:"false"` | ||
| } | ||
|
|
There was a problem hiding this comment.
Perhaps this should be in a separate file? Or further down in this file? I'm not sure.
There was a problem hiding this comment.
Oh, and I'm not sure whether the protobuf tags are needed here, or on the new GUIConfiguration fields. At least, it seems I didn't have to make any changes to the XXX_* methods in order for WebAuthn to work. Do we need those for this?
There was a problem hiding this comment.
These .pb.go-files are auto generated. The proper way to add fields is to add them to the .proto files under proto/ at the repo top level, then run go run build.go proto (which may need some dependencies, I don't fully remember...).
There was a problem hiding this comment.
Alright, thanks! Should be fixed now.
|
That's awesome! I'll look into this and answer questions. In the meantime, after just glancing at your description, I think it would be nice to axe the http basic auth prompt completely. That is, if we have a real login page, we should present that always. (Possibly excepting the case where basic auth is present without us having prompted for it, like in a wrapper that adds it automatically.) |
|
This seems to have poor browser support though. I've tested the code locally, and the GUI doesn't load in IE11 at all. Also, according to https://webauthn.me/browser-support and https://caniuse.com/webauthn, the minimum Android requirement is version 7+, while Syncthing itself currently supports Android 4.1. Would it be possible to load the new authentication only when the browser/wrapper supports it? |
Great, I agree! And yes, we can easily keep checking for the presence of the And right, I meant to point something out in the diff but forgot to: currently the login form works by sending an XHR with the
Hm, I thought this check would fix that, but honestly I forgot to test it because I don't have a non-compatible browser on hand. Do you get any error in the JavaScript console? |
Sure, this is the output. The problem is about using backticks in https://github.com/syncthing/syncthing/pull/8417/files#diff-77bf455925e2b856359d0a3f63bd07b75fe2990adf70d4996de0df0a0cfe8b29R1546. The fix is at https://stackoverflow.com/a/43048518. The other error still persists though, although the GUI does load now. Edit: This is unrelated, but I'm seeing the following in all browsers
with the cryptic |
|
As we're adding a proper loginpage (yay!) could we also fix the favicon for password managers while we're at it? tl;dr provide a static /favicon.ico without requiring authentication |
Ah, of course. Will fix, thanks!
I believe this is because the translation string isn't in place, at least it seemed to fix itself when I added them. You can try running on commit 9dea90a to confirm.
Hmaybe? The favicon assets |
|
At least for Bitwarden it should be enough to have the initial favicon in the HTML without requiring JS and the image to be reachable without authentication. |
… with UI state Before, it was easy for backend state and UI state to end up with conflicts. For example, if you delete a credential in the frontend and attempt to register the same authenticator again before saving, the backend would still include the deleted credential in `excludeCredentials` and the authenticator wouldn't be able to be registered again. By instead setting `excludeCredentials` in the frontend, the above scenario is no longer a problem because the `excludeCredentials` setting is consistent with what's shown in the UI. There is still a remaining state conflict, though: if you cancel the settings dialog without saving, any credentials registered will remain registered even though they "should" be removed because the UI state wasn't explicitly saved.
|
Alright, I've fixed some of the issues encountered so far, and added a todo checklist to the top post. @tomasz1986 please take another look at the support for older browsers! I'll be back with updates for the remaining todo items. @bt90 I'm pretty sure the favicon can be done quite easily once the auth-split router is in place, but it seems out of scope for this PR. |
|
but it seems out of scope for this PR.
All good :)
|
emlun
changed the title
emlun
requested review from
acolomb and
tomasz1986
and removed request for
acolomb and
tomasz1986
|
I was going to do a quick test right now, however I'm getting stuck right at the very beginning. On the first try, I input the username and password, then clicked save, then immediately refreshed the page. The GUI got stuck at the standard login prompt. However, the username and password weren't working at all. I decided to try again. I wiped out the config and started from scratch. I followed the same steps, however after saving the changes, I waited a few seconds and only then refreshed the GUI. This time the new login screen loaded. I logged in, then opened the settings and removed both username and password, then saved the changes, then refreshed the GUI, and this time again the GUI got stuck at the standard login prompt 😵. Edit: The same happens when pressing the "Save changes and reload page…" button as well. Also, on the login screen, most of the buttons/modals inside the Actions menu are either broken or don't load anything useful. It will probably be good to simply disable most of them. For the record, I'm testing everything in Chromium. Just for information, this is all with a completely clean installation and a freshly generated
|
|
Thanks @tomasz1986! I'll look into these issues tomorrow. |
|
Please ignore the above (#8417 (comment)). The problem was that in the past, when launching a new instance of Syncthing, it used to select a random port for the GUI if the default When it comes to the actual testing, here are some of my comments and questions. I've also added a few others directly in the code. Firstly, when it comes to the HTML.
Now, the above were just a few comments, however, this is the big one. How does the new login cooperate with the Android app? Not sure if you're aware, but the app currently uses a hard-coded username of "syncthing" and the password set to the API key. The user isn't allowed to change them (or they can try, but the two get reversed upon restart).
|
|
At least from my understanding, only the Basic Auth prompt is removed. So the web UI integration should still behave seamlessly:
Keeping the credentials locked is a valid concern though. |
| @@ -92,6 +93,7 @@ type Wrapper interface { | |||
| RawCopy() Configuration | |||
| RequiresRestart() bool | |||
| Save() error | |||
| Finish(w http.ResponseWriter, waiter Waiter) | |||
There was a problem hiding this comment.
This is an api specific utility, i.e. shouldn't live in the config package and remain in the api package.
Incidentlally it's also what makes CI break (and tests not run), as you'd need to run go run build.go testmocks to regenerate the mock struct for this interface. However not needed when just not adding it here.
There was a problem hiding this comment.
Ok, thanks, I'll see if I can refactor this.
| if updatedCred.Authenticator.CloneWarning && signCountBefore != 0 { | ||
| l.Warnln(fmt.Sprintf("Invalid WebAuthn signature count for credential \"%s\": expected > %d, was: %d. The credential may have been cloned.", authenticatedCredName, signCountBefore, parsedResponse.Response.AuthenticatorData.Counter)) | ||
| } |
There was a problem hiding this comment.
Can you explain what this counter mechanism does?
Also the messages mentions a comparison to the auth response thing, but the actual condition is just > 0.
And the counter isn't config, right? that should go into the db not the config.
There was a problem hiding this comment.
It's an optional feature that WebAuthn authenticators can implement, in which case every authentication signature by the credential will include a monotonically increasing counter value. So if a new authentication signature has a counter value less than or equal to the last seen counter value, then that means there likely exists two copies of the credential private key. But the feature is optional, and authenticators that don't implement it instead leave the counter constant at zero, so the check is only made if either the new or the last seen counter value is nonzero. You can read more about it here. But the library encapsulates all that in the CloneWarning output, so the zero check here is unrelated.
I think the != 0 check here is just because as far as I understand, the Go convention for searching in a list is to initialize to a sentinel value and use a for loop (unlike, say, Rust where you'd .find(...).map(...) -> Option<u32>). I'd love to be wrong about that and learn there's a better way, though. 🙂
And the counter isn't config, right? that should go into the db not the config.
Hmyeah, I guess so? In that case the whole credential entries should perhaps go in the database instead, as it's not really feasible to type them in manually either. How do you reckon that would work with configuring them in the settings view? Where should I start looking at how to work with the database?
| @@ -59,25 +59,88 @@ require ( | |||
| ) | |||
|
|
|||
| require ( | |||
| github.com/calmh/incontainer v0.0.0-20221224152218-b3e71b103d7a | |||
| github.com/duo-labs/webauthn v0.0.0-20221205164246-ebaf9b74c6ec | |||
There was a problem hiding this comment.
This is an archived repo, with it's readme point out this as it's successor: https://github.com/go-webauthn/webauthn
There was a problem hiding this comment.
Yeah, I noticed but haven't gotten around to migrating it yet. I'll put it on the todo list.
|
I just had a quick look over this PR to get a high level understanding of what's going on, and trying to get into the weeds of the api backend implementation I can only echo this:
It would help me a lot with review if this were broken up into smaller bits - single PR and multiple clean commits or multiple PRs. From a quick glance splitting like proposed in login page (possible pure refactor first?) and webauthn should work and makes sense semantically - anything else is fine with me too. I know what you really want is webauthn - I promise I'll review both (when I review either - can't promise anything there, wouldn't be honest with my current track record. And you can easily enforce it by not addressing my review if I were to ignore the webauthn bit :) ). |
Ok, thanks, glad to hear it. 🙂
Thanks, I'll look into them!
Hm, I haven't tested, but I assume the app uses HTTP Basic Authorization then? In that case I think the app should be unaffected, but for it to continue working in a standard browser it probably needs the advanced setting to be enabled by default in the server. The new login form should work, of course, but you'd need to know that API key and hard-coded username and without the setting it probably won't work to just open a URL with embedded basic auth from within the app. |
|
Whatever works best for you - reducing this one is perfectly fine for me |
Yeah, I don't know the details when it comes to the app code, but if the advanced setting is indeed required, then a) it would need to be enabled automatically upon upgrade or otherwise the user may be stuck with inaccessible Syncthing, and b) the setting would have to be enforced, so that the user basically can't change it (similarly to the currently hard-coded username and password). Basically, the app mustn't allow the user to break it 😛. This is especially true as once locked out, the user probably won't know the API key, and in contrary to the desktop, there is no way to even access I've got the build environment for the app already set up, so I should be able to test what actually happens when trying to use the GUI code from this PR. |
Well I suppose the app itself does have access to the file, so could of course restore access when the service is not running. |
|
I realized I can't change the branch name of this PR, and it doesn't make much sense to have branch |
Whatever works best for you. If the webauthn PR builds on top of the login page one, you might want to PR against the login page branch. Then only the relevant changes are shown in the PR, and once the login page one is merged it will automatically target main. |
|
🤖 beep boop I'm going to close this pull request as it has been idle for more than 90 days. This is not a rejection, merely a removal from the list of active pull requests that is periodically reviewed by humans. The pull request can be reopened when there is new activity, and merged once any remaining issues are resolved. To permanently exempt this PR from bot nagging, add the |
|
#8757 is now merged! With that I'll (soon 😅) proceed to revive this with the remaining additions needed to also support WebAuthn. I'll probably start fresh with a new pull request, but I'll post a link here for the record. |
|
This PR is now replaced by PR #9175. |
st-review
added
the
frozen-due-to-age
Fixes #8409.
Remaining things to do
Store transports and return inNot currently supported by duo-labs/webauthnallowCredentialsPurpose
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.
Testing
Manual tests performed:
Basic authorization prompt appears like beforeRemoved!PUT /rest/configignores WebAuthn credentials not already registered (identified by ID)excludeCredentialsis used to prevent registering the same authenticator twiceI 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
HTML login form
New GUI settings
Registering a new credential
Renaming a credential
Interactive warnings
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.