[BUG] symlinks are clobbered by the app

[msfjarvis]

Describe the bug A password will fail to decrypt if it is a soft link as opposed to a normal file.

To Reproduce Steps to reproduce the behavior:

1. Create a password that's a soft link to an existing password using ln -s <passwd>.gpg softlink.gpg
2. Pull the repository on device
3. Try to decrypt the password
4. See error

Expected behavior Password is decrypted and shown in the UI

Device information (please complete the following information):

• Device: Google Pixel 2
• OS: Android R DP1
• App version 1.6.0-SNAPSHOT

Additional context

Relevant log snippet

E  onError getErrorId: 0
E  onError getMessage: No valid OpenPGP encrypted or signed data found!

[Dioxo]

I was looking into the function decryptPassword from PasswordStore.kt and tried to first validate that the file is a symbolik link with Files.isSymbolicLink(item.file.toPath()) as a test to validate its type. But it appears that file is not even a symbolik link ! To validate this, i created 2 other files (also symbolic links) from 2 different ways, I did it hard coded, it's not the best, but just to quickly validate my hypothesis.

I created a file called "test" that's gonna be normal file and 2 symbolik link files. Os.symlink("/data/data/dev.msfjarvis.aps.debug/files/store/test", "/data/data/dev.msfjarvis.aps.debug/files/store/test2.gpg")

Runtime.getRuntime().exec("ln -s /data/data/dev.msfjarvis.aps.debug/files/store/test" + " /data/data/dev.msfjarvis.aps.debug/files/store/test3.gpg")

And i opened the Device File Explorer from Android Studio and went where the file is stored. Just to see that the 2 files that i created doesn't have any issue. They are linking my file 'test' and working well with decryption. I even changed the password from test to see if others files worked well, they did.

So... If what i'm saying is right, the problem is when importing the files and not the file itself.

I'll try to validate this also, but just to know what you think. @msfjarvis

[msfjarvis]

I don't think that's the case, but I only checked with softlinks I created on my PC. I used ln -s test_real.gpg test_softlink.gpg then verified that it indeed was correctly pointing to the source. After I committed both files and pushed to git, the entries failed to deserialize on my phone.

[Dioxo]

Yeah, i did the same to create the file on my PC. After that i run to validate if it's a symbolik link on my phone Files.isSymbolicLink(item.file.toPath()), and it said no.

So to validate, i created 2 files dinamically, and they went all good.

Try to see the file from the Device File Explorer. You'll see that's a normal file.

[msfjarvis]

Yeah, i did the same to create the file on my PC. After that i run to validate if it's a symbolik link on my phone Files.isSymbolicLink(item.file.toPath()), and it said no.

So to validate, i created 2 files dinamically, and they went all good.

Try to see the file from the Device File Explorer. You'll see that's a normal file.

That's just the thing, a symlink is not a normal file! See this as an example, chmod does not affect a symlink like it affects a normal file.

[Dioxo]

Yes, i did what you mention, i have my files in my PC like this

I did a push, later pull from my phone and I created 2 more files dinamically on my phone, test2 and test3 that are soft links poining to normal_file.gpg.

In the Device File Explorer, i can see that my files test2 and test3 are linking to normal_file.gpg.

BUT the file soft_link don't say that it's a symbolik link, also the rights aren't set normally to what it should be as a soft link.

[msfjarvis]

Right, so what do you believe is going wrong? Is JGit parsing these wrongly from the index?

[Dioxo]

I think that the problem is when importing the files. Not sure when exactly. I'll try to see this tomorrow.

Also the logic from decrypting a file that (really) is a soft link, works fine, i can decrypt test2 and test3 without problems. And now it's normal why we are getting "No valid OpenPGP encrypted or signed data found!", because the content from the file soft_link is just plain text with content 'normal_file' as text. That's why i said it's a normal file

[Dioxo]

@msfjarvis After reading more about how git handles symbolic links, I can not see a good solution for this issue.

Git just stores the contents of the link (i.e. the path of the file system object that it links to) in a 'blob' just like it would for a normal file. It then stores the name, mode and type (including the fact that it is a symlink) in the tree object that represents its containing directory. When you checkout a tree containing the link, it restores the object as a symlink regardless of whether the target file system object exists or not.

Source : https://stackoverflow.com/a/954575

Now i undestand why the soft_link.gpg file stores the file path that is linking to (in our case normal_file.gpg)

In order to try it, i did a test. I have the file 'test_soft' that links to 'normal_file', i added, commited, pushed after this With the command git ls-files -s ./test_soft.gpg I can see the mode that git associates to my file

(Note 120000 is the mode listed in ls-files output. It would be something like 100644 for a regular file)

After some modifications to my repository in my phone (commits and push), if i pull from my PC I see that my file is not anymore a soft link.

With git diff is easier to see that my file changed its mode from 120000 to 100644, and also added the path file to its content. deleted file mode 120000 -normal_file.gpg

new file mode 100644 +normal_file.gpg

So... I don't really see how to resolve this issue, each time that we pull from the phone, it's going to be a normal file, because it's a problem with git and how it stores files. A possible option that i can think is to read the content from file and go to search the file that reference the content. Because the file's content has the path to the file. But it's going to be harder to try to validate that the path points to a real file (because is going to be a normal file, so we can't assure that is not the user trying to add a normal file with a path instead of a soft link that becames a normal file) , also there's the problem to read the file and know that it's not an encrypted one.

[msfjarvis]

@FabianHenneke what do you think? Would it be too expensive to try reading the first line of a file and seeing if it is a valid relative path before sending the file off to OpenKeychain? We can special-case this scenario but I still think this is a JGit bug rather than Git problem.

[msfjarvis]

We're also pretty behind on JGit releases, is the blocker still Java 8 APIs?

[fmeum]

@FabianHenneke what do you think? Would it be too expensive to try reading the first line of a file and seeing if it is a valid relative path before sending the file off to OpenKeychain? We can special-case this scenario but I still think this is a JGit bug rather than Git problem.

I'm also pretty sure that this is a JGit bug that will only really go away once JGit does... I personally don't like these kind of hacks and would rather focus efforts on replacing JGit with something more easily maintainable. When I tried to use symbolic links in APS, they failed for completely different reasons, so I don't feel like opening this can of worms. But please keep in mind that I'm not using symbolic links in my pass repos myself, so I may be biased.

[Dioxo]

Well, i don't know much about JGit so i can't really say if the probleme is with it. And i couldn't recreate the issue using only my PC (messing around with soft links), so it makes me wonder if it's maybe JGit's fault.

[Dioxo]

In the doc from JGit i see they support Symbolic links, but i'm not sure which version we're using from JGit and if there's a bug in that specific version.

Native symbolic links are supported, provided the file system supports them.

https://github.com/eclipse/jgit#warningscaveats

[fmeum]

I'm not even sure whether symbolic links are supported on all phones. On FAT-formatted SD cards, this is certainly not the case, but this may also be something that could cause issues seemingly out of nowhere.

[Dioxo]

I'd like to know what you think about this issue and possible solutions @FabianHenneke @msfjarvis

[fmeum]

I'd like to know what you think about this issue and possible solutions @FabianHenneke @msfjarvis

Based on my limited understanding of this issue, git is doing the only reasonable thing here to serialize symlinks. When they end up broken on the phone after a push, that has to be JGits fault. I googled for a bit and found https://github.com/jenkinsci/git-client-plugin/compare/11d9be244177...d48982900e34, which indicates to me that #719 may fix the symbolic link issue. @msfjarvis @Dioxo Could you give this a try?

[ismail]

Just hit this issue with the latest release on Play Store, fyi.

[msfjarvis]

Just hit this issue with the latest release on Play Store, fyi.

That's why it's still open :) We haven't found a way to resolve it across all Android versions yet.

[Dioxo]

I was looking again at this issue to see if I could find a solution and it magically worked with the recommendation that Fabian had given before, when integrating the java7 version of jgit, it recognizes the symbolic links. I add a screenshot. See file test.gpg and other.gpg

The only problem is that if the link doesn't have a .gpg extension, APS doesn't show the file, in my case the file soft is not shown in the application. I think we can have a confirmation to show the file if it's soft link and doesn't have extension .gpg

What do you think @FabianHenneke @msfjarvis ? I'll do a pull request to try again this solution?

[msfjarvis]

I was looking again at this issue to see if I could find a solution and it magically worked with the recommendation that Fabian had given before, when integrating the java7 version of jgit, it recognizes the symbolic links.

Interesting, does it also work in release builds? Fabian's PR certainly didn't work for me and I usually test PRs on release builds so there's a good chance it doesn't, and needs additional R8 rules.

I add a screenshot. See file test.gpg and other.gpg

The only problem is that if the link doesn't have a .gpg extension, APS doesn't show the file, in my case the file soft is not shown in the application. I think we can have a confirmation to show the file if it's soft link and doesn't have extension .gpg

No, we're not going to do that. Symlink or actual, a password file has to end in .gpg to be considered a password.

[Dioxo]

I don't really know how to test it in release builds. How do I do that? And I tell you if that works. ;)

[fmeum]

I was looking again at this issue to see if I could find a solution and it magically worked with the recommendation that Fabian had given before, when integrating the java7 version of jgit, it recognizes the symbolic links.

Interesting, does it also work in release builds? Fabian's PR certainly didn't work for me and I usually test PRs on release builds so there's a good chance it doesn't, and needs additional R8 rules.

I think what @Dioxo was referring to is not my most recent PR, but rather the older observation that a more recent version of JGit does correctly deal with symlinks (likely because it has access to toRealPath and friends).

[Dioxo]

Yeah, I'm talking about this one

https://github.com/android-password-store/Android-Password-Store/pull/719

[msfjarvis]

I don't really know how to test it in release builds. How do I do that? And I tell you if that works. ;)

Generate a keystore following the steps here, then create a file called keystore.properties in the root of your repository with the following fields, with the alias and passwords you used when generating the keystore earlier. storeFile is a path relative to the keystore.properties file.

keyAlias=
keyPassword=
storeFile=
storePassword=

I was looking again at this issue to see if I could find a solution and it magically worked with the recommendation that Fabian had given before, when integrating the java7 version of jgit, it recognizes the symbolic links.

Interesting, does it also work in release builds? Fabian's PR certainly didn't work for me and I usually test PRs on release builds so there's a good chance it doesn't, and needs additional R8 rules.

I think what @Dioxo was referring to is not my most recent PR, but rather the older observation that a more recent version of JGit does correctly deal with symlinks (likely because it has access to toRealPath and friends).

I'm aware.

[Dioxo]

I'll do it and I'll tell you what I got.

[Dioxo]

Well, i did what you mentioned, i created my private key using keytool and the file keystore.properties, After that i did Build/Generate signed apk in android studio and it creates a app-release.apk. I tried the apk on my phone and i works.

So not sure if it was like that the process to create the release build. What do you think @msfjarvis?

[msfjarvis]

Well, i did what you mentioned, i created my private key using keytool and the file keystore.properties, After that i did Build/Generate signed apk in android studio and it creates a app-release.apk. I tried the apk on my phone and i works.

So not sure if it was like that the process to create the release build. What do you think @msfjarvis?

If app-release.apk was generated then it's a signed build. I guess I'll have to recheck that PR on my end too...

[Dioxo]

Cool, keep me updated to see if we finally solve this issue.

[msfjarvis]

Symlinks are still being clobbered for me on the release build.

[Dioxo]

Could I go to the Gitter room and test the apk that you generate?. @msfjarvis

[msfjarvis]

Could I go to the Gitter room and test the apk that you generate?. @msfjarvis

Sure I'll upload it there.

[fmeum]

Re-opening since we had to revert #1016.

In order to resolve this, one would need to make a stub version of FS_POSIX_Java7 that would only add the symlink features and hide them behind a Build.VERSION check.

[msfjarvis]

I'll take this one.

[msfjarvis]

JGit's using java.nio underneath in the Java 7 compat layer so we can't really backport this to below API 26 even if we get extremely lucky.

[fmeum]

JGit's using java.nio underneath in the Java 7 compat layer so we can't really backport this to below API 26 even if we get extremely lucky.

Can we just use the Java6 layer + add symlink support behind a version code check and call that FS_POSIX_Java7 so it's picked up by the magic heuristic? We wouldn't need FileUtil then.

[msfjarvis]

JGit's using java.nio underneath in the Java 7 compat layer so we can't really backport this to below API 26 even if we get extremely lucky.

Can we just use the Java6 layer + add symlink support behind a version code check and call that FS_POSIX_Java7 so it's picked up by the magic heuristic? We wouldn't need FileUtil then.

I'll look into it.

[msfjarvis]

I can't find a Java6 artifact in the v3.7 tree :confused:

[fmeum]

I can't find a Java6 artifact in the v3.7 tree

Not quite sure I understand your problem. My suggestion would be to extend FS_POSIX_Java6 in this way:

package org.eclipse.jgit.java7

import android.os.Build
import androidx.annotation.RequiresApi
import java.io.File
import java.nio.file.Files
import java.nio.file.LinkOption
import org.eclipse.jgit.util.FS
import org.eclipse.jgit.util.FS_POSIX_Java6

class Java7FSFactory : FS.FSFactory() {

    override fun detect(cygwinUsed: Boolean?): FS {
        return FS_POSIX_Java6_with_optional_symlinks()
    }
}

class FS_POSIX_Java6_with_optional_symlinks : FS_POSIX_Java6() {

    override fun supportsSymlinks() = Build.VERSION.SDK_INT >= Build.VERSION_CODES.O

    @RequiresApi(Build.VERSION_CODES.O)
    override fun isSymLink(file: File) = Files.isSymbolicLink(file.toPath())

    @RequiresApi(Build.VERSION_CODES.O)
    override fun readSymLink(file: File) = Files.readSymbolicLink(file.toPath()).toString()

    @RequiresApi(Build.VERSION_CODES.O)
    override fun createSymLink(source: File, target: String) {
        val sourcePath = source.toPath()
        if (Files.exists(sourcePath, LinkOption.NOFOLLOW_LINKS))
            Files.delete(sourcePath)
        Files.createSymbolicLink(sourcePath, File(target).toPath())
    }
}

[msfjarvis]

I can't find a Java6 artifact in the v3.7 tree

Not quite sure I understand your problem. My suggestion would be to extend FS_POSIX_Java6 in this way:

I wasn't paying attenting and started looking for a separate Java 6 library like we have for Java 7. I'll try and get this done tonight.

[XenGi]

Do I understand that correctly that this should fix the issue of having symlinks in the store and moving this to an android phone and back converts them into files with the symlink target because of missing symlink support in jgit and java6? because I still have this problem. :(

[msfjarvis]

If it doesn't work then the reason for that could be our raw File-based access corrupting it. I'll try to prioritize this over the next few weeks.

[532910]

symlinks still doesn't work, v1.13.1 and v2.0.0

Error from OpenKeyChain : No valid OpenPGP encrypted or signed data found!

[msfjarvis]

symlinks still doesn't work, v1.13.1 and v2.0.0

Error from OpenKeyChain : No valid OpenPGP encrypted or signed data found!

Yep, there's still a few potential sources for corruption. I'll take a look eventually.

[532910]

Files that should be symlinks are still plaintext files with symlink target as a content.

[msfjarvis]

Since this is still a bug, might as well.

[fmeum]

I just started debugging the issue and turned up very confused: I expected the issue to be that we don't set our custom FS during the clone operation, given that the CloneCommand setup does not involve repository. However, even with a statically set FS with symlink support, symlinks are not created. In fact, for a symlink, DirCacheEntry.getFileMode() returns the "regular file" type. If I didn't mess up, this means that the corruption would already occur in the Git-internal file mode bits.

[msfjarvis]

I just started debugging the issue and turned up very confused: I expected the issue to be that we don't set our custom FS during the clone operation, given that the CloneCommand setup does not involve repository. However, even with a statically set FS with symlink support, symlinks are not created. In fact, for a symlink, DirCacheEntry.getFileMode() returns the "regular file" type. If I didn't mess up, this means that the corruption would already occur in the Git-internal file mode bits.

Well that does throw a spanner in the works. Upgrading JGit could help, and desugaring is now available on stable AGP so we can give that a shot and see how far we can get without breaking API 23. Or we can just commit, go minSdk 26, and forget about the 115 Play Store users that would alienate. If people are surviving on APS 1.3.3 still (I got another "I recently tried switching from the legacy app" email today, so this is definitely real) they sure can handle APS 1.13.1?

[fmeum]

Well that does throw a spanner in the works. Upgrading JGit could help, and desugaring is now available on stable AGP so we can give that a shot and see how far we can get without breaking API 23. Or we can just commit, go minSdk 26, and forget about the 115 Play Store users that would alienate. If people are surviving on APS 1.3.3 still (I got another "I recently tried switching from the legacy app" email today, so this is definitely real) they sure can handle APS 1.13.1?

Switching to a higher minSdk for 2.0 would probably make sense. Most of our recent feature additions are only applicable to Oreo and up anyway and we are even dropping accessibility support.

[msfjarvis]

Well that does throw a spanner in the works. Upgrading JGit could help, and desugaring is now available on stable AGP so we can give that a shot and see how far we can get without breaking API 23. Or we can just commit, go minSdk 26, and forget about the 115 Play Store users that would alienate. If people are surviving on APS 1.3.3 still (I got another "I recently tried switching from the legacy app" email today, so this is definitely real) they sure can handle APS 1.13.1?

Switching to a higher minSdk for 2.0 would probably make sense. Most of our recent feature additions are only applicable to Oreo and up anyway and we are even dropping accessibility support.

I'll try and upgrade JGit over the weekend then.

[timvisee]

Any progress on this?

Just came across the same issue. After doing a Android Password Store sync, it added a commit with replaced the symlink with a plain text file (having the symlink path in it). Based on the comments above, I'm assuming that JGit fails to clone/pull properly and creates a plaintext file instead of a symlink. This is basically breaking my password store (on other devices) at the moment.