[Freeipa-devel] [PATCH 0053] Implement OTP token importing

[Jan Cholasta]

Hi,

On 13.5.2014 18:40, Nathaniel McCallum wrote:
> On Tue, 2014-05-13 at 12:38 -0400, Nathaniel McCallum wrote:
>> This patch adds support for importing tokens using RFC 6030 key
>> container files. This includes decryption support. For sysadmin sanity,
>> any tokens which fail to add will be written to the output file for
>> examination. The main use case here is where a small subset of a large
>> set of tokens fails to validate or add. Using the output file, the
>> sysadmin can attempt to recover these specific tokens.
>>
>> This code is implemented as a server-side script. However, it doesn't
>> actually need to run on the server. This was done because importing is
>> an odd fit for the IPA command framework:
>> 1. We need to write an output file.
>> 2. The operation may be long-running (thousands of tokens).
>> 3. Only admins need to perform this task and it only happens
>> infrequently.
>
> I forgot to put the link to the ticket in the commit message. Fixed.

1) I think you should initialize NSS in ipa_otptoken_import.py, not in 
the ipa-otptoken-import script.


2) The pep8 tool reports a lot of errors in ipa_otptoken_import.py.


3) Other error messages are in the form "message: %s", I think this one 
should use that form as well:

+        if encoding != 'DECIMAL':
+            raise ValidationError('Unsupported encoding (%s)!' % encoding)


4) This is not right:

+                except:
+                    self.log.warn("Error adding token: " + 
str(sys.exc_info()[1]))

I think it should be something like this instead:

     except ValidationError, e:
         self.log.warn("Error adding token: %s", e)


5) There is no man page for ipa-otptoken-import.


6) Output file is created even when ipa-otptoken-import fails with 
"Unable to connect to LDAP! Did you kinit?" and other initialization 
errors, which makes subsequent ipa-otptoken-import fail with "Output 
file already exists!".


7) When a key is specified by reference in Key/KeyReference instead of 
directly in Key/Data/Secret like in 
<http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-figure4.xml>, 
import fails with "Key not found in token!". I would expect a different 
error message.


8) Importing 
<http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-figure5.xml> 
produces this output:

/usr/lib/python2.7/site-packages/ipaserver/install/ipa_otptoken_import.py:307: 
FutureWarning: The behavior of this method will change in future 
versions. Use specific 'len(elem)' or 'elem is not None' test instead.
   if data.get('pinpolicy', None):
Error adding token: 'NoneType' object has no attribute 'strip'


9) Using an arbitrary file in -k produces this output:

(SEC_ERROR_INVALID_KEY) The key does not support the requested operation.
Traceback (most recent call last):
   File "/usr/sbin/ipa-otptoken-import", line 29, in <module>
     nss.nss_shutdown()
nss.error.NSPRError: (SEC_ERROR_BUSY) NSS could not shutdown. Objects 
are still in use.


10) Importing 
<http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-figure7.xml> 
and 
<http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-figure8.xml> 
produces this output:

Error adding token: object of type 'NoneType' has no len()


11) Importing 
<http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-all.xml> 
or 
<http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-all-signed.xml> 
produces this output:

/usr/lib/python2.7/site-packages/ipaserver/install/ipa_otptoken_import.py:304: 
FutureWarning: The behavior of this method will change in future 
versions. Use specific 'len(elem)' or 'elem is not None' test instead.
   if data.get('maxtransact', None):
/usr/lib/python2.7/site-packages/ipaserver/install/ipa_otptoken_import.py:307: 
FutureWarning: The behavior of this method will change in future 
versions. Use specific 'len(elem)' or 'elem is not None' test instead.
   if data.get('pinpolicy', None):


12) Importing 
<http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-invalid.xml> 
succeeds, but it should fail.


13) Importing 
<http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/libpskc/examples/pskc-mini.xml> 
fails, but it should succeed, I think.


Honza

-- 
Jan Cholasta