RSA wrapper python

Question

#!/usr/bin/env python3
'''
Asymmetric criptography of chat messages.
'''
#TODO: implement perfect foreward secrecy
#Because of forward secrecy an attacker would need to have access to the internal SSH state of either the client or server at the time the SSH connection still exists.
#TODO: protect aainst traffic analysis
# Everything is peer to peer, which is cool, but the IP fetching needs to be anonymysed as well.
import os
from pathlib import Path
import random
import rsa
Priv = rsa.key.PrivateKey
Pub = rsa.key.PublicKey
Keypair = (Priv, Pub)
# Location to first look for a private key.
DEFAULT_PRIV = Path(Path.home() / '.ssh/id_rsa')
# 'MD5', 'SHA-1', 'SHA-224', 'SHA-256', 'SHA-384' or 'SHA-512'
HASH = 'SHA-256'
def generate_keypair(bits: int=1024) -> Keypair:
 pub, priv = rsa.newkeys(bits)
 return priv, pub
def write_keypair(priv: rsa.key.PrivateKey
 , pub: rsa.key.PublicKey=None
 , p: Path=DEFAULT_PRIV):
 '''Obviously this function violates the RAM-only constraint.'''
 if p.exists():
 raise BaseException('Refusing to ovewrite an existing private key: '
 + str(p))
 with open(p, 'wb') as f:
 f.write(priv.save_pkcs1())
 if pub:
 with open(p.with_suffix('.pub'), 'wb') as f:
 f.write(pub.save_pkcs1())
def regenerate_pub(path_priv: Path=DEFAULT_PRIV):
 os.run('ssh-keygen -y -f ' + path_priv
 + ' > ' + path_priv + '.pub')
def read_keypair(p: Path=DEFAULT_PRIV) -> Keypair:
 with open(p, mode='rb') as priv_file:
 key_data = priv_file.read()
 priv = rsa.PrivateKey.load_pkcs1(key_data)
 pub = None
 p = Path(p)
 if not Path(p.with_suffix('.pub')).is_file():
 regenerate_pub()
 with open(p.with_suffix('.pub'), 'rb') as f:
 key_data = f.read()
 pub = rsa.PublicKey.load_pkcs1(key_data)
 assert(pub is not None)
 return priv, pub
def encrypt(text: str, pub: Pub) -> bytes:
 '''Encrypt a message so that only the owner of the private key can read it.'''
 bytes = text.encode('utf8')
 encrypted = rsa.encrypt(bytes, pub)
 return encrypted
def decrypt(encrypted: bytes, priv: Priv) -> str:
 try:
 bytes = rsa.decrypt(encrypted, priv)
 string = bytes.decode('utf8')
 except rsa.pkcs1.DecryptionError:
 # Printing a stack trace leaks information about the key.
 print('ERROR: DecryptionError!')
 string = ''
 return string
def sign(msg: str, priv: Priv) -> bytes:
 '''Prove you wrote the message.
 It is debatable should signing be performed on the plaintext
 or on the encrypted bytes.
 The former has been chosen because it is not vulnerable to the following.
 Tim sends an encrypted and then sign packet to a server containing a password.
 Joe intercepts the packet, strips the signature, signs it with his own key
 and gets access on the server ever though he doesn't know Tim's password.
 Furthermore it increases privacy.
 Only the recepient can validatet the sender instead of anyone intercepting.
 '''
 signature = rsa.sign(msg.encode('utf8'), priv, HASH)
 return signature
def verify(msg: str, signature: bytes, pub: Pub):
 '''VerificationError - when the signature doesn't match the message.'''
 rsa.verify(msg.encode('utf8'), signature, pub)
def test():
 priv, pub = generate_keypair()
 p = Path('/tmp/whatever' + str(random.randint(0, 1e6)))
 write_keypair(priv, pub, p)
 newpriv, newpub = read_keypair(p)
 assert(priv == newpriv)
 assert(pub == newpub)
 msg = "We come in peace!"
 bytes = encrypt(msg, pub)
 newmsg = decrypt(bytes, priv)
 assert(msg == newmsg)
 signature = sign(msg, priv)
 verify(msg, signature, pub)
if __name__ == '__main__':
 test()

This is my rsa wrapper(GitHub). I am posting this in anticipation for learning about modern python best practices. Some of the concerns are:

• commented with # just under the module string
• Path and IP instead of str - which should be used?
• not all functions have docstrings - is that bad?
• types have been annotated but import typing is nowhere in sight
• the test ... I should probably ask on QA.SE

Answer 1

Class imports

Replace

import rsa
Priv = rsa.key.PrivateKey
Pub = rsa.key.PublicKey

with

from rsa.key import PrivateKey as Priv, PublicKey as Pub

Though I'm not convinced that the short forms are entirely necessary, this would be the more standard way to do it.

Keypair = (Priv, Pub)

is styled like a class name but is actually a global tuple, so should be KEY_PAIR. However, if you're using it as a type, as in

def generate_keypair(bits: int=1024) -> Keypair:

then you should instead

KeyPair = Tuple[Priv, Pub]

Function shims

If you don't care too much about the default and you're mainly doing this to annotate types, then

def generate_keypair(bits: int=1024) -> Keypair:
 pub, priv = rsa.newkeys(bits)
 return priv, pub

can become

generate_keypair: Callable[[int], KeyPair] = rsa.newkeys

Goofy commas

def write_keypair(priv: rsa.key.PrivateKey
 , pub: rsa.key.PublicKey=None
 , p: Path=DEFAULT_PRIV):

neither helps legibility nor is it PEP8-compliant; just write it like a human would:

def write_keypair(priv: rsa.key.PrivateKey,
 pub: Optional[rsa.key.PublicKey]=None,
 p: Path=DEFAULT_PRIV):

Path.open instead of open(Path)

with open(p, 'wb') as f:

should be

with p.open('wb') as f:

subprocess instead of os.run

Replace your os.run calls with calls to the appropriate subprocess methods. You probably also should not bake in a Bash-style redirect; instead, open the file handle yourself and use it as the stdout argument in subprocess.

Asserts

load_pkcs1 does not indicate that it might return None. Is that actually possible? If not, your assert is redundant.

Top-level exceptions

decrypt should not bake in an except. Catch that at the outer scope instead.

If you're confident that this is true (which I'm not):

# Printing a stack trace leaks information about the key.

then decrypt can try/except/raise a new exception that includes all information about the failure you consider to be informative but non-compromising.

Don't roll your own temp handling

Particularly since you care about security, don't do this:

'/tmp/whatever' + str(random.randint(0, 1e6)

Instead, read https://docs.python.org/3/library/tempfile.html .

Answer 2

Here are some cryptography related remarks (below the code).

Asymmetric criptography of chat messages.

That's "cryptography". Cipher and cypher are modern and older spellings, but crypto is always with a "y". There are more spelling mistakes such as "foreward". This makes your application look amateurish, even when that's not necessarily the case.

#TODO: implement perfect foreward secrecy

I don't see this being used for transport security (I hope). In that case just forward security doesn't work because it is impossible to trust temporary keys without additional measures.

def generate_keypair(bits: int=1024) -> Keypair:

Your default is considered insecure. Try 4096 or something similar.

with open(p.with_suffix('.pub'), 'wb') as f:

Don't let the user of a library give you one path and then create another file using another path. That's not according to the principle of least surprise.

def regenerate_pub(path_priv: Path=DEFAULT_PRIV):

I don't see why you don't just always save the public key as well. That way you don't need to call the function externally either.

bytes = text.encode('utf8')

Better perform this function separately because now it is hidden. You need at least describe your protocol I suppose.

encrypted = rsa.encrypt(bytes, pub)

It is not a good idea to directly encrypt data using RSA only. Generally you would use hybrid cryptosystem (RSA encrypting a random data key).

That's very necessary if you use sign-then-encrypt or your encryption key must be much larger than the signing key. Currently your test method only signs / verifies the message separately, hiding the issue.

print('ERROR: DecryptionError!')
 string = ''
 return string

Please never return empty on error. You should not print errors, they log them.

• Vorac: Thanks a ton, your answer shows me how very little I know about the whole security business. Could you please elaborate on the following parts: 1. the utf-8 part, 2. encrypting data using RSA only (I was under the impression AES is being thrown in purely for efficiency of large messages e.g. videos) 3. So I should raise DecryptionError instead?
• Maarten Bodewes: 1. UTF-8 is correct, but it's hidden in the function, so either describe it or use another function for it that needs to be explicitly called (encoding is not really part of encryption). 2. Yes, for RSA can be used for small messages, but generally we just always use hybrid encryption (why would the message size be have to be dictated by the cipher?) 3. Yes, just raise an error and have the calling party deal with decryption errors. Note that might well mean that you ignore the error because returning precise crypto errors to the user is dangerous. Logging them is fine though.