Jimin/dev5 key #1123

jiminwen-msft · 2022-05-26T18:03:17Z

task:

Related PR: #1097

This PR tries to

Introduce a reliable approach of refreshing push notification registration using worker manager. There are changes to both SDK and sample app. Contoso APP is required to make configuration change in order to run WorkManager in background. Otherwise, client faces RuntimeException.
Introduce key-store to persist secrete keys. Secrete keys are used for push notification registration and decrypt push notification payloads. Using key-store ensure the keys are persisted even APP gets closed. And instead of storing two pairs of secrete keys. We change to store 10 pairs of keys. And try all of them when decrypt push notification payload.
We set registration renewal work to repeat every 15 minutes. And set TTL of push notification registration as 45 minutes. So that we have 3 chances of execute worker before it expires.

More specific changes:

SDK side:

RegistrationRenewalWorker: The worker that renews registration.
RegistrationRenewalWorkerFactory : Factory for the worker. It is created because we could not rely on the default factory. We need customized factory which injects the CommunicationCredential. And this CommunicationCredential needs to be injected from Tontoso APP during the initiation phase of the APP.
RegistrationDataContainer : Data container for updating and storing secrete keys shared by PushNotificationClient and RenewalTokenWorker. It wraps key-store inside. So that it could persist all pairs of secrete keys in file system.
Gradle: inject worker related dependency

For Contoso. Configuration changes are required to use the WorkerManager.

Create customized configuration to use RenewalTokenWorkerFactory
manifest change to refer to the configuration
Gradle: inject worker related dependency

…into jimin/dev5_key

...zure/android/communication/chat/implementation/notifications/fcm/PushNotificationClient.java

...e/android/communication/chat/implementation/notifications/fcm/RegistrationRenewalWorker.java

...zure/android/communication/chat/implementation/notifications/fcm/PushNotificationClient.java

.../azure/android/communication/chat/implementation/notifications/fcm/RegistrationKeyStore.java

…into jimin/dev5_key

vcolin7

Added some comments in general and a couple about the use of KeyStore.

samples/sample-chat-app/build.gradle

.../azure/android/communication/chat/implementation/notifications/fcm/KeyCreationTimeStore.java

...zure/android/communication/chat/implementation/notifications/fcm/RegistrationKeyManager.java

vcolin7 · 2022-07-22T10:32:40Z

...zure/android/communication/chat/implementation/notifications/fcm/RegistrationKeyManager.java


    private boolean lastExecutionSucceeded = true;

    //The duration we persist keys in key-store
-    public static final int EXPIRATION_TIME_MINUTES = 45;
+    public static final int EXPIRATION_TIME_MINUTES = 25;


I remember the previous expiry time was 45, was this changed on the server side as well or is there a particular reason it got client-side?

Not that I'm opposed to rotating keys earlier, I was simply curious.

vcolin7 · 2022-07-22T10:34:25Z

...zure/android/communication/chat/implementation/notifications/fcm/RegistrationKeyManager.java

@@ -56,6 +82,10 @@ public static RegistrationKeyManager instance() {
        return registrationKeyManager;
    }

