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

Restore: Handle incorrect encryption key #5284

Merged
merged 4 commits into from
Apr 23, 2020

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Apr 23, 2020

Jira - DGRAPH-1281 and DGRAPH-1282

When the restore is started, we open a badger instance with the provided encryption key and then check if the backup file can be opened using the encryption key. If the backup cannot be opened using the encryption key, we return an error to the user but the badger DB stores the encryption key.
When the user tries to restore the same backup to the same badger instance without a key (because backup is unencrypted), badger complaints about the missing key.

This PR fixes by opening badger after opening the backup file. This ensures we don't open an encrypted badger db for an unencrypted backup file.


This change is Reviewable

@jarifibrahim jarifibrahim requested a review from a team April 23, 2020 10:35
worker/restore.go Outdated Show resolved Hide resolved
Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments.

Copy link
Contributor Author

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @manishrjain and @parasssh)


worker/restore.go, line 58 at r1 (raw file):

Previously, parasssh wrote…

keyfile is optional. Just return the err.

But the error would be Invalid Header. This doesn't give any valid information to the user.


worker/restore.go, line 60 at r1 (raw file):

Previously, parasssh wrote…

Dont need this whole comment. Just the first sentence will suffice.

Done.

Copy link
Contributor Author

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @manishrjain and @parasssh)


worker/restore.go, line 58 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

But the error would be Invalid Header. This doesn't give any valid information to the user.

The actual error returned is

gzip: invalid header

Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @parasssh)


worker/restore.go, line 58 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

The actual error returned is

gzip: invalid header

Ok. But the keyfile is optional. And if it fails for other reasons then it will confuse the error. Maybe do this error only if keyfile is non-nil.

Copy link
Contributor Author

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @manishrjain)


worker/restore.go, line 58 at r1 (raw file):

Previously, parasssh wrote…

Ok. But the keyfile is optional. And if it fails for other reasons then it will confuse the error. Maybe do this error only if keyfile is non-nil.

Ah! I see what you mean. I've made the change. Thank you!

Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jarifibrahim and @manishrjain)

a discussion (no related file):
:lgtm:


Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarifibrahim and @manishrjain)

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarifibrahim and @manishrjain)

@jarifibrahim jarifibrahim merged commit 25c37c8 into master Apr 23, 2020
@jarifibrahim jarifibrahim deleted the ibrahim/encrypted-backup-fix branch April 23, 2020 18:51
jarifibrahim pushed a commit that referenced this pull request Apr 23, 2020
Jira - DGRAPH-1281 and DGRAPH-1282

When the restore is started, we open a badger instance with the
provided encryption key and then check if the backup file can be
opened using the encryption key. If the backup cannot be opened
using the encryption key, we return an error to the user but the
badger DB stores the encryption key.
When the user tries to restore the same backup to the same badger
instance without a key (because backup is unencrypted), badger
complaints about the missing key.

This PR fixes by opening badger after opening the backup file.
This ensures we don't open an encrypted badger db for an
unencrypted backup file.

(cherry picked from commit 25c37c8)
jarifibrahim pushed a commit that referenced this pull request Apr 23, 2020
Jira - DGRAPH-1281 and DGRAPH-1282

When the restore is started, we open a badger instance with the
provided encryption key and then check if the backup file can be
opened using the encryption key. If the backup cannot be opened
using the encryption key, we return an error to the user but the
badger DB stores the encryption key.
When the user tries to restore the same backup to the same badger
instance without a key (because backup is unencrypted), badger
complaints about the missing key.

This PR fixes by opening badger after opening the backup file.
This ensures we don't open an encrypted badger db for an
unencrypted backup file.

(cherry picked from commit 25c37c8)
jarifibrahim pushed a commit that referenced this pull request Apr 23, 2020
Jira - DGRAPH-1281 and DGRAPH-1282

When the restore is started, we open a badger instance with the
provided encryption key and then check if the backup file can be
opened using the encryption key. If the backup cannot be opened
using the encryption key, we return an error to the user but the
badger DB stores the encryption key.
When the user tries to restore the same backup to the same badger
instance without a key (because backup is unencrypted), badger
complaints about the missing key.

This PR fixes by opening badger after opening the backup file.
This ensures we don't open an encrypted badger db for an
unencrypted backup file.

(cherry picked from commit 25c37c8)
jarifibrahim pushed a commit that referenced this pull request Apr 23, 2020
Jira - DGRAPH-1281 and DGRAPH-1282

When the restore is started, we open a badger instance with the
provided encryption key and then check if the backup file can be
opened using the encryption key. If the backup cannot be opened
using the encryption key, we return an error to the user but the
badger DB stores the encryption key.
When the user tries to restore the same backup to the same badger
instance without a key (because backup is unencrypted), badger
complaints about the missing key.

This PR fixes by opening badger after opening the backup file.
This ensures we don't open an encrypted badger db for an
unencrypted backup file.

(cherry picked from commit 25c37c8)
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
Jira - DGRAPH-1281 and DGRAPH-1282

When the restore is started, we open a badger instance with the 
provided encryption key and then check if the backup file can be
opened using the encryption key. If the backup cannot be opened
using the encryption key, we return an error to the user but the
badger DB stores the encryption key.
When the user tries to restore the same backup to the same badger
instance without a key (because backup is unencrypted), badger
complaints about the missing key. 

This PR fixes by opening badger after opening the backup file.
This ensures we don't open an encrypted badger db for an
unencrypted backup file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants