Moving it to config makes sense. I'd prefer if you moved it into the gui config, since it's only relevant to the gui, and then ignored it for restarts. There's already prior art for doing that: https://github.com/syncthing/syncthing/blob/main/lib/api/api.go#L484

I second the request to move these credential settings to the GUI configuration section. However, with one restriction:

When using the WebAuthn login, the signature counter and last used timestamp fields are updated each time. Both are done for added security. But it's a new scenario regarding our config handling, because so far starting up Syncthing without doing any configuration changes won't try rewriting the config file. Thus in theory, config.xml could be write-protected, but no longer with this change.

So I suggest to keep these two dynamically changing fields out of the config. We can add them to the database for the added security, but that could even be a second step after getting WebAuthn working at all. When the database is reset, this info would be lost, but that's uncritical. The signature counter just needs to be smaller than what the authenticator reports (which zero always is), and the last used timestamp is just for the user to investigate.

This would also mitigate some of the concurrency / locking concerns, at least for the registration side, where serializing config changes is already taken care of.

What do you think @emlun and @imsodin?

IMO changing config is a non-concern resp. non-writable config is not workable anyway. Pretty sure syncthing will crash and burn eventually. E.g. when the first device/folder share request comes in.

In principle I agree that such counters/timestamps are a stretch to call config. Practically I don't feel like requiring changes/complexity just to avoid that (my opinon only of course).

I am out of the loop on the concurrency/locking concerns, no comment there.

Why should it crash with a read-only config? Sharing requests are stored in the DB. What else do you see that requires config writing at random times? I think it's a worthy goal to not depend on writable config files.

Device config pending and ignored folders. Crash and burn was meant figuratively, and yeah acked, would be better to avoid that in my comms. And checking what happens for real, indeed it won't crash and burn, it will just not persist the change and warn the user about that.

My main issue is feature creap. Irrespective of a goal being worthy, I don't think it's fair to put higher standards to a new contribution than what's existing. Also worth repeating that I do agree with the desire to not put those values into config, I just want to weigh that against additional work and complexity it may induce. If emlun agrees and feels the work isn't an issue, by all means lets move it out of the config, but I don't feel it's worth blocking the PR over.

Well this is already a meta-PR into the actual syncthing PR and I find that approach works out very well. We can have thorough discussions and help out with finishing the feature, without inflating the main PR needlessly. And correct me if I got it wrong @emlun, but I think you welcome this input to make sure the feature is integrated fully-baked, with all concerns about the code addressed. In this case I don't think it's feature creep, as the info needs to be stored somewhere and we already have working proposals for both DB and config storage. Separating and moving around some settings shouldn't be much more effort then.

I've experienced what it takes to change such misplaced storage decisions, that was actually my first big PR to Syncthing - moving the pending folders / devices from config to the DB (syncthing#6443). That's why I'm certain this part is not something that prevents having a read-only config. People who care about write-protecting a config file will be aware that saving settings from the GUI will fail. But pending device requests or updating counters for the login process happens in the background and should not fail silently.

Of course it would be nice if one could always nail it on the first try, but I've done software dev long enough to know that most features take more than a few iterations before you decide on a solution you're happy with. 🙂 And yes, I tend to prefer getting things complete enough to not require additional changes for them to be functional or to fix things that (knowingly) broke in the initial change set.

I do agree that most of this isn't really "config", since you can't type the credential ID and public key in manually - even with the help of format converter tools - but need to perform the setup ceremony together with the authenticator. But maybe config is still the best home for it, at least of the currently built in storage options. At least the static parts, that is. I like the idea of moving just the dynamic parts (counter and last used) into DB - as you say, these two can always be reconstructed on next login in case the user resets the DB, so that's no big deal either. I tried this out and I think I like it, so I'll update the PR with this. I've rebased on top of the current state of syncthing#9175 to untangle this from #5.

@imsodin @acolomb Do you want to re-review these changes here before I merge them into syncthing#9175?

Also just had a quick look over it - no further review outstanding from my side to merge this.