xpipe-io / xpipe

Your entire server infrastructure at your fingertips
https://xpipe.io
Apache License 2.0
2.82k stars 77 forks source link

[Bug] Git sync doesn't seem to work as expected #334

Closed jbmay closed 2 weeks ago

jbmay commented 3 weeks ago

Have recently started testing xpipe out and am interested in the sync function. The current git based sync doesn't seem to work 100% as expected. Some things I have noticed:

Contents of vault data directory are stored in plain text

I like the ability to add a private key file for a connection to the vault to be synced, but I would expect the contents to be encrypted the same way passwords are encrypted. Even when the "Encrypt all vault data" setting is enabled, the private key I am testing with is visible in plain text in the git repo that it is being committed to.

I think either the contents of the data directory should always be encrypted like connection passwords, or encrypting the contents should be toggleable with its own setting that is similar to the "Encrypt all vault data" setting and have it default to being enabled to prevent accidental committing of private keys. It is silly to store connection configs with encrypted passwords, but then show a link to a private key that is stored in plaintext in the same repo that can be used in place of the connection password.

When "Encrypt all vault data" is enabled it doesn't seem to be syncing anything new into the "stores" subdirectory of the git repo

I have tested by turning on the encrypt all setting and then adding a category to the git vault. I see the categories directory with a category.json file show up in the git repo, but no stores directory. If I turn off the encrypt all setting, the stores subdirectory immediately gets populated with the partially encrypted configuration contents. Once data is in the stores directory if I then turn on the encrypt all setting again, the contents of files get encrypted.

I would expect to not need to add things unencrypted before being able to turn on the encrypt all setting to have them get encrypted. Not much of a point encrypting file contents after the unencrypted values are stored in git history.

Example repo contents with "Encrypt all vault data" enabled and then adding a category to the git repo. Category file gets added and the readme gets updated with links to nonexistent "stores" directory subpaths. image

Immediately after turning off the "Encrypt all vault data" setting, the stores directory is populated with the connection configuration data that is only partially encrypted. image

It is too easy to accidentally commit things unencrypted

Currently if you toggle the "Encrypt all vault data" setting there is no confirmation of any kind. If the setting is on and your git repo contains fully encrypted configuration files, turning the setting off immediately replaces the fully encrypted file contents with the unencrypted values. Once you have commits containing unencrypted values, encrypting them in future commits is less beneficial since they can just be accessed via git history.

At a minimum I would expect disabling the "Encrypt all vault data" setting to prompt you to confirm that you want to disable it and inform the user that it will result in anything that is currently committed being updated with unencrypted contents when they exit the settings. Even better would be if this prompt either turned off syncing and told the user it was disabled to prevent accidental committing of unencrypted contents, or give the option to toggle off syncing from the prompt. This way committing unencrypted configuration has to be a more conscious choice from the user instead of just automatically happening when this setting is turned off.

crschnick commented 3 weeks ago

That is already implemented for XPipe 11 and there are early versions available at https://github.com/xpipe-io/xpipe-ptb

Essentially all data files are now encrypted by default and only these encrypted files are committed. However, that only works right now for newly commited files and not the old ones. Mainly because that would require a git history rewrite, which is difficult to do.

jbmay commented 3 weeks ago

Awesome to hear that! I will try the beta out then.

crschnick commented 2 weeks ago

This is now released in XPipe 11: https://github.com/xpipe-io/xpipe/releases/

Feel free to report any issues if it does not work as expected.

jbmay commented 2 weeks ago

Just updated to version 11 and still seem to be encountering issues with encrypted contents not actually being committed.

Updated to version 11, disabled syncing temporarily, recreated my remote git repo to remove any unencrypted git history, removed the .git directory from the xpipe storage directory, turned syncing back on (with the encrypt all vault data setting enabled), and it still doesn't appear to be syncing any encrypted files to the remote git repo. Only difference now from before is that the data directory is also not being synced.

Screenshot of the remote repo: image

Screenshot of the git status in the xpipe storage directory showing untracked files that aren't being committed: image

jbmay commented 2 weeks ago

I just tried restarting xpipe and after that the ssh key that I added to the data subdirectory did show up in the git remote and it is encrypted. But the connection configs that get synced under the stores subdirectory if encryption is disabled are still not getting committed.

crschnick commented 2 weeks ago

The data file encryption is always enabled now, you don't have to turn on the setting. The setting is intended for the json data.

