mostro icon indicating copy to clipboard operation
mostro copied to clipboard

Database keys encryption

Open arkanoider opened this issue 8 months ago β€’ 5 comments

Hi @grunch,

I will drop some infos here about the idea of encryption on the identity keys in mostro database tables to decouple in a more definitive way relations between orders and nostr users.

I will use these two crates for the encryption part:

Argon2 is a famous and reccomended KDF, with it I will create a hashed value of the user password to be safely saved in the mostro database.

Cha Cha is a valid and quick algo for ciphering the keys that we want in the db.

Below a simple flowchart of what will happens when mostro will be started from admin:

flowchart TD
    A[Start Mostro] -->|mostro.db file <br>exists| B(Ask user database<br> password )
    A[Start Mostro] -->|mostro.db file <br>not exists| C(Ask user a strong<br> password)
    C --> D[Double check <br>password]
    D --> |check failed|C
    D --> |valid password|E[Password saved in mostro.db]
    B --> |read password<br>from user|F[compare with hashed<br>password]
    F --> |valid password|G[Start mostro with encryption]
    F --> |not valid password|B

My initial idea is to save a new user, who is the admin in this special case, when mostro is run for the first time. I will add a new string field in users table ( optional - I will let also user give an empty string to have no encryption in case ) and this will be the admin password hashed in the format of Argon2, for those who wants to play a bit with argon2 this is a nice online site with a generator:

Argon2 generator online

A typical stored string is:

$argon2d$v=19$m=1024,t=1,p=1$c2FsdHlzYWx0eXNhbHR5$VucvoVfuAxUpW4FTuc+8JbgZYBRnw5b/41AJugHoIkY

When admin password is in place, with Argon2 crate is easy to check against the user password with a verify_password method.

User password is now set in RAM using a rust OnceCell so it's just inited once and then will be used during encrypt/decrypt operations over the keys in database. I have introduced also another small crate called secrecy that helps avoiding leak of password in RAM in the logs/debug messages.

I am still working on this but basically login part seems working ( still debugging edge cases...) than I will update also mostro-core with the method to ecnrypt/decrypt the requested data.

Summary by CodeRabbit

  • New Features

    • Introduced password-based encryption for sensitive database fields, including secure storage and verification of admin passwords.
    • Added enforcement of strong password requirements for database protection.
    • Terminal screen now clears at startup for improved user experience.
  • Security

    • Master buyer and seller public keys are now encrypted before being stored and decrypted when accessed, enhancing data confidentiality.
    • Database file permissions are restricted for added security on Unix systems.
  • Bug Fixes

    • Improved error handling and logging for service errors and scheduler tasks.
  • Chores

    • Updated dependencies to support new encryption and password handling features.
    • Added a new admin password column to the user table for secure credential storage.

arkanoider avatar Apr 14 '25 09:04 arkanoider

Walkthrough

This update introduces password-based encryption for sensitive database fields and enforces strong password authentication for database access. It adds new dependencies to support encryption, secure password handling, and terminal management. The database schema and access logic are refactored to handle encrypted public keys, and password prompts are integrated into the database connection flow. Several modules are updated to support these changes.

Changes

File(s) Change Summary
Cargo.toml Updated dependencies: bumped nostr-sdk version, bumped mostro-core version, added rpassword, argon2, secrecy, and clearscreen. No removals or feature changes.
migrations/20231005195154_users.sql Added admin_password char(64) column to the users table.
src/db.rs Implemented password-based encryption/authentication: prompts for strong password on DB creation, enforces password strength, stores password hash, restricts DB file permissions, and authenticates on access. Refactored queries to handle encrypted fields, added test for encryption/decryption, improved error handling, and updated several public functions to support encrypted data and password logic.
src/main.rs Added config module, global static MOSTRO_DB_PASSWORD, and terminal clearing at startup. Updated imports and logging. Added TODO for DB pool sharing.
src/app/take_buy.rs
src/app/take_sell.rs
src/util.rs
Changed storage of master buyer/seller pubkeys to use encryption with the database password. Updated relevant imports and error handling. In util.rs, also decrypts keys when needed and adds logging.
src/app/trade_pubkey.rs Decrypts master buyer/seller pubkeys before comparison with event sender; updates logic to handle encrypted keys and error mapping.
src/app/admin_take_dispute.rs Refactored dispute handling: uses decrypted keys, updates message construction, uses event sender for solver/admin, improves error handling, and updates event sending logic. Function signatures updated to receive keys as parameter.
src/app.rs Enhanced error logging, improved signature verification by deserializing rumor content to (Message, Option<Signature>), updated admin dispute action call, and switched to structured logging.
src/app/rate_user.rs Updated imports and changed how master pubkeys are retrieved (now using password). Simplified user presence check.
src/app/release.rs Updated imports, error handling, and function signatures. Handles encrypted keys, passes password to key retrieval functions, and adjusts event/tag creation logic.
src/scheduler.rs Updated import paths, changed event sending to use references, improved logging, changed error handling in scheduled jobs to log and return early, and updated client creation logic.
src/cli.rs Refactored settings_init to return Result<(), Box<dyn std::error::Error>>, added documentation, and updated config file initialization logic and imports.
src/config/util.rs Reformatted error construction for missing home directory in init_configuration_file for clarity.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Terminal
    participant Main
    participant DB as Database
    participant Crypto as CryptoUtils

    User->>Terminal: Start application
    Main->>Terminal: Clear screen
    Main->>User: Prompt for DB password (if new DB)
    User->>Main: Enter password
    Main->>Crypto: Validate password strength
    Crypto-->>Main: OK/Fail
    alt New DB
        Main->>Crypto: Hash password
        Main->>DB: Store admin password hash
        Main->>DB: Restrict file permissions
        Main->>Crypto: Store password in MOSTRO_DB_PASSWORD
    else Existing DB
        loop Up to 3 attempts
            Main->>User: Prompt for password
            User->>Main: Enter password
            Main->>DB: Retrieve stored hash
            Main->>Crypto: Verify password
            Crypto-->>Main: OK/Fail
        end
        alt Password correct
            Main->>Crypto: Store password in MOSTRO_DB_PASSWORD
        else
            Main->>Terminal: Exit (authentication failed)
        end
    end
    Main->>DB: Open connection (with password)

Possibly related PRs

  • MostroP2P/mostro#404: Updates user-related database handling and user rating features, with overlapping changes in user presence and rating updates.
  • MostroP2P/mostro#463: Modifies admin_take_dispute_action in a similar way, updating dispute handling and user info retrieval.
  • MostroP2P/mostro#481: Refactors configuration handling and import paths related to Settings struct, closely related to config module changes in this PR.

Suggested reviewers

  • grunch
  • Catrya

Poem

In burrows deep, the passwords hide,
Encryption now stands by our side.
With keys held close and secrets tight,
Our data hops from byte to byte.
The database safe, the warnings clearβ€”
Mostro’s future, secure and dear!
πŸ°πŸ”βœ¨

[!NOTE]

⚑️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Learn more here.


πŸ“œ Recent review details

Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 444b6ce5b179ff453bb0c4ca7b9f26713c5af44b and fec0d3d017beba729b66512c0ee9c5e570684707.

πŸ“’ Files selected for processing (1)
  • src/app/release.rs (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/release.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: tests
✨ Finishing Touches
  • [ ] πŸ“ Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❀️ Share
πŸͺ§ Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Apr 14 '25 09:04 coderabbitai[bot]

What do you think about to wrap the current hex sec key that user could have and multiply it by the password: That would be strong enough, don't?

fabohax avatar Apr 14 '25 18:04 fabohax

@grunch @Catrya

UPDATE:

Trying to execute the flow of the orders I have found some possible limitations, mostly caused by the fact the encryption/decryption requires CPU time.

Examples:

Now when a message comes in Mostrod we check if the user is new and wants to be rated, in that case if it's index0 key (identity key) is not present we add it. If we encrypt the key what happens next? Let's suppose we have 1000 user in the db all their keys are encrypted and when one of them sends a new order we have to check that he's yet in the database list. To check we must decrypt all the 1000 keys and check against the pubkey in the message of the client --> SLOW and it grows with number of users

My idea is to treat only orders table of database like this:

image

master_buyer_pubkey and master_seller_pubkey in the image are encrypted using a derived key with argon and chacha cyphered them, finally they were base64 encoded to have strings. Doing that we detach users table identity key ( index0) from orders table so orders cannot be related to their taker/maker index0 key.

In this way I am yet able to do simple order flow and timings seems acceptable. There is still two points in takesell/takebuy where we need to check against some key and it's when we call:

buyer_has_pending_order

seller_has_pending_order

in these case we have to extract the list of order in a particular state and check if user is having orders pending so we should cycle decrypting some keys, will check if we can improve the check in some ways, but i think it's tolerable.

Ok will go on testing orders cycle, but I am glad to have your opinions.

arkanoider avatar Apr 23 '25 20:04 arkanoider

@grunch ,

I added a nice ( I think... ) idea for caching keys and speed up search when mostro is scanning the vector of index0 keys encrypted and we have to check if one is yet present:

Cache Argon2 Key Derivations Using the password and the same salt i get the same encryption key with Argon2, so if I can cache some combinations of salt+password I can use a HashMap<(Salt+Password), DecryptKey> to get the keys for decryption in zero time from RAM.

This works because for a given (password, salt), the derived key is always the same.

I have created a tests with good results, these inserts 10 encrypted string and then some are decrypted normally and some are cached:

running 1 test
In-memory database and table created for test.
Inserting 10 entries...
Inserting value Entry 0
Salt: 3bLfcYlFRs41Jm/6Byd46A

Inserting value Entry 1
###Salt: c2FsdE5QZXBwYVBlcHBhMQ

Inserting value Entry 2
Salt: bi4gVvI4Na9xfQiDvxtUMw
Inserting value Entry 3
Salt: pHnzGJzxZDUisXV/D8CQng

Inserting value Entry 4
###Salt: c2FsdE5QZXBwYVBlcHBhMQ

Inserting value Entry 0
Salt: dHtIk1RRv+BDTU7NYi0rKw
Inserting value Entry 1
Salt: zCO424lJY0Kvo71J8YJjsw

Inserting value Entry 2
###Salt: c2FsdE5QZXBwYVBlcHBhMQ

Inserting value Entry 3
Salt: SX1JLNbyhQwc52njNWD9bA
Inserting value Entry 4
Salt: f5YLnA46iuOdOjBG4qbt2Q
Entries inserted.
Fetching 'value' column...
Cache key: 16609968908560343058
Key not found in cache, creating new one! 16609968908560343058
Time taken to decrypt: 289 ms
Cache key: 14650601133528733758
Key not found in cache, creating new one! 14650601133528733758
Time taken to decrypt: 209 ms
Cache key: 7000585519848012028
Key not found in cache, creating new one! 7000585519848012028
Time taken to decrypt: 194 ms
Cache key: 11579821871313292092
Key not found in cache, creating new one! 11579821871313292092
Time taken to decrypt: 226 ms
Cache key: 14650601133528733758

###Key found in cache!
###Time taken to decrypt: 0 ms

Cache key: 7881818280167815519
Key not found in cache, creating new one! 7881818280167815519
Time taken to decrypt: 278 ms
Cache key: 16204407719138663129
Key not found in cache, creating new one! 16204407719138663129
Time taken to decrypt: 269 ms
Cache key: 14650601133528733758

###Key found in cache!
###Time taken to decrypt: 0 ms

Cache key: 8474487473980109098
Key not found in cache, creating new one! 8474487473980109098
Time taken to decrypt: 264 ms
Cache key: 14569125712987388781
Key not found in cache, creating new one! 14569125712987388781
Time taken to decrypt: 258 ms
Entry 0 found in the hash set.
Test passed!
test db::tests::test_fetch_string_column_scalar ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 13 filtered out; finished in 4.32s

You can see that there is an insertion of three entries with the same salt and the result in decryption that two are with 0 time because the combination of Salt+Password was in cache. I used queue of 50 elements now to start, but it is just a number we can also increase it.

So in this way typically new user will have the first time some delay due to encrytpion decryption, but than with cache they are super fast response.

I tested just locally with rust tests, tonite I will try this in a real scenario while completing flows of orders.

arkanoider avatar Apr 24 '25 14:04 arkanoider

Ok! @grunch @Catrya

Opened for review, just delete your mostro.db file from .mostro folder and run mostro with locally with mostro core on this pull request

I will look to rabbit comments asap.

arkanoider avatar Apr 26 '25 08:04 arkanoider

@arkanoider I hadn't noticed the conflicts, I'm sure you had, but just in case, I remind you to keep an eye on it :)

