(With #4 merged, this now targets syncthing#9175 directly)

Wow, this is a rather elaborate change. I'd frankly get the WebAuthn stuff merged first before starting on this refinement. At least in this level of detail / sophistication, I don't see an immediate need or it being a blocker for the main PR.

One more thing that comes to mind, but should also be punted off for a later PR: We do show a message and block WebAuthn login when no credentials are configured. But I think we don't do the same for the opposite case when no user / password is configured but only WebAuthn. Should be discussed whether giving that indication would actually weaken security.

Wow, this is a rather elaborate change. I'd frankly get the WebAuthn stuff merged first before starting on this refinement. At least in this level of detail / sophistication, I don't see an immediate need or it being a blocker for the main PR.

Ok, you'll have to debate that with @imsodin who requested it be part of the WebAuthn PR. Or alternatively, I guess we could also isolate this as a standalone change on the main branch.

We do show a message and block WebAuthn login when no credentials are configured. [...] Should be discussed whether giving that indication would actually weaken security.

Yeah, this does leak the information that WebAuthn is not configured, which - without this explicit "require auth" option - implies that password auth is configured, which is likely an easier target to break. When WebAuthn is configured, there is no indication of whether or not password is configured.

In order to efficiently support external security keys with limited storage space, we need to first query the server for the allowCredentials parameter to initialize the WebAuthn request. Since PR syncthing#9175 implements WebAuthn without a password as a first factor, this information needs to be exposed to a non-authenticated user. So within those constraints (which I'll admit I haven't documented very well) we can't really eliminate this information leak anyway - the condition will be observable in the HTTP API whether we indicate it in the UI or not.

There are a few ways we could eliminate the information leak:

1. Use an empty allowCredentials parameter. This would require all WebAuthn credentials to be discoverable credentials, and consequently consume storage space on the authenticator. Apart from this information leak, we have no other technical reason to make this restriction. See the documentation PR for more explanation and discussion of this.

   This would also make the user experience much more confusing - people would upgrade and get the option to "Log in with WebAuthn" offered as if it's supposed to work. Their browser/OS would do its best to help them make it work under the assumption it should, even when the user hasn't yet set it up and the server knows that it won't work.

2. Require password authentication before revealing a non-empty allowCredentials. I am personally against this, because much of the point of why I want WebAuthn in the first place is to get away from password authentication, not primarily for two-factor authentication.

3. Inject fake entries in allowCredentials as described in §14 Privacy Considerations 6.2. Username Enumeration of the spec. This is probably easier said than done, though I'd be less opposed to implementing it in an application than as a library feature for other applications to depend on. But this would also introduce the same user experience issues as option (1).

Weighed against these concerns, my personal opinion is that the information leak seems fine. I don't think it seems like much of a problem in Syncthing's case. I assume that most Syncthing instances are only accessible via loopback or within a restricted intranet, and very few are exposed to the internet, so it doesn't seem very useful to be able to probe around for Syncthing instances that allow password auth (which, by the way, is currently every Syncthing instance that requires auth, unless it uses LDAP).

Actually - as I alluded to at the beginning - this very PR is a kind of 4th option: add the option to disable password auth, while still requiring auth even if no authentication method is configured. I hadn't considered it before writing this, but this actually also mitigates the impact of the information leak, because the fact that auth is required no longer necessarily tells you whether or not password auth is enabled.

OK, this is a lot to get my head around, but I've taken some time to rethink this whole "conditional authentication" thing.

add the option to disable password auth, while still requiring auth even if no authentication method is configured.

That is definitely an undesired state (total lock-out). Sure, it doesn't disclose to an attacker which vector would be feasible, but to get the same usability (none), one could simply block the GUI access completely. 😉 So let's put aside the information leak for now.

I really don't like this additional "require auth" boolean option, mainly because it does open all these possibilities for inconsistent configuration (a.k.a. footguns). I do however support moving the WebAuthn config to the regular config.xml storage (see #7, with slight restrictions). So this change was basically motivated by the "auth configured, but accidentally lost all credentials" case, which would be prevented by #7. Because if all configuration lives in the same place, screwing up there would have the same probability for all cases. Locking yourself out and accidently granting unauthenticated GUI access are the two extremes, so let's look at how to reach each of them:

1. Locked out: No user or password is set, so that way to authenticate is implicitly blocked. In addition, WebAuthn credentials are configured but not usable, because of a wrong RP ID. That's why changing the RP ID is made hard by hiding it under advanced settings. Accidentally removing the user & password is possible, but can be recovered as long as one doesn't log out, invalidating the session cookie. It's expected that misconfiguration may result in lock-out, so users should be aware of it and carefully check before logging out without a way to recover.
2. Authentication bypass: Ideally this would require removing all WebAuthn credentials and unsetting the user or password field. As long as this happens through the same config UI, and there is no way to accidentally lose credentials by resetting the DB (for some unrelated reason), I see this as having acceptably low footgun potential.

I propose the following logic, instead of adding this additional boolean config:

Authentication required := (username AND password)
                           OR WebAuthn credentials defined
WebAuthn credentials defined := length(WebAuthn config) > 0   # disregarding RP ID mismatches

This results in:

Lock-out situation := NOT (username AND password)
                      AND WebAuthn credentials defined
                      AND length(eligble WebAuthn credentials) == 0

Main difference to the current state of syncthing#9175: Ineligible credentials (with RP ID mismatch) still lead to requiring auth, but do not facilitate logging in. Preventing lock-out could be done by requiring a new, eligible, WebAuthn credential to be added simultaneously when changing RP ID, or all now ineligible credentials being deleted, or having password auth available as a fall-back. These conditions could be checked upon saving, or simply stated in the documentation. I expect anyone changing the RP ID in advanced settings to have read the docs first, so avoiding lock-out programmatically could very well be excluded from the big WebAuthn PR (at first).

Leaking that password auth is available is not any worse than the pre-webauthn state, so it's definitely not a blocker. Beyond that I don't care about details and in case of tradeoffs would go in favor of UX or simplicity/feasibility (getting this change done). Notably I did not forget security, I do mean to say I don't care, see first point. Just my opinion.

As for using some criteria like proposed by acolomb over this additional config flag: Fine as well with me, combined with a warning in the UI whenever a config change causes the "auth required state" to go from true to false. Not even needed to be targeted, just put something like "Continuing will disable auth for the web UI, are you sure you want that?" in the users face before actually saving the config change. I just want it to be very hard resp. have the user be very explicit in their intent, before making the web UI world accessible when it was previously not.

Ok, so what I'm hearing is we drop this PR and instead add a pre-config-save warning before saving config changes that would disable the authentication requirement. Did I get that right? If not, let me know and we can re-open this and reconsider.

Yes, in principle. Let's first concentrate on #7 and as I said above, that reduces the risk significantly already. The remaining potential for lock-out or accidentally leaving the GUI completely open should be checked while saving the config. But IMHO that part could be punted off for a later PR, not needing to block the WebAuthn integration in general.