[Pki-devel] [PATCH] 194 - Initial work for python client API

Mon Feb 17 17:36:13 UTC 2014

See inline below:
On Thu, 2014-02-13 at 11:42 -0600, Endi Sukma Dewata wrote:
> On 2/12/2014 2:41 PM, Ade Lee wrote:
> > Hi all,
> >
> > Attached is a patch with some initial work for the Python client
> > library, specifically focused on the interactions between the DRM and
> > the python client, and the functionality exposed in the
> > KeyRequestResource and KeyResource REST APIs.
> >
> > The easiest way to see this code in action is to run the main function
> > in the KRAClient module.  I usually run this in Eclipse.
> >
> > You will need to set up a few things first though:
> > 1.  Install a CA/KRA.  It this is not on the default ports, you will
> > need to modify the connection information in KRAClient.__main__
> >
> > 2. The python code uses python-requests to talk to the server, and
> > requests uses openssl.  That means you need to export your DRM admin
> > cert to a PEM file, so that it can be used for client auth.  I did this
> > as follows:
> >
> >   openssl pkcs12 -in ~/.dogtag/pki-tomcat/ca_admin_cert.p12 -out temp4.pem -nodes
> >
> > Without any changes, the code in KRAClient.__main__ assumes this file
> > will be in /tmp/temp4.pem.
> >
> > 3.  We do some crypto functions using NSS commands (like generating a
> > symmetric key or wrapping using the transport cert).  Therefore, we need
> > to create an NSS database and populate it with the transport cert.  The
> > code expects it to be at /tmp/drmtest/certdb
> >
> > I did this as follows:
> > mkdir /tmp/drmtest/certdb
> > certutil -N -d /tmp/drmtest/certdb
> > chmod +r /tmp/drmtest/certdb/*
> >
> > certutil -L -d /var/lib/pki/pki-tomcat/alias/ -n "transportCert cert-pki-tomcat KRA" -a > transport_cert.txt
> > certutil -A -d /tmp/drmtest/certdb/ -n "kra transport cert" -i ./transport_cert.txt -a -t "u,u,u"
> >
> > 4. Then just run kraclient.__main__ with no arguments.
> >
> > The story is still somewhat incomplete, but its a lot of code - so I
> > want to start getting some eyes on this.
> >
> > Here is what I plan to add in subsequent patches:
> >
> > 1. Extract the NSS functionality into a separate module nssutil.py, and
> > import that module only if python-nss is actually installed.  Similarly,
> > only define those functions that need nss if python-nss is installed.
> >
> > The idea here is that the client library does not necessarily require
> > NSS.  If NSS (and python-nss) is not installed, then the client needs to
> > provide things like symmetric keys and wrapping etc.
> >
> > This will almost certainly be the case for Barbican, where they will
> > generate their own keys, and do their own wrapping.
> >
> > 2.  Complete the generate_pki_archive_options() function, and then test
> > archival.
> >
> > 3.  Add logic to handle exceptions.  This will almost certainly take the
> > form of decorator classes that handle the various exceptions and return
> > codes expected.
> >
> > 4.  The new modules pass most of the PEP8 conventions as detected by
> > pylint.  One thing that pylint does not like is having attribute names
> > that are camelCased.  The reason those are there is because they
> > represent the actual names/attributes as passed over in JSON by the
> > server.  We populate the relevant objects by doing something like this:
> >
> > def from_dict(self, attr_list):
> >          ''' Return KeyInfo from JSON dict '''
> >          for key in attr_list:
> >              setattr(self, key, attr_list[key])
> >          return self
> >
> > I plan to add a small conversion function that convert camelCased
> > attribute names to camel_cased to conform with PEP8.
> >
> > Please review,
> > Ade
> 
> Some comments:
> 
> 1. The 'created' date in cert.py should be updated.
> 
Done - also added a note that the implementation in cert.py is
incomplete and needs to be tested.

> 2. The CertId.__init__() doesn't parse the hex number.
> 
Will be fixed in subsequent patch.

> 3. To be consistent with the Java API, all from_dict() methods probably 
> should be a @staticmethod or @classmethod.
> 
Done.

> 4. To be consistent the decode_from_json() can be called from_json(). Is 
> there any difference between this method and from_dict()?
> 
Done.

> 5. The CertResource should be called CertClient for consistency with 
> Java API. Same thing with KeyResource and KeyRequestResource. The 
> *Resource classes in Java are internal implementation. The classes 
> actually exposed to the client applications are the *Client classes.
> 
Done.

> 6. The following line is executed each time getCert() is called:
>    e.TYPES['CertData'] = CertData()
> It should be executed just once. See system.py.
> 
Done.
> 7. The TYPES list in #6 holds references to classes, not instances. This 
> is probably related to #3 as well.
> 
Done.
> 8. The listCerts() should have passed the status as a query parameter.
> 
Will be fixed in subsequent patch when certs are addressed.

> 9. As in #6, the following line in searchCerts() should be executed just 
> once:
>    e.TYPES['CertSearchRequest'] = CertSearchRequest
> 
Done.
> 10. I have not tested the code, but searchCerts() doesn't seem to work 
> with the new Jackson-style JSON format. See system.py.
> 
Will be fixed in subsequent patch when cert.py is fixed.

> 11. The get_transport_cert() is defined in CertResource. In the Java API 
> getTransportCert() is defined in SystemCertResource and used by 
> KRAClient. There's no SystemCertClient yet, but this method should be 
> defined there.
> 
Done
> 12. KeyRequestResponse.decode_from_json() does not populate keyData 
> attribute.
> 
Fixed.
> 13. As in #6, in KeyResource.retrieve_key() the e.NOTYPES registration 
> should only be executed once.
> 
Fixed.
> 14. The key.py's main program is a sample of a client application using 
> the Python library, but it does the e.NOTYPES registrations. Although 
> the registrations are only executed once, any client application should 
> not need to do these registrations. The Python library itself should do 
> the registrations automatically. See system.py.
> 
Fixed.
> 15. It would be better to move the main programs in these Python library 
> into a separate test folder because they become functional tests and 
> contain hard-coded test configuration (e.g. hostname, port, path, 
> request ID, client ID). Please include the instruction to setup the test 
> environment as mentioned in the above email.
> 
Done - put into kra/functional/drmtest.py and added drmtest.readme.txt.
Also changed the name of drmclient.py and its readme to
drmclient_deprecated.py.  We will remove these files as soon as we have
extracted all the useful code out of them.

> 16. From client application's perspective, it would be better if the 
> kraclient.generate_sym_key() can take a list of usages, instead of 
> requiring the client app to join the usages manually.
> 

Done

> 17. Ideally the Key/KeyRequest-specific methods in KRAClient should be 
> moved into KeyClient/KeyRequestClient classes to avoid cluttering up the 
> KRAClient class. In the Java client library user-specific methods are 
> grouped into UserClient under KRAClient.
> 
> 18. Instead of adding barbican_encode() which basically wraps 
> generate_sym_key(), the KeyRequestResponse could provide get_key_id() 
> which will get the key ID from the requestInfo so the client app can 
> call generate_sym_key() directly:
> 
>      response = kraclient.generate_sym_key(...)
>      key_id = response.get_key_id()
> 
Done

> 19. Instead of adding barbican_decode(), the KRAClient could provide a 
> more generic method that can be used by other client applications. 
> Something like this:
> 
>      class KRAClient(object):
>          def retrieve_key(self, key_id):
> 
>              response = request_recovery(key_id)
>              approve_request(response.get_request_id())
> 
>              transport_cert = get_transport_cert()
>              session_key = generate_session_key(...)
>              wrapped_session_key = wrap_session_key(...)
> 
>              request = KeyRecoveryRequest(key_id, ...)
>              response = key_client.retrieve_key(request)
> 
>              return unwrap_key(response.wrappedPrivateData, ...))
> 
> Client apps such as Barbican would be able to subclass and override the 
> generate_session_key() to provide its own key, possibly using other 
> crypto library.
> 

OK - there have been a bunch of changes to address this.  I have also
extracted the crypto functionality.  See the commit message.

Attached is a new patch.  Please apply this on top of the previous
patch.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-vakwetu-0195-Additional-changes-as-per-review.patch
Type: text/x-patch
Size: 68809 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140217/bb5bd4d0/attachment.bin>