Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encrypt API tokens #2360

Merged
merged 3 commits into from
Feb 18, 2022
Merged

Encrypt API tokens #2360

merged 3 commits into from
Feb 18, 2022

Conversation

adriankumpf
Copy link
Collaborator

With this PR, API tokens are stored in encrypted form in the database.

During the database migration a randomly generated key will be used encrypt the tokens if no ENCRYPTION_KEY environment variable was provided.

If the application is started without the presence of an ENCRYPTION_KEY (or if the key failed to decrypt the existing tokens), the UI will display a warning with further instructions.

@adriankumpf adriankumpf added the WIP Work in progress label Jan 23, 2022
@Hypfer
Copy link

Hypfer commented Jan 25, 2022

Please just ignore this comment if I missed something as I haven't used this software, but I can't but wonder if this is really the correct approach.

So if I understand it correctly, the issue here was that Grafana used the same db credentials as the data collection service, meaning that it also had access to the tokens stored in the db, right?
To prevent that from happening, this PR will encrypt the tokens stored in the DB with a secret that is only known to the data collection service provided via an env variable, yes?

What confuses me about is is:
Why not instead just use a different postgres user with different permissions for grafana, so that it can only see the collected data but not the configuration stored in the db?

I feel like I'm missing something here

@adriankumpf
Copy link
Collaborator Author

adriankumpf commented Jan 25, 2022

Please just ignore this comment if I missed something as I haven't used this software, but I can't but wonder if this is really the correct approach.

So if I understand it correctly, the issue here was that Grafana used the same db credentials as the data collection service, meaning that it also had access to the tokens stored in the db, right? To prevent that from happening, this PR will encrypt the tokens stored in the DB with a secret that is only known to the data collection service provided via an env variable, yes?

What confuses me about is is: Why not instead just use a different postgres user with different permissions for grafana, so that it can only see the collected data but not the configuration stored in the db?

I feel like I'm missing something here

Ultimately, this is a second layer of protection should someone manage to gain access to Grafana.

Another Postgres user that does not have access to the tokens table would work just as well. Just note that the upgrade path for existing users (and the setup for new ones) should be as simple as possible. I don't see that being the case with that approach. Whereas the encryption can be done automatically during the update.

@Hypfer
Copy link

Hypfer commented Jan 25, 2022

Whereas the encryption can be done automatically during the update.

It does still require the user to edit the container config so that the secret gets stored though

@adriankumpf
Copy link
Collaborator Author

adriankumpf commented Jan 25, 2022

When starting the container after the update, the tokens will be encrypted with a random key (if no key was provided by the user). So you will still be logged in after the update even though the tokens are already encrypted.

A warning message displayed in the UI will then provide further instructions on how to proceed in this case. So even if you do not read the release notes and upgrade blindly, TeslaMate will continue to work after the update. Only after a restart you'll be required to log in again (if the container config has not been updated at that point).

@martinh2011
Copy link
Contributor

And in postgresql backups the tokens will be encrypted, too. So even if somebody gets hold of a teslamate database backup, the encrypted token is useless. Not a very likely threat, though, but nice to have.

@adriankumpf adriankumpf changed the title Draft: Encrypt API tokens Encrypt API tokens Jan 25, 2022
@adriankumpf adriankumpf removed the WIP Work in progress label Feb 18, 2022
@adriankumpf adriankumpf merged commit 0d6e288 into master Feb 18, 2022
@adriankumpf adriankumpf deleted the encrypt-api-tokens branch February 18, 2022 16:03
@parkr
Copy link

parkr commented Feb 18, 2022

@adriankumpf Are there guidelines/best practices for generating an ENCRYPTION_KEY? I was going to add this to my config today but am not sure what is allowed in this field. Thanks!

@adriankumpf
Copy link
Collaborator Author

@adriankumpf Are there guidelines/best practices for generating an ENCRYPTION_KEY? I was going to add this to my config today but am not sure what is allowed in this field. Thanks!

In general, the same guidelines apply to the encryption key as to strong passwords:

  • Long passwords are more secure
  • Make your password unique

There are no limits on character length or characters allowed.

@parkr
Copy link

parkr commented Feb 20, 2022

@adriankumpf Sounds good. I was mostly curious about allowed character sets, but it sounds like any bytes will do. Thanks!

adriankumpf added a commit to fmossott/teslamate that referenced this pull request Apr 22, 2022
* master:
  Update timeline.json (teslamate-org#2601)
  Update charging-stats.json (teslamate-org#2566)
  Update to projects page (doc/website) (teslamate-org#2518)
  Update States top row panels height (teslamate-org#2487)
  Do no start EPMD
  Bump grafana/grafana from 8.4.1 to 8.4.5 in /grafana
  Bump docker/build-push-action from 2.9.0 to 2.10.0
  Bump erlef/setup-beam from 1.10 to 1.11
  Bump docker/login-action from 1.12.0 to 1.14.1
  Bump actions/cache from 2.1.7 to 3.0.1
  Bump website deps
  Bump deps
  Bump actions/checkout from 2.4.0 to 3
  Restart stream process if token expired
  Update charging-stats.json (teslamate-org#2481)
  Fix typo
  Update POT files
  Update CHANGELOG
  Move encryption key warning into .container
  Encrypt API tokens (teslamate-org#2360)
  Update frontend deps
  DC charge curve scatter graph (teslamate-org#2093)
  Update CHANGELOG
  Bump Grafana to 8.4.1
  Added panel with the cost of charges in SuC (teslamate-org#2448)
  Add datasource to table and map panels. (teslamate-org#2391)
  Update charging-stats.json (teslamate-org#2461)
  moved subselects to "from" clause (teslamate-org#2464)
  Added ProxyPreserveHost On to the Grafana entries in Apache2 config (teslamate-org#2471)
  Render Trip piechart legend (teslamate-org#2473)
  Bump follow-redirects from 1.14.7 to 1.14.8 in /website
  Update default.po (teslamate-org#2479)
@okamiokami
Copy link

okamiokami commented Oct 15, 2022

As a non technically minded user (not to this level anyway) I find this extremely confusing.

Teslamate is working, which is the most important thing for me, but I keep getting the “create encryption key” notification.

I have close to zero experience with Docker and I blindly followed the install guide 1.5 years ago.

Now, I made a few tries and I got a copy of the yml file, but editing it (adding the encryption key taken form the logs, with the correct syntax from the guide) had no effect on the notification, which is still there. I am also on Postgres 13 and changing the Postgres bit in the yml setting resulted in downloading Postgres 14 and Teslamate not working anymore (which shows that I put the YML back in the right place, and confirms that just adding the encryption key had no effect). I stopped and started the container (and the NAS) a few times in between.

My question is: is there any easy (or completely guided) way to add the encryption key to settings?

Alternatively, what would happen if I backed up my current data, erased Teslamate completely and reinstall it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants