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

feat: Add local gocryptfs support #1897

Draft
wants to merge 30 commits into
base: dev
Choose a base branch
from

Conversation

daviewales
Copy link
Contributor

@daviewales daviewales commented Oct 8, 2024

Blocked by

This is a rebase of Germar Reitze's gocrypt branch on the latest dev branch.

It seems to work, so I thought we should try to merge it as a base for future gocryptfs work.

I've tested backing up a test file, deleting it, then restoring it.

Related to #1734.

@buhtz
Copy link
Member

buhtz commented Oct 8, 2024

Wow that is a big step. Awesome!

Do we need the latest GoCryptFS when running TavisCI? Are there known Issues with gocryptfs from the Ubuntu repos?

@buhtz buhtz added External depends on others/upstream EncFS using the EncFS file system PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. labels Oct 8, 2024
@daviewales
Copy link
Contributor Author

I suspect that we don't need the latest version. I tested with the version in the latest Linux Mint, and it seemed to work.

@daviewales
Copy link
Contributor Author

The code fetching the latest version was already in Germar's original branch. I haven't made many changes here, other than fixing merge conflicts.

@buhtz
Copy link
Member

buhtz commented Oct 11, 2024

Please let me know if you prefer me to solve this merge conflicts.

@daviewales
Copy link
Contributor Author

I'll do another rebase, and switch Travis to fetch the packaged version of gocryptfs rather than the latest release while I'm at it.

@daviewales
Copy link
Contributor Author

daviewales commented Oct 15, 2024

I've rebased on the latest dev.
I also added a quick patch to the qt/configure script to include the new qt/manageprofiles directory when installing.

I confirmed that I can still backup a file, delete it, then restore it.

@buhtz
Copy link
Member

buhtz commented Oct 15, 2024

When my refactoring work on the manage profiles dialog is finished I think about a release candidate and release 1.5.3.
After this we can merge your PR, after intense testing of course, and later release it as 1.6.0 somewhere around Christmas or New Year.

@buhtz
Copy link
Member

buhtz commented Oct 15, 2024

FYI: I do plan a 1.5.3 release in the near future. After that release I would target your PR for a 1.6.0 release.

@daviewales
Copy link
Contributor Author

I'll apply the suggestions above when I get a chance. There are also a few lint errors, so I'll fix those up too.

@daviewales
Copy link
Contributor Author

The remaining lint errors are because there is similarity between the encfs and gocryptfs modules... Perhaps this can be ignored?

@buhtz
Copy link
Member

buhtz commented Oct 18, 2024

The remaining lint errors are because there is similarity between the encfs and gocryptfs modules... Perhaps this can be ignored?

Usually I would say "no" and recommend to create a base class to avoid code duplication. But we will remove encfs, so I see no problem to add an ignore clause for pylint.

# pylint: disable=duplicate-code

Not sure if this need to be place in both files.

@daviewales
Copy link
Contributor Author

daviewales commented Oct 18, 2024

Note to self: I need to test setting up a new gocryptfs backup from scratch. I've been tested this branch with one which is already configured.

EDIT: I've tested setting up a new backup profile from scratch, and the dialog needs some tweaking as it's missing a few inputs. I'll update it to match the EncFS local dialog.

@daviewales
Copy link
Contributor Author

The gocryptfs functionality works. But Germar changed the mount API slightly to require mount backends to include an init_backend() method. (Was originally init(), but I changed it to avoid confusion.)

However, the branch I rebased never updated the encfs mount backends to include this method.

From my initial inspection, I think that init_backend is very similar to the existing _mount method, but it extracts the code required to setup a new backend if it is not yet configured.

@buhtz
Copy link
Member

buhtz commented Jan 10, 2025

Great! You can add your name to the header of the files (see: SPDX-FileCopyrightText:). Decide yourself if you provide a nickname or your real name. Using email might cause spam.

I promoted that PR

@daviewales
Copy link
Contributor Author

Thanks. I'll try to find time over the next week or two to update the EncFS module to use the new init_backend method. After that, I think it might be ready.

