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

[Nathaniel McCallum]

On Wed, 2014-06-11 at 14:24 +0200, Jan Cholasta wrote:
> 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.

Fixed.

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

Fixed (mostly). The remaining output from pep8 is, I think, entirely
justifiable.

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

Fixed.

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

Fixed.

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

TODO (tomorrow).

> 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!".

Fixed.

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

This error is now: Referenced keys are not supported!

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

This now states:
Error adding token: PINPolicy policy not supported!
Error adding token: Unsupported token type!

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

What do you mean by "arbitrary file"? A file that is not the key?
Like /dev/null? I'm not able to reproduce this.

> 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()

Import fails with:
Derived keys are not currently supported!
 or
X.509 keys are not currently supported!

It would be nice to support these in the future.

> 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):

Both of these now output:
Error adding token: NumberOfTransactions policy not supported!

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

This now errors with:
PSKC file is invalid!

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

I think this should fail in our case since we can't possibly support
that configuration.

Nathaniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-npmccallum-0053.1-Implement-OTP-token-importing.patch
Type: text/x-patch
Size: 21605 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140611/fb81a17c/attachment.bin>