Skip to content

Check CSR #10713

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 6, 2023
Merged

Check CSR #10713

merged 4 commits into from
Jan 6, 2023

Conversation

@tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Sep 5, 2022 •
edited
Loading

Signed-off-by: tobiasKaminsky [email protected]

  • Tests written, or not not needed

@tobiasKaminsky tobiasKaminsky mentioned this pull request Sep 5, 2022
Merged
@tobiasKaminsky
Copy link
Member Author

tobiasKaminsky commented Sep 6, 2022

@AlvaroBrey
it seems that with gplayDebug flavor we have:

java.lang.ClassCastException: com.google.android.gms.org.conscrypt.OpenSSLRSAPublicKey cannot be cast to org.conscrypt.OpenSSLRSAPublicKey
at com.owncloud.android.util.EncryptionTestIT.testModulus(EncryptionTestIT.java:187)

but it runs fine locally on genericDebug.

I do not see/understand why conscrypt is behaving differently on geplay/generic flavor.

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Sep 6, 2022

@AlvaroBrey it seems that with gplayDebug flavor we have:

java.lang.ClassCastException: com.google.android.gms.org.conscrypt.OpenSSLRSAPublicKey cannot be cast to org.conscrypt.OpenSSLRSAPublicKey
at com.owncloud.android.util.EncryptionTestIT.testModulus(EncryptionTestIT.java:187)

but it runs fine locally on genericDebug.

I do not see/understand why conscrypt is behaving differently on geplay/generic flavor.

At this line: https://github.com/nextcloud/android/blob/71f16d6e978141d58a6d92d5ee3616ddcae13379/app/src/main/java/com/owncloud/android/utils/EncryptionUtils.java#L690-L690 , KeyPairGenerator.getInstance returns different classes (org.conscrypt... in generic, com.android.gms.org.conscrypt... in gplay).

We install the Conscrypt provider here in MainApp. But in the gplay flavor, ProviderInstaller is called later, installing the gms provider and overriding the previous one.

ProviderInstaller installs up-to-date conscrypt security providers patched by Google Services regularly (documentation).

The solution to all this should be to use the RSAPublicKey interface instead of the implementation-specific OpenSSLRSAPublicKey. Pushed 19c6095 to do just that, let's see how that goes.

AlvaroBrey
AlvaroBrey approved these changes Nov 25, 2022
View reviewed changes
Copy link
Member

@AlvaroBrey AlvaroBrey left a comment

Choose a reason for hiding this comment

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

Needs rebase & conflict resolution, otherwise looks fine

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Dec 12, 2022

/rebase

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Dec 13, 2022

/rebase

1 similar comment
@tobiasKaminsky
Copy link
Member Author

tobiasKaminsky commented Dec 15, 2022

/rebase

@tobiasKaminsky
Copy link
Member Author

tobiasKaminsky commented Dec 15, 2022

/rebase

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Dec 15, 2022

Exception is java.lang.ClassCastException: com.google.android.gms.org.conscrypt.OpenSSLRSAPublicKey cannot be cast to org.conscrypt.OpenSSLRSAPublicKey, we already saw this in a previous test, it needs to be the common interface instead

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Dec 15, 2022

Exception is java.lang.ClassCastException: com.google.android.gms.org.conscrypt.OpenSSLRSAPublicKey cannot be cast to org.conscrypt.OpenSSLRSAPublicKey, we already saw this in a previous test, it needs to be the common interface instead

Fixed in 59b225f

tobiasKaminsky and others added 2 commits January 5, 2023 09:04
@tobiasKaminsky
Check CSR
7abef86 
Signed-off-by: tobiasKaminsky <[email protected]>
@AlvaroBrey @tobiasKaminsky
Use RSAPublicKey interface instead of OpenSSLRsaPublicKey to avoid co…
66e1984 
…nscrypt version conflicts

OpenSSLRsaPublicKey has a different package when using the open-source conscrypt vs. the one inserted
by Play Services.

Signed-off-by: Álvaro Brey <[email protected]>
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

blue-Light-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10713-Screenshot-blue-Light-08-36

@tobiasKaminsky
Revert to master library branch
d605ffe 
Signed-off-by: tobiasKaminsky <[email protected]>
@tobiasKaminsky
Fix test
5b52b44 
Signed-off-by: tobiasKaminsky <[email protected]>
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

Codacy

Lint

TypemasterPR
Warnings8282
Errors00

SpotBugs

CategoryBaseNew
Bad practice2727
Correctness4444
Dodgy code335335
Internationalization99
Multithreaded correctness99
Performance5858
Security1818
Total500500

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10713.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot
Copy link
Collaborator

nextcloud-android-bot commented Jan 5, 2023

master-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/7269-IT-master-16-40

@tobiasKaminsky tobiasKaminsky merged commit 41ec8b1 into master Jan 6, 2023
@delete-merged-branch delete-merged-branch bot deleted the checkCSR branch January 6, 2023 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlvaroBrey AlvaroBrey AlvaroBrey approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tobiasKaminsky @AlvaroBrey @nextcloud-android-bot