Catrya avatar May 07 '25 21:05 Catrya

@arkanoider I hadn't noticed the conflicts, I'm sure you had, but just in case, I remind you to keep an eye on it :)

Hi @Catrya , i was able to reproduce it....seems that for a reason I cannot understand CLI didn't send in that case the next_trade_keys on release so the order is not saved and you have the error in db search. Anyway I am fixing some things on mostro-core and mostro. So please do a round of test again on your side.

arkanoider avatar May 07 '25 21:05 arkanoider

@arkanoider I hadn't noticed the conflicts, I'm sure you had, but just in case, I remind you to keep an eye on it :)

Hi @Catrya , i was able to reproduce it....seems that for a reason I cannot understand CLI didn't send in that case the next_trade_keys on release so the order is not saved and you have the error in db search. Anyway I am fixing some things on mostro-core and mostro. So please do a round of test again on your side.

Okay, but that bug I mentioned isn't related to this PR; it's older. What do you think about fixing that bug in a separate PR?

@arkanoider I hadn't noticed the conflicts, I'm sure you had, but just in case, I remind you to keep an eye on it :)

I was referring to that here in this PR says that there are branch conflicts that prevent this PR from being merged.

Catrya avatar May 07 '25 21:05 Catrya

@arkanoider I hadn't noticed the conflicts, I'm sure you had, but just in case, I remind you to keep an eye on it :)

