I’d like to obscure API keys in the config. The API key is essentially a password and we should not store passwords in clear text. I envision the following as “doing it properly”:
We store the bcrypt hash (same as the user password) of the API key, together with a comment and creation time.
We add a new config screen “API Keys” where the user can:
Create a new API key. The flow here is to tap “New API key” at which point we generate one and ask for a comment to attach to it. This is the last time the API key is available in clear text, and the dialog explains this.
See which keys exist (comment and creation time only)
Revoke any of the existing API keys.
We continue supporting the environment variable STGUIAPIKEY. If it is set, it is added to the set of valid API keys on startup.
We remove the -gui-apikey command line flag. Command line flags are visible to other processes and users in ps output or the platform equivalent. There are ways to hide this but it’s not trivial on all platforms. Environment variables are typically private to the process that sets them and its children. Maybe we add a command similar to -generate to add an API key to the config of a not running Syncthing.
When adopting this scheme we hash and save the currently configured API key as “Default API key” or similar. In the future we ship without a default API key as it does not make sense.
Doing a bcrypt comparison against all API keys for every request is however way too expensive. We will therefore keep an in memory map of recently seen API keys and their validity. This must be cleared when a config change is posted. The usual delays can be added to requests with incorrect API keys.
Sounds good, but not sure why you suddenly think this is an issue. I feel we will have a lot of questions of “how do I get the API key” after the fact of generation.
If we display this to the user, we’ll probably want to have a means of associating a name with this key and making it unrevokable. Alternatively we could just not display this key to the user and keep them blissfully happy…
One reason is that the latest vulnerability wouldn’t have happened with this in place. And just a general feeling that we should not have sensitive stuff floating around in clear text. I didn’t invent the scheme above though, it’s how gmail handles device passwords for example, so at least some users should be familiar with it.
I think adding this, and allowing it to work with a running Syncthing instance, is key to providing a migration path for various wrappers.
I think a wrapper needs to be able to call syncthing -generate-api-key "Syncthing GTK", and get back an API key to use. Otherwise user interaction has to be introduced into the setup, which makes life harder for everyone.
But if syncthing-inotify can no longer sniff the API key from the config you have to provide it via the -api cli option which, in my opinion, is a change from bad to worse.
I suppose syncthing-inotify could be modified to get it’s own config file but this might also be a good argument for Issue #2741 Merge syncthing-inotify.
While I understand the desire to remove the API key from the config file, I’d like to add the following thoughts:
As long as not all tools switch to reading the API key from stdin, it can always be spoofed at least in Linux with ps aux when specified on command line or via /proc/<pid>/... if as environment variable.
Also, and in my opinion even more important, the config.xml is in the same directory as cert.pem and key.pem, so if an attacker can read the configuration file, he can “own” way more than the API key.
One problem is that the config can be read through the API, and the API key is included in this. So you don’t need filesystem access to read the API key.
Not sure what you’re trying to say in your comment about spoofing.
One solution, in addition to suggestions made above, may be to put an API key in a file in the config directory, with permissions 600. This can’t be read by the API, but can be read by tools providing integration, which are already running with the same permissions as Syncthing.
True, but to read it via API, I need either an API key or the username/password, right? Either way, I already “own” the installation.
I mean, that for other processes on the same machine, even by other users, it is possible to read any command-line argument. And (at least for root, don’t know about non-root) it’s possible to read the environment variables of a process via /proc/. So even we are thinking about security, we should also remove the possibility to specify API key via command-line at all and only have a possibility to be in a file and/or to be read via stdin.
Does this provide more security than keeping it in the config.xml? We could simply make the API-key non-retrievable via API.
No, you do not need an API key to read the config. This combined with a CORS issue lead to a recent vulnerability. See calmh’s first messages in this thread for the motivations for this change.
Calmh already addressed deprecating the mechanism to pass an API key on the command line. Not sure about env vars.
Under the current scheme, the API needs to be able to retrieve the API key, in order for the GUI to display it to the user. One of the reasons behind changing the scheme is so that the API key is now not shown to the user, which means that it doesn’t need to be retrievable over the API.
The advantage of putting a plain-text API key in a separate file is that 1) you can our more stringent permissions on it, and 2) third party programs don’t need to try and parse the config file: remember that the format of the config file can change at any time.
Just a quick question, how is client application going to guess API key for already running Syncthing instance in this scheme? Syncthing-GTK has ability to connect to daemon that is not started by it, but started by service manager for example.
And by the way, using STGUIAPIKEY is no more secure than current API key stored in config file. If you can read syncthing’s config.xml, you can read /proc/<syncthing_pid>/environ as well.
I think you guys are over thinking this. The API key is essentially a password. We should not be storing and exposing passwords in clear text. There is no justification to do so in my mind.
As for accessing the config on disk and environment from /proc, as has been said - if you can do that, you can also write to the config, read the secret key file, and access all files that syncthing is syncing. Clearly not having the API key in the environment would be more secure than having it there, but on balance I think the convenience is worth it for the wrapper authors.
One option here could be to have a named pipe/socket in the config dir, read/writeable only by the user running syncthing. Commands given there could be accepted as authenticated. It would be a way to access syncthing from the same user without API key. I know this works on unixes but don’t know if the same mechanisms exist on Windows?
I’m mostly suggesting this as a way for syncthing to talk to syncthing, but it could of course be used by any integration thing. In Go at least its fairly trivial to start the HTTP/API server on a named pipe and talk to it.
But calling syncthing -generate or STGUIAPIKEY=XYZ syncthing will not work if syncthing is already running and I just want to connect to that instance.
I’m seriously worried that this use case will not be possible without readable API key. And I’m using syncthing like that
[quote=“calmh, post:16, topic:7493”]
It would be a way to access syncthing from the same user without API key. I know this works on unixes but don’t know if the same mechanisms exist on Windows?
[/quote]Windows have Named Pipes and quick google search shows go package for it, but I’m really not sure how is security handled on those.
Connect to a running syncthing on some random machine on the internet. You need an address and an API key from the user.
Manage a locally running syncthing. You start it, so you set the API key in the environment.
A locally running syncthing not controlled by you is a special case of the first use case. But if you’re running as the same user it could be solved by using a named pipe or using -generate and telling the user they must restart the reconfigured syncthing.
Being able to pick credentials out of the config file in clear text is a historical accident and not how API keys in general are ever intended to work.
I hear you, but my point remains. I don’t think having to enter an API key into a separate UI controlling a service of some kind is an undue burden, but if we can securely simplify that some way that’s nice.
