Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Wed, 18 Jun 2014, Nathaniel McCallum wrote:
On Wed, 2014-06-18 at 17:48 -0400, Simo Sorce wrote:
On Wed, 2014-06-18 at 17:34 -0400, 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.
Attached is revision 4. I believe this addresses all the points given
over the last few days in all emails. The ipa_otptoken_import.py has
been significantly reworked to make it simpler and easy to test, but
none of the logic has changed.
I have removed most of the inheritance and sorted out most of the style
issues (like map() vs comprehension). I did not change the XML parsing
because it appears that network access is disabled by default.
I have also included a test suite which should have 100% code coverage.
It even tests for features we don't support yet (like X.509). All tests
pass for me.
Nathaniel
+++ b/install/tools/man/ipa-otptoken-import.1
@@ -0,0 +1,36 @@
+.\ A man page for ipa-compat-manage
Bad Copypaste here ^^^
Thanks! Fixed.
There is whitespace warning in the man page, needs to be fixed.
Also, spec file changes are incomplete, man page is not there.
The patch itself works fine for me with the test suite.
Attached is the specfile fix, with that one and whitespace removal --
ACK.
Attached also is a small fix for ipaplatform changes as specfile now has
wrong scoping for the platform files.
--
/ Alexander Bokovoy
From c4c303fc471e8ad2561bd8e3180985485981472d Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy aboko...@redhat.com
Date: Wed, 25 Jun 2014 09:46:39 +0300
Subject: [PATCH 08/10] fixup! Implement OTP token importing
---
freeipa.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 63f7477..2ba9786 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -775,6 +775,7 @@ fi
%{_mandir}/man1/ipa-backup.1.gz
%{_mandir}/man1/ipa-restore.1.gz
%{_mandir}/man1/ipa-advise.1.gz
+%{_mandir}/man1/ipa-otptoken-import.1.gz
%files server-trust-ad
%{_sbindir}/ipa-adtrust-install
--
1.9.3
From 6e5aabecc290e170882f53de52987d675a9b78b6 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy aboko...@redhat.com
Date: Tue, 24 Jun 2014 15:51:45 +0300
Subject: [PATCH 06/10] Fix packaging issue with doubly specified directories
---
freeipa.spec.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index ed125e5..63f7477 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -837,7 +837,7 @@ fi
%dir %{python_sitelib}/ipaplatform
%dir %{python_sitelib}/ipaplatform/base
%dir %{python_sitelib}/ipaplatform/fedora
-%{python_sitelib}/ipaplatform/*
+%{python_sitelib}/ipaplatform/*.py*
%{python_sitelib}/ipaplatform/base/*.py*
%{python_sitelib}/ipaplatform/fedora/*.py*
%attr(0644,root,root) %{python_sitearch}/default_encoding_utf8.so
--
1.9.3
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On 06/25/2014 12:40 PM, Alexander Bokovoy wrote: On Wed, 18 Jun 2014, Nathaniel McCallum wrote: On Wed, 2014-06-18 at 17:48 -0400, Simo Sorce wrote: On Wed, 2014-06-18 at 17:34 -0400, 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. Attached is revision 4. I believe this addresses all the points given over the last few days in all emails. The ipa_otptoken_import.py has been significantly reworked to make it simpler and easy to test, but none of the logic has changed. I have removed most of the inheritance and sorted out most of the style issues (like map() vs comprehension). I did not change the XML parsing because it appears that network access is disabled by default. I have also included a test suite which should have 100% code coverage. It even tests for features we don't support yet (like X.509). All tests pass for me. Nathaniel +++ b/install/tools/man/ipa-otptoken-import.1 @@ -0,0 +1,36 @@ +.\ A man page for ipa-compat-manage Bad Copypaste here ^^^ Thanks! Fixed. There is whitespace warning in the man page, needs to be fixed. Also, spec file changes are incomplete, man page is not there. The patch itself works fine for me with the test suite. Attached is the specfile fix, with that one and whitespace removal -- ACK. Attached also is a small fix for ipaplatform changes as specfile now has wrong scoping for the platform files. I pushed all 3 patches to master. (I did not realize you want to just fixup your patch 0008 before I pushed the first, so I chosen a better name for it.) Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Wed, 2014-06-18 at 17:34 -0400, 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. Attached is revision 4. I believe this addresses all the points given over the last few days in all emails. The ipa_otptoken_import.py has been significantly reworked to make it simpler and easy to test, but none of the logic has changed. I have removed most of the inheritance and sorted out most of the style issues (like map() vs comprehension). I did not change the XML parsing because it appears that network access is disabled by default. I have also included a test suite which should have 100% code coverage. It even tests for features we don't support yet (like X.509). All tests pass for me. Nathaniel +++ b/install/tools/man/ipa-otptoken-import.1 @@ -0,0 +1,36 @@ +.\ A man page for ipa-compat-manage Bad Copypaste here ^^^ Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Fri, 13 Jun 2014, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
Other than the first point though, anything else looks good to me.
I read through the patch and apart from the points Simo made, I don't
have much complaints. One additional thing to do is how to test it --
how to create the payload? Since this code doesn't require anything
specific, it could be well tested through a CI infrastructure.
Additionally, man page is missing.
--
/ Alexander Bokovoy
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On 06/14/2014 12:05 AM, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
If you want to use idiomatic Python,
starmap(lambda a, b: a ^ b, izip(u, [ord(c) for c in l]))
can be rewritten as
[a ^ ord(b) for a, b in zip(u, l)]
(izip is overkill if the list isn't too long)
then you can use
dk.extend(u)
to keep a single list of ints in dk, and at the end,
return ''.join(chr(c) for c in dk[:self.klen])
to turn part you need into a string.
It would be nice to have some tests for this.
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
Other than the first point though, anything else looks good to me.
Simo.
I see the patch contains lots of classes with no state and a single
method. I believe it would simplify the code greatly if functions were
used instead.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Fri, 2014-06-13 at 18:05 -0400, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Argh. Yes. Thanks for this.
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
There is only one PBKDF2 with differing PRFs. This is precisely what is
implemented. However, I'm still working with jdennis to see if I can
coax python-nss to do this for me. I'm happy to delete my own
implementation.
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
My understanding is that comprehensions are now preferred. I know
reduce() has been removed in Python 3.x. However, map() still appears to
be there. Does anyone have the official word on the preferred style?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
Sounds good. Let's see if nss can do this. If not, I'll clean this up.
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
In general, this kind of complexity is reduced somewhat in Python 3.x's
new bytes/str division. I have a bytes subclass for Py3k that I like to
use which implements the bitwise operators. I've even had ideas about
submitting this as an upstream Python patch for the bytes class.
Other than the first point though, anything else looks good to me.
Thanks for looking at this!
Nathaniel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On 06/16/2014 06:23 PM, Nathaniel McCallum wrote:
On Fri, 2014-06-13 at 18:05 -0400, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Argh. Yes. Thanks for this.
lxml.etree.XMLParser has a separate option for this, no_network, which
is True by default.
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
There is only one PBKDF2 with differing PRFs. This is precisely what is
implemented. However, I'm still working with jdennis to see if I can
coax python-nss to do this for me. I'm happy to delete my own
implementation.
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
My understanding is that comprehensions are now preferred. I know
reduce() has been removed in Python 3.x. However, map() still appears to
be there. Does anyone have the official word on the preferred style?
Here's my view, strictly unofficial:
Comprehensions are better in almost all cases.
This (converting each value) is just about the only one where map is
more concise, and roughly equally readable.
So, for consistency, it's better to use comprehensions everywhere.
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
Sounds good. Let's see if nss can do this. If not, I'll clean this up.
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
In general, this kind of complexity is reduced somewhat in Python 3.x's
new bytes/str division. I have a bytes subclass for Py3k that I like to
use which implements the bitwise operators. I've even had ideas about
submitting this as an upstream Python patch for the bytes class.
Try the python-ideas list first. There's a recent thread there from Nick
Coghlan on a similar topic, you might want to read it.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Mon, 2014-06-16 at 11:53 +0300, Alexander Bokovoy wrote:
On Fri, 13 Jun 2014, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
Other than the first point though, anything else looks good to me.
I read through the patch and apart from the points Simo made, I don't
have much complaints. One additional thing to do is how to test it --
how to create the payload? Since this code doesn't require anything
specific, it could be well tested through a CI infrastructure.
I don't see why we can't use the samples that jcholast dug up as test
data since they come from the RFC. The noted fix for Figure 7 should be
included so that the test works.
Additionally, man page is missing.
The man page should be in 0053.2.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
On Wed, 2014-06-11 at 17:41 -0400, Nathaniel McCallum wrote:
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).
Fixed.
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.
I added support for derived keys. However, pskc-figure7.xml appears to
have a problem. This problem is actually in RFC 6030 itself (where
pskc-figure7.xml comes from). According to the xenc11 standard, all of
these tags should be in the xenc11 namespace (not pkcs5 or undefined as
given in RFC 6030):
pkcs5:PBKDF2-params
Salt
SpecifiedEj7/PEpyEpw=/Specified
/Salt
IterationCount1000/IterationCount
KeyLength16/KeyLength
PRF/
/pkcs5:PBKDF2-params
When fixing this error in pskc-figure7.xml, key derivation works in this
latest patch.
11) Importing
http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-all.xml
or
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
Other than the first point though, anything else looks good to me.
Simo.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
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
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
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
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
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.
From b7576825077b68ed68ff3a811c05cbc3eb4e4c12 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum npmccal...@redhat.com
Date: Thu, 8 May 2014 11:06:16 -0400
Subject: [PATCH] Implement OTP token importing
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.
https://fedorahosted.org/freeipa/ticket/4261
---
freeipa.spec.in | 2 +
install/tools/Makefile.am| 1 +
install/tools/ipa-otptoken-import| 29 +++
ipaserver/install/ipa_otptoken_import.py | 419 +++
4 files changed, 451 insertions(+)
create mode 100755 install/tools/ipa-otptoken-import
create mode 100644 ipaserver/install/ipa_otptoken_import.py
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 4e3fd7351757be773fae0b02c55549910c5b37ad..850cca85b6deb5ce4a5656fb2328c7a4d6bcc8cb 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -307,6 +307,7 @@ Requires: python-netaddr
Requires: libipa_hbac-python
Requires: python-qrcode
Requires: python-pyasn1
+Requires: python-dateutil
Obsoletes: ipa-python = 1.0
@@ -660,6 +661,7 @@ fi
%{_sbindir}/ipa-csreplica-manage
%{_sbindir}/ipa-server-certinstall
%{_sbindir}/ipa-ldap-updater
+%{_sbindir}/ipa-otptoken-import
%{_sbindir}/ipa-compat-manage
%{_sbindir}/ipa-nis-manage
%{_sbindir}/ipa-managed-entries
diff --git a/install/tools/Makefile.am b/install/tools/Makefile.am
index 2cf66c6dfc1c272bb423253902e7339e7d159567..485be91b7bca2b0f3822a70d0f027793208918c1 100644
--- a/install/tools/Makefile.am
+++ b/install/tools/Makefile.am
@@ -20,6 +20,7 @@ sbin_SCRIPTS = \
ipa-nis-manage \
ipa-managed-entries \
ipa-ldap-updater \
+ ipa-otptoken-import \
ipa-upgradeconfig \
ipa-backup \
ipa-restore \
diff --git a/install/tools/ipa-otptoken-import b/install/tools/ipa-otptoken-import
new file mode 100755
index ..8184a6afe04e816727a34085edc458a4d90bf470
--- /dev/null
+++ b/install/tools/ipa-otptoken-import
@@ -0,0 +1,29 @@
+#! /usr/bin/python2 -E
+# Authors: Nathaniel McCallum npmccal...@redhat.com
+#
+# Copyright (C) 2014 Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see http://www.gnu.org/licenses/.
+#
+
+from ipaserver.install.ipa_otptoken_import import OTPTokenImport
+import nss.nss as nss
+
+nss.nss_init_nodb()
+
+try:
+OTPTokenImport.run_cli()
+finally:
+nss.nss_shutdown()
diff --git a/ipaserver/install/ipa_otptoken_import.py b/ipaserver/install/ipa_otptoken_import.py
new file mode 100644
index ..b8281a90a876cef0eff68396dd2c60f446e6ce98
--- /dev/null
+++ b/ipaserver/install/ipa_otptoken_import.py
@@ -0,0 +1,419 @@
+# Authors: Nathaniel McCallum npmccal...@redhat.com
+#
+# Copyright (C) 2014 Red Hat
+# see file 'COPYING' for use and
