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

[John Magne]

I'll piggy back on this ACK. Just a few comments interspersed.



----- Original Message -----
> From: "Endi Sukma Dewata" <edewata at redhat.com>
> To: alee at redhat.com, pki-devel at redhat.com
> Sent: Monday, February 24, 2014 12:53:37 PM
> Subject: Re: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in KeyResource server and client code.
> 
> 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.

For this db exists already? Is this really the end of the world?
Could we just go ahead and open it for them and move on? For instance
nss now has the notion of a completely common db where one could 
conceivably just add to it.


> 
> 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?

I agree with this, if it's feasible. This way they don't have to worry about
initializing anything, they just put in the params to identify the db.



> 
>      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
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
>