@daviewales
Copy link
Contributor Author

daviewales commented Jan 15, 2025

I've updated EncFS with an init_backend method to match the new Mount structure in the gocryptfs branch.
It seems to work.

I've noticed one more missing feature.
EncFS would backup the EncFS configuration into the local config folder.
But currently, we don't have an equivalent backupConfig method for gocryptfs.
It should be pretty straightforward to copy the EncFS implementation so I'll try that soon.

EDIT: There's still a bug with EncFS. I'll see if I can track it down.

We are rebasing previous gocryptfs work from 2017. This work added an
`init_backend` method which  separates the concerns of first-time
initialisation vs subsequent use.
@buhtz
Copy link
Member

buhtz commented Feb 1, 2025

EDIT: There's still a bug with EncFS. I'll see if I can track it down.

In this case I recommend to set the PR into Draft mode.

If you describe the bug others might be able to help.

@daviewales daviewales marked this pull request as draft February 1, 2025 17:17
@daviewales
Copy link
Contributor Author

In the branch which I've rebased, it defines a new method for MountControl, which explicitly initialises the backend. In the original branch this was called init, but I've renamed it to init_backend to avoid confusion.

The new Gocryptfs code in this branch works with this refactored MountControl API, but the EncFS code was never updated to use it. In my latest commits, I've tried to adapt the EncFS code to use the explicit init_backend method. However, there is a bug such that when you create a new EncFS profile, somehow the mount is not correctly created, and an exception is raised.

I would appreciate your thoughts about whether we should really be changing the MountControl API in this PR, or whether we should try to split these changes. For example if you agree that the API change to require explicitly initialising the backend is a good idea, perhaps we could try to get just that change working and merged first, then come back to this Gocryptfs work.

On the other hand, if you feel that this init_backend change is a bad idea, then I can rework this branch to avoid making any changes to the MountControl API.

@buhtz
Copy link
Member

buhtz commented Feb 1, 2025

Hello David,

thank you for the summary.

I would appreciate your thoughts about whether we should really be changing the MountControl API in this PR, or whether we should try to split these changes. For example if you agree that the API change to require explicitly initialising the backend is a good idea, perhaps we could try to get just that change working and merged first, then come back to this Gocryptfs work.

Splitting the changes sounds the most elegant to me. First refactor/fix the mount API please.

Regards,
Christian

@buhtz
Copy link
Member

buhtz commented Feb 3, 2025

Just FYI. Debian announced their schedule for the next release. The two important dates are the 15th April (Soft Freeze) and 15th May (Hard Freeze). These dates are not written in stone and it is not uncommon for these to be postponed even further.

Anyway we should try to get the next upstream release of BIT done until 1st April, which is two weeks before the Soft Freeze. In this case our Debian package maintainer has two weeks to finalize the packaging.

Before the release, I would like to make a release candidate for a period as long as possible. But if there is not enough time for this, so be it.

Under circumstances and with friendly Debian maintainers it might be possible for us to bring a new BIT release into Debian even after the Soft Freeze, but before the Hard Freeze. So there might be one month more for us. The EncFS removal should be a good argument, related to security, to propose this.

Please don't get me wrong. This is a FOSS project with volunteers only. No one get hurt or fired if we don't reach the deadlines. So be it. We do our best with respect to our self and our private life.

@buhtz
Copy link
Member

buhtz commented Feb 8, 2025

Note to myself: Adapt man page to new situation.

@daviewales
Copy link
Contributor Author

daviewales commented Feb 8, 2025

No guarantees for the April deadline, but I'll see what I can do. I'm trying a more cautious incremental approach to avoid the issues above. My plan is to submit the following changes as three separate PRs:

  • Create tests for encfstools (WIP: Test Mount API #2039)
  • Update mount API with explicit init_backend method (and ensure it passes the encfstools tests)
  • Add gocryptfs support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EncFS using the EncFS file system External depends on others/upstream HELP-WANTED Used by 24pullrequests.com to suggest issues High PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants