Fix/crypto module routing #193
Conversation
seba-aln
force-pushed
the
fix/crypto_module_routing
branch
from
5e7fb41 to
ad3fd90
Compare
pubnub/crypto.py
Outdated
| @@ -69,7 +68,7 @@ def extract_random_iv(self, message, use_random_iv): | |||
|
|
|||
| def get_initialization_vector(self, use_random_iv): | |||
| if self.pubnub_configuration.use_random_initialization_vector or use_random_iv: | |||
| return "{0:016}".format(random.randint(0, 9999999999999999)) | |||
| return secrets.token_hex(16)[0:16] | |||
There was a problem hiding this comment.
there's something suspicious about this line... you're generating 16 random bytes, represented as a text string, then throw away half of it (0:16 is half of the 32 character long hex string), and return the text string that can be only made up of 0-9a-f, which has even fewer possible combinations...
why not use just secrets.token_bytes(16) if you need 16 bytes and leave it at that?
There was a problem hiding this comment.
Yes you are right. I replaced it with a different method that gives me full 16 character long high entropy string
| if self.pubnub.config.crypto_module: | ||
| stringified_message = self.pubnub.config.crypto_module.encrypt(stringified_message) | ||
| elif self.pubnub.config.cipher_key is not None: # The legacy way | ||
| stringified_message = '"' + self.pubnub.config.crypto.encrypt( |
There was a problem hiding this comment.
seems risky to keep doing '"' + x + '"' everywhere, are you sure x can't contain a " character? also here above in line 62 you don't add the " - is it required? I'm just pointing it out because it's different than in other places
There was a problem hiding this comment.
good catch. Fixed missing quote concat. the method to "stringify" message escapes double quote so it shouldn't exist by itself in the wild
seba-aln
force-pushed
the
fix/crypto_module_routing
branch
from
7b52a90 to
e98bb65
Compare
| @@ -68,7 +68,7 @@ def extract_random_iv(self, message, use_random_iv): | |||
|
|
|||
| def get_initialization_vector(self, use_random_iv): | |||
| if self.pubnub_configuration.use_random_initialization_vector or use_random_iv: | |||
| return secrets.token_hex(16)[0:16] | |||
| return secrets.token_urlsafe(16)[:16] | |||
There was a problem hiding this comment.
why clip to :16 characters again? I think I don't know enough about the underlying implementation to understand why we need strings here instead of bytes, but I'm fairly sure that reducing the output of 16 random BYTES from token_urlsafe to 16 url safe characters makes this again much less random. If we don't need 16 random bytes that's fine, but it the requirement is to have 16 (secure)random bytes, then I don't think this satisfies it
There was a problem hiding this comment.
It has 16 bytes but character wise it's length is 22. And in this old implementation we deal with strings. So while it's the old, deprecated implementation I don't want to update it more than necessary
fix: Fix for routing crypto module if custom one was defined