feat: Encrypted Backups #14126

Anurag0911 · 2021-09-06T06:27:13Z

Problem: Backups were not encrypted.

Changes:

Added Encryption using GPG.
Checkbox to enable Encryption of backups
Button to Access the Encryption Key in the Download backups page.

Approach

For Encryption
- Creating .gpg file
- Renaming the file
For Decryption
- Renaming file as .gpg
- Decryption
- Deletion of Encrypted file
- Renaming
- Rollbackup if any issues or error Occured

PR.Summary.mp4

CheckBox to enable/disable backup encryption

Backup Downloads page
Updates-

Icons to depict encrypted files
Button to get Encryption Key
Click on the 'Get Encryption Key' to look at the encryption key for the backups

Backup encryption with files.

Use --backup-encryption-key flag to decrypt the file at the given path.
No need to provide the encryption key on the same website as it is stored in the site config during the encryption.

Compressed files encryption.

All the types of files are encrypted including .tgz

Partial files encryption

Partial backup are also encrypted.

Rollback

The Encryption/Decryption is made as atomic as possible for any issues occurring during the processes.
Examples:
- Providing invalid key.
- Providing wrong root password.

codecov · 2021-09-06T06:40:50Z

Codecov Report

Merging #14126 (7ce0c54) into develop (b2f78de) will decrease coverage by 0.02%.
The diff coverage is 15.09%.

❗ Current head 7ce0c54 differs from pull request most recent head e92feb3. Consider uploading reports for the commit e92feb3 to get more accurate results

@@             Coverage Diff             @@
##           develop   #14126      +/-   ##
===========================================
- Coverage    56.25%   56.22%   -0.03%     
===========================================
  Files          499      503       +4     
  Lines        41290    41536     +246     
===========================================
+ Hits         23226    23352     +126     
- Misses       18064    18184     +120

abhishekbalam · 2021-09-13T11:51:05Z

@Anurag0911

Create Docs and paste link in description.
Typo and case fixes.

gavindsouza · 2021-09-17T12:46:48Z

frappe/commands/site.py

