feat: encrypted backup and restore

[lvca]

This pull request introduces encryption support for database backups and restores, along with associated tests and refactoring. The most significant changes include adding encryption settings to the backup and restore processes, implementing encryption and decryption logic, and updating the grammar and tests to support these features.

Encryption Support for Backup and Restore:

• BackupDatabaseStatement (SQLGrammar.jjt, BackupDatabaseStatement.java): Added support for specifying encryption settings (encryptionAlgorithm and encryptionKey) in SQL commands using the WITH clause. These settings are stored in a Map and applied during backup execution. [1] [2] [3]
• BackupSettings and RestoreSettings (BackupSettings.java, RestoreSettings.java): Added encryptionAlgorithm and encryptionKey fields with default values. Updated parameter parsing to handle encryption-related options. [1] [2] [3] [4]

Encryption and Decryption Logic:

• FullBackupFormat and FullRestoreFormat (FullBackupFormat.java, FullRestoreFormat.java): Implemented encryptFile and decryptFile methods using AES encryption. These methods wrap the backup and restore streams with encryption or decryption logic based on the provided settings. [1] [2]

Grammar and Test Updates:

• BackupDatabaseTest and FullBackupIT (BackupDatabaseTest.java, FullBackupIT.java): Added new test cases to verify encrypted backups and restores using encryption keys. These tests ensure the integrity and correctness of encrypted backups. [1] [2]

These changes collectively enhance the database's backup and restore functionality by introducing encryption, ensuring data security during these operations.

[gemini-code-assist]

Hello @lvca, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team, gemini-code-assist here with a summary of this pull request. This PR introduces significant functionality by adding support for encrypting database backups and decrypting them during the restore process. The changes span multiple areas, including the SQL grammar for specifying encryption settings, the backup and restore settings classes, the core backup/restore format implementations for handling the encryption/decryption logic, and new test cases to verify the functionality.

Highlights

• Encrypted Backup SQL Syntax: The BACKUP DATABASE SQL command now supports a WITH clause to specify encryption settings like encryptionAlgorithm and encryptionKey.
• Encryption/Decryption Logic: AES encryption/decryption logic has been implemented within the FullBackupFormat and FullRestoreFormat classes, wrapping the standard zip streams with cipher streams when an encryption key is provided. The Initialization Vector (IV) is written to the beginning of the encrypted backup file.
• Settings Updates: BackupSettings and RestoreSettings classes have been updated to include fields for encryptionAlgorithm and encryptionKey, allowing these settings to be passed and parsed.
• New Test Coverage: New unit and integration tests have been added to specifically verify the encrypted backup and restore processes using different encryption keys.

Changelog

Click here to see the changelog

• engine/src/main/grammar/SQLGrammar.jjt
  • Added a WITH clause to the BackupDatabaseStatement grammar rule (lines 4192-4199) to allow specifying key-value settings, intended for encryption parameters.
• engine/src/main/java/com/arcadedb/query/sql/parser/BackupDatabaseStatement.java
  • Added fields key, value, and a settings map to store parameters from the WITH clause (lines 39-41).
  • Added logic in executeSimple to iterate through the settings map and use reflection to call setEncryptionAlgorithm and setEncryptionKey methods on the Backup object if the corresponding keys are found (lines 74-81).
• integration/src/main/java/com/arcadedb/integration/backup/Backup.java
  • Added public setter methods setEncryptionAlgorithm (lines 83-86) and setEncryptionKey (lines 88-91) to the Backup class to allow configuring encryption settings.
• integration/src/main/java/com/arcadedb/integration/backup/BackupSettings.java
  • Added encryptionAlgorithm (defaulting to "AES") and encryptionKey fields (lines 32-33).
  • Updated the parseParameter method to handle encryptionAlgorithm and encryptionKey parameters (lines 65-66).
• integration/src/main/java/com/arcadedb/integration/backup/format/FullBackupFormat.java
  • Introduced a private BackupCallback interface (lines 36-38).
  • Refactored backupDatabase to use a new encryptFile method, passing the core backup logic as a callback (lines 72-102).
  • Added the encryptFile method (lines 129-160) which handles creating a FileOutputStream, optionally wrapping it with a CipherOutputStream using AES/CTR/NoPadding based on settings.encryptionKey, writing the IV, and then creating the ZipOutputStream.
