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

GPG encryption #135

Closed
wants to merge 2 commits into from
Closed

GPG encryption #135

wants to merge 2 commits into from

Conversation

kongwy
Copy link

@kongwy kongwy commented Oct 25, 2023

Add the ability to encrypt file(s) with GPG before uploading.

Change Details

Added two environment variables:

  • NEW: GPG_ENABLE
    • Description: Toggle the GPG feature.
    • Default: FALSE
  • NEW: GPG_PUBKEY
    • Description: Path to the public key for HPH encryption.
  • CHANGE: ZIP_PASSWORD
    • Default: "WHEREISMYPASSWORD?" -> "" (empty str)
    • Reason: It is no longer necessary to be compulsory as GPG encryption is an alternative.

P.S. I'm not quite familiar with shell script. Please feel free to make any further changes.

@kongwy kongwy mentioned this pull request Oct 25, 2023
@ttionya
Copy link
Owner

ttionya commented Oct 26, 2023

Thank you for your pull request.

There's another matter I believe is worth discussing, regarding the environment variable GPG_PUBKEY.

The environment variable GPG_PUBKEY specifies the path to the GPG public key, and users are required to mount the GPG public key file into the container when using it. However, I think it might be preferable to have the public key path determined internally rather than by the user.

Additionally, the /config/ path is defined as the configuration folder for rclone. While placing the GPG public key inside it is feasible, the provided docker-compose.yml file already directs /config/ to the vaultwarden-rclone-data volume, which could lead to confusion. Therefore, specifying it as /gpg/key.pub might be a better choice.

Furthermore, since containers can be launched using non-root users, mounting a file could present permission issues for these users.

So, I'm wondering if it's possible to have GPG_PUBKEY filled with the content of the public key rather than the file, and then use a script to write this content into the container. This approach could potentially avoid permission issues. Users who prefer to mount the file directly would need to handle any permission problems themselves. I'm not familiar with the gpg command, so I don't know whether it's possible to use the public key content directly instead of a public key file.

We can discuss this matter first and come to a solution. I would like to handle the implementation, and then you can assist with the review afterward.

@kongwy
Copy link
Author

kongwy commented Oct 26, 2023

@ttionya Fair enough. Filling GPG_PUBKEY with the public key directly sounds good. Although it seems gpg doesn't accept inline key content directly, I reckon it wouldn't be difficult to save the key as a file with cat in the script.

Again, I'm not really familiar with all these either. I just feel it would be great if this project could have GPG encryption, as all contents in a password manager backend should be considered sensitive data, while some of them (e.g. attachments) are not internally encrypted by Vaultwarden.

@ttionya
Copy link
Owner

ttionya commented Oct 26, 2023

Alright, I will modify GPG_PUBKEY to accept a public key string instead of a file path.

Once you've made the changes based on the review, I'll go ahead and merge this.

@ttionya
Copy link
Owner

ttionya commented Nov 5, 2023

@kongwy ,

I noticed that this pull request has had no new commits over 10 days. Are you willing to make code changes based on the review feedback?

@AtomAsking
Copy link

@ttionya Is there a plan to advance this PR? GPG encryption is very useful for me, I am looking forward to it. Thank you for everything you've done. Thanks.

@ttionya
Copy link
Owner

ttionya commented May 9, 2024

@AtomAsking ,

The development has been completed, but it lacks testing. I will check the existing code over the weekend and release a beta version for testing.

@ttionya
Copy link
Owner

ttionya commented May 11, 2024

Moved to PR #152 .

@ttionya ttionya closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants