Yubico / python-pyhsm

Python code for YubiHSM
https://developers.yubico.com/python-pyhsm/
BSD 2-Clause "Simplified" License
73 stars 24 forks source link

KSM database backend doesn't respect --key-handles #13

Open michael-lazar opened 7 years ago

michael-lazar commented 7 years ago

Scenario

I'm setting up a distributed system that runs multiple KSMs using the python KSM included in this package (yubikey_ksm.py). I will be connecting each KSM to a dedicated yubiHSM. I'm using a centralized MySQL database to store the AEAD keys for all of the servers. I would like each of my KSMs to use a unique key handle for decryption. In my example I have 5 servers, so server 1 decrypts using key handle 1, server 2 decrypts using key handle 2, etc. The AEAD keys are encrypted with all 5 of the key handles and stored together in a single table on the database.

Problem

When I start a server with the --key-handle 3 argument, I expect that it will load the AEAD encrypted with key handle 3 from the database. This behavior is correct when the AEAD keys are stored using the file system backend. However, with the database backend, it doesn't respect the --key-handle argument and instead returns an AEAD with a random key handle.

Here's the offending logic:

https://github.com/Yubico/python-pyhsm/blob/master/pyhsm/ksm/yubikey_ksm.py#L220

s = sqlalchemy.select([self.aead_table]).where(self.aead_table.c.public_id == public_id)
result = connection.execute(s)

for row in result:
    kh_int = row['keyhandle']
    aead = pyhsm.aead_cmd.YHSM_GeneratedAEAD(None, kh_int, '')
    aead.data = row['aead']
    aead.nonce = row['nonce']
return aead

You can see that it's iterating over all of the AEADs that match the yubikey and returning the last one outside of the loop. The row that ends up being returned is random. If it doesn't match a key handle that is shared with the HSM, the decryption will fail.

Solution

I was able to fix this by updating the SQL query to filter on only the key handles that were specified via --key-handles.

s = sqlalchemy.select([self.aead_table]).where(
    (self.aead_table.c.public_id == public_id)
    & self.aead_table.c.keyhandle.in_([kh[1] for kh in self.key_handles]))
result = connection.execute(s)

for row in result:
    kh_int = row['keyhandle']
    aead = pyhsm.aead_cmd.YHSM_GeneratedAEAD(None, kh_int, '')
    aead.data = row['aead']
    aead.nonce = row['nonce']
return aead

I have a fork with my changes here https://github.com/michael-lazar/python-pyhsm/blob/ksm_db_backend/pyhsm/ksm/yubikey_ksm.py#L208. I would be happy to submit a pull request if you're interested in getting this upstream

klali commented 7 years ago

I think that makes sense and should be fixed. Does the query as constructed by sql alchemy hit the index for public_id + keyhandle? Please submit it as a pull request and I'll get it merged.

michael-lazar commented 7 years ago

Thank you for the quick response! Yes it does appear to hit the primary key index.

MariaDB [ykksm]> EXPLAIN SELECT aead_table.public_id, aead_table.keyhandle, aead_table.nonce, aead_table.aead FROM aead_table WHERE aead_table.public_id = 'djccccccccbb' AND aead_table.keyhandle IN (1, 2);
+------+-------------+------------+-------+---------------+---------+---------+------+------+-------------+
| id   | select_type | table      | type  | possible_keys | key     | key_len | ref  | rows | Extra       |
+------+-------------+------------+-------+---------------+---------+---------+------+------+-------------+
|    1 | SIMPLE      | aead_table | range | PRIMARY       | PRIMARY | 22      | NULL |    2 | Using where |
+------+-------------+------------+-------+---------------+---------+---------+------+------+-------------+
1 row in set (0.00 sec)