@@ -47,6 +47,7 @@ def new_site(site, mariadb_root_username=None, mariadb_root_password=None, admin

 @click.command('restore')
 @click.argument('sql-file-path')
+@click.option('--backup-encryption-key', help='Backup encryption Key')


Suggested change

@click.option('--backup-encryption-key', help='Backup encryption Key')

@click.option('--encryption-key', help='Backup encryption Key')

Backup seems a bit redundant here

Already have "encryption key" for passwords. Won't it cause confusion?

@Anurag810

The reason we kept a separate key was that we didn't want customers to have access to the site encryption key.
But we let them download the site config which has the key anyway.

So replace this key with the site encryption key. Reuse that, revert changes.

gavindsouza · 2021-09-17T12:50:59Z

frappe/commands/site.py

@@ -56,7 +57,7 @@ def new_site(site, mariadb_root_username=None, mariadb_root_password=None, admin
 @click.option('--with-private-files', help='Restores the private files of the site, given path to its tar file')
 @click.option('--force', is_flag=True, default=False, help='Ignore the validations and downgrade warnings. This action is not recommended')
 @pass_context
-def restore(context, sql_file_path, mariadb_root_username=None, mariadb_root_password=None, db_name=None, verbose=None, install_app=None, admin_password=None, force=None, with_public_files=None, with_private_files=None):
+def restore(context, sql_file_path, backup_encryption_key=None, mariadb_root_username=None, mariadb_root_password=None, db_name=None, verbose=None, install_app=None, admin_password=None, force=None, with_public_files=None, with_private_files=None):


Best to add new kwargs at the end. Don't know if someone is using these commands programmatically.

+1 agree @Anurag0911

gavindsouza · 2021-09-17T12:53:31Z

frappe/commands/site.py

+
+	if not os.path.exists(sql_file_path):
+		print("Invalid path", sql_file_path)
+		sys.exit(1)


Files riddles with trailing whitespaces.

gavindsouza · 2021-09-17T12:55:56Z

frappe/commands/site.py

+		sys.exit(1)
+
+	# Check if file is encrypted 
+	if "enc" in sql_file_path:


Seems like a very weak check to figure out if a backup is encrypted

We can try reading the file and decrypt it in except block but adding a header in an encrypted file is not an option it causes issues with the decrypted files. Any other suggestions for identifying these files?

@gavindsouza

We tried having a header in the tar, but it didn't work out as this encrypts the whole thing.

We should have a try-catch if this fails along with this.

gavindsouza · 2021-09-17T12:57:36Z

frappe/commands/site.py

@@ -66,10 +67,61 @@ def restore(context, sql_file_path, mariadb_root_username=None, mariadb_root_pas
 		is_partial,
 		validate_database_sql
 	)
+	from frappe.utils.backups import backup_decryption, decryption_rollback


Encryption and Decryption methods should be a part of the BackupGenerator. Either that or a separate class that represents a Backup alone.

gavindsouza · 2021-09-17T13:07:32Z

frappe/utils/backups.py

+def backup_encryption_key():
+	"""
+	Checks if backup encryption key exists
+		else create one
+	Return:
+		Backup encryption key 
+	"""
+	from frappe.installer import update_site_config
+	if 'backup_encryption_key' not in frappe.local.conf:
+		confirm = click.confirm(
+			"No encrytion key found. Generate new Key?"
+		)
+		if not confirm:
+			sys.exit(1)
+		backup_encryption_key = Fernet.generate_key().decode()
+		update_site_config('backup_encryption_key', backup_encryption_key)
+		frappe.local.conf.backup_encryption_key = backup_encryption_key
+	return frappe.local.conf.backup_encryption_key


This function does too many things; break it down to either get user input, set encryption key in config or change it. If ever invoked from anywhere except the CLI, it would cause it to stall and/or crash.

Need more information regarding this. User input is required once and there should be a warning... it's harmless to remove, is it good idea to create even it's required once?

gavindsouza · 2021-09-17T13:09:24Z

frappe/utils/backups.py

+			decryptedfile = file_path[:-4],
+		)
+
+	frappe.utils.execute_in_shell(command)


if file_path does not exist, NameError?

gavindsouza · 2021-09-17T13:14:52Z

frappe/utils/backups.py

+def get_backup_encryption_key():
+	if 'backup_encryption_key' in frappe.local.conf:
+		message = frappe.local.conf.backup_encryption_key
+	else:
+		message = "No key found."
+	return frappe.msgprint(message,'Backup Encryption Key')


API endpoints should respond with some specific data, just returning frappe.conf.backup_encryption_key would do here. The user message should be handled at the frontend.

Also, this API should have a restriction such as only for System Managers, or the Administrator user.

gavindsouza · 2021-09-17T13:15:51Z

frappe/utils/backups.py

+			file_type_slugs = {
+				"database": "*-{{}}-{}database.sql.gz".format('*' if partial else ''),
+				"public": "*-{}-files.tar",
+				"private": "*-{}-private-files.tar",
+				"config": "*-{}-site_config_backup.json",
+			}
+		else:
+			file_type_slugs = {
+				"database": "*-{{}}-{}database.enc.sql.gz".format('*' if partial else ''),
+				"public": "*-{}-files.enc.tar",
+				"private": "*-{}-private-files.enc.tar",
+				"config": "*-{}-site_config_backup.json",
+			}


Can be DRY-er

gavindsouza · 2021-09-17T13:22:11Z

frappe/commands/site.py

+
+			#Get the key from site config
+			site = get_site(context)
+			frappe.init(site)


Why not do this whole block right before calling partial_restore? There doesn't seem to be any reason to initialize werkzeug locals and release them twice here.

…evelop

This reverts commit 56ab25a.

This reverts commit a905ab0.

stale · 2021-09-27T12:25:22Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

Anurag0911 added 7 commits September 2, 2021 21:28

feat: Backup Encryption

5e2f282

fix: Optimizations and Partial Files handling

97459ac

Merge branch 'develop' of https://github.com/frappe/frappe into develop

39db327

Merge branch 'develop' of https://github.com/frappe/frappe into develop

8417679

Merge branch 'develop' of https://github.com/frappe/frappe into develop

e67e835

fix: Minor details check

a45daea

fix: Minors

df09f2c

Anurag0911 requested review from gavindsouza, surajshetty3416 and a team as code owners September 6, 2021 06:27

gavindsouza changed the title ~~Backup Encryption~~ feat: Encrypted Backups Sep 6, 2021

gavindsouza self-assigned this Sep 6, 2021

Anurag0911 marked this pull request as draft September 6, 2021 06:35

abhishekbalam self-requested a review September 6, 2021 09:04

Anurag0911 added 2 commits September 8, 2021 21:42

fix: Decryption using Site Config.

b5b3dcd

fix: Required Changes

5e84ec6

abhishekbalam self-assigned this Sep 9, 2021

Anurag0911 marked this pull request as ready for review September 10, 2021 07:35

Anurag0911 added 8 commits September 10, 2021 14:18

fix: Conventions

5c65e20

fix: Comments and finishing

432887f

fix: Missing argument.

e537471

fix: Error messages.

5f27ff6

fix: Error messages conventions.

44e5952

fix: Sider.

7daaa73

fix: Indentation.

f1ccc5f

fix: Indentation.

7ce0c54

fix: Alert title.

7ef9080

gavindsouza requested changes Sep 17, 2021

View reviewed changes

Anurag0911 added 2 commits September 18, 2021 09:31

develop

a905ab0

Merge branch 'develop' of https://github.com/Anurag0911/frappe into d…

b985939

…evelop

Anurag0911 requested review from leela, netchampfaris, ruchamahabal and shariquerik as code owners September 18, 2021 04:01

Anurag0911 marked this pull request as draft September 18, 2021 04:06

Anurag0911 and others added 5 commits September 18, 2021 13:57

develop

56ab25a

Revert "develop"

3fbc308

This reverts commit 56ab25a.

Revert "develop"

7592956

This reverts commit a905ab0.

fix: Conflicts

9ef4756

fix: Conflicts

b021bfb

Anurag0911 mentioned this pull request Sep 19, 2021

docs: How to Enable Backup Encryption frappe/frappe_docs#204

Merged

fix: Whitespaces, Werkzeug locals, Invalid paths

e92feb3

stale bot added the inactive label Sep 27, 2021

stale bot closed this Sep 30, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Encrypted Backups #14126

feat: Encrypted Backups #14126

Anurag0911 commented Sep 6, 2021 •

edited

codecov bot commented Sep 6, 2021 •

edited

abhishekbalam commented Sep 13, 2021

gavindsouza Sep 17, 2021

Anurag0911 Sep 18, 2021

abhishekbalam Sep 20, 2021

gavindsouza Sep 17, 2021

abhishekbalam Sep 20, 2021

gavindsouza Sep 17, 2021

gavindsouza Sep 17, 2021

Anurag0911 Sep 18, 2021

abhishekbalam Sep 20, 2021

gavindsouza Sep 17, 2021

gavindsouza Sep 17, 2021

Anurag0911 Sep 18, 2021 •

edited

gavindsouza Sep 17, 2021

gavindsouza Sep 17, 2021

gavindsouza Sep 17, 2021

gavindsouza Sep 17, 2021

stale bot commented Sep 27, 2021

	@click.option('--backup-encryption-key', help='Backup encryption Key')
	@click.option('--encryption-key', help='Backup encryption Key')

feat: Encrypted Backups #14126

feat: Encrypted Backups #14126

Conversation

Anurag0911 commented Sep 6, 2021 • edited

Problem: Backups were not encrypted.

CheckBox to enable/disable backup encryption

codecov bot commented Sep 6, 2021 • edited

Codecov Report

abhishekbalam commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anurag0911 Sep 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Sep 27, 2021

Anurag0911 commented Sep 6, 2021 •

edited

codecov bot commented Sep 6, 2021 •

edited

Anurag0911 Sep 18, 2021 •

edited