+    private char[] getKeyStorePassword() {
+        return "com.azure.android.communication.chat.android".toCharArray();


Why are we using a hard-coded value as a password? Is this standard practice for KeyStore? I would be very surprised if that's the case.

The password field is actually optional. I am adding it here to slightly increase the difficulty of hack the system. Having a password that keeps changing is challenging to implement. Which way do you suggest? 1. Adding some transform function to make this password complex 2. Skipping the password 3. Alternative ways that I am not aware of.

Based on offline discussion. The password field will be empty/null. I am investigating changing keystore type from default "AES" to "AndroidKeyStore"

vcolin7 · 2022-07-22T10:38:24Z

...zure/android/communication/chat/implementation/notifications/fcm/RegistrationKeyManager.java

+                Log.i("RenewRegistration", "crypto alias: " + crypAlias + ", crypto string: " + crypStr + ", time: " + timeDiff);
+                Log.i("RenewRegistration", "auth alias: " + auAlias + ", auth string: " + aKeyStr + ", time: " + timeDiff);


Remember to log using ClientLogger. Maybe this was left off from manual testing?

...zure/android/communication/chat/implementation/notifications/fcm/RegistrationKeyManager.java

vcolin7

Added a suggestion for the new exception added for when the push notification configuration is incorrect.

vcolin7 · 2022-07-25T22:23:36Z

...e/android/communication/chat/implementation/notifications/fcm/RegistrationRenewalWorker.java

+
+    public RegistrationRenewalWorker(@NonNull Context context, @NonNull WorkerParameters workerParameters) {
+        super(context, workerParameters);
+        throw new RuntimeException("Missing worker manager configurations. Please follow quick-start guidance to properly set it up: https://docs.microsoft.com/en-us/azure/communication-services/quickstarts/chat/get-started?pivots=programming-language-android");


Suggested change

throw new RuntimeException("Missing worker manager configurations. Please follow quick-start guidance to properly set it up: https://docs.microsoft.com/en-us/azure/communication-services/quickstarts/chat/get-started?pivots=programming-language-android");

throw clientLogger.logExceptionAsError(new RuntimeException("Missing worker manager configuration for enabling push notifications. Please follow quick-start guidance to properly set it up: https://docs.microsoft.com/en-us/azure/communication-services/quickstarts/chat/get-started?pivots=programming-language-android"));

…into jimin/dev5_key

config/spotbugs/spotbugs-exclude.xml

.../com/azure/android/communication/chat/implementation/notifications/fcm/KeyMetaDataStore.java

...e/android/communication/chat/implementation/notifications/fcm/RegistrationRenewalWorker.java

...zure/android/communication/chat/implementation/notifications/fcm/RegistrationKeyManager.java

…spectively.

vcolin7 · 2022-08-11T20:38:52Z

/check-enforer evaluate

vcolin7 · 2022-08-11T20:41:51Z

/check-enforcer override.

vcolin7 · 2022-08-11T20:42:00Z

/check-enforcer override

jiminwen-msft added 9 commits May 9, 2022 00:38

Code back up

5474689

Snap for whats working

06367af

Supporting key-store

8f1daae

Adding comments

20c2f92

Updating TTL and remove old imples

e948981

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-android …

ee90dc7

…into jimin/dev5_key

Adding comments

30609d1

Removing static values

8dc5d9d

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-android …

277a601

…into jimin/dev5_key

jiminwen-msft requested review from JianpingChen and a team as code owners May 26, 2022 18:03

ghost added the Communication label May 26, 2022

jiminwen-msft requested review from pohtsng, vcolin7 and angellan-msft May 26, 2022 23:06

jiminwen-msft added 2 commits May 26, 2022 18:33

Reformat

5fc6287

Reformat2

a6f8739

angellan-msft approved these changes May 27, 2022

View reviewed changes

pohtsng reviewed May 27, 2022

View reviewed changes

...zure/android/communication/chat/implementation/notifications/fcm/PushNotificationClient.java Outdated Show resolved Hide resolved

...zure/android/communication/chat/implementation/notifications/fcm/PushNotificationClient.java Outdated Show resolved Hide resolved

pohtsng reviewed May 27, 2022

View reviewed changes

...zure/android/communication/chat/implementation/notifications/fcm/PushNotificationClient.java Outdated Show resolved Hide resolved

pohtsng reviewed May 27, 2022

View reviewed changes

...e/android/communication/chat/implementation/notifications/fcm/RegistrationRenewalWorker.java Outdated Show resolved Hide resolved

pohtsng reviewed May 27, 2022

View reviewed changes

...zure/android/communication/chat/implementation/notifications/fcm/PushNotificationClient.java Show resolved Hide resolved

jiminwen-msft added 7 commits May 27, 2022 19:32

Resolving comments

07fc9dd

Move exception handler to worker

48b00c8

Fixing check style

b99408f

Singleton thread safe

338ba46

Customized key store

18cce06

Removing logs

52dc43c

Change TTL

b64869f

pohtsng reviewed Jun 7, 2022

View reviewed changes

jiminwen-msft added 3 commits July 18, 2022 01:32

Remove import

41f1521

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-android …

e684094

…into jimin/dev5_key

Fixing duration

cb87c13

vcolin7 reviewed Jul 22, 2022

View reviewed changes

jiminwen-msft added 3 commits July 22, 2022 13:16

Adding error message

c8737dd

Remove bos.close()

17289a0

Remove checking and typo

a058b0a

vcolin7 reviewed Jul 26, 2022

View reviewed changes

jiminwen-msft added 8 commits July 27, 2022 22:23

Android key-store

59a253e

demos

a4eee77

Android key-store

794ccd5

Updating comments

2bc7671

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-android …

57b38f5

…into jimin/dev5_key

Removing imports

6ba9432

Formatting

bab2a3d

Formatting format

2ee7233

vcolin7 reviewed Aug 10, 2022

View reviewed changes

vcolin7 reviewed Aug 11, 2022

View reviewed changes

jiminwen-msft and others added 6 commits August 11, 2022 01:32

handle exceptions

6e4ab61

Changed casing for a few crypto variables.

310e0c2

Changed instances of encryptText to ciphertext in KeyMetadataStore.

a293e6f

Fixed casing for the variable "ciphertext".

c1db020

Renamed encryptText() and decryptData() to encrypt() and decrypt() re…

c1b02e1

…spectively.

Fixed compilation error.

23fb30f

vcolin7 approved these changes Aug 11, 2022

View reviewed changes

jiminwen-msft merged commit bccb27a into main Aug 11, 2022

jiminwen-msft deleted the jimin/dev5_key branch August 11, 2022 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jimin/dev5 key #1123

Jimin/dev5 key #1123

jiminwen-msft commented May 26, 2022 •

edited

Loading

vcolin7 left a comment

vcolin7 Jul 22, 2022

vcolin7 Jul 22, 2022

vcolin7 Jul 22, 2022

jiminwen-msft Jul 22, 2022

jiminwen-msft Jul 27, 2022

vcolin7 Jul 22, 2022

vcolin7 left a comment •

edited

Loading

vcolin7 Jul 25, 2022

vcolin7 commented Aug 11, 2022

vcolin7 commented Aug 11, 2022

vcolin7 commented Aug 11, 2022

		Log.i("RenewRegistration", "crypto alias: " + crypAlias + ", crypto string: " + crypStr + ", time: " + timeDiff);
		Log.i("RenewRegistration", "auth alias: " + auAlias + ", auth string: " + aKeyStr + ", time: " + timeDiff);

	throw new RuntimeException("Missing worker manager configurations. Please follow quick-start guidance to properly set it up: https://docs.microsoft.com/en-us/azure/communication-services/quickstarts/chat/get-started?pivots=programming-language-android");
	throw clientLogger.logExceptionAsError(new RuntimeException("Missing worker manager configuration for enabling push notifications. Please follow quick-start guidance to properly set it up: https://docs.microsoft.com/en-us/azure/communication-services/quickstarts/chat/get-started?pivots=programming-language-android"));

Jimin/dev5 key #1123

Jimin/dev5 key #1123

Conversation

jiminwen-msft commented May 26, 2022 • edited Loading

vcolin7 left a comment

Choose a reason for hiding this comment

vcolin7 Jul 22, 2022

Choose a reason for hiding this comment

vcolin7 Jul 22, 2022

Choose a reason for hiding this comment

vcolin7 Jul 22, 2022

Choose a reason for hiding this comment

jiminwen-msft Jul 22, 2022

Choose a reason for hiding this comment

jiminwen-msft Jul 27, 2022

Choose a reason for hiding this comment

vcolin7 Jul 22, 2022

Choose a reason for hiding this comment

vcolin7 left a comment • edited Loading

Choose a reason for hiding this comment

vcolin7 Jul 25, 2022

Choose a reason for hiding this comment

vcolin7 commented Aug 11, 2022

vcolin7 commented Aug 11, 2022

vcolin7 commented Aug 11, 2022

jiminwen-msft commented May 26, 2022 •

edited

Loading

vcolin7 left a comment •

edited

Loading