-
-
Notifications
You must be signed in to change notification settings - Fork 385
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
Add support for Ente Auth import #1470
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
When the suggestion was made to add support for importing from Ente Auth and I initially reviewed their documentation, it seemed to suggest that they're using plain XChaCha20-Poly1305. But actually, they're using libsodium's secretstream
algorithm (as you've also discovered).
I'm a bit torn on this. The dependency on libsodium comes with a couple of issues:
- Adds ~1MB (~20%) to Aegis' APK size, a bit much for just an importer.
- Breaks the F-Droid build because lazysodium vendors the binary libsodium shared library.
- Unconfirmed: Unit tests that (indirectly) use libsodium functions will probably only work in an Android emulator
I don't think we can review/merge this PR as is. There are three options (from easiest to hardest):
- Option 1: Don't support importing from encrypted Ente Auth files
- Option 2: Write our own
secretstream
implementation. I don't think we have to wait for BouncyCastle to support XChaCha20-Poly1305, as we're only missing HChaCha20 which is fairly simple. - Option 3: Proper integration of building libsodium as part of Aegis. Very painful, but would allow us to solve some other long standing issues like Key derivation causes OOM crash on low-end devices #114, making it easier to justify the 1MB increase in APK size.
I think for now, option 1 is the easiest and gets us at least some level of support for importing from Ente Auth. If there turns out to be lots of demand for support for encrypted Ente Auth files, we can consider options 2 and 3.
9caf83c
to
31153b6
Compare
Thank you for your detailed response! Indeed, I realize that the extra size of the APK file and the f-droid requirements are significant shortcomings. I reworked the code as per your option 1 and now it only supports importing unencrypted Ente Auth files. Hopefully this will fulfill the requirements! Please let me know if there is anything else I can do to improve it further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've left a couple of comments.
Could you add a test for this as well? Instructions can be found here:
Aegis/app/src/test/java/com/beemdevelopment/aegis/importers/DatabaseImporterTest.java
Lines 38 to 45 in 17f106f
/** | |
* The procedure for adding new importer tests is as follows: | |
* 1. Generate QR codes for each test vector: | |
* -> while read line; do (qrencode "$line" -o - | feh -); done < ./app/src/test/resources/com/beemdevelopment/aegis/importers/plain.txt | |
* 2. Scan the QR codes with the app we want to test our import functionality of | |
* 3. Create an export and add the file to the importers resource directory. | |
* 4. Add a new test for it here. | |
*/ |
app/src/main/res/values/strings.xml
Outdated
@@ -576,4 +576,5 @@ | |||
<item quantity="one">%d item selected</item> | |||
<item quantity="other">%d items selected</item> | |||
</plurals> | |||
<string name="importer_help_ente_auth">Supply an Ente Auth export file. Currently only unencrypted files are supported.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this string to spot where the other importer_help_*
strings are grouped together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine :) I made the changes as you suggested.
try { | ||
new JSONObject(file); | ||
} catch (JSONException e) { | ||
return new DecryptedState(parseUris(file)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can delegate basically all logic to a different importer like this:
GoogleAuthUriImporter importer = new GoogleAuthUriImporter(requireContext());
return importer.read(new ByteArrayInputStream(bytes));
And then remove DecryptedState
and parseUris
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine :) I made the changes as you suggested.
return new DecryptedState(parseUris(file)); | ||
} | ||
throw new DatabaseImporterException("Encrypted files are currently not supported."); | ||
} catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catch IOException
here instead of the generic Exception
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine :) I made the changes as you suggested.
Thank you for your thoughtful suggestions! I have made the changes and would like to know if this meets your expectations :) If there is anything I need to modify further, please let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing: I'd prefer an actual Ente Auth export for the test. You can use this one: https://alexbakker.me/u/vnbj9ih8fq.txt. Call it ente_auth.txt
and place it in app/src/test/resources/com/beemdevelopment/aegis/importers
.
Please squash everything into a single commit as well.
I think it's ready😊, but I'd like to know if it needs any further revisions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@sigmundxia @alexbakker Hi, it seems to work but we have a minor issue. could you please revise the code, the plan text import from ente app shows few line site text containing, "totp" and few in CAPS "TOTP", the import fails when the file have caps TOTP. only selected account which have "TOTP" fails. Thanks. |
Hello dear developers,
I've added the functionality to import configurations from Ente Auth into Aegis, supporting both encrypted and unencrypted formats. This should address the problem discussed in this issue.
I hope this PR proves useful! If there are any adjustments or improvements you'd like me to make, please don't hesitate to reach out.
Best regards!