[Pki-devel] [PATCH] 198-203 patches to address multiple issues in KeyResource server and client code.

[Endi Sukma Dewata]

On 2/21/2014 11:52 PM, Ade Lee wrote:
> Endi, Jack and I met to discuss various improvements to the
> Key/KeyResource client/server parts.  Some of these are addressed in the
> attached patches.  Some will be handled in separate tickets.
>
> Separate Tickets to be filed:
> 1. Add nonce mechanism for approvals.
> 2. Add openssl subclass for CryptoUtil
> 3. Extend generate_session_key() to return key in same call
> 4. Allow CLI to call python? (to be filed as separate ticket)
>
> Done in attached patches:
> 5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
>     and extend to allow addition of trans_wrapped_session_key.
> 6. Add getActiveKey() to python client.
> 7. client_id -> client_key_id
> 8. constants in python API for key status
> 9.  Add sanity checks to python client code
> 10. Move functions out of KRAClient.py and into key.py
> 11. from_dict() -> from)json()
> 12. Add methods to create nss certdb and import transport cert
> 13. All inputs/outputs from CryptoUtil are unencoded.
> 14. Fix usages in main function of SymKeyGenerationRequest
> 15. Fix bugs when retrieving invalid keyId.
> 16.  Fix bugs when generating key with only clientID provided.
>
> To be done in next patch:
> 17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
> provide a more restricted convenience function generate_session_key()
>
> To be considered further:
> 1. rename session_key -> encryption_key/ wrapping_key?
> 2. revamp archival to not require client to generate PkiArchiveOptions
> object.
> 3. should retrieve functions return unwrapped key?
>
> Please review attached patches.
>
> Ade

Patch #199:

1. The setup_database() throws a ValueError if the database already exists.

   raise exceptions.ValueError(
     "Directory already exists and over_write is false")

I'm not sure it's an appropriate exception: 
http://docs.python.org/2/library/exceptions.html#exceptions.ValueError. 
We probably could just throw a generic Exception.

2. The error message probably should just say "Directory already 
exists". The end user should not see a variable name in the error 
message. If the program ends with this error the user would know that an 
existing database causes a conflict, so either they will remove it or 
use a different path or overwrite it with a flag.

3. Since we're storing password, in NamedTemporaryFile() we probably 
should specify some params to use user's temp dir instead of global temp 
dir, and delete the temp file after closing. See: 
http://docs.python.org/2/library/tempfile.html.

4. Minor thing. The data param in symmetric_unwrap() doc is followed by 
a colon, which is inconsistent with the other params in that doc.

   :param data:           Data to be unwrapped

Is there a standard way of documenting params?

5. The "!=" operator could be replaced with "is not".

   if transport_cert_nick is not None:

6. I think ideally client shouldn't be required to encode/decode the 
trans_wrapped_session_key, wrappedPrivateData, nonceData, 
transport_cert, etc. The base64 encoding should only be part of 
marshalling/unmarshalling, hidden from the client. Thoughts?

7. Can these 3 invocations be combined:

     cryptoutil.NSSCryptoUtil.setup_database(
         certdb_dir, certdb_password, over_write=True)
     crypto = cryptoutil.NSSCryptoUtil(certdb_dir, certdb_password)
     crypto.initialize_db()

into this?

     crypto = cryptoutil.NSSCryptoUtil(
         certdb_dir, certdb_password, over_write=True)

Just a question, is it possible to use 2 different NSS databases at the 
same time?

-- 
Endi S. Dewata