mirror of
https://github.com/louislam/uptime-kuma.git
synced 2026-03-02 22:57:00 -05:00
Can we use database encryption instead of writing everything as plaintext #3349
Labels
No labels
A:accessibility
A:api
A:cert-expiry
A:core
A:dashboard
A:deployment
A:documentation
A:domain expiry
A:incidents
A:maintenance
A:metrics
A:monitor
A:notifications
A:reports
A:settings
A:status-page
A:ui/ux
A:user-management
Stale
ai-slop
blocked
blocked-upstream
bug
cannot-reproduce
dependencies
discussion
duplicate
feature-request
feature-request
good first issue
hacktoberfest
help
help wanted
house keeping
invalid
invalid-format
invalid-format
question
releaseblocker 🚨
security
spam
type:enhance-existing
type:new
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/uptime-kuma#3349
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @SharonMenachem on GitHub (May 21, 2024).
📑 I have found these related issues/pull requests
No found any
🏷️ Feature Request Type
Other
🔖 Feature description
Database file encryption (Plaintext) in the SQLite to avoid any other users with the same access to watch on IP's logs and others (Using Hash sha-1 like the password of the users)
✔️ Solution
I would love if the plaintext in the database would not be able to read by any standard db browser.
❓ Alternatives
No response
📝 Additional Context
No response
@CommanderStorm commented on GitHub (May 21, 2024):
So you have FS-level access-permissions to read the database of the node itsself...
How would we write to the database? We would likely have an encryption key lying around somewhere on the same disk
=> you could just read the encryption key and read the db anyway
I don't see this as being possible or adding security.
=> reclassifying as
helpWhat is your actual usecase? I think this is a XY-Problem.
@thielj commented on GitHub (May 22, 2024):
A use case is for example container volumes. When they are detached, plain text passwords remain on the volume or the underlying storage system. The solution in many other projects is to provide an encryption key or seed through env variables or secrets that are not written to disk or into the database.
Or consider the case of an exploit enabling an attacker to download the database. It doesn't even have to be a vulnerability in U-K. This is exactly the scenario I was talking about when I mentioned splitting monitoring duties between an internal instance that isn't exposed to the net, but instead 'contributing' to an external instance that's used for simple reachability checks and displaying status pages.
@CommanderStorm commented on GitHub (May 22, 2024):
In v1, the performance budget is too tight to include this.
In v2, you can use MariaDB and https://mariadb.com/kb/en/data-at-rest-encryption-overview/ to achieve this.
Please see #4500 for the status of said release and what needs to happen before we can publish a beta.
Enabling the SQLite crypto extension https://www.sqlite.org/see/doc/trunk/www/readme.wiki does not seem worth dev-time as I don't think this would help with actual security that much.
Think this resolves your request.
If you have insights of how SQLITE-SEE works and if this is simple to configure/maintain and performant enough (provide a benchmark ^^), we can reopen the issue.@thielj commented on GitHub (May 23, 2024):
It's unnecessary to encrypt heartbeats and events; but connection strings, authentication headers and such shouldn't be persisted in plaintext, as much as you don't store login passwords in plaintext.
There's no performance issue with that either.
@CommanderStorm commented on GitHub (May 23, 2024):
We are not splitting our database.
This would be exponentially harder to test (at least I can't come up with a design that would not be a major rewrite, having quite subtle bugs or missing features).
Remember that we still need to support both sqlite and mariadb/mysql.
=> would be way too risky given the current test coverage
About the performance aspect:
Yes, I have gotten shit for this yesterday as well while sitting on the couch with @Valentin-Metz already.
Still don't think we should add this extra complexity into the sqlite part.
If people need it, they can reach for https://mariadb.com/kb/en/data-at-rest-encryption-overview/.
@thielj commented on GitHub (May 23, 2024):
I didn't say "split the database"; you actually don't even need to change the database at all. Where you save plaintext strings now, you save encrypted bytes - base64 encoded if you prefer.
And re MariaDB, that option is mainly targeted at cloud databases with keys managed by the provider or some other means. Even with that enabled, you would still encrypt valuable data yourself.
Give me one good reason why my login information to other systems should be stored less secure than my login information to your app.
@thielj commented on GitHub (May 23, 2024):
See second paragraph here: https://learn.microsoft.com/en-us/azure/mariadb/concepts-security
Without a (hardware based) KMS infrastructure in place, MariaDB's encryption at rest features won't be of much help.
@CommanderStorm commented on GitHub (May 23, 2024):
As I read the mariadb docs, this is not the case.
Hashicorp and File are both valid KMS plugins.
From the Microsoft article I don't quite get what you are trying to get at.
At some point keys are going to be stored and written to disk. If they are not, they would not survive a reboot...
Cloud providers have developed custom hardware for this.
I don't see why I would trust my environment variables but not the keys managed by my database. Seems like the same level of security to me...
@CommanderStorm commented on GitHub (May 23, 2024):
Think running your own crypto falls under the exponentially harder to test category.
Given the current test coverage, I would classify the risk as being too high.
If we can outsource this risk to maridb, that would be a good choice for the moment.
@thielj commented on GitHub (May 25, 2024):
Every backup will be in cleartext (except for file system based snapshots before you start nitpicking).
Every other leak of database connection details will be able to access the data in plaintext.
Explain to me why you protect your admin password by hashing it, please.
Being the evil advocate and following your logic, I could just say: "there's no need to do that. if someone compromises the app, they can obviously get in without the password. So why bother. And if someone can download the database with the plaintext password, they already have all the data anyway. So why bother."
And, yet you hash your passwords.
@thielj commented on GitHub (May 25, 2024):
As I said before, I'm no JS programmer. But it's something like this to set you up.
@SharonMenachem commented on GitHub (May 26, 2024):
My final intention is as the password of the users are hashed so also the list of IP addresses will be in the table in the database. Hope that I can get some help with it :)
@CommanderStorm commented on GitHub (May 26, 2024):
I think the password hashing response is a bad analogy and I am not going to react to that bait.
Let's get this discussion back to being productive:
So your concern has changed in terms of what the attacker has access to:
As I read your last comment the attacker has dumped the database credentials for an encrypted maridb instances from the environment variables.
I don't think that for this attacker, having symmetric crypto stored in the DB would make a difference, if we store the key material in the environment variables.
@thielj commented on GitHub (May 26, 2024):
We're running in circles here. Just read my comment you marked as spam.
You have two choices:
@thielj commented on GitHub (May 26, 2024):
Where do you plan to store the connection string to that secure Maria DB by the way? Answers on a postcard...
@CommanderStorm commented on GitHub (May 26, 2024):
The relevant part of https://github.com/louislam/uptime-kuma/issues/4784#issuecomment-2127922497 which this discussion was missing:
@CommanderStorm commented on GitHub (May 26, 2024):
Good point.
The user can configure the connection settings in the ui on initial setup or via environment variables.
It is then (=in both cases) written to disk as
db-config.jsonin the formator
So in the
v2-design, the database credentials are written to disk which means that the malicious local admin could also read them.We have the constraint, that we want to offer simple setup of embedded mariadb. We like our users to have as simple a time configuring us as possible => forcing environment variables is not really something we want.
An option to allow for no disk storage of these would be to change the way we handle from writing to disk to staying in the environment variables.
=> remove this line
github.com/louislam/uptime-kuma@11007823e7/server/setup-database.js (L80)This would then allow users to have a encrypted, external mariadb instance which they connect via the credentials in the enviroment variabes and those to never be written to disk.
What do you think about this?
@thielj commented on GitHub (May 27, 2024):
It's ridiculous
@CommanderStorm commented on GitHub (May 27, 2024):
What part is ridiculous?
(Could we get back to a productive discussion on how to fix this issue?)
@thielj commented on GitHub (May 27, 2024):
You're not just planning to do something which you repeatedly argue against as making no difference - you're even going further and plan to write that information to disk.
I'm out of this discussion, but let me give you some points on the way:
Forget about Sqlite SEE.
some performance notes
about the security aspects of this issue:
@CommanderStorm commented on GitHub (May 27, 2024):
Most parts of their licence are fine. The only part that is problematic would be
customers cannot make additional copies of the software to use for other purposes.We could not guarantee that our users don't extend the software via
FROM uptime-kuma:2in another docker image=> You are correct, SEE is not pheasable.
@CommanderStorm commented on GitHub (May 27, 2024):
Re:some performance notes
About the performance problems of
v1:They are mostly because we don't agregate at all and store nearly everything.
We likely could have improved our utilisation a lot by optimising the db layout.
Anyway, we have now committed to using aggregated tables => solving this issue for all platforms.
Scanning 1-10 gigabytes for queries is somewhat slow, but some things in sqlites design such as their query optimisation engine are not optimised for this. Being concurrent here is better, as we execute a lot of database queries (this could have been another optimisation route which we did not take).
While we might have been able to solve it via other ways, this is the way we chose. I don't see us changing out that part as the current push is already nearly done. See #4500 for the bugs left.
Please see https://github.com/louislam/uptime-kuma/pull/3748#issuecomment-1871465660
The relevant query is currently (
v1) at ~70ms (dependent on your monitor count and reteintion, mileage may vary). Please see https://github.com/louislam/uptime-kuma/pull/2750#issuecomment-1487114885We may be using it incorrectly, but we are not db experts.
@CommanderStorm commented on GitHub (May 27, 2024):
We currently don't have a feature where we automatically upload+restore backups to/from a S3 bucket. If we had, that would be encrypted.
The database export feature is getting removed in v2, as it is currently unmaintainable.
Our current project goals are that we are simple to setup and allow great UX.
If one of these goals comes into conflict with security or data protection, have in the past chosen to prioritised better UX and allowed users to downgrade their security/privacy. (this obviously only covers the cases where users might be depending on the old behavior or need an exception for some reason)
Easier said than done (at least concearning this issue).
We don't want to force hundreds of environement variables.
The ones which we have are likely aready pretty intimidating for new users: https://github.com/louislam/uptime-kuma/wiki/Environment-Variables
I currently don't see good compromises that:
This part might change in a
v3orv4.=> For this feature, I currently see it as an either or.
Pushing people towards a tool which we don't have to maintain like https://mariadb.com/kb/en/data-at-rest-encryption-overview/ is quite attactive as this gets around the maintenance problem. Assuming we do the change I proposed here https://github.com/louislam/uptime-kuma/issues/4778#issuecomment-2132374854, this should solve the problem of malicious local admins without environemnt variables.
You are correct that it does not solve the problem of users sharing database credentials via unsecure means (and the db is publicly routable), but I don't think that this is an attack vector we actually have to protect against.
As an analogy:
If a user posts their login credentials on a public site and has not enabled 2FA, I would not consider this a security risk on our end.
@thielj commented on GitHub (May 31, 2024):
Put that on top of your README
@CommanderStorm commented on GitHub (May 31, 2024):
Jea, that was likely not the best way to write what I meant and could be misunderstood. Sorry, should not have been this blunt.
I have rephrased the comment above what I really mean
@PinguDEV-original commented on GitHub (Jul 1, 2024):
Is this still a release blocker?
@miljw002 commented on GitHub (Aug 2, 2024):
Hi All,
Just chiming in as I’ve been watching, waiting and am really looking forward to version 2. The main thing I’m looking forward to is MySQL support, as it addresses a number of requirements for me (performance, size, alternate backup approaches). In general I prefer to keep the WFE and database backend seperate where I can, as there are many advantages.
If I’ve read this thread correctly, I don’t think this is a “blocker” for the next release.
I can see a lot of debate over effort vs security, and I think it’s important to remember that we have this as a number of people are being very kind with their time. I also already have my own strategies (at an environment level) to address the scenarios being discussed.
Unless there are a heap of users saying they really need and/or want this, I don’t think it’s worth holding up the V2 release for it.
J
@andreasbrett commented on GitHub (Sep 26, 2024):
Agreed, although I'd very much prefer a more secure approach for storing secrets, v2 is more important for now.