Encrypt credentials in memory #1349

avazirna · 2023-10-10T11:22:26Z

Product Description

Changes for dimagi/commcare-android#2723

shubham1g5

I have not had a look at code yet but wanted to make sure this and connect work is on the same page before getting into review. Have you been able to look at connect code for keystore encryption yet ? Connect branch already have a lot of the keystore encryption code implemented in here . We should try to utilise that code and restructure if needed to avoid having 2 different pathways for triggering Keystore classes. We basically don't want 2 separate sets of methods to utilise the same functionality from Keystore.

shubham1g5 · 2023-10-14T16:51:57Z

src/main/java/org/commcare/util/EncryptionUtils.java

+
+    public static final String USER_CREDENTIALS_KEY_ALIAS = "user-credentials-key-alias";
+
+    public static final String ANDROID_KEYSTORE_PROVIDER_NAME = "AndroidKeyStore";


Core classes should not contain any platform specific implementation details. You can probably use the platform classes to abstract the keystore name for respective platforms in their respective platform implementations.

Done 5b66cc4. I opted for a static non-final field to store the name of the platform keyStore.

shubham1g5 · 2023-10-14T16:52:34Z

src/main/java/org/commcare/util/EncryptionUtils.java

            throw new EncryptionException("Error encountered while decrypting the message", e);
        }
    }

+    public static boolean isAndroidKeyStoreSupported() {


Should be in commcare-android

What about giving it a generic name (like isKeyStoreSupported or isPlatformKeyStoreAvailable) and making it part of the CommCare platform classes?

99e70fd. The method was renamed to isPlatformKeyStoreAvailable. I decided to leave the method in commcare-core as this is supposed to be platform agnostic. In case the keystore name is blank, the user credentials will be store in memory in plaintext.

avazirna · 2023-10-15T20:37:43Z

I have not had a look at code yet but wanted to make sure this and connect work is on the same page before getting into review. Have you been able to look at connect code for keystore encryption yet ? Connect branch already have a lot of the keystore encryption code implemented in here . We should try to utilise that code and restructure if needed to avoid having 2 different pathways for triggering Keystore classes. We basically don't want 2 separate sets of methods to utilise the same functionality from Keystore.

Absolutely, from our brief discussions during stand-ups, it seemed that we took similar approaches but I'm not 100% about the actual implementation, I will look into that.

avazirna · 2023-10-16T22:31:25Z

in here

about this @shubham1g5, I took a look at the Connect code and found many similarities, however, the biggest difference is that in Connect, the cryptographic methods are in commcare-android while in this PR it's in commcare-core. I actually just reworked around the existing methods, which makes me think that commcare-core is probably the best place, as everyone can access. I will try to block some time with the Connect team to understand better their design?

shubham1g5 · 2023-10-18T15:07:25Z

which makes me think that commcare-core is probably the best place

I don't mind this code to be in commcare-core though we should try to replicate the same code structure as the connect code around Encryption classes like EncryptionKeyAndTransform and EncryptionKeyProvider either in core or android so that it's easy to merge this code in together on connect side. You should be able to pull those classes directly from connect branch to yours.

shubham1g5 · 2023-10-17T21:00:10Z

src/main/java/org/commcare/util/CommCarePlatform.java

@@ -232,4 +236,8 @@ public PropertyManager getPropertyManager() {
    public StorageManager getStorageManager() {
        return storageManager;
    }
+
+    public static String getPlatformKeyStoreName(){
+        return platformKeyStoreName;


there is no reason to make a variable for something constant. You can just return null from here directly instead.

So, this is kind of a hack, platformKeyStoreName is static but not final, so not a constant. Reason why is that we want child cases to initialize this with the KeyStore name they support. I don't think we can override static methods. What do you think?

In this commit is where I set the Android Key Store name.

ahh it should not really be static, thinking again probably the easiest way to handle this is pass this as an argument directly to the encryptionutils methods

@shubham1g5 wouldn't that mean having platform specific terminology on core? In this case, the EncryptionUtils method are called from the User class, adding as a parameter means that we still need to pull that from some place, right?

Hmm I do see your issue. I think it's fine for now. I will see if I can find a better solution here.

How about we make new methods in user class as getUsername(keystoreName) and setUsername(keystoreName) and use those instead if it's not too complicates to implement and change.

The problem with that is that some of the calls are made from within core, so we would still have the problem of having to pass on the name from there. But also, I find it more elegant to reference the KeyStore of the platform, instead of having to include in each call, it feels redundant.

shubham1g5 · 2023-10-18T11:48:39Z

src/main/java/org/commcare/util/EncryptionUtils.java

+                platformKeyStore.load(null);
+            } catch (KeyStoreException | IOException | NoSuchAlgorithmException |
+                     CertificateException e) {
+                throw new RuntimeException(e);


should we throw a specific exception that callers can handle for and take appropriate fallbacks.

shubham1g5 · 2023-10-18T11:51:34Z

src/main/java/org/commcare/util/EncryptionUtils.java

+
+    private enum CryptographicOperation {Encryption, Decryption}
+
+    public static KeyStore getPlatformKeyStore() {


can be private ?

shubham1g5 · 2023-10-18T12:45:23Z

src/main/java/org/commcare/util/EncryptionUtils.java

+        try {
+            key = retrieveKeyFromKeyStore(alias, CryptographicOperation.Encryption);
+        } catch (KeyStoreException | UnrecoverableEntryException | NoSuchAlgorithmException e) {
+            throw new RuntimeException(e);


should bubble up the specific exceptions to caller.

shubham1g5 · 2023-10-18T13:05:45Z

src/main/java/org/commcare/util/EncryptionUtils.java

+            // This is not very relevant at the moment as the RSA algorithm is only used to encrypt
+            // user credentials on devices runnning Android 5.0 - 5.1.1 for the KeyStore


unnecessary comment, It's still relevant even if it's getting used on a small set of devices.

Not necessarily, we only use RSA when working with the keyStore on devices running versions of the android sdk prior to 23, this method is used to convert a Base64 encoded key into a SecretKey, PrivateKey or PublicKey, so not called needed when using the KeyStore. For all other purposes we use 'AES'. Maybe I should remove this.

True, you can just say - RSA is only used for Android 5.0 - 5.1.1

shubham1g5 · 2023-10-18T13:08:14Z

src/main/java/org/commcare/util/EncryptionUtils.java

+            try {
+                keyFactory = KeyFactory.getInstance(RSA_ALGORITHM_KEY);
+            } catch (NoSuchAlgorithmException e) {
+                throw new EncryptionException("There is no Provider for the selected algorithm", e);


use RSA algorithm instead of selected algorithm since we already know the selected algo

shubham1g5 · 2023-10-18T13:14:17Z

src/main/java/org/commcare/util/EncryptionUtils.java

+        return platformKeyStore;
+    }
+
+    public static String encryptUsingKeyFromKeyStore(String message, String alias)


encryptUsingKeyFromKeyStore -> encryptWithKeyStore
alias -> keyAlias

same comments for decrypt methods as well

shubham1g5 · 2023-10-18T14:31:03Z

src/main/java/org/javarosa/core/model/User.java

+        if (!isPlatformKeyStoreAvailable()) {
+            return this.username;
+        } else {
+            try {
+                return EncryptionUtils.decryptUsingKeyFromKeyStore(this.username, USER_CREDENTIALS_KEY_ALIAS);
+            } catch (EncryptionUtils.EncryptionException e) {
+                throw new RuntimeException("Error encountered while decrypting the Username ", e);
+            }
+        }


this logic should be abstracted probably into EncryptionUtil as well as another method as we don't wanna repeat this for every property we encrypt or decrypt.

@shubham1g5 in this comment it seems that we prefer the key retrieval exceptions to be handled by the caller method, is there a reason to treat that differently than the cryptographic operation itself? My thinking was the opposite, handle Key retrieval exception locally (as this is not known to the caller) and let the caller deal with any issues during encryption/decryption.

My thinking was the opposite, handle Key retrieval exception locally (as this is not known to the caller)

Caller should expect/know that we are using Platform keystore to do encryption here. We should make it explicit with a Java doc on these methods.

shubham1g5 · 2023-10-18T14:44:37Z

src/main/java/org/commcare/util/EncryptionUtils.java

 public class EncryptionUtils {

+    public static final String USER_CREDENTIALS_KEY_ALIAS = "user-credentials-key-alias";
+
+    public static final String RSA_ALGORITHM_KEY = "RSA";


we already have tests for AES as part of xpath encrypt and decrypt methods, but we will need to add tests for RSA encyrption as well as it's a sensitive code base area.

shubham1g5 · 2023-10-18T14:54:09Z

src/main/java/org/commcare/util/EncryptionUtils.java

+
+    public static String decryptUsingBase64EncodedKey(String algorithm, String message, String key)
+            throws EncryptionException {
+        Key secret = null;


nit: null assignment should not be needed.

avazirna · 2024-01-12T21:36:43Z

src/main/java/org/commcare/util/EncryptionKeyServiceProvider.java

+        return serviceProvider;
+    }
+
+    public IEncryptionKeyProvider serviceImpl(boolean useKeyStoreIfAvailable) {


@shubham1g5 I added a flag to determine the preference in terms of using or not the platform KeyStore, however, this doesn't prevent a non preferred implementation from being returned when there is no KeyStore available.

ctsims

Hi Ahmad - There are some good changes here, and I see what this code is getting at, but I don't think the core shift here is the right implementation.

It adds a lot of complexity to the code by coupling the needs of one core concept (the need for a helper class that can wrap a payload just during runtime in either a keystore or an encrypted slug) into the breadth of the encryption helper / utility code, which complicates both.

I think the, like, Runtime Encrypted Secret helper needs to be its own object with its own lifecycle that doesn't bleed over into the other pieces of the encryption code. It's dependent on the cryptography environment, but I don't think the rest of the cryptography environment should be dependent on it. Some of the other key concepts here (like compartmentalizing some of the encryption operations, and unifying error handling) are probably solid, but right now they're all too tied in with that code to be able to tell.

I'd focus on pulling out the piece that needs to depend on state from the rest of the crypto code here, which as far as I can tell can mostly remain stateless.

ctsims · 2024-01-16T18:00:14Z

src/main/java/org/commcare/core/encryption/CryptUtil.java

+            generator.initialize(keyLength, new SecureRandom());
+            return generator.genKeyPair();
+        } catch (NoSuchAlgorithmException e) {
+            e.printStackTrace();


This method already calls the EncryptionException, so there's no real reason we shouldn't just wrap and rethrow there instead of returning null, right? Otherwise callers will end up needing two return paths for effectively the same outcome

ctsims · 2024-01-16T18:01:06Z

src/main/java/org/commcare/core/encryption/CryptUtil.java

        KeyGenerator generator;
        try {
            generator = KeyGenerator.getInstance("AES");
-            generator.init(256, new SecureRandom());
+            generator.init(keylength, new SecureRandom());
            return generator.generateKey();
        } catch (NoSuchAlgorithmException e) {
            e.printStackTrace();


Same as below, if we're ever going to throw exceptions in these methods, we should start centralizing the return paths to the new EncryptionException pattern and wrap/rethrowing here rather than continuing to return null.

ctsims · 2024-01-16T18:09:31Z

src/main/java/org/commcare/util/BasicEncryptionKeyProvider.java

+        }
+
+        if (key == null) {
+            throw new EncryptionException("Encryption key conversion failed");


I don't think this is a good catch-all. If the fallback is going to be throwing an exception anyway, I think it makes more sense to do it above, when the right context was available. This return path effectively is hiding the real error that caused the key to be null without providing us with the context why in the stack trace.

ctsims · 2024-01-16T18:15:31Z

src/main/java/org/commcare/util/BasicEncryptionKeyProvider.java

+    }
+
+    @Override
+    public void generateCryptographicKeyInKeyStore(String keyAlias) {


Something strikes me as off about the way this is set up today.

It's a bit confusing to both use hard declarations (IE: isKeyStoreAvailable), which intend to push the burden of the pattern of usage downstream to the caller, and soft internal implementations which have users call methods that might perform an action whether or not it does, letting the implementation determine whether a keyStore is available and doing the right thing if so.

If we are following the hard pattern, I'd have this method return an error, since it's being called in a context where it's not appropriate. If we are following the soft pattern, I'd probably title this method something less implementation focused (like "initializeKeyForUse") that isn't as proscriptive about what the method is actually going to do, since as outlined currently you seem to be implying that a caller may call this method with the expectation that it won't actually generate a cryptographic key in a keystore, since that action won't happen but an error isn't produced

ctsims · 2024-01-16T18:16:55Z

src/main/java/org/commcare/util/BasicEncryptionKeyProvider.java

+            transformation = "RSA/ECB/PKCS1Padding";
+        }
+        // This will cause an error
+        return transformation;


As above, I'd have this throw an exception rather than passively return null.

ctsims · 2024-01-16T18:50:04Z

src/main/java/org/commcare/util/EncryptionHelper.java

+                encryptionKeyProvider.getKey(algorithm, keyOrAlias, CryptographicOperation.Encryption);
+
+        try {
+            Cipher cipher = Cipher.getInstance(keyAndTransformation.getTransformation());


It's dangerous for this particular code to be buried in this method's encrypt / decrypt implementation.

EncryptionHelper is a lifecycle object. It's goal is to dispatch user intent to different implementations in different contexts based on internal state from the key provider.

This actual try/catch by comparison is stateless cryptography, and should be able to, say, be unit tested independently without depending on the state and configuration of the EncryptionHelper object. Seeing this code, for instance, I'd want to see a unit test that does a round trip encrypt/decrypt cycle for the representative pairings of KeyAndTransforms that are commonly used, and that unit test would be very weird/awkward given that this class depends on static ServiceImplementations and other context state.

ctsims · 2024-01-16T18:52:28Z

src/main/java/org/commcare/util/EncryptionHelper.java

+     * containing the ciphertext, and when applicable, a random IV which was used to encrypt
+     * the message.
+     *
+     * @param algorithm  the algorithm to be used to encrypt/decrypt. This is only relevant when


I don't think it makes sense for this parameter to be provided just-in-time with the call. Is there a reason the algorithm can't be established when the class is instantiated?

The KeyStore forced the need for the encrypt/decrypt methods to support asymmetric algorithms and as a way to standardize the behavior, this application was also extended to base64 encoded keys. This parameter is to determine the key algorithm for such keys (those in the KeyStore are stored with all the info) however, we don't actually use this, we could just thrown an exception instead.

ctsims · 2024-01-16T19:00:03Z

src/main/java/org/javarosa/xpath/expr/XPathDecryptStringFunc.java

            throw new XPathException("Unknown algorithm \"" + algorithm +
                    "\" for " + NAME);
        }

        try {
-            return decrypt(message, key);
-        } catch (EncryptionUtils.EncryptionException e) {
+            return encryptionHelper.decrypt(algorithm, message, key);


ah, I see, I don't think this is actually the right way for this to work. EncryptionHelper is playing way too many roles in this context. I'm very worried that things like the existence of a KeyStore are coming into play under the hood of calls like this, which are dangerous to be mixing together.

I don't see a specific reason why it makes sense for this class's implementation to switch from static to object oriented, and there are meaningful runtime performance implementation differences from coupling new lifecycle objects into this code.

ctsims · 2024-01-16T19:02:57Z

src/main/java/org/commcare/util/EncryptionException.java

@@ -0,0 +1,15 @@
+package org.commcare.util;
+
+public class EncryptionException extends Exception {


This exception needs javadocs explaining when it will be thrown and how it should be handled. The core purpose of this method seems to be to wrap other Encryption errors, but it's playing dual duty with two different meanings that either should be split into two different exceptions, or somehow differentiated in documentation for methods on how to handle.

One meaning is "The local platform doesn't support a type of encryption" and the other meaning is "the caller provided a bad input to a method (IE: A secret key of the wrong length)" and the difference between those two is potentially quite large, since the caller might be expected to handle one but fail with the other.

ctsims · 2024-01-16T19:20:48Z

src/main/java/org/commcare/util/EncryptionKeyServiceProvider.java

+    public IEncryptionKeyProvider serviceImpl(boolean useKeyStoreIfAvailable) {
+        IEncryptionKeyProvider service = null;
+        if (loader.iterator().hasNext()) {
+            service = loader.iterator().next();


This is a super interesting pattern, but I don't think it's the right one here.

What this code is really doing is determining if the system has a keystore provider available and using it if so, but there's no meaningful way to choose between multiple keystore-compatible providers, so it's just complex way of having two different things registered, and we should probably just have both in static memory from an initial registration if that's the case.

This pattern is really for things like dependency injection from static libraries, and for doing fallback driven loading where, say, the code is looking for a keyprovider compatible with a specific algorithm/key type and it's going to choose the next one if it's not available.

avazirna · 2024-01-16T22:15:27Z

Hi Ahmad - There are some good changes here, and I see what this code is getting at, but I don't think the core shift here is the right implementation.

It adds a lot of complexity to the code by coupling the needs of one core concept (the need for a helper class that can wrap a payload just during runtime in either a keystore or an encrypted slug) into the breadth of the encryption helper / utility code, which complicates both.

I think the, like, Runtime Encrypted Secret helper needs to be its own object with its own lifecycle that doesn't bleed over into the other pieces of the encryption code. It's dependent on the cryptography environment, but I don't think the rest of the cryptography environment should be dependent on it. Some of the other key concepts here (like compartmentalizing some of the encryption operations, and unifying error handling) are probably solid, but right now they're all too tied in with that code to be able to tell.

I'd focus on pulling out the piece that needs to depend on state from the rest of the crypto code here, which as far as I can tell can mostly remain stateless.

I understand. Indeed, the initial approach was to keep things as simple as possible and without much consolidation of the encryption code, but as the conversation progressed came the perception that maybe we have too much crypto code spread around and this could be a good moment to introduce some convergence, also reinforced when some of the CCC components were brought in for alignment. One of such components was the EncryptionKey provider which led to the decision of whether it should serve only KeyStore needs or all the encryption key needs. The current implementation follows the latter. But going bak to the suggestion, I agree that the current implementation couples too closely encryption key aspects and encryption operations, I'm inclined to let the caller handle any key retrieval/conversion before calling any encryption operation.

This reverts commit e151f5f.

This was added to standardize the behavior of both encryption key sources (Key store and base64 encoded key) however, there no immediate application for it and the direction now is to isolate the RSA logic and only use it on devices running versions prior to Android 6

This was a non-keystore provider

This helper will be responsible for retrieving an encryption key from either the KeyStore or by converting a base64 encoded String

…on key handling

avazirna · 2024-01-23T23:11:09Z

@ctsims I took a few steps back to be able to address the concerns you raised, main changes are:

RSA is now only used when handling KeyStore keys in devices running Android versions prior to 6;
SPI is now being used to inject an implementation of an encryption key generator for the Key store, IKeyStoreEncryptionKeyProvider. The scope was reduced to isolate the key generation logic for the KeyStore. Another approach I entertained was to have implementation for each key algorithm, RSA (Android 5 - 5.1.1) and another for AES (Android 6+). Another benefit with SPI is during unit testing, we can easily inject an alternative implementation;
A new class EncryptionKeyHelper was introduced to be responsible for retrieving the encryption key, either when the key needs to be converted (when supplied by the caller) or fetched from the KeyStore. I tried to maintain a single entry point to retrieve the encryption key, but I couldn’t get to a satisfactory design.
EncryptionKeyException was introduced to umbrella key handling/generation exceptions. Some of the methods in the CryptUtil class are now throwing this exception (instead of returning null), however, this required changes to many other classes to ensure that the error message gets delivered appropriately. There are still a couple of methods that require the same change, but I wanted to hear you thoughts on the current approach;

avazirna added 6 commits October 10, 2023 01:57

Initialize Android KeyStore

49c6586

Retrieve key from Android KeyStore

09d9900

Refactor getSecretKey

c95b7c8

Add method to check whether Android KeyStore is supported

676442b

Update encrypt and decrypt methods

93f33c8

Encrypt user credentials in memory

cd6c546

avazirna mentioned this pull request Oct 10, 2023

Encrypt credentials in memory dimagi/commcare-android#2723

Open

avazirna added the product/invisible label Oct 10, 2023

avazirna added this to the 2.54 milestone Oct 10, 2023

Merge branch 'master' into encrypt-credentials-in-memory

f20986c

avazirna requested a review from shubham1g5 October 10, 2023 20:10

avazirna added 8 commits October 11, 2023 23:42

Return plain username

7968312

Lint

c5d3f94

Pass algorithm to cryptographic methods

7b0a54d

Refactor

a22ff5a

Add support to convert RSA keys

3647d76

Set cryptographic transformation according to algorithm

c485e5a

Allow null IV for RSA ciphers

b075ac9

Lint

eff308b

avazirna marked this pull request as ready for review October 13, 2023 21:00

shubham1g5 requested changes Oct 14, 2023

View reviewed changes

avazirna added 5 commits October 16, 2023 17:15

Refactor

99e70fd

Refactor

efdc30a

Lint

448f074

Update comments

0c39c5a

Set platform KeyStore name

5b66cc4

shubham1g5 requested changes Oct 18, 2023

View reviewed changes

avazirna requested a review from shubham1g5 January 12, 2024 21:26

avazirna commented Jan 12, 2024

View reviewed changes

ctsims requested changes Jan 16, 2024

View reviewed changes

avazirna added 15 commits January 19, 2024 11:35

Revert "Add unit tests for RSA"

19fc7d8

This reverts commit e151f5f.

Rmove BasicEncryptionKeyProvider

5b8f281

This was a non-keystore provider

Add EncryptionKeyHelper class

2f269e8

This helper will be responsible for retrieving an encryption key from either the KeyStore or by converting a base64 encoded String

Refactor IEncryptionKeyProvider to support KeyStore Key generation only

df1575a

Update javadoc

f7ab55b

Refactor

10352e1

Add key generation in KeyStore when alias is not found

c3b79b9

Make EncryptionHelper stateless

94b0062

Refactor

f176952

Add EncryptionKeyException to umbrella exceptions related to encrypti…

7a82c35

…on key handling

Check IV lenght to apply GCMParameterSpec

96f0b04

Check if there are valid EncryptionKeyProvider implementations

ffc0c96

Throw EncryptionKeyException when key generation fails

58ffb32

Rename IEncrypitonKeyProvider to IKeyStoreEncryptionKeyProvider

b2a8da9

avazirna force-pushed the encrypt-credentials-in-memory branch from d26e90f to b2a8da9 Compare January 23, 2024 12:24

avazirna added 4 commits January 23, 2024 15:39

Refactor

8c7363a

Update javadocs

188e22b

Refactor

a2928bd

Bubble up encryption key exceptions

bd512d6

avazirna requested a review from ctsims January 23, 2024 23:11

Throw exception when there is no keyStore facility

4716bda

avazirna force-pushed the encrypt-credentials-in-memory branch from 836801f to 4716bda Compare January 24, 2024 22:08

avazirna modified the milestones: 2.54, 2.55 Aug 23, 2024


		public static final String USER_CREDENTIALS_KEY_ALIAS = "user-credentials-key-alias";

		public static final String ANDROID_KEYSTORE_PROVIDER_NAME = "AndroidKeyStore";


		private enum CryptographicOperation {Encryption, Decryption}

		public static KeyStore getPlatformKeyStore() {

		// This is not very relevant at the moment as the RSA algorithm is only used to encrypt
		// user credentials on devices runnning Android 5.0 - 5.1.1 for the KeyStore

		@@ -0,0 +1,15 @@
		package org.commcare.util;

		public class EncryptionException extends Exception {

Encrypt credentials in memory #1349

Are you sure you want to change the base?

Encrypt credentials in memory #1349

Conversation

avazirna commented Oct 10, 2023 • edited Loading

Product Description

shubham1g5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

avazirna commented Oct 15, 2023

avazirna commented Oct 16, 2023 • edited Loading

shubham1g5 commented Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

avazirna Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

ctsims left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna commented Jan 16, 2024

avazirna commented Jan 23, 2024 • edited Loading

avazirna commented Oct 10, 2023 •

edited

Loading

avazirna Oct 16, 2023 •

edited

Loading

avazirna commented Oct 16, 2023 •

edited

Loading

shubham1g5 commented Oct 18, 2023 •

edited

Loading

avazirna Oct 18, 2023 •

edited

Loading

avazirna Jan 12, 2024 •

edited

Loading

avazirna commented Jan 23, 2024 •

edited

Loading