Re: [Freeipa-devel] [PATCH] convert SAFE_STRING_PATTERN (was: Why do we have so much duplicated code?)
On 01/27/2010 10:30 PM, Rob Crittenden wrote: Is this code used by anything any more? This was part of the old XML-RPC server. It was used to determine whether a data type needed to be the XML-RPC Binary type or not. In v2 Jason wrote a similar function that bases the output type based on the python type (strings are unicode, binary data is str). So I think this can just be dropped (after testing to confirm, of course). That's a great question. In fact this was one of the things which caused me to spend time yesterday trying to figure out what's actually in use, it's also an example of code that is duplicated identically in two places. I don't think it's in use at all. I believe the functionality in ipapython/ipautil.py and ipaserver/ipautil.py has been superseded by xml_wrap and xml_unwrap in ipalib/rpc.py. My vote would be to remove the code and I'll supply a patch which does. FWIW SAFE_STRING_PATTERN can't exist in it's current form or I can't submit my next patch for i18n support because the string causes xgettext to abort. It would be nice if we cleaned some of this legacy cruft out of the source tree. Having not been involved in the code base continuously it makes it confusing to figure out how pieces of the code fit together when code isn't used and/or is defined in multiple places. P.S.: sorry about about the format of the patch I submitted. I was playing around with git send-email and it didn't work quite the way I expected :-) -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] Remove (un)wrap_binary_data cruft from */ipautil.py
Remove SAFE_STRING_PATTERN, safe_string_re, needs_base64(),
wrap_binary_data(), unwrap_binary_data() from both instances
of ipautil.py. This code is no longer in use and the
SAFE_STRING_PATTERN regular expression string was causing xgettext
to abort because it wasn't a valid ASCII string.
---
ipapython/ipautil.py | 62 --
ipaserver/ipautil.py | 62 --
2 files changed, 0 insertions(+), 124 deletions(-)
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 1399c70..7c41d78 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -284,68 +284,6 @@ class CIDict(dict):
return (key,value)
-#
-# The safe_string_re regexp and needs_base64 function are extracted from the
-# python-ldap ldif module, which was
-# written by Michael Stroeder
-# http://python-ldap.sourceforge.net
-#
-# It was extracted because ipaldap.py is naughtily reaching into the ldif
-# module and squashing this regexp.
-#
-SAFE_STRING_PATTERN = '(^(\000|\n|\r| |:|<)|[\000\n\r\200-\377]+|[ ]+$)'
-safe_string_re = re.compile(SAFE_STRING_PATTERN)
-
-def needs_base64(s):
- """
- returns 1 if s has to be base-64 encoded because of special chars
- """
- return not safe_string_re.search(s) is None
-
-
-def wrap_binary_data(data):
-"""Converts all binary data strings into Binary objects for transport
- back over xmlrpc."""
-if isinstance(data, str):
-if needs_base64(data):
-return xmlrpclib.Binary(data)
-else:
-return data
-elif isinstance(data, list) or isinstance(data,tuple):
-retval = []
-for value in data:
-retval.append(wrap_binary_data(value))
-return retval
-elif isinstance(data, dict):
-retval = {}
-for (k,v) in data.iteritems():
-retval[k] = wrap_binary_data(v)
-return retval
-else:
-return data
-
-
-def unwrap_binary_data(data):
-"""Converts all Binary objects back into strings."""
-if isinstance(data, xmlrpclib.Binary):
-# The data is decoded by the xmlproxy, but is stored
-# in a binary object for us.
-return str(data)
-elif isinstance(data, str):
-return data
-elif isinstance(data, list) or isinstance(data,tuple):
-retval = []
-for value in data:
-retval.append(unwrap_binary_data(value))
-return retval
-elif isinstance(data, dict):
-retval = {}
-for (k,v) in data.iteritems():
-retval[k] = unwrap_binary_data(v)
-return retval
-else:
-return data
-
class GeneralizedTimeZone(datetime.tzinfo):
"""This class is a basic timezone wrapper for the offset specified
in a Generalized Time. It is dst-ignorant."""
diff --git a/ipaserver/ipautil.py b/ipaserver/ipautil.py
index 7042c1c..cd0fdb5 100644
--- a/ipaserver/ipautil.py
+++ b/ipaserver/ipautil.py
@@ -116,68 +116,6 @@ class CIDict(dict):
return (key, value)
-#
-# The safe_string_re regexp and needs_base64 function are extracted from the
-# python-ldap ldif module, which was
-# written by Michael Stroeder
-# http://python-ldap.sourceforge.net
-#
-# It was extracted because ipaldap.py is naughtily reaching into the ldif
-# module and squashing this regexp.
-#
-SAFE_STRING_PATTERN = '(^(\000|\n|\r| |:|<)|[\000\n\r\200-\377]+|[ ]+$)'
-safe_string_re = re.compile(SAFE_STRING_PATTERN)
-
-def needs_base64(s):
-"""
-returns 1 if s has to be base-64 encoded because of special chars
-"""
-return not safe_string_re.search(s) is None
-
-
-def wrap_binary_data(data):
-"""Converts all binary data strings into Binary objects for transport
- back over xmlrpc."""
-if isinstance(data, str):
-if needs_base64(data):
-return xmlrpclib.Binary(data)
-else:
-return data
-elif isinstance(data, list) or isinstance(data,tuple):
-retval = []
-for value in data:
-retval.append(wrap_binary_data(value))
-return retval
-elif isinstance(data, dict):
-retval = {}
-for (k, v) in data.iteritems():
-retval[k] = wrap_binary_data(v)
-return retval
-else:
-return data
-
-
-def unwrap_binary_data(data):
-"""Converts all Binary objects back into strings."""
-if isinstance(data, xmlrpclib.Binary):
-# The data is decoded by the xmlproxy, but is stored
-# in a binary object for us.
-return str(data)
-elif isinstance(data, str):
-return data
-elif isinstance(data, list) or isinstance(data,tuple):
-retval = []
-for value in data:
-retval.append(unwrap_binary_data(value))
-return retval
-elif isinstance(data, dict):
-retval = {}
-for (k, v) in data.iteritems():
-retval[k] = unwrap_binary_data(v)
-return retval
-else:
-
[Freeipa-devel] [PATCH] fix error message to be i18n translator friendly
This error message was producing a warning from xgettext
because there were multiple substations in the string.
In some languages it may be necessary to reorder the
substitutions for a proper translation, this is only
possible if the substitutions use named values.
---
ipaserver/plugins/selfsign.py |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/ipaserver/plugins/selfsign.py b/ipaserver/plugins/selfsign.py
index af832a6..79f9d33 100644
--- a/ipaserver/plugins/selfsign.py
+++ b/ipaserver/plugins/selfsign.py
@@ -100,7 +100,8 @@ class ra(rabase.rabase):
if str(base).lower() != str(new_request).lower():
subject_base='CN=%s, %s' % (hostname, subject_base)
new_request.reverse()
-raise errors.CertificateOperationError(error=_('Request subject \'%s\' does not match the form \'%s\'' % (", ".join(new_request), subject_base)))
+raise errors.CertificateOperationError(error=_('Request subject "%(request_subject)s" does not match the form "%(subject_base)s"') % \
+ {'request_subject' : ', '.join(new_request), 'subject_base' : subject_base})
except errors.CertificateOperationError, e:
raise e
except Exception, e:
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] jderose 036 Remove PluginProxy hold-overs
Jason Gerard DeRose wrote: This patch removes some cruft left over from when we were still using my ill-fated PluginProxy to wrap Plugin instances. This patch: 1. Removes special __public__ class attribute from Plugin and its descendants 2. Removes special __proxy__ class attribute from same 3. Removes the Plugin.implements() and Plugin.implemented_by() methods 4. Updates unit-tests where they expected any of the above None of these features were being used except by the unit-tests, so this should be a very safe change. ack, pushed to master rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 358 remove files on uninstall
Remove some IPA configuration files when doing an uninstallation. rob freeipa-358-remove.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Remove (un)wrap_binary_data cruft from */ipautil.py
John Dennis wrote: Remove SAFE_STRING_PATTERN, safe_string_re, needs_base64(), wrap_binary_data(), unwrap_binary_data() from both instances of ipautil.py. This code is no longer in use and the SAFE_STRING_PATTERN regular expression string was causing xgettext to abort because it wasn't a valid ASCII string. --- ipapython/ipautil.py | 62 -- ipaserver/ipautil.py | 62 -- 2 files changed, 0 insertions(+), 124 deletions(-) ack, pushed to master rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 359 be smarter about decoding certs
Be a bit smarter about decoding certificates that might be base64 encoded. First see if it only contains those characters allowed before trying to decode it. This reduces the number of false positives. rob freeipa-359-cert.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 360 be smarter about decoding certs
Gah, got the description mixed up with the last patch :-( Be a bit smarter about decoding certificates that might be base64 encoded. First see if it only contains those characters allowed before trying to decode it. This reduces the number of false positives. rob freeipa-360-service.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 359 allow cert renewal
Rob Crittenden wrote: Be a bit smarter about decoding certificates that might be base64 encoded. First see if it only contains those characters allowed before trying to decode it. This reduces the number of false positives. rob Er, duh, I got this description goofed up. This patch is related to certificate renewal/issuing a new certificate for a service that already has one. We used to reject inserting a new certificate altogether. This patch adds a new flag, --renew, that lets you replace a certificate. The existing certificate is revoked. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 361 fix cert tests
This fixes some problems with the cert plugin tests. - It checks to see if a self-signed CA is available in ~/.ipa/alias. If not the tests are skipped - Be a bit smarter about cleaning up by moving it to a separate test - This relies on patch the service fix in 360. Some binary certs were being decoded as base64 resulting in an unparsable cert for the ASN.1 parser. I also added a bit of documentation on how to set up the self-signed CA. It is a one-time thing. rob freeipa-361-tests.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 360 be smarter about decoding certs
On 01/28/2010 04:15 PM, Rob Crittenden wrote: Gah, got the description mixed up with the last patch :-( Be a bit smarter about decoding certificates that might be base64 encoded. First see if it only contains those characters allowed before trying to decode it. This reduces the number of false positives. I'm not sure the test is doing what you want or even if it's the right test. The test is saying "If there is one or more characters in the bas64 alphabet then try and decode. That means just about anything will match, which doesn't seem like a very strong test. Why not just try and decode it and let the decoder decide if it's really base64, the decoder has much strong rules about the input, including assuring the padding is correct. -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 359 allow cert renewal
On 01/28/2010 04:16 PM, Rob Crittenden wrote: Rob Crittenden wrote: Be a bit smarter about decoding certificates that might be base64 encoded. First see if it only contains those characters allowed before trying to decode it. This reduces the number of false positives. rob Er, duh, I got this description goofed up. ACK to the correct patch -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 362 remove group pwd policy on group deletion
Try to remove a group password policy when a group is deleted. No need to leave that hanging around. rob freeipa-362-group.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 363 find all group pwd policy
Provide pwpolicy-find command to display all group-specific password policies. find is a bit of a misnomer since you can't provide any terms to limit the search scope, but it's a start. I'm not sure this is the kind of thing we need/want to be able to query things like "give me all the policies where the max lifetime is > 20 days". But I could be wrong. rob freeipa-363-pwpolicy.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Fix File parameter validation when prompting.
On Wed, 2010-01-27 at 17:53 +0100, Pavel Zuna wrote: > cli.prompt_interactively now loads files before validating the parameter > value. > It also populates a list of already loaded files, so that cli.load_files > knows > when a parameter already contains the file contents. > > Fix #557163 > > Pavel ack. This looks reasonable to me, but I'd really like you to add some tests for this, especially testing that it works correctly for a command with multiple File params. Rob and John, do you see any problems with this approach? Does this address the needs of the cert plugins? ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Fix File parameter validation when prompting.
On 01/28/2010 06:56 PM, Jason Gerard DeRose wrote: On Wed, 2010-01-27 at 17:53 +0100, Pavel Zuna wrote: cli.prompt_interactively now loads files before validating the parameter value. It also populates a list of already loaded files, so that cli.load_files knows when a parameter already contains the file contents. Fix #557163 Pavel ack. This looks reasonable to me, but I'd really like you to add some tests for this, especially testing that it works correctly for a command with multiple File params. Rob and John, do you see any problems with this approach? Does this address the needs of the cert plugins? nck Maybe I'm reading the code wrong but it seems like there are cases where the already_loaded_files list is not updated. If load_files() is called then the file is loaded but already_loaded_files is not updated to reflect that. Shouldn't the function load_file() update the already_loaded_files list to assure already_loaded_files is always in sync? Also, wouldn't already_loaded_files be better implemented as a set rather than a list? Aside from the above state consistency issue I think it fixes the problem, but I'll be honest I'm not in love with it because it's kind of a hack to get around deficiencies in the command parameter framework. I always get nervous when I see special case exceptions surgically inserted into select pieces of code rather than conforming to a regular and consistent framework. Also this only works because the cli object is short lived and single use. If that assumption is ever violated the persistent state in the object will present a consistency problem. Bottom line, I'll ack it if the first issue is resolved or Pavel explains why load_files() can't produce an inconsistent state. -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Fix File parameter validation when prompting.
I've been thinking about this a bit more. I wonder if part of the inelegance is due to the fact we're trying to shoehorn two distinct concepts into one item when a proper relationship does not exist. It does not seem logical that a file is a subclass of a string which is how this is set up now. Files simply do not derive from strings, they are two entirely distinct concepts. Consider any typical command line program you're familiar with. You might pass string data as an argument or it might be read from stdin. But if you want that command to read from a file you have to pass it a different argument specifying the filename to open. Take the case of sed for example. If you pass a script on the command line you use the -e arg, however, if you want to pass the script as a file you use the -f arg. They aren't the same at the point of invocation, internally the program maps one to the other. Maybe that's why the proposed mechanism is awkward, we're trying to conflate one concept with another. Perhaps there should be distinct arguments available for the user to the use, one arg passes a string, a different arg passes a filename. The filename parameter opens the file, reads the content and then assigns the value to the appropriate keyword parameter. For the file arg you would have: kw[param.alias] = value instead of: kw[param.name] = value where param.alias is the name of the "destination" parameter associated with an alternative arg for supplying the same value. In this scheme the you don't need to keep state, you don't need to special case any code, you can use the existing normalize and validate mechanisms. Thoughts? -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 360 be smarter about decoding certs
John Dennis wrote: On 01/28/2010 04:15 PM, Rob Crittenden wrote: Gah, got the description mixed up with the last patch :-( Be a bit smarter about decoding certificates that might be base64 encoded. First see if it only contains those characters allowed before trying to decode it. This reduces the number of false positives. I'm not sure the test is doing what you want or even if it's the right test. The test is saying "If there is one or more characters in the bas64 alphabet then try and decode. That means just about anything will match, which doesn't seem like a very strong test. Why not just try and decode it and let the decoder decide if it's really base64, the decoder has much strong rules about the input, including assuring the padding is correct. The reason is I had a binary cert that was correctly decoded by the base64 encoder. I don't know the why's and wherefores but there it is. I see what you mean about my regex being a bit weak though, it really should require that the entire string conform. I'll see what I can do. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 355 allow named to use ldapi
On Wed, 2010-01-27 at 14:53 -0500, Rob Crittenden wrote: > Add SELinux rules so named can communicate to the DS over ldapi. > > This should fix the installation error when --setup-dns is set and > SELinux is enforcing. > > rob I'm trying to test this out, but I'm not sure what I need to enter for the DNS forwarder: """ Enter IP address for a DNS forwarder (empty to stop): """ Any advice? ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 360 be smarter about decoding certs
On 01/28/2010 10:30 PM, Rob Crittenden wrote: John Dennis wrote: On 01/28/2010 04:15 PM, Rob Crittenden wrote: Gah, got the description mixed up with the last patch :-( Be a bit smarter about decoding certificates that might be base64 encoded. First see if it only contains those characters allowed before trying to decode it. This reduces the number of false positives. I'm not sure the test is doing what you want or even if it's the right test. The test is saying "If there is one or more characters in the bas64 alphabet then try and decode. That means just about anything will match, which doesn't seem like a very strong test. Why not just try and decode it and let the decoder decide if it's really base64, the decoder has much strong rules about the input, including assuring the padding is correct. The reason is I had a binary cert that was correctly decoded by the base64 encoder. I don't know the why's and wherefores but there it is. Then testing to see if each byte is in the base64 alphabet would not have prevented this error. For a while now I've been feeling like we need to associate a format attribute to the certificate (e.g. DER, PEM, BASE64, etc.). Or we need to adopt a convention that certs are always in one canonical format and the interface is responsible for assuring what it accepts as input is converted to the canonical form. I see what you mean about my regex being a bit weak though, it really should require that the entire string conform. I'll see what I can do. rob -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