Hi @Catrya , i was able to reproduce it....seems that for a reason I cannot understand CLI didn't send in that case the next_trade_keys on release so the order is not saved and you have the error in db search. Anyway I am fixing some things on mostro-core and mostro. So please do a round of test again on your side.

Okay, but that bug I mentioned isn't related to this PR; it's older. What do you think about fixing that bug in a separate PR?

@arkanoider I hadn't noticed the conflicts, I'm sure you had, but just in case, I remind you to keep an eye on it :)

I was referring to that here in this PR says that there are branch conflicts that prevent this PR from being merged.

Yes I will rebase this branch on main when we close smaller prs. Will take care of that. Yes I agree that is better to split that possible mostro-cli issue in another PR when the cause it's clear.

arkanoider avatar May 07 '25 22:05 arkanoider

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/db.rs (1)> 782-802: Consider optimizing encrypted key searches.

The current approach of fetching and decrypting all matching orders in sequence could become a performance bottleneck as your database grows. Consider these optimization options:

  1. Add indexes on the relevant columns to speed up queries
  2. Consider batch processing larger result sets
  3. Look into adding a cache specifically for recently decrypted keys (if security considerations allow)

Example of adding a basic cache:

// At the module level
lazy_static! {
    static ref DECRYPTION_CACHE: Mutex<LruCache<String, String>> = 
        Mutex::new(LruCache::new(100)); // Cache size of 100 items
}

// In the decryption function
let master_pubkey_decrypted = {
    let mut cache = DECRYPTION_CACHE.lock().unwrap();
    if let Some(cached) = cache.get(&master_key) {
        cached.clone()
    } else {
        let decrypted = CryptoUtils::decrypt_data(master_key.clone(), MOSTRO_DB_PASSWORD.get())
            .map_err(MostroInternalErr)?;
        cache.put(master_key, decrypted.clone());
        decrypted
    }
};

πŸ“œ Review details

This mechanic is yet inside the decrypt function. A simple cache is used to get recently used decryption keys, and verified that in that case times are below 1ms.

arkanoider avatar May 19 '25 14:05 arkanoider

Hi @Catrya what is this PR status? is this ready to be merge? πŸ˜ƒ

grunch avatar May 19 '25 14:05 grunch

Hi @Catrya what is this PR status? is this ready to be merge? πŸ˜ƒ

Let me test it again because there are some new commits after I approved it days ago.

Catrya avatar May 19 '25 14:05 Catrya

Hi @Catrya what is this PR status? is this ready to be merge? πŸ˜ƒ

Let me test it again because there are some new confirmations after I approved it days ago.

Yep I was writing to you about this @Catrya , great! Let me just push the update with cargo.toml updated to latest release so you can compile all with no issues.

arkanoider avatar May 19 '25 14:05 arkanoider

Hi @arkanoider when admin tries to take a dispute of child order in privacy mode

2025-05-20T05:43:23.859013Z  WARN mostrod::app: Error in AdminTakeDispute with context Error in database access: no rows returned by a query that expected to return at least one row - inner message no rows returned by a query that expected to return at least one row

Great pick! I will check @Catrya , so just when the dispute is about a child order.

arkanoider avatar May 20 '25 06:05 arkanoider

But when is it in privacy mode. In normal mode mostro-cli panics so I can't test mostrod in that case

Catrya avatar May 20 '25 06:05 Catrya

@Catrya last commit should fix the missing user information in a child order when the creator of the order is not in full privacy mode. Please test it when you're available, will do the same tonite at home.

arkanoider avatar May 21 '25 09:05 arkanoider

Hi @arkanoider if the order is in privacy mode, with range, whe is republished is mising in the 38383 event:

      [
        "rating",
        "[\"rating\",{\"days\":0,\"total_rating\":0.0,\"total_reviews\":0}]"
      ],

Catrya avatar May 22 '25 16:05 Catrya

Hi @arkanoider if the order is in privacy mode, with range, whe is republished is mising in the 38383 event:

      [
        "rating",
        "[\"rating\",{\"days\":0,\"total_rating\":0.0,\"total_reviews\":0}]"
      ],

This should be fixed with last commit @Catrya ! Thanks for founding a way to replicate the encryption error...great job! Tomorrow I will analyze it!

arkanoider avatar May 22 '25 21:05 arkanoider

@Catrya i found the cause of decryption error, last commit has the fix. When you want and you can test it let me know if it's ok also for you!

arkanoider avatar May 24 '25 11:05 arkanoider

utACK

Finallyyyyy!!!! Great @grunch also #OFF can't stop you from mostro...thanks this is good to move on with other things.

arkanoider avatar May 29 '25 13:05 arkanoider