The stores data is only added if categories are explicitly set to sync and individual connections can be synced (E.g. local ones can't be synced). image

crschnick commented 2 weeks ago

Ideally the workflow now would be that once the git repository is connected, it should be opened in the browser. There should be a readme committed that should explain how to add connections to the git repo as they are not by default. I think committing all available connections by default would be a little bit overreaching

jbmay commented 2 weeks ago

I have explicitly added a category to the git repo: image

It is exhibiting the same behavior as when I was seeing in version 10. The stores data will only be committed initially if the setting for encrypting the full json data is disabled. This commits the partially encrypted json files. Then if I turn the setting back on, the json files become fully encrypted in the git repo.

The bug appears to be that it won't commit new fully encrypted config files. Fully encrypted configs only seem to be getting synced if have already been committed only partially encrypted.

crschnick commented 2 weeks ago

Alright let me take a look at that. That might have slipped through testing.

jbmay commented 2 weeks ago

I wouldn't expect it to be the issue, but just in case it matters, this is being observed on an ARM macbook pro running macOS 14.5.

crschnick commented 2 weeks ago

So I can't reproduce this issue. I tried in various different ways but it's committed as it should for me. Technically the encryption should be independent of the git integration. Maybe I'm missing something.

But I was able to add a warning when disabling the advanced encryption again. I can release that in 11.1

jbmay commented 2 weeks ago

Some additional context to hopefully help troubleshooting this.

When I remove a category from the git repo and open up the xpipe log file I can search for "git rm" and find log lines showing xpipe is trying to remove the json files for the connection configuration from under the "stores" subdirectory in the git repo. They aren't actually committed, so this isn't doing anything, but it is at least logging that it is running the git rm commands as you would expect.

But when I try to add the category back to the git repo and then search for "git add" in the logs I can only find lines showing it adding changes to the README, vaultversion, vaultkey, preferences, and the category.json file for the category I added. There are no log lines referencing anything under the stores subdirectory. So it doesn't appear to even be trying to add the connection configuration, or at least it isn't logging that it is attempting to add those files or any errors related to doing so.

crschnick commented 2 weeks ago

And this problem persists if you restart xpipe after changing the encryption option?

jbmay commented 2 weeks ago

Okay, kind of narrowed it down more.

I still had a category with 3 connections added to the git repo that weren't showing up in the remote repo. I tested creating a new category, added it to the git repo before it had any connections under it, then I moved a connection to that category and that specific connection got committed to the repo. The connections still in the old category were still not committed however and clicking on one of the README links to a connection in the old category threw a 404 because it wasn't in the repo.

Then I moved the connection from the testing category back to the original category and now all of the connections in that original category appear to have been committed correctly. With data under the "stores" directory already committed, I can create a new category, fill it with connections, then add it to the git repo and they all get committed as expected. I then removed all categories from the git repo so that the stores and categories directories are no longer committed in the remote repo. Then if I add one of the categories that already contains connections, I again observe the original behavior of the stores directory not being committed as expected.

So the specific issue seems to be when no categories/connections have been added to the git repo yet, enabling the "Encrypt all vault data" setting, then adding a category that already contains connections to the git repo, the category gets committed and the README is updated with links to the stores config files, but the json connection data under the stores directory doesn't get committed.

Hope that is easy to follow. Seems like something you wouldn't be able to reproduce unless you completely remove all connections from your git repo and then try to add a category that already contains connections to the git repo with the "Encrypt all vault data" setting enabled.

jbmay commented 2 weeks ago

Might be easier to follow if I word it as a "steps to reproduce" list instead of long run on sentences. lol

  1. Either remove all categories from the git repo or start with a new completely empty git repo so the "stores" and "categories" subdirectories are not present in the latest commit of the git repo
  2. Enable the "Encrypt all vault data" setting
  3. Create a category and add some connections to it
  4. Add that category that has connections to the git repo
  5. On the git remote you should see the categories subdirectory show a category file for the category you added and README links to stores files that should be there, but there will be no stores subdirectory committed
crschnick commented 2 weeks ago

Found the issue, it has something to do with the very specific condition that no store entry has been added to the git repo yet. It has nothing to do with the encryption setting in particular.

That will also be included in 11.1.

jbmay commented 2 weeks ago

Awesome! I guess I was only trying specifically to add categories with the setting already turned on so assumed it was specific to that since you hadn't encountered it before. Glad it was able to be resolved!

crschnick commented 2 weeks ago

This fix has been released and should hopefully work now as expected