• integration/src/main/java/com/arcadedb/integration/restore/RestoreSettings.java
  • Added encryptionAlgorithm (defaulting to "AES") and encryptionKey fields (lines 30-31).
  • Updated the parseParameter method to handle encryptionAlgorithm and encryptionKey parameters using a switch expression (lines 44-50).
• integration/src/main/java/com/arcadedb/integration/restore/format/FullRestoreFormat.java
  • Introduced a private RestoreCallback interface (lines 34-36).
  • Refactored restoreDatabase to use a new decryptFile method, passing the core restore logic as a callback (lines 76-94).
  • Added the decryptFile method (lines 151-180) which handles creating the initial InputStream, reading the IV, optionally wrapping it with a CipherInputStream using AES/CTR/NoPadding based on settings.encryptionKey, and then creating the ZipInputStream.
• integration/src/test/java/com/arcadedb/integration/backup/BackupDatabaseTest.java
  • Added a new test testBackupEncryption (lines 30-37) to verify the BACKUP DATABASE SQL command with an encryption key.
• integration/src/test/java/com/arcadedb/integration/backup/FullBackupIT.java
  • Added a new integration test testEncryptedFullBackupCommandLineOK (lines 77-95) to verify the full backup and restore process using command-line parameters with an encryption key, including database comparison.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

Data safe and sound,
Encrypted, can't be found,
Backup key is gold.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a significant and valuable feature: encrypted backup and restore capabilities. The use of AES/CTR for encryption is a solid choice. The changes are well-structured, particularly the separation of encryption/decryption logic using callbacks.

However, there are critical security concerns regarding how the encryption key is derived from the user-provided passphrase. Additionally, there are several areas for improvement in terms of correctness, maintainability, and adherence to best practices, such as handling expression evaluation for settings and using constants for cryptographic parameters and setting keys.

Addressing the security issue is paramount before this PR can be considered for merging.

Summary of Findings

• Security: Encryption Key Derivation: The most critical issue is the direct use of the user-provided passphrase (padded/truncated) as the AES encryption key. This is a significant security vulnerability. A strong Key Derivation Function (KDF) like PBKDF2, scrypt, or Argon2, along with a salt, must be used to derive the actual encryption key from the passphrase. This was noted for both FullBackupFormat.java and FullRestoreFormat.java.
• Correctness: Setting Value Retrieval in SQL Parser: In BackupDatabaseStatement.java, the retrieval of encryption setting values like ((BaseExpression) entry.getValue().value).string might incorrectly include quotes from the string literal. It should use entry.getValue().execute(null, context) to get the properly evaluated string.
• Maintainability: Magic Strings for Settings and Parameters: Strings like "encryptionAlgorithm" and "encryptionKey" are used as magic strings in BackupDatabaseStatement.java, BackupSettings.java, and RestoreSettings.java. These should be defined as constants.
• Maintainability: Hardcoded Cryptographic Parameters: Key and IV lengths (32 and 16 bytes) are hardcoded in FullBackupFormat.java and FullRestoreFormat.java. These should be named constants.
• Correctness: Charset for Encryption Key: Using DatabaseFactory.getDefaultCharset() for converting the encryption key string to bytes can be problematic if the default charset varies. Explicitly using StandardCharsets.UTF_8 is recommended for consistency.
• Maintainability: Reflection Usage: Reflection is used in BackupDatabaseStatement.java to call methods on the Backup class. While this might be for decoupling, it reduces type safety and refactorability. If possible, direct calls or interfaces are preferred.
• Style: Wildcard Imports (Not Commented): Several files (BackupDatabaseStatement.java, FullBackupFormat.java, FullRestoreFormat.java) use wildcard imports (e.g., java.io.*, java.util.*). While not a critical issue, explicit imports are often preferred for clarity. This was not commented on due to review settings (low severity).
• Testing: Hardcoded Keys in Tests (Not Commented): Test files BackupDatabaseTest.java and FullBackupIT.java use hardcoded encryption keys. This is standard for testing but worth noting. This was not commented on due to review settings (low severity).

Merge Readiness

This pull request introduces a crucial feature for data security. However, due to the critical security vulnerability in how encryption keys are handled (direct use of passphrase instead of a KDF), this PR is not ready for merging. This issue must be addressed urgently.

Additionally, there are several high and medium severity issues related to correctness in parsing SQL settings, and maintainability (magic strings, hardcoded crypto parameters, reflection usage) that should also be resolved.

I am unable to approve this pull request in its current state. Please address the identified issues, especially the security concerns, before seeking further review and merging.

[gemini-code-assist]

Using settings.encryptionKey.getBytes(...) directly (padded/truncated to 32 bytes) as an AES key is a significant security vulnerability. Passphrases or user-provided strings should never be used directly as cryptographic keys.

A Key Derivation Function (KDF) such as PBKDF2WithHmacSHA256, scrypt, or Argon2 should be used to derive a cryptographically strong key from the user-provided encryptionKey. This process involves a salt (which should be randomly generated and stored alongside the IV) and a configurable iteration count.

For example, to derive a key using PBKDF2:

// Generate a random salt (e.g., 16 bytes)
byte[] salt = new byte[16];
new java.security.SecureRandom().nextBytes(salt);
// Store this salt at the beginning of the backup file, similar to the IV.

SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
// Iteration count should be high, e.g., 65536 or more
KeySpec spec = new PBEKeySpec(settings.encryptionKey.toCharArray(), salt, 65536, 256); // 256-bit key
SecretKey tmp = factory.generateSecret(spec);
byte[] derivedKeyBytes = tmp.getEncoded();
final javax.crypto.SecretKey secretKey = new javax.crypto.spec.SecretKeySpec(derivedKeyBytes, settings.encryptionAlgorithm);

This derived key should then be used for encryption. The same salt and iteration count must be used during decryption.

Could you refactor this to use a proper KDF?

[lvca]

@gemini I agree, Can you do modify this?

[gemini-code-assist]

This is the same critical security vulnerability as in FullBackupFormat.java. The user-provided encryptionKey is being converted to bytes and used directly (padded/truncated) as the AES key.

A Key Derivation Function (KDF) like PBKDF2WithHmacSHA256 must be used here as well. The salt used during backup (which should be stored in the backup file, e.g., before the IV) must be read here and used with the same KDF, iteration count, and user passphrase to re-derive the exact same cryptographic key for decryption.

Could this be updated to use a KDF, ensuring it's compatible with the KDF used during backup?

[gemini-code-assist]

There are a couple of concerns here:

1. Correctness of Setting Value Retrieval: The method ((BaseExpression) entry.getValue().value).string to retrieve the string value for encryption settings appears to fetch the raw string literal, potentially including quotes (e.g., "'mySecretKey'"). This is likely not the intended behavior, as the encryption mechanism would then use a key that includes these quotes. The correct way to get the evaluated string value from an Expression is typically by calling its execute() method. Could you clarify if entry.getValue().execute(null, context) would be more appropriate here to get the actual string value?

2. Maintainability - Magic Strings: The strings "encryptionAlgorithm" and "encryptionKey" are used directly. It's better practice to define these as constants, perhaps in BackupSettings or a shared constants class, to avoid typos and improve maintainability.

3. Maintainability - Reflection: While reflection might be used for decoupling, directly invoking methods like setEncryptionAlgorithm and setEncryptionKey via reflection can make the code harder to debug, refactor, and less type-safe. If com.arcadedb.integration.backup.Backup is accessible, consider a more direct invocation method or an interface if loose coupling is strictly necessary. If reflection must be used, consider defining method names as constants.

Suggested change

	switch (entry.getKey().toString()) {
	case "encryptionAlgorithm" -> clazz.getMethod("setEncryptionAlgorithm", String.class)
	.invoke(backup, ((BaseExpression) entry.getValue().value).string);
	case "encryptionKey" ->
	clazz.getMethod("setEncryptionKey", String.class).invoke(backup, ((BaseExpression) entry.getValue().value).string);
	switch (entry.getKey().toString()) {
	case BackupSettings.ENCRYPTION_ALGORITHM_SETTING: // Assuming ENCRYPTION_ALGORITHM_SETTING = "encryptionAlgorithm"
	Object algoValue = entry.getValue().execute(null, context);
	if (!(algoValue instanceof String)) {
	throw new CommandExecutionException("Value for 'encryptionAlgorithm' must be a string.");
	}
	clazz.getMethod("setEncryptionAlgorithm", String.class).invoke(backup, (String) algoValue);
	break;
	case BackupSettings.ENCRYPTION_KEY_SETTING: // Assuming ENCRYPTION_KEY_SETTING = "encryptionKey"
	Object keyValue = entry.getValue().execute(null, context);
	if (!(keyValue instanceof String)) {
	throw new CommandExecutionException("Value for 'encryptionKey' must be a string.");
	}
	clazz.getMethod("setEncryptionKey", String.class).invoke(backup, (String) keyValue);
	break;
	}

[gemini-code-assist]

The case labels "encryptionAlgorithm" and "encryptionKey" are magic strings. To improve maintainability and reduce the risk of typos, would it be better to define these as public static final String constants within this class or a shared constants class and use those constants here?

    // Example constants in BackupSettings class:
    // public static final String ENCRYPTION_ALGORITHM_SETTING = "encryptionAlgorithm";
    // public static final String ENCRYPTION_KEY_SETTING = "encryptionKey";

    switch (name) {
    case ENCRYPTION_ALGORITHM_SETTING -> encryptionAlgorithm = value;
    case ENCRYPTION_KEY_SETTING -> encryptionKey = value;

[gemini-code-assist]

Using DatabaseFactory.getDefaultCharset() for converting the encryptionKey string to bytes can be risky if this default charset is not guaranteed to be consistent across all environments where backup and restore operations might occur (e.g., UTF-8). If different charsets are used, the derived byte array for the key will differ, leading to decryption failures.

It's generally safer to explicitly specify a standard charset, like StandardCharsets.UTF_8, when converting passphrases to bytes for cryptographic purposes, especially before feeding into a Key Derivation Function (which is also recommended for this encryptionKey).

What are your thoughts on standardizing this to StandardCharsets.UTF_8?

[gemini-code-assist]

The key size (32 bytes for AES-256) and IV size (16 bytes for AES) are hardcoded. While these are standard values for AES, defining them as named constants (e.g., AES_256_KEY_SIZE_BYTES, AES_IV_LENGTH_BYTES) would improve code readability and maintainability, making it clear what these numbers represent.

For example:

private static final int AES_KEY_SIZE_BYTES = 32; // 256-bit
private static final int AES_IV_LENGTH_BYTES = 16;
// ...
final byte[] keyBytes = Arrays.copyOf(settings.encryptionKey.getBytes(...), AES_KEY_SIZE_BYTES);
// ...
final byte[] iv = new byte[AES_IV_LENGTH_BYTES];

Would you consider refactoring these magic numbers into constants?

[gemini-code-assist]

Similar to BackupSettings, the case labels "encryptionAlgorithm" and "encryptionKey" are magic strings. Would it be beneficial to define these as public static final String constants, possibly shared with BackupSettings or in a common constants class, to enhance maintainability?

      // Example constants (could be in a shared class or this class)
      // public static final String ENCRYPTION_ALGORITHM_SETTING = "encryptionAlgorithm";
      // public static final String ENCRYPTION_KEY_SETTING = "encryptionKey";

      case ENCRYPTION_ALGORITHM_SETTING -> {
        encryptionAlgorithm = value;
        yield 2;
      }
      case ENCRYPTION_KEY_SETTING -> {
        encryptionKey = value;
        yield 2;
      }

[gemini-code-assist]

As with the backup process, using DatabaseFactory.getDefaultCharset() for converting the encryptionKey to bytes can lead to issues if the default charset isn't consistent. For robust decryption, it's crucial that the byte representation of the key matches exactly what was used during encryption.

Consider standardizing on StandardCharsets.UTF_8 here as well, especially when used in conjunction with a KDF.

[gemini-code-assist]

The key size (32 bytes) and IV size (16 bytes) are hardcoded here as well. Similar to the backup format, defining these as named constants (e.g., AES_256_KEY_SIZE_BYTES, AES_IV_LENGTH_BYTES) would improve clarity and maintainability.

For example:

private static final int AES_KEY_SIZE_BYTES = 32; // 256-bit
private static final int AES_IV_LENGTH_BYTES = 16;
// ...
final byte[] keyBytes = Arrays.copyOf(settings.encryptionKey.getBytes(...), AES_KEY_SIZE_BYTES);
// ...
final byte[] iv = new byte[AES_IV_LENGTH_BYTES];

Would you consider this refactoring?

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -1.31%	✅ 63.24%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (4d093f4)	69977	44828	64.06%
Head commit (b127e5b)	72299 (+2322)	45366 (+538)	62.75% (-1.31%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#2222)	9520	6020	63.24%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[robfrank]

LGTM for me, maybe you can implement what static analysis and Gemini are suggesting to improve the code