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

Add support for Ente Auth import #1470

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

sigmundxia
Copy link
Contributor

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!

Copy link
Member

@alexbakker alexbakker left a 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.

@sigmundxia
Copy link
Contributor Author

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.

Copy link
Member

@alexbakker alexbakker left a 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:

/**
* 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.
*/

@@ -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>
Copy link
Member

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.

Copy link
Contributor Author

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));
Copy link
Member

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.

Copy link
Contributor Author

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) {
Copy link
Member

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.

Copy link
Contributor Author

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.

@sigmundxia
Copy link
Contributor Author

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.

Copy link
Member

@alexbakker alexbakker left a 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.

@sigmundxia
Copy link
Contributor Author

I think it's ready😊, but I'd like to know if it needs any further revisions.

Copy link
Member

@alexbakker alexbakker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@alexbakker alexbakker merged commit 356fa8a into beemdevelopment:master Sep 19, 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.

2 participants