Re: [Freeipa-devel] [PATCH] 971 detect binary LDAP data
On 02/28/2012 09:50 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/28/2012 04:45 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/28/2012 04:02 AM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/27/2012 05:10 PM, Rob Crittenden wrote:
Rob Crittenden wrote:
Simo Sorce wrote:
On Mon, 2012-02-27 at 09:44 -0500, Rob Crittenden wrote:
We are pretty trusting that the data coming out of LDAP matches
its
schema but it is possible to stuff non-printable characters into
most
attributes.
I've added a sanity checker to keep a value as a python str type
(treated as binary internally). This will result in a base64
encoded
blob be returned to the client.
Shouldn't you try to parse it as a unicode string and catch
TypeError to
know when to return it as binary ?
Simo.
What we do now is the equivalent of unicode(chr(0)) which returns
u'\x00' and is why we are failing now.
I believe there is a unicode category module, we might be able to
use
that if there is a category that defines non-printable characters.
rob
Like this:
import unicodedata
def contains_non_printable(val):
for c in val:
if unicodedata.category(unicode(c)) == 'Cc':
return True
return False
This wouldn't have the exclusion of tab, CR and LF like using ord()
but
is probably more correct.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
If you're protecting the XML-RPC calls, it'd probably be better to
look
at the XML spec directly: http://www.w3.org/TR/xml/#charsets
Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] |
[#x1-#x10]
I'd say this is a good set for CLI as well.
And you can trap invalid UTF-8 sequences by catching the
UnicodeDecodeError from decode().
Replace my ord function with a regex that looks for invalid
characters.
Now catching that exception too and leaving as a str type.
rob
You're matching an Unicode regex against a bytestring. It'd be
better to
match after the decode.
The unicode regex was force of habit. I purposely do this comparison
before the decoding to save decoding something that is binary. Would it
be acceptable if I just remove the u'?
rob
No: re.search('[\uDFFF]', u'\uDFFF'.encode('utf-8')) -- None
Actually I'm not sure what encoding is used behind the scenes here; I
wouldn't recommend mixing Unicode and bytestrings even if it did work on
my machine.
The data coming out of LDAP is a plain string. We encode to utf-8 if is
not a binary as defined in _SYNTAX_MAPPING in ipaserver/plugins/ldap2.py
Now we're just confusing each other, it seems.
By plain string you mean a bytestring in either UTF-8 or IA5 (which is
almost an UTF-8 subset), right?
We convert that to an Unicode string if possible.
Anyway just dropping the u won't work. The \u escape only works in
Unicode strings:
list('\u1234')
['\\', 'u', '1', '2', '3', '4']
Also, match() only looks at the beginning of the string; use search().
Sorry for not catching that earlier.
good point.
OCD notes:
- the matches variable name is misleading, there's either one match or
None.
- use `is not None` instead of `!= None` as per PEP8.
- The contains_non_printable method seems a bit excessive now that it
essentially just contains one call. Unless it's meant to be subclassed,
in which case the docstring should probably look different.
It doesn't need to be a public method. I broke it out mostly because
embedding the documentation on why it is there overwhelmed encode().
rob
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0008 Documentation improvement, configuration check
On Tue, 2012-02-28 at 14:19 -0500, Dmitri Pal wrote: On 02/28/2012 08:46 AM, Adam Tkac wrote: On 02/28/2012 02:44 PM, Petr Spacek wrote: On 02/24/2012 01:42 PM, Petr Spacek wrote: Hello, this patch is documentation improvement configuration check for situations, where persistent search and zone refresh are enabled at same time. (Which is not allowed.) It's related to fix https://fedorahosted.org/bind-dyndb-ldap/ticket/43 - hold bind and plugin global settings in LDAP. ... there is the forgotten patch :-) Ack, push it to master, please. Who has the commit rights to push these patches? I see that both Adam and Petr Spacek have upstream commit rights. I think it would be also good for Petr to get the rights for bind-dyndb-ldap package in Fedora as well, so that both of them can release updated package versions. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 968 don't allow reconnection to deleted master
On Tue, 2012-02-28 at 16:36 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Sat, 2012-02-25 at 17:43 -0500, Rob Crittenden wrote: This patch does two things: 1. Prompts when deleting a master to make clear that this is irreversible 2. Does not allow a deleted master to be reconnected. Reconnecting to a deleted master causes all heck to break loose because we delete principals as part of deletion process. If you reconnect to a deleted master then we replicate those deletes and the connected master is now unusable (no principals). A simple test is: Install master Install replica ipa-replica-manage del replica ipa-replica-manage connect replica ipa-server-uninstall -U on replica re-install replica The re-install should be successful. rob Generally, it looks and works well. I just miss some unattended way to deleted a replica, from other script for example. I think we may either re-use --force flag for this purpose or introduce an --unattended flag. I also found an issue with S4U2Proxy memberPrincipal added for each replica. Since the memberPrincipal values for deleted replica are not removed when a replica is being deleted, ipa-replica-install reports a (benign) error when it tries to add a duplicate value afterwards. I filed a ticket for this one: https://fedorahosted.org/freeipa/ticket/2451 Martin OK, went with --force. rob The approach should be OK, but the patch you included is wrong. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 971 detect binary LDAP data
On 28.2.2012 18:58, Rob Crittenden wrote: Jan Cholasta wrote: On 28.2.2012 18:02, Petr Viktorin wrote: On 02/28/2012 04:45 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/28/2012 04:02 AM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/27/2012 05:10 PM, Rob Crittenden wrote: Rob Crittenden wrote: Simo Sorce wrote: On Mon, 2012-02-27 at 09:44 -0500, Rob Crittenden wrote: We are pretty trusting that the data coming out of LDAP matches its schema but it is possible to stuff non-printable characters into most attributes. I've added a sanity checker to keep a value as a python str type (treated as binary internally). This will result in a base64 encoded blob be returned to the client. I don't like the idea of having arbitrary binary data where unicode strings are expected. It might cause some unexpected errors (I have a feeling that --addattr and/or --delattr and possibly some plugins might not handle this very well). Wouldn't it be better to just throw away the value if it's invalid and warn the user? This isn't for user input, it is for data stored in LDAP. User's are going to have no way to provide binary data to us unless they use the API themselves in which case they have to follow our rules. Well my point was that --addattr and --delattr cause an LDAP search for the given attribute and plugins might get the result of a LDAP search in their post_callback and I'm not sure if they can cope with binary data. Shouldn't you try to parse it as a unicode string and catch TypeError to know when to return it as binary ? Simo. What we do now is the equivalent of unicode(chr(0)) which returns u'\x00' and is why we are failing now. I believe there is a unicode category module, we might be able to use that if there is a category that defines non-printable characters. rob Like this: import unicodedata def contains_non_printable(val): for c in val: if unicodedata.category(unicode(c)) == 'Cc': return True return False This wouldn't have the exclusion of tab, CR and LF like using ord() but is probably more correct. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel If you're protecting the XML-RPC calls, it'd probably be better to look at the XML spec directly: http://www.w3.org/TR/xml/#charsets Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x1-#x10] I'd say this is a good set for CLI as well. And you can trap invalid UTF-8 sequences by catching the UnicodeDecodeError from decode(). I don't think we should care about XML-RPC in LDAP-specific code at all. If you want to do some additional checks, do them in XML-RPC-specific code. We are trusting that the data in LDAP matches its schema. This is just belt and suspenders verifying that it is the case. Sure, but I still think we should allow any valid unicode data to come from LDAP, not just what is valid in XML-RPC. rob Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 69 Configure SSH features of SSSD in ipa-client-install
On 28.2.2012 23:42, Rob Crittenden wrote: Jan Cholasta wrote: Hi, this patch configures the new SSH features of SSSD in ipa-client-install. To test it, you need to have SSSD 1.8.0 installed. Honza Is there a better name for 'GlobalKnownHostsFile2'? What do you mean? The option name or the file name? Either way, I don't think there is a better name. When is PubKeyAgent used?I tried in RHEL 6.2, F-11 and F15-17 and it was an unknown option in all. It's in openssh in RHEL 6.0. Should you test for the existence of /usr/bin/sss_ssh_knownhostsproxy and /usr/bin/sss_ssh_authorizedkeys before setting it in a config file? It depends. Do we want to support clients with SSSD 1.8.0? How would you recommend testing this? Enroll a client and try to log into the IPA server? To test host authentication, you need an IPA host with SSH public keys set (which is done automatically in ipa-client-install, so any IPA host should work) and try to ssh into that host from other (actually, it can be the same) IPA host. You should not see The authenticity of host ... can't be estabilished ssh message. To test user authentication, you need an IPA user with SSH public keys set. To do that, you need to set the public keys using ipa user-mod. You should then be able to authenticate using your private key on any IPA host. rob Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0010 Use stricter semantics when checking IP address for DNS records
On 02/15/2012 12:57 PM, Martin Kosek wrote: On Wed, 2012-02-15 at 11:20 +0100, Petr Viktorin wrote: This fixes https://fedorahosted.org/freeipa/ticket/2379 by using inet_pton instead of inet_aton. Yeah, this would fix the stricter checking. I planed to improve A/ validation in a scope of this ticket, I plan to use CheckedIPAddress to be more consistent with the rest of the plugin. I made the change you just did in CheckedIPAddress already. My point is that we may want to be even stricter and forbid for example broadcast or multicast addresses to be placed to A/ records. Martin That was a NACK; Martin wanted to this himself. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0010 Use stricter semantics when checking IP address for DNS records
On Wed, 2012-02-29 at 10:56 +0100, Petr Viktorin wrote: On 02/15/2012 12:57 PM, Martin Kosek wrote: On Wed, 2012-02-15 at 11:20 +0100, Petr Viktorin wrote: This fixes https://fedorahosted.org/freeipa/ticket/2379 by using inet_pton instead of inet_aton. Yeah, this would fix the stricter checking. I planed to improve A/ validation in a scope of this ticket, I plan to use CheckedIPAddress to be more consistent with the rest of the plugin. I made the change you just did in CheckedIPAddress already. My point is that we may want to be even stricter and forbid for example broadcast or multicast addresses to be placed to A/ records. Martin That was a NACK; Martin wanted to this himself. I changed my mind, this approach is OK for now. Rejecting any multicast or broadcast addresses may be too restrictive, I would rather just follow the relevant RFC and just check the A record syntax in this case. Thus, your approach is sufficient. ACK. Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options
On 02/28/2012 03:19 PM, Jan Cholasta wrote: On 28.2.2012 11:54, Petr Viktorin wrote: On 02/27/2012 10:44 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/20/2012 08:51 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2159 says various config options are not marked Required, so entering an empty value for it will pass validation (and IPA will blow up later when it expects a string,not None). Forexample the following: $ ipa config-mod --groupsearch= fails with AttributeError: 'NoneType' object has no attribute 'split' There is a more general problem behind this, though: even if the attributes *are* marked as Required, an empty string will pass validation. This is because `None` is used in `Param.validate` to mean both No value supplied and Empty value supplied. The method currently assumes the former, and skips validation entirely for `None` values to optional parameters. For example, the following will delete membergroup, even though it's a required attribute : $ ipa delegation-add --attrs=street --group=editors \ --membergroup=admins td1 $ ipa delegation-mod --membergroup= td1 Note that some LDAPObjects handle this with a _check_empty_attrs function, so they aren't affected. That function is specific to LADP objects, though. So I needed to tackle this on a lower level. This patch solves the problem by * adding a 'nonempty' flag when a required parameter of a CRUD Update object is auto-converted to a non-required parameter * making the`validate` method aware of whether the parameter was supplied; and if it was, honor the nonempty flag. The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by marking required config options as required. This looks good but I think there are other things to protect in config as well such as the default e-mail domain. It is probably safe to say that everything in there is required. rob Let me just double-check this with you. According to code in the user plugin (around line 330), if the default e-mail domain is not set, users don't get an address auto-assigned. Do we really want to require user e-mails? ipaconfigstring (the password plugin flags) are a set (multivalue, not required). The rest of the values I left as not required are for optional features or limits: search results time limit, max. username length, password expiry notification. Currently if these are missing, the feature/limit is disabled (well, except for the time limit). But, there are also special values (0 or -1) that have the same effect as a missing value. Sometimes they're documented. So we want to enforce that users use these special values instead of removing the config entry? I think we want to enforce that these are defined. It will be confusing for users if these are not there at all. I don't think we need to show the special options, just declare that the attribute is required. rob Attaching updated patch 13. Only the default e-mail domain (https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are still optional. You have removed all the config-related defensive code in the patch, is this a good idea? What will happen if someone e.g. deletes a required config attribute directly from LDAP? Then IPA crashes. The defensive code wasn't there for all cases anyway, as ticket #2159 shows. If we want to protect against this it would probably be better to make the config class itself give the default when a required value is missing. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options
On 29.2.2012 11:09, Petr Viktorin wrote: On 02/28/2012 03:19 PM, Jan Cholasta wrote: On 28.2.2012 11:54, Petr Viktorin wrote: On 02/27/2012 10:44 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/20/2012 08:51 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2159 says various config options are not marked Required, so entering an empty value for it will pass validation (and IPA will blow up later when it expects a string,not None). Forexample the following: $ ipa config-mod --groupsearch= fails with AttributeError: 'NoneType' object has no attribute 'split' There is a more general problem behind this, though: even if the attributes *are* marked as Required, an empty string will pass validation. This is because `None` is used in `Param.validate` to mean both No value supplied and Empty value supplied. The method currently assumes the former, and skips validation entirely for `None` values to optional parameters. For example, the following will delete membergroup, even though it's a required attribute : $ ipa delegation-add --attrs=street --group=editors \ --membergroup=admins td1 $ ipa delegation-mod --membergroup= td1 Note that some LDAPObjects handle this with a _check_empty_attrs function, so they aren't affected. That function is specific to LADP objects, though. So I needed to tackle this on a lower level. This patch solves the problem by * adding a 'nonempty' flag when a required parameter of a CRUD Update object is auto-converted to a non-required parameter * making the`validate` method aware of whether the parameter was supplied; and if it was, honor the nonempty flag. The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by marking required config options as required. This looks good but I think there are other things to protect in config as well such as the default e-mail domain. It is probably safe to say that everything in there is required. rob Let me just double-check this with you. According to code in the user plugin (around line 330), if the default e-mail domain is not set, users don't get an address auto-assigned. Do we really want to require user e-mails? ipaconfigstring (the password plugin flags) are a set (multivalue, not required). The rest of the values I left as not required are for optional features or limits: search results time limit, max. username length, password expiry notification. Currently if these are missing, the feature/limit is disabled (well, except for the time limit). But, there are also special values (0 or -1) that have the same effect as a missing value. Sometimes they're documented. So we want to enforce that users use these special values instead of removing the config entry? I think we want to enforce that these are defined. It will be confusing for users if these are not there at all. I don't think we need to show the special options, just declare that the attribute is required. rob Attaching updated patch 13. Only the default e-mail domain (https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are still optional. You have removed all the config-related defensive code in the patch, is this a good idea? What will happen if someone e.g. deletes a required config attribute directly from LDAP? Then IPA crashes. The defensive code wasn't there for all cases anyway, as ticket #2159 shows. If we want to protect against this it would probably be better to make the config class itself give the default when a required value is missing. This, and raise an error in cases where no default is available (the check should probably be done in ldap.get_ipa_config). Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 956 user lockout status
On 02/27/2012 06:31 PM, Martin Kosek wrote: 4) Minor change: -except Exception: +except: Don't do that. It would for example disable Ctrl+C by trapping KeyboardInterrupt. PEP8 has a paragraph on this, search for 'except Exception:' -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 080-085 DNS UI update
On 02/24/2012 11:00 PM, Endi Sukma Dewata wrote: ACK. Feel free to push once the required server piece is ready. Patches 80,81,82-1,83,84,85,90,91,92,93 pushed to master and ipa-2-2 On 2/23/2012 7:06 AM, Petr Vobornik wrote: 3. When adding an A/ record and checking the 'create reverse' option, if the reverse zone doesn't exist it will show an error dialog box saying it cannot create the reverse record. The A/ record is actually created but the page is not refreshed. I think it should detect the error 4304 and refresh the page. Fixed: I generalized and reused the code in host adder dialog. I'm wondering if it would be better to put it in command code so it would be default handler for this error type. It's done in separate patch: 92. Yeah, there probably should be a way to define the default handlers for various error codes which can still be overridden for a specific situation. Maybe we can register the default handlers in an error_handlers map in the IPA object? Sounds good. 8. The behavior of the checkboxes for idnsforwardpolicy is a bit unusual because you can only select at most one value. Usually checkboxes allow you to select multiple values. It might be better to use 3 radio buttons for all possible values: only, first, and none/default. It is unusual. I think the 'none/default' can be a bit unusual/confusing too. It may not be clear that the value will be '', user can expect that the value will be set to 'none' or actual default - if the attribute has some. What about radios an 'unset' link below them? We probably can use a more descriptive label such as 'Forward only' instead of just 'only', but the idea is we provide 3 radio buttons for the 3 possible options. The general understanding is that with radio buttons the field have exactly 1 value whereas with checkboxes there could be 0-n values. Right now we have 2 checkboxes but we can only select 0 or 1 value, that's why it's a bit unusual. Adding an unset link is possible too, but it won't be as intuitive as selecting one of the radio buttons. You are probably right. I'll mark it for myself as possible improvement. (after a release I'll probably go through all these marked mails and make some tickets). -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 14 ipa permission-add does not fail if using invalid attribute
On 02/28/2012 09:57 PM, Rob Crittenden wrote:
Ondrej Hamada wrote:
On 02/27/2012 03:22 PM, Rob Crittenden wrote:
Ondrej Hamada wrote:
When adding or modifying permission with both type and attributes
specified, check whether the attributes are allowed for specified
type.
In case of disallowed attributes the InvalidSyntax error is raised.
New tests were also added to the unit-tests.
https://fedorahosted.org/freeipa/ticket/2293
https://www.redhat.com/mailman/listinfo/freeipa-devel
NACK. You should use obj.object_class_config to determine if the
default list of objectclasses comes from LDAP.
I think that may be it, otherwise the patch reads ok.
I'm very glad to see unit tests!
rob
Corrected
Sorry, found a couple of more things I should have found the first
review.
Please use the dn module to construct dn_ipaconfig. Or you can also
get the DN on-the-fly since the config object using get_dn().
Probably just as safe to call: if obj.object_class_config: ... rather
than hasattr. I suppose its just a style thing.
Done.
I wonder if ObjectclassViolation is a better exception. SyntaxError
means the data type is wrong, not that it isn't allowed.
I agree that it makes more sense and I've updated the patch that way,
but the documentation says: permission operation fails with schema
syntax errors - maybe we should also update the documentation.
rob
--
Regards,
Ondrej Hamada
FreeIPA team
jabber: oh...@jabbim.cz
IRC: ohamada
From 1f98c50a64cfa5f564ac77f60796d952f2d44edf Mon Sep 17 00:00:00 2001
From: Ondrej Hamada oham...@redhat.com
Date: Wed, 29 Feb 2012 11:40:31 +0100
Subject: [PATCH] Validate attributes in permission-add
When adding or modifying permission with both type and attributes
specified, check whether the attributes are allowed for specified type.
In case of disallowed attributes raises the ObjectclassViolation
exception.
New tests were also added to the unit-tests.
https://fedorahosted.org/freeipa/ticket/2293
---
ipalib/plugins/permission.py| 55 ++
tests/test_xmlrpc/test_permission_plugin.py | 65 +++
2 files changed, 120 insertions(+), 0 deletions(-)
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 08781ce2ef3df30d10565a071a338edf77c52d23..c9fd5649f338b5c92b86e471fb817b9d964084d3 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -23,6 +23,7 @@ from ipalib import api, _, ngettext
from ipalib import Flag, Str, StrEnum
from ipalib.request import context
from ipalib import errors
+from ipalib.dn import DN
__doc__ = _(
Permissions
@@ -89,6 +90,43 @@ output_params = (
),
)
+dn_ipaconfig = str(DN('cn=ipaconfig,cn=etc,%s' % api.env.basedn))
+
+def check_attrs(attrs, type):
+# Trying to delete attributes - no need for validation
+if attrs is None:
+return True
+allowed_objcls=[]
+disallowed_objcls=[]
+obj=api.Object[type]
+
+if obj.object_class_config:
+(dn,objcls)=api.Backend.ldap2.get_entry(
+dn_ipaconfig,[obj.object_class_config]
+)
+allowed_objcls=objcls[obj.object_class_config]
+else:
+allowed_objcls=obj.object_class
+if obj.possible_objectclasses:
+allowed_objcls+=obj.possible_objectclasses
+if obj.disallow_object_classes:
+disallowed_objcls=obj.disallow_object_classes
+
+allowed_attrs=[]
+disallowed_attrs=[]
+if allowed_objcls:
+allowed_attrs=api.Backend.ldap2.get_allowed_attributes(allowed_objcls)
+if disallowed_objcls:
+disallowed_attrs=api.Backend.ldap2.get_allowed_attributes(disallowed_objcls)
+failed_attrs=[]
+for attr in attrs:
+if (attr not in allowed_attrs) or (attr in disallowed_attrs):
+failed_attrs.append(attr)
+if failed_attrs:
+raise errors.ObjectclassViolation(info='attribute(s) \%s\ not allowed' % ','.join(failed_attrs))
+return True
+
+
class permission(LDAPObject):
Permission object.
@@ -192,6 +230,8 @@ class permission_add(LDAPCreate):
opts['permission'] = keys[-1]
opts['aciprefix'] = ACI_PREFIX
try:
+if 'type' in entry_attrs and 'attrs' in entry_attrs:
+check_attrs(entry_attrs['attrs'],entry_attrs['type'])
self.api.Command.aci_add(keys[-1], **opts)
except Exception, e:
raise e
@@ -273,6 +313,21 @@ class permission_mod(LDAPUpdate):
except errors.NotFound:
self.obj.handle_not_found(*keys)
+# check the correctness of attributes only when the type is specified
+type=None
+attrs_to_check=[]
+current_values=self.api.Command.permission_show(attrs['cn'][0])['result']
+if 'type' in entry_attrs:
+type = entry_attrs['type']
+elif 'type' in current_values:
+type = current_values['type']
+if 'attrs' in entry_attrs:
+
Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options
On 02/29/2012 11:14 AM, Jan Cholasta wrote: On 29.2.2012 11:09, Petr Viktorin wrote: On 02/28/2012 03:19 PM, Jan Cholasta wrote: On 28.2.2012 11:54, Petr Viktorin wrote: On 02/27/2012 10:44 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/20/2012 08:51 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2159 says various config options are not marked Required, so entering an empty value for it will pass validation (and IPA will blow up later when it expects a string,not None). Forexample the following: $ ipa config-mod --groupsearch= fails with AttributeError: 'NoneType' object has no attribute 'split' There is a more general problem behind this, though: even if the attributes *are* marked as Required, an empty string will pass validation. This is because `None` is used in `Param.validate` to mean both No value supplied and Empty value supplied. The method currently assumes the former, and skips validation entirely for `None` values to optional parameters. For example, the following will delete membergroup, even though it's a required attribute : $ ipa delegation-add --attrs=street --group=editors \ --membergroup=admins td1 $ ipa delegation-mod --membergroup= td1 Note that some LDAPObjects handle this with a _check_empty_attrs function, so they aren't affected. That function is specific to LADP objects, though. So I needed to tackle this on a lower level. This patch solves the problem by * adding a 'nonempty' flag when a required parameter of a CRUD Update object is auto-converted to a non-required parameter * making the`validate` method aware of whether the parameter was supplied; and if it was, honor the nonempty flag. The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by marking required config options as required. This looks good but I think there are other things to protect in config as well such as the default e-mail domain. It is probably safe to say that everything in there is required. rob Let me just double-check this with you. According to code in the user plugin (around line 330), if the default e-mail domain is not set, users don't get an address auto-assigned. Do we really want to require user e-mails? ipaconfigstring (the password plugin flags) are a set (multivalue, not required). The rest of the values I left as not required are for optional features or limits: search results time limit, max. username length, password expiry notification. Currently if these are missing, the feature/limit is disabled (well, except for the time limit). But, there are also special values (0 or -1) that have the same effect as a missing value. Sometimes they're documented. So we want to enforce that users use these special values instead of removing the config entry? I think we want to enforce that these are defined. It will be confusing for users if these are not there at all. I don't think we need to show the special options, just declare that the attribute is required. rob Attaching updated patch 13. Only the default e-mail domain (https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are still optional. You have removed all the config-related defensive code in the patch, is this a good idea? What will happen if someone e.g. deletes a required config attribute directly from LDAP? Then IPA crashes. The defensive code wasn't there for all cases anyway, as ticket #2159 shows. If we want to protect against this it would probably be better to make the config class itself give the default when a required value is missing. This, and raise an error in cases where no default is available (the check should probably be done in ldap.get_ipa_config). Honza Would a better approach be to modify the LDAP schema to require these values? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 956 user lockout status
On Wed, 2012-02-29 at 11:20 +0100, Petr Viktorin wrote: On 02/27/2012 06:31 PM, Martin Kosek wrote: 4) Minor change: -except Exception: +except: Don't do that. It would for example disable Ctrl+C by trapping KeyboardInterrupt. PEP8 has a paragraph on this, search for 'except Exception:' Good to know, thanks. Rob, in that case please ignore issue #4. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 088-089 Added attrs to permission when target is group or filter:
On 02/28/2012 03:18 PM, Endi Sukma Dewata wrote: ACK. Some comments: Pushed to master, ipa-2-2 When adding attributes for filter permission it will show undo buttons. For consistency it might be better to use Delete links instead of undo buttons. However, instead of crossing out the values like in the details page, the Delete links will remove the entire row just like the undo button. We don't use undo buttons in dialogs because the values are new, so there is nothing to undo. OK. I used it mostly because 'delete' link kinda means that the value was there before and also undo isn't entirely bad behavior - the value is changed from nothing to something. Using such thinking we could say that other fields should have undo enabled too. We don't want that though. So you are right. Do you think it's possible to make the fields generic enough so it can be used with any type of widgets? So maybe instead of mapping an attribute into multiple fields we can map a field into multiple widgets. Fields/attributes to an entity is like columns to a table, so ideally they shouldn't be widget specific. We would have to change a way how we determine if a field is dirty. But it would be probably possible if we move the dirty logic to widget - which maybe better - for example we already use it in multivalued field. Anyway this should be well thought to make it finally right. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 094 Fixed redirection in Add and edit in automember hostgroup.
On 02/28/2012 03:18 PM, Endi Sukma Dewata wrote: On 2/23/2012 7:42 AM, Petr Vobornik wrote: Redirection in 'Add and edit' in automember hostgroup now navigates to correct facet. https://fedorahosted.org/freeipa/ticket/2422 ACK. Pushed to master, ipa-2-2. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 095 Fixed selection of single value in combobox
On 02/28/2012 03:19 PM, Endi Sukma Dewata wrote: On 2/23/2012 9:39 AM, Petr Vobornik wrote: Attaching patch On 02/23/2012 04:34 PM, Petr Vobornik wrote: Patch description: When editable combobox had only one option and input field was cleared, the option couldn't be selected if it was selected before. This patch adds click handler to option elements. The handler calls select_on_change. When different option is selected select_on_change is executed twice. To avoid duplicate call of value_changed an open state of option area is checked. In first pass the area will be closed so it won't be executed in second. When selected option is clicked, only onclick handler is processed. This patch assumes that select event will be processed before click event. https://fedorahosted.org/freeipa/ticket/2070 ACK. Pushed to master, ipa-2-2. Last time there was a problem with test automation using Sahi. Hopefully this trick with double execution will fix it. You are talking about not being able to select value or search for a new in a expanded combobox, right? I'm not sure if this is connected with this problem. Sometimes, when I'm developing (I'm using mostly chromium, in FF it seems OK), I run into this problem. So far I didn't found out what is the exact cause. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0016-17 Fixes for{add, set, del}attr with managed attributes
On 02/27/2012 11:03 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
Patch 16 defers validation conversion until after {add,del,set}attr is
processed, so that we don't search for an integer in a list of strings
(this caused ticket #2405), and so that the end result of these
operations is validated (#2407).
Patch 17 makes these options honor params marked no_create and no_update.
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
NACK on Patch 17 which breaks patch 16.
How is patch 16 broken? It works for me.
*attr is specifically made to be powerful. We don't want to arbitrarily
block updating certain values.
Noted
Not having patch 17 means that the problem reported in 2408 still
occurs. It should probably check both the schema and the param to
determine if something can have multiple values and reject that way.
I see the problem now: the certificate subject base is defined as a
multi-value attribute in the LDAP schema. If it's changed to
single-value the existing validation should catch it.
Also, most of the config attributes should probably be required in the
schema. Am I right?
I'm a newbie here; what do I need to do when changing the schema? Is
there a patch that does something similar I could use as an example?
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 69 Configure SSH features of SSSD in ipa-client-install
On Wed, 2012-02-29 at 10:52 +0100, Jan Cholasta wrote:
On 28.2.2012 23:42, Rob Crittenden wrote:
Jan Cholasta wrote:
Hi,
this patch configures the new SSH features of SSSD in ipa-client-install.
To test it, you need to have SSSD 1.8.0 installed.
Honza
Is there a better name for 'GlobalKnownHostsFile2'?
What do you mean? The option name or the file name? Either way, I don't
think there is a better name.
When is PubKeyAgent used?I tried in RHEL 6.2, F-11 and F15-17 and it was
an unknown option in all.
It's in openssh in RHEL 6.0.
Should you test for the existence of /usr/bin/sss_ssh_knownhostsproxy
and /usr/bin/sss_ssh_authorizedkeys before setting it in a config file?
It depends. Do we want to support clients with SSSD 1.8.0?
How would you recommend testing this? Enroll a client and try to log
into the IPA server?
To test host authentication, you need an IPA host with SSH public keys
set (which is done automatically in ipa-client-install, so any IPA host
should work) and try to ssh into that host from other (actually, it can
be the same) IPA host. You should not see The authenticity of host ...
can't be estabilished ssh message.
To test user authentication, you need an IPA user with SSH public keys
set. To do that, you need to set the public keys using ipa user-mod. You
should then be able to authenticate using your private key on any IPA host.
rob
Honza
I get this exception when running ipa-client-install with your patch.
# ipa-client-install --enable-dns-updates
Discovery was successful!
Hostname: vm-138.idm.lab.bos.redhat.com
Realm: IDM.LAB.BOS.REDHAT.COM
DNS Domain: idm.lab.bos.redhat.com
IPA Server: vm-068.idm.lab.bos.redhat.com
BaseDN: dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
Continue to configure the system with these values? [no]: y
User authorized to enroll computers: admin
Synchronizing time with KDC...
Unable to sync time with IPA NTP server, assuming the time is in sync.
Password for ad...@idm.lab.bos.redhat.com:
Enrolled in IPA realm IDM.LAB.BOS.REDHAT.COM
Created /etc/ipa/default.conf
Traceback (most recent call last):
File /usr/sbin/ipa-client-install, line 1514, in module
sys.exit(main())
File /usr/sbin/ipa-client-install, line 1501, in main
rval = install(options, env, fstore, statestore)
File /usr/sbin/ipa-client-install, line 1326, in install
if configure_sssd_conf(fstore, cli_realm, cli_domain, cli_server,
options):
File /usr/sbin/ipa-client-install, line 711, in configure_sssd_conf
sssdconfig.activate_service('ssh')
File /usr/lib/python2.7/site-packages/SSSDConfig.py, line 1516, in
activate_service
raise NoServiceError
SSSDConfig.NoServiceError
SSSD version: sssd-1.8.1-0.20120228T2018Zgit751b121.fc16.x86_64
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 918, 919 update sudo schema
On 17.1.2012 04:55, Rob Crittenden wrote:
Jan Cholasta wrote:
Dne 13.1.2012 17:39, Rob Crittenden napsal(a):
Jan Cholasta wrote:
Dne 14.12.2011 16:21, Rob Crittenden napsal(a):
Jan Cholasta wrote:
Dne 14.12.2011 15:23, Rob Crittenden napsal(a):
Jan Cholasta wrote:
Dne 14.12.2011 05:20, Rob Crittenden napsal(a):
The sudo schema now defines sudoOrder, sudoNotBefore and
sudoNotAfter
but these weren't available in the sudorule plugin.
I've added support for these. sudoOrder enforces uniqueness
because
duplicates are undefined.
I also added support for a GeneralizedTime parameter type. This is
similar to the existing AccessTime parameter but it only handles a
single time value.
You should parse the date/time part of the value with
time.strptime(timestr, '%Y%m%d%H%M%S') instead of doing it
manually,
that way you'll get most of the validation for free.
Yes but it gives a crappy error message, just saying that some
data is
left over not what is wrong.
IMHO having a separate error message for every field in the time
string
(like you do in the patch) is an overkill, simple invalid time
and/or
unknown time format should suffice (we don't have errors like
invalid
3rd octet for IP adresses either).
Well, the work is done, hard to go back on a better error message.
Also, it would be nice to be able to enter the value in more
user-friendly format (e.g. 2011-12-14 13:01:25 +0100) and
normalize
that to LDAP generalized time.
When dealing with time there are so many ways to input and display
the
same values this becomes difficult.
I'd expect that the times for these two attributes will be
relatively
simple and I somehow doubt users are going to want seconds, leap
seconds
or fractions, but we'll need to consider how to do it for future
consistency (otherwise we could have a case where time is entered in
one
format for some attributes and another for others).
If we input in a nice way we need to output in the same way.
We could make the preferred input/output time format
user-configurable,
defaulting to current locale time format. This format would be used
for
output. For input, we could go over a list of formats (first the
user-configured format, then current locale format, then a handful of
standard formats like -MM-DD HH:MM:SS) and use the first format
that can be successfully used to parse the time string.
See how far you get into the rabbit hole with even this simple format?
I don't mind, as long as it is the right thing to do (IMHO) :)
Anyway, I think this could be done on the client side, so we might use
your patch without changes. However, I would prefer if the parameter
class was more generic, so we could use it (hypothetically) to store
time in some other way than LDAP generalized time attribute (at least
name it DateTime please).
Ok, I'm fine with that.
Thanks.
The LDAP GeneralizedTime needs to be either in GMT or include a
differential. This gets us into the territory where the client
could be
in a different timezone than the server which leads us to why we
dropped
AccessTime in the first place.
Speaking of time zones, the differential alone is not a sufficient time
zone description, as it doesn't account for DST. Is there a way to
store
time in LDAP with full time zone name (just in case it's needed
sometime
in future)?
There is no way to store DST in LDAP (probably for good reason). Oddly
enough the older LDAP v3 RFC (2252) strongly recommends using only GMT
but the RFC that obsoletes it (4517) does not include this.
Thanks for the info.
So I'd like the user to supply the
timezone themselves so I don't have to guess (wrongly) and let them
worry about differing timezones.
We don't have to guess, IIRC there is a way to get the local timezone
differential in both Python and JavaScript, so the client could supply
it automatically if necessary.
I was thinking more about non-IPA clients (like sudo and notBefore).
I think this can still be done at least in CLI, but it could be done in
a separate patch.
Updated patches attached.
rob
Patch 919 doesn't cleanly apply on current master (neither does 916 BTW).
Honza
Rebased patch (and 916 too, separately).
rob
Patch 918:
1. LDAP generalized time allows you to omit minutes from time zone
differential, your code treats such values as invalid
2. IMO a better pattern could be used, such as
u'^([0-9]{2}){5,7}([.,][0-9]+)?([-+]([0-9]{2}){1,2}|Z)$'
3. 20120229000Z has malformed minutes, but the error message says
Malformed seconds
4. 20120229000+ has malformed minutes, but the error message says
Missing operator for differential or malformed time string
5. 20120229+ is valid generalized time, but it causes Missing
operator for differential or malformed time string error
6. Invalid month/day combinations (such as 20120231Z) are treated as
valid
7. When + or - is missing, the error message says Missing operator ...
- IMO a better term than operator is sign in this case
Re: [Freeipa-devel] [PATCH] 0008 Documentation improvement, configuration check
On 02/29/2012 10:04 AM, Martin Kosek wrote: On Tue, 2012-02-28 at 14:19 -0500, Dmitri Pal wrote: On 02/28/2012 08:46 AM, Adam Tkac wrote: On 02/28/2012 02:44 PM, Petr Spacek wrote: On 02/24/2012 01:42 PM, Petr Spacek wrote: Hello, this patch is documentation improvement configuration check for situations, where persistent search and zone refresh are enabled at same time. (Which is not allowed.) It's related to fix https://fedorahosted.org/bind-dyndb-ldap/ticket/43 - hold bind and plugin global settings in LDAP. ... there is the forgotten patch :-) Ack, push it to master, please. Who has the commit rights to push these patches? I see that both Adam and Petr Spacek have upstream commit rights. I Every patch is reviewed by Adam before commit, of course. (First several patches wasn't sent to the list, only to Adam - sorry.) Petr^2 Spacek think it would be also good for Petr to get the rights for bind-dyndb-ldap package in Fedora as well, so that both of them can release updated package versions. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 69 Configure SSH features of SSSD in ipa-client-install
On 29.2.2012 14:24, Martin Kosek wrote:
On Wed, 2012-02-29 at 10:52 +0100, Jan Cholasta wrote:
On 28.2.2012 23:42, Rob Crittenden wrote:
Jan Cholasta wrote:
Hi,
this patch configures the new SSH features of SSSD in ipa-client-install.
To test it, you need to have SSSD 1.8.0 installed.
Honza
Is there a better name for 'GlobalKnownHostsFile2'?
What do you mean? The option name or the file name? Either way, I don't
think there is a better name.
When is PubKeyAgent used?I tried in RHEL 6.2, F-11 and F15-17 and it was
an unknown option in all.
It's in openssh in RHEL 6.0.
Should you test for the existence of /usr/bin/sss_ssh_knownhostsproxy
and /usr/bin/sss_ssh_authorizedkeys before setting it in a config file?
It depends. Do we want to support clients with SSSD 1.8.0?
How would you recommend testing this? Enroll a client and try to log
into the IPA server?
To test host authentication, you need an IPA host with SSH public keys
set (which is done automatically in ipa-client-install, so any IPA host
should work) and try to ssh into that host from other (actually, it can
be the same) IPA host. You should not see The authenticity of host ...
can't be estabilished ssh message.
To test user authentication, you need an IPA user with SSH public keys
set. To do that, you need to set the public keys using ipa user-mod. You
should then be able to authenticate using your private key on any IPA host.
rob
Honza
I get this exception when running ipa-client-install with your patch.
# ipa-client-install --enable-dns-updates
Discovery was successful!
Hostname: vm-138.idm.lab.bos.redhat.com
Realm: IDM.LAB.BOS.REDHAT.COM
DNS Domain: idm.lab.bos.redhat.com
IPA Server: vm-068.idm.lab.bos.redhat.com
BaseDN: dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
Continue to configure the system with these values? [no]: y
User authorized to enroll computers: admin
Synchronizing time with KDC...
Unable to sync time with IPA NTP server, assuming the time is in sync.
Password for ad...@idm.lab.bos.redhat.com:
Enrolled in IPA realm IDM.LAB.BOS.REDHAT.COM
Created /etc/ipa/default.conf
Traceback (most recent call last):
File /usr/sbin/ipa-client-install, line 1514, inmodule
sys.exit(main())
File /usr/sbin/ipa-client-install, line 1501, in main
rval = install(options, env, fstore, statestore)
File /usr/sbin/ipa-client-install, line 1326, in install
if configure_sssd_conf(fstore, cli_realm, cli_domain, cli_server,
options):
File /usr/sbin/ipa-client-install, line 711, in configure_sssd_conf
sssdconfig.activate_service('ssh')
File /usr/lib/python2.7/site-packages/SSSDConfig.py, line 1516, in
activate_service
raise NoServiceError
SSSDConfig.NoServiceError
SSSD version: sssd-1.8.1-0.20120228T2018Zgit751b121.fc16.x86_64
Martin
Does your /etc/sssd/sssd.conf and /usr/share/sssd/sssd.api.conf contain
[ssh] section?
--
Jan Cholasta
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 69 Configure SSH features of SSSD in ipa-client-install
On Wed, 2012-02-29 at 14:44 +0100, Jan Cholasta wrote:
On 29.2.2012 14:24, Martin Kosek wrote:
On Wed, 2012-02-29 at 10:52 +0100, Jan Cholasta wrote:
On 28.2.2012 23:42, Rob Crittenden wrote:
Jan Cholasta wrote:
Hi,
this patch configures the new SSH features of SSSD in ipa-client-install.
To test it, you need to have SSSD 1.8.0 installed.
Honza
Is there a better name for 'GlobalKnownHostsFile2'?
What do you mean? The option name or the file name? Either way, I don't
think there is a better name.
When is PubKeyAgent used?I tried in RHEL 6.2, F-11 and F15-17 and it was
an unknown option in all.
It's in openssh in RHEL 6.0.
Should you test for the existence of /usr/bin/sss_ssh_knownhostsproxy
and /usr/bin/sss_ssh_authorizedkeys before setting it in a config file?
It depends. Do we want to support clients with SSSD 1.8.0?
How would you recommend testing this? Enroll a client and try to log
into the IPA server?
To test host authentication, you need an IPA host with SSH public keys
set (which is done automatically in ipa-client-install, so any IPA host
should work) and try to ssh into that host from other (actually, it can
be the same) IPA host. You should not see The authenticity of host ...
can't be estabilished ssh message.
To test user authentication, you need an IPA user with SSH public keys
set. To do that, you need to set the public keys using ipa user-mod. You
should then be able to authenticate using your private key on any IPA host.
rob
Honza
I get this exception when running ipa-client-install with your patch.
# ipa-client-install --enable-dns-updates
Discovery was successful!
Hostname: vm-138.idm.lab.bos.redhat.com
Realm: IDM.LAB.BOS.REDHAT.COM
DNS Domain: idm.lab.bos.redhat.com
IPA Server: vm-068.idm.lab.bos.redhat.com
BaseDN: dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
Continue to configure the system with these values? [no]: y
User authorized to enroll computers: admin
Synchronizing time with KDC...
Unable to sync time with IPA NTP server, assuming the time is in sync.
Password for ad...@idm.lab.bos.redhat.com:
Enrolled in IPA realm IDM.LAB.BOS.REDHAT.COM
Created /etc/ipa/default.conf
Traceback (most recent call last):
File /usr/sbin/ipa-client-install, line 1514, inmodule
sys.exit(main())
File /usr/sbin/ipa-client-install, line 1501, in main
rval = install(options, env, fstore, statestore)
File /usr/sbin/ipa-client-install, line 1326, in install
if configure_sssd_conf(fstore, cli_realm, cli_domain, cli_server,
options):
File /usr/sbin/ipa-client-install, line 711, in configure_sssd_conf
sssdconfig.activate_service('ssh')
File /usr/lib/python2.7/site-packages/SSSDConfig.py, line 1516, in
activate_service
raise NoServiceError
SSSDConfig.NoServiceError
SSSD version: sssd-1.8.1-0.20120228T2018Zgit751b121.fc16.x86_64
Martin
Does your /etc/sssd/sssd.conf and /usr/share/sssd/sssd.api.conf contain
[ssh] section?
sssd.api.conf did contain the ssh section:
# grep -C 3 ssh /usr/share/sssd/sssd.api.conf
# autofs service
autofs_negative_timeout = int, None, false
[ssh]
# ssh service
[provider]
#Available provider types
sssd.conf did not.
Either case, we should not crash but handle the issue in some more
friendly way.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] discussion needed: 0009 Support for IPv6 elements in idnsForwarders attribute
Hello, this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/49 , but I want to discuss one (unimplemented) change: I propose a change in (currently very strange) forwarders syntax. Current syntax: IP[.port] examples: 1.2.3.4 (without optional port) 1.2.3.4.5553 (optional port 5553) A::B (IPv6, without optional port) A::B.5553 :::1.2.3.4 (6to4, without optional port) :::1.2.3.4.5553 (6to4, with optional port 5553) I find this syntax confusing, non-standard and not-typo-proof. IMHO better choice is to follow BIND forwarders syntax: IP [port ip_port] (port is string delimited with spaces) (From: http://www.zytrax.com/books/dns/ch7/queries.html#forwarders) *Current syntax is not documented*, so probably is not used anywhere. (And DNS server on non-standard port is probably useful only for testing purposes, but it's another story.) What is you opinion? -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 968 don't allow reconnection to deleted master
Martin Kosek wrote:
On Tue, 2012-02-28 at 16:36 -0500, Rob Crittenden wrote:
Martin Kosek wrote:
On Sat, 2012-02-25 at 17:43 -0500, Rob Crittenden wrote:
This patch does two things:
1. Prompts when deleting a master to make clear that this is irreversible
2. Does not allow a deleted master to be reconnected.
Reconnecting to a deleted master causes all heck to break loose because
we delete principals as part of deletion process. If you reconnect to a
deleted master then we replicate those deletes and the connected master
is now unusable (no principals).
A simple test is:
Install master
Install replica
ipa-replica-manage del replica
ipa-replica-manage connect replica
ipa-server-uninstall -U on replica
re-install replica
The re-install should be successful.
rob
Generally, it looks and works well. I just miss some unattended way to
deleted a replica, from other script for example.
I think we may either re-use --force flag for this purpose or introduce
an --unattended flag.
I also found an issue with S4U2Proxy memberPrincipal added for each
replica. Since the memberPrincipal values for deleted replica are not
removed when a replica is being deleted, ipa-replica-install reports a
(benign) error when it tries to add a duplicate value afterwards. I
filed a ticket for this one:
https://fedorahosted.org/freeipa/ticket/2451
Martin
OK, went with --force.
rob
The approach should be OK, but the patch you included is wrong.
Martin
OK, this should be right.
rob
From 9ce941b7b18e1e9f9b77562f49e6d49537fd5347 Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Fri, 24 Feb 2012 18:17:47 -0500
Subject: [PATCH] Warn that deleting replica is irreversible, try to detect
reconnection.
Using ipa-replica-manage del replica is irreversible. You can't
turn around and do a connect to it, all heck will break loose. This is
because we clean up all references to the replica when we delete so if
we connect to it again we'll end up deleting all of its principals.
When a connection is deleted then the agreement is removed on both sides.
What isn't removed is the nsDS5ReplicaBindDN so we can use that to
determine if we previously had a connection.
https://fedorahosted.org/freeipa/ticket/2126
---
install/tools/ipa-replica-manage | 23 +++
install/tools/man/ipa-replica-manage.1 |2 +-
2 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index da327e5b976b147721b7c5510041df553b4cdbce..f1f5425ca8d4083e7cb11f8d25f327ffb4ab2520 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -29,6 +29,7 @@ from ipaserver.install import bindinstance
from ipaserver import ipaldap
from ipapython import version
from ipalib import api, errors, util
+from ipalib.dn import DN
from ipapython.ipa_log_manager import *
CACERT = /etc/ipa/ca.crt
@@ -287,6 +288,7 @@ def del_master(realm, hostname, options):
# 3. If an IPA agreement connect to the master to be removed.
repltype = thisrepl.get_agreement_type(hostname)
if repltype == replication.IPA_REPLICA:
+winsync = False
try:
delrepl = replication.ReplicationManager(realm, hostname, options.dirman_passwd)
except Exception, e:
@@ -308,8 +310,17 @@ def del_master(realm, hostname, options):
replica_names = delrepl.find_ipa_replication_agreements()
else:
# WINSYNC replica, delete agreement from current host
+winsync = True
replica_names = [options.host]
+if not winsync and not options.force:
+print Deleting a master is irreversible.
+print To reconnect to the remote master you will need to prepare \
+ a new replica file
+print and re-install.
+if not ipautil.user_input(Continue to delete?, False):
+sys.exit(Deletion aborted)
+
# 4. Remove each agreement
for r in replica_names:
try:
@@ -390,6 +401,18 @@ def add_link(realm, replica1, replica2, dirman_passwd, options):
options.passsync, options.win_subtree,
options.cacert)
else:
+# First see if we already exist on the remote master. If so this was
+# a previously deleted connection.
+try:
+repl2 = replication.ReplicationManager(realm, replica2, dirman_passwd)
+master_dn = repl2.replica_dn()
+binddn = str(DN(('krbprincipalname','ldap/%s@%s' % (replica1, api.env.realm)),(api.env.container_service),(api.env.basedn)))
+master = repl2.conn.getEntry(master_dn, ldap.SCOPE_BASE)
+binddns = master.getValues('nsDS5ReplicaBindDN')
+if binddns and binddn in binddns:
+sys.exit(You cannot connect to a previously deleted master)
+except errors.NotFound:
+pass
Re: [Freeipa-devel] [PATCH] discussion needed: 0009 Support for IPv6 elements in idnsForwarders attribute
And there is the patch, sorry.
Petr^2
On 02/29/2012 03:10 PM, Petr Spacek wrote:
Hello,
this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/49 ,
but I want to discuss one (unimplemented) change:
I propose a change in (currently very strange) forwarders syntax.
Current syntax:
IP[.port]
examples:
1.2.3.4 (without optional port)
1.2.3.4.5553 (optional port 5553)
A::B (IPv6, without optional port)
A::B.5553
:::1.2.3.4 (6to4, without optional port)
:::1.2.3.4.5553 (6to4, with optional port 5553)
I find this syntax confusing, non-standard and not-typo-proof.
IMHO better choice is to follow BIND forwarders syntax:
IP [port ip_port] (port is string delimited with spaces)
(From: http://www.zytrax.com/books/dns/ch7/queries.html#forwarders)
*Current syntax is not documented*, so probably is not used anywhere.
(And DNS server on non-standard port is probably useful only for testing
purposes, but it's another story.)
What is you opinion?
From 215e600a17c51fdb1a4a7fc667a0b62673579797 Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Wed, 29 Feb 2012 14:29:35 +0100
Subject: [PATCH] Add support for IPv6 elements in idnsForwarders attribute
Fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/49
Signed-off-by: Petr Spacek pspa...@redhat.com
---
src/ldap_helper.c | 109 ++--
1 files changed, 71 insertions(+), 38 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index d9e8629..7ebe590 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -777,22 +777,24 @@ cleanup:
/**
- * @brief
+ * @brief Convert IP address from string to sockaddr.
*
- * @param nameserver
- * @param sa
+ * Only AF_INET and AF_INET6 are supported.
*
- * @return
+ * @param[in] nameserver IP address as string
+ * @param[out] ss sockaddr with obtained IP address
+ *
+ * @return ISC_R_SUCCESS, ISC_R_FAILURE (ss untouched)
*/
static isc_result_t
-sockaddr_fromchar(char *nameserver, struct sockaddr *sa)
+sockaddr_fromchar(const char *nameserver, struct sockaddr_storage *ss)
{
isc_result_t result = ISC_R_FAILURE;
struct addrinfo *ai;
struct addrinfo hints;
int res;
- REQUIRE(sa != NULL);
+ REQUIRE(ss != NULL);
memset(hints, 0, sizeof(hints));
hints.ai_flags = AI_NUMERICHOST;
@@ -800,8 +802,11 @@ sockaddr_fromchar(char *nameserver, struct sockaddr *sa)
res = getaddrinfo(nameserver, NULL, hints, ai);
if (res == 0) {
if ((ai-ai_family == AF_INET) || (ai-ai_family == AF_INET6)) {
- memcpy(sa, ai-ai_addr, ai-ai_addrlen);
+ memcpy(ss, ai-ai_addr, ai-ai_addrlen);
result = ISC_R_SUCCESS;
+ } else {
+ log_bug(Unknown ai_family type);
+ return ISC_R_FAMILYNOSUPPORT;
}
freeaddrinfo(ai);
}
@@ -809,29 +814,37 @@ sockaddr_fromchar(char *nameserver, struct sockaddr *sa)
}
/**
- * Parse nameserver IP address with or without
- * port separated with a dot.
+ * Parse nameserver IP address with or without port separated with a dot.
+ *
+ * IPv4 and IPv6 addresses are supported.
+ *
+ * @param[in] token String with IP address and optionally port.
+ * @param[out] ss Socket address storage structure.
*
* @example
- * 192.168.0.100.53 - { address:192.168.0.100, port:53 }
+ * 192.168.0.100 - { address:192.168.0.100, port:53 }
+ * 192.168.0.100.553 - { address:192.168.0.100, port:553 }
+ * 0102:0304:0506:0708:090A:0B0C:0D0E:0FAA -
+ * { address:0102:0304:0506:0708:090A:0B0C:0D0E:0FAA, port:53 }
+ * 0102:0304:0506:0708:090A:0B0C:0D0E:0FAA.553 -
+ * { address:0102:0304:0506:0708:090A:0B0C:0D0E:0FAA, port:553 }
*
- * @param token
- * @param sa Socket address structure.
*/
static isc_result_t
-parse_nameserver(char *token, struct sockaddr *sa)
+parse_nameserver(char *token, struct sockaddr_storage *ss)
{
isc_result_t result = ISC_R_FAILURE;
char *dot;
long number;
REQUIRE(token != NULL);
- REQUIRE(sa != NULL);
+ REQUIRE(ss != NULL);
- result = sockaddr_fromchar(token, sa);
- if (result == ISC_R_SUCCESS)
+ result = sockaddr_fromchar(token, ss);
+ if (result == ISC_R_SUCCESS || result == ISC_R_FAMILYNOSUPPORT)
return result;
+ /* Address parsing failed, try to extract port and retry. */
dot = strrchr(token, '.');
if (dot == NULL)
return ISC_R_FAILURE;
@@ -841,42 +854,51 @@ parse_nameserver(char *token, struct sockaddr *sa)
return ISC_R_FAILURE;
*dot = '\0';
- result = sockaddr_fromchar(token, sa);
+ result = sockaddr_fromchar(token, ss);
*dot = '.'; /* restore value */
if (result == ISC_R_SUCCESS) {
in_port_t port = htons(number);
- switch (sa-sa_family) {
+ switch (ss-ss_family) {
case AF_INET :
- ((struct sockaddr_in *)sa)-sin_port = port;
+ ((struct sockaddr_in *)ss)-sin_port = port;
break;
case AF_INET6 :
- ((struct sockaddr_in6 *)sa)-sin6_port = port;
+ ((struct sockaddr_in6 *)ss)-sin6_port = port;
break;
default:
- log_bug(Unknown sa_family type);
- return ISC_R_FAILURE;
+ log_bug(Unknown ss_family type);
+
Re: [Freeipa-devel] [PATCH] 972 fix migration
On Tue, 2012-02-28 at 17:36 -0500, Rob Crittenden wrote: We were setting the GID of migrated users to that of the default user's group (ipausers) when it should have been the same as the UID unless UPG was disabled. This does the right thing and fixes migration which was broken when we made ipausers a non-posix group. rob NACK This is a good start, but you missed a case when UPGs are disabled. We crash in that case: # ipa-managed-entries -e 'UPG Definition' disable Disabling Plugin # ipa migrate-ds --user-container=ou=People --group-container=ou=Groups ldap://vm-054.idm.lab.bos.redhat.com --bind-dn=cn=Directory Manager Password: ipa: ERROR: an internal error has occurred /var/log/httpd/error_log: [Wed Feb 29 09:15:36 2012] [error] ipa: ERROR: non-public: KeyError: 'gidnumber' [Wed Feb 29 09:15:36 2012] [error] Traceback (most recent call last): [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipaserver/rpcserver.py, line 314, in wsgi_execute [Wed Feb 29 09:15:36 2012] [error] result = self.Command[name](*args, **options) [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line 443, in __call__ [Wed Feb 29 09:15:36 2012] [error] ret = self.run(*args, **options) [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line 721, in run [Wed Feb 29 09:15:36 2012] [error] return self.execute(*args, **options) [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 667, in execute [Wed Feb 29 09:15:36 2012] [error] ldap, config, ds_ldap, ds_base_dn, options [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 605, in migrate [Wed Feb 29 09:15:36 2012] [error] **blacklists [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 125, in _pre_migrate_user [Wed Feb 29 09:15:36 2012] [error] ctx['def_group_gid'] = g_attrs['gidnumber'][0] [Wed Feb 29 09:15:36 2012] [error] KeyError: 'gidnumber' [Wed Feb 29 09:15:36 2012] [error] ipa: INFO: ad...@idm.lab.bos.redhat.com: migrate_ds(u'ldap://vm-054.idm.lab.bos.redhat.com', u'', binddn=u'cn=Directory Manager', usercontainer=u'ou=People', groupcontainer=u'ou=Groups', userobjectclass=(u'person',), groupobjectclass=(u'groupOfUniqueNames',u'groupOfNames'), userignoreobjectclass=None, userignoreattribute=None, groupignoreobjectclass=None, groupignoreattribute=None, groupoverwritegid=False, schema=u'RFC2307bis', continue=False, exclude_groups=None, exclude_users=None): KeyError Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 936 support defaultNamingContext and basedn in migration
On Mon, 2012-01-30 at 17:16 -0500, Rob Crittenden wrote: Add support for defaultNamingContext which is available in 389-ds 1.2.10-0.9.a8. If the attribute isn't returned continue to use namingContexts to determine the basedn. While I was in poking at this I added support to the migration plugin to be able to pass in the basedn of the remote server. rob ACK. I tested client enrollments and various migration scenarios against DS with multiple suffixes and everything worked pretty well. I just needed to apply your patch 972 to make migration working in current IPA with non-posix ipausers. Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 971 detect binary LDAP data
Jan Cholasta wrote: On 28.2.2012 18:58, Rob Crittenden wrote: Jan Cholasta wrote: On 28.2.2012 18:02, Petr Viktorin wrote: On 02/28/2012 04:45 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/28/2012 04:02 AM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/27/2012 05:10 PM, Rob Crittenden wrote: Rob Crittenden wrote: Simo Sorce wrote: On Mon, 2012-02-27 at 09:44 -0500, Rob Crittenden wrote: We are pretty trusting that the data coming out of LDAP matches its schema but it is possible to stuff non-printable characters into most attributes. I've added a sanity checker to keep a value as a python str type (treated as binary internally). This will result in a base64 encoded blob be returned to the client. I don't like the idea of having arbitrary binary data where unicode strings are expected. It might cause some unexpected errors (I have a feeling that --addattr and/or --delattr and possibly some plugins might not handle this very well). Wouldn't it be better to just throw away the value if it's invalid and warn the user? This isn't for user input, it is for data stored in LDAP. User's are going to have no way to provide binary data to us unless they use the API themselves in which case they have to follow our rules. Well my point was that --addattr and --delattr cause an LDAP search for the given attribute and plugins might get the result of a LDAP search in their post_callback and I'm not sure if they can cope with binary data. It wouldn't be any different than if we had the value as a unicode. We treat the python type str as binary data. Anything that is a str gets based64 encoded before json or xml-rpc transmission. The type unicode is considered a string and goes in the clear. We determine what this type should be not from the data but from the schema. This is a big assumption. Hopefully this answer's Petr's point as well. We decided long ago that str means Binary and unicode means String. It is a bit clumsy perhaps python handles it well. It will be more clear when we switch to Python 3.0 and we'll have bytes and str instead as types. We are trusting that the data in LDAP matches its schema. This is just belt and suspenders verifying that it is the case. Sure, but I still think we should allow any valid unicode data to come from LDAP, not just what is valid in XML-RPC. This won't affect data coming out of LDAP, only the way it is transmitted to the client. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0016-17 Fixes for{add, set, del}attr with managed attributes
Petr Viktorin wrote:
On 02/27/2012 11:03 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
Patch 16 defers validation conversion until after {add,del,set}attr is
processed, so that we don't search for an integer in a list of strings
(this caused ticket #2405), and so that the end result of these
operations is validated (#2407).
Patch 17 makes these options honor params marked no_create and
no_update.
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
NACK on Patch 17 which breaks patch 16.
How is patch 16 broken? It works for me.
My point is they rely on one another, IMHO, so without 17 the reported
problem still exists.
*attr is specifically made to be powerful. We don't want to arbitrarily
block updating certain values.
Noted
Not having patch 17 means that the problem reported in 2408 still
occurs. It should probably check both the schema and the param to
determine if something can have multiple values and reject that way.
I see the problem now: the certificate subject base is defined as a
multi-value attribute in the LDAP schema. If it's changed to
single-value the existing validation should catch it.
Also, most of the config attributes should probably be required in the
schema. Am I right?
I'm a newbie here; what do I need to do when changing the schema? Is
there a patch that does something similar I could use as an example?
The framework should be able to impose its own single-value will as
well. If a Param is designated as single-value the *attr should honor it.
Updating schema is a bit of a nasty business right now. See
10-selinuxusermap.update for an example.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options
Petr Viktorin wrote: On 02/29/2012 11:14 AM, Jan Cholasta wrote: On 29.2.2012 11:09, Petr Viktorin wrote: On 02/28/2012 03:19 PM, Jan Cholasta wrote: On 28.2.2012 11:54, Petr Viktorin wrote: On 02/27/2012 10:44 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/20/2012 08:51 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2159 says various config options are not marked Required, so entering an empty value for it will pass validation (and IPA will blow up later when it expects a string,not None). Forexample the following: $ ipa config-mod --groupsearch= fails with AttributeError: 'NoneType' object has no attribute 'split' There is a more general problem behind this, though: even if the attributes *are* marked as Required, an empty string will pass validation. This is because `None` is used in `Param.validate` to mean both No value supplied and Empty value supplied. The method currently assumes the former, and skips validation entirely for `None` values to optional parameters. For example, the following will delete membergroup, even though it's a required attribute : $ ipa delegation-add --attrs=street --group=editors \ --membergroup=admins td1 $ ipa delegation-mod --membergroup= td1 Note that some LDAPObjects handle this with a _check_empty_attrs function, so they aren't affected. That function is specific to LADP objects, though. So I needed to tackle this on a lower level. This patch solves the problem by * adding a 'nonempty' flag when a required parameter of a CRUD Update object is auto-converted to a non-required parameter * making the`validate` method aware of whether the parameter was supplied; and if it was, honor the nonempty flag. The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by marking required config options as required. This looks good but I think there are other things to protect in config as well such as the default e-mail domain. It is probably safe to say that everything in there is required. rob Let me just double-check this with you. According to code in the user plugin (around line 330), if the default e-mail domain is not set, users don't get an address auto-assigned. Do we really want to require user e-mails? ipaconfigstring (the password plugin flags) are a set (multivalue, not required). The rest of the values I left as not required are for optional features or limits: search results time limit, max. username length, password expiry notification. Currently if these are missing, the feature/limit is disabled (well, except for the time limit). But, there are also special values (0 or -1) that have the same effect as a missing value. Sometimes they're documented. So we want to enforce that users use these special values instead of removing the config entry? I think we want to enforce that these are defined. It will be confusing for users if these are not there at all. I don't think we need to show the special options, just declare that the attribute is required. rob Attaching updated patch 13. Only the default e-mail domain (https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are still optional. You have removed all the config-related defensive code in the patch, is this a good idea? What will happen if someone e.g. deletes a required config attribute directly from LDAP? Then IPA crashes. The defensive code wasn't there for all cases anyway, as ticket #2159 shows. If we want to protect against this it would probably be better to make the config class itself give the default when a required value is missing. This, and raise an error in cases where no default is available (the check should probably be done in ldap.get_ipa_config). Honza Would a better approach be to modify the LDAP schema to require these values? I think that may be a longer-term fix. I propose we keep the defensive code in for now and correct it in the future. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 12 When migrating warn user if compat is enabled
On 02/28/2012 10:52 PM, Rob Crittenden wrote:
Ondrej Hamada wrote:
On 02/27/2012 09:47 PM, Rob Crittenden wrote:
Ondrej Hamada wrote:
On 02/21/2012 02:32 PM, Ondrej Hamada wrote:
On 02/20/2012 06:53 PM, Rob Crittenden wrote:
Ondrej Hamada wrote:
https://fedorahosted.org/freeipa/ticket/2274
Added check into migration plugin to warn user when compat is
enabled.
If compat is enabled, the migration fails and user is warned
that he
must turn the compat off or run the script with (the newly
introduced)
option '--compat'.
'--compat' is just a flag, by default set to false. If it is
set, the
compat check is skipped.
Interesting approach. I think this is probably good, preventing
migration when the compat plugin is enabled unless you specifically
decide to.
I think the option may need another name, maybe --with-compat or
something.
I think in the message we should use enabled instead of on. That
is the language of ipa-compat-manage.
The migration help should have a discussion of why this is a problem
too, and what compat really is (provides a different view of the
data
to be compatible with non RFC2703bis systems).
rob
corrected
Ondra
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
I forget to update the commit message about the change of flag name.
Corrected patch attached.
This works ok it just seems to be making an assumption on the client
when to print this. I think a similar value like enabled needs to be
created to explicitly say why we are returning.
rob
sorry for that, value created
Ondra
I think you need to define beter what compat means in the output, it
coudl be very confusing. You can return a value for it without testing
whether it is actually a problem or not.
I think what compat is supposed to mean is Am I failing because of
compat and not an indication of whether compat is enabled or not.
Some documentation at a minimum should be added.
It otherwise seems to work ok.
rob
You could return a value for compat here without
I've updated the description of 'compat' value in output and also
changed the condition when this value is set to False. Now it is set to
False only when the migration fails because of compatibility plugin.
--
Regards,
Ondrej Hamada
FreeIPA team
jabber: oh...@jabbim.cz
IRC: ohamada
From f88df9859c1ea7a04a63b3c9d18d561c8aeee75d Mon Sep 17 00:00:00 2001
From: Ondrej Hamada oham...@redhat.com
Date: Wed, 29 Feb 2012 15:21:24 +0100
Subject: [PATCH] Migration warning when compat enabled
Added check into migration plugin to warn user when compat is enabled.
If compat is enabled, the migration fails and user is warned that he
must turn the compat off or run the script with (the newly introduced)
option '--with-compat'.
'--with-compat' is new flag. If it is set, the compat status is ignored.
https://fedorahosted.org/freeipa/ticket/2274
---
API.txt |4 +++-
VERSION |2 +-
ipalib/plugins/migration.py | 34 --
3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/API.txt b/API.txt
index 548fc93d48128aab5cebd60dda7fd304b569785b..a44e391e2ab79cb566455def3299ed25714e 100644
--- a/API.txt
+++ b/API.txt
@@ -1893,7 +1893,7 @@ output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('value', type 'unicode', None)
command: migrate_ds
-args: 2,14,3
+args: 2,15,4
arg: Str('ldapuri', cli_name='ldap_uri')
arg: Password('bindpw', cli_name='password', confirm=False)
option: Str('binddn?', autofill=True, cli_name='bind_dn', default=u'cn=directory manager')
@@ -1908,11 +1908,13 @@ option: Str('groupignoreattribute*', autofill=True, cli_name='group_ignore_attri
option: Flag('groupoverwritegid', autofill=True, cli_name='group_overwrite_gid', default=False)
option: StrEnum('schema?', autofill=True, cli_name='schema', default=u'RFC2307bis', values=(u'RFC2307bis', u'RFC2307'))
option: Flag('continue?', autofill=True, default=False)
+option: Flag('compat?', autofill=True, cli_name='with_compat', default=False)
option: Str('exclude_groups*', autofill=True, cli_name='exclude_groups', csv=True, default=())
option: Str('exclude_users*', autofill=True, cli_name='exclude_users', csv=True, default=())
output: Output('result', type 'dict', None)
output: Output('failed', type 'dict', None)
output: Output('enabled', type 'bool', None)
+output: Output('compat', type 'bool', None)
command: netgroup_add
args: 1,9,3
arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]+$', pattern_errmsg='may only include letters, numbers, _, -, and .', primary_key=True, required=True)
diff --git a/VERSION b/VERSION
index
[Freeipa-devel] [PATCH] 096 Fixed content type check in login_password
login_password is expecting that request content_type will be
'application/x-www-form-urlencoded'.
Current check is an equality check of content_type http header.
RFC 3875 defines that content type can contain parameters separated by
';'. For example: when firefox is doing ajax call it sets the request
header to 'application/x-www-form-urlencoded; charset=UTF-8' which leads
to negative result.
This patch makes the check more benevolent to allow such values.
Patch is a fix-up for:
https://fedorahosted.org/freeipa/ticket/2095
--
Petr Vobornik
From aabee55ec63b119a5556677508ae4d2e2b9daac4 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 29 Feb 2012 15:25:40 +0100
Subject: [PATCH] Fixed content type check in login_password
login_password is expecting that request content_type will be 'application/x-www-form-urlencoded'.
Current check is an equality check of content_type http header.
RFC 3875 defines that content type can contain parameters separated by ';'. For example: when firefox is doing ajax call it sets the request header to 'application/x-www-form-urlencoded; charset=UTF-8' which leads to negative result.
This patch makes the check more benevolent to allow such values.
Patch is a fixup for:
https://fedorahosted.org/freeipa/ticket/2095
---
ipaserver/rpcserver.py |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py
index c383f0482171e264c379aa594568f036feafe915..3ada8b48ff2ed16bf9d935c6a6f87539e2f1d9db 100644
--- a/ipaserver/rpcserver.py
+++ b/ipaserver/rpcserver.py
@@ -894,7 +894,7 @@ class login_password(Backend, KerberosSession, HTTP_Status):
# Get the user and password parameters from the request
content_type = environ.get('CONTENT_TYPE', '').lower()
-if content_type != 'application/x-www-form-urlencoded':
+if not content_type.startswith('application/x-www-form-urlencoded'):
return self.bad_request(environ, start_response, Content-Type must be application/x-www-form-urlencoded)
method = environ.get('REQUEST_METHOD', '').upper()
--
1.7.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 973 fix nested netgroups in nis
On Tue, 2012-02-28 at 22:13 -0500, Rob Crittenden wrote: The wrong attribute was being used to handle nested netgroup membership in slapi-nis. Nalin worked this out for us (thanks). This patch should fix both new installs and upgrades. See the ticket and bug for testing information. rob Works for me, ACK. ypcat now shows nested netgroups correctly. Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 097 Added logout button
Logout button was added to Web UI.
A click on logout button executes session_logout command. If command
succeeds or xhr stutus is 401 (unauthorized - already logged out) page
is redirected to logout.html.
logout.html is a simple page with You have been logged out text and a
link to return back to main page.
https://fedorahosted.org/freeipa/ticket/2363
--
Petr Vobornik
From a99a2deaa45a9d400f7a15acee11d4e5e3e2a384 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Voborn=C3=ADk?= pvobo...@redhat.com
Date: Fri, 24 Feb 2012 15:31:55 +0100
Subject: [PATCH] Added logout button
Logout button was added to Web UI.
Click on logout button executes session_logout command. If command succeeds or xhr stutus is 401 (unauthorized - already logged out) page is redirected to logout.html.
logout.html is a simple page with You have been logged out text and a link to return back to main page.
https://fedorahosted.org/freeipa/ticket/2363
---
freeipa.spec.in |4 ++
install/ui/Makefile.am |1 +
install/ui/index.html|9 --
install/ui/ipa.js| 46 +-
install/ui/logout.html | 30 +++
install/ui/test/data/ipa_init.json |4 ++-
install/ui/test/data/session_logout.json |7
install/ui/webui.js |5 +++
ipalib/plugins/internal.py |4 ++-
9 files changed, 104 insertions(+), 6 deletions(-)
create mode 100644 install/ui/logout.html
create mode 100644 install/ui/test/data/session_logout.json
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 47d0f281fe97ca564269e9eea7e0f28d1e713d88..44e4828643da8ee4f4f715ffe4b7302e6c0b14f7 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -551,6 +551,7 @@ fi
%{_usr}/share/ipa/migration/migration.py*
%dir %{_usr}/share/ipa/ui
%{_usr}/share/ipa/ui/index.html
+%{_usr}/share/ipa/ui/logout.html
%{_usr}/share/ipa/ui/*.ico
%{_usr}/share/ipa/ui/*.css
%{_usr}/share/ipa/ui/*.js
@@ -667,6 +668,9 @@ fi
%ghost %attr(0644,root,apache) %config(noreplace) %{_sysconfdir}/ipa/ca.crt
%changelog
+* Wed Feb 29 2012 Petr Vobornik pvobo...@redhat.com - 2.2.0-13
+- Add Web UI logout page
+
* Mon Feb 27 2012 Rob Crittenden rcrit...@redhat.com - 2.2.0-12
- Add Requires to ipa-client on oddjob-mkhomedir
diff --git a/install/ui/Makefile.am b/install/ui/Makefile.am
index d87a0944ca47e1885a9a2781e6ed03e30c662fe6..a4d083ac029446747559c63b0e039790620afb6d 100644
--- a/install/ui/Makefile.am
+++ b/install/ui/Makefile.am
@@ -38,6 +38,7 @@ app_DATA =\
jquery.js \
jquery.ordered-map.js \
json2.js \
+ logout.html \
navigation.js \
net.js\
netgroup.js \
diff --git a/install/ui/index.html b/install/ui/index.html
index db314331ab515b41ea17a1f7550c7444941bedd4..6b1be869bbff53fcd8aead7519f7ed085304b4dc 100644
--- a/install/ui/index.html
+++ b/install/ui/index.html
@@ -66,14 +66,17 @@
div id=header
span class=header-logo
- a href=#img src=images/ipa-logo.png /img src=images/ipa-banner.png //a
+a href=#img src=images/ipa-logo.png /img src=images/ipa-banner.png //a
/span
span class=header-right
span id=loggedinas class=header-loggedinas
- a href=#span id=login_headerLogged in as/span: strongu...@freeipa.org/strong/a
+a href=#span id=login_headerLogged in as/span: strongu...@freeipa.org/strong/a
+/span
+span class=header-loggedinas
+| a href=#logout id=logoutLogout/a
/span
span id=header-network-activity-indicator class=network-activity-indicator
- img src=images/spinner-header.gif /
+img src=images/spinner-header.gif /
/span
/span
/div
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index d1bb04210071c6651a8fc64f6d3b57d9ab30b752..0afa4f0970538f517d0d52d066178d1ade9b781a 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -295,7 +295,6 @@ IPA.get_credentials = function() {
status = xhr.status;
}
-
function success_handler(data, text_status, xhr) {
status = xhr.status;
}
@@ -313,6 +312,51 @@ IPA.get_credentials = function() {
return status;
};
+IPA.logout = function() {
+
+function show_error(message) {
+var dialog = IPA.message_dialog({
+message: message,
+title: IPA.messages.login.logout_error
+});
+dialog.open();
+}
+
+function redirect () {
+window.location = 'logout.html';
+}
+
+function success_handler(data, text_status, xhr) {
+if (data data.error) {
+show_error(data.error.message);
+} else {
+redirect();
+}
+}
+
+function error_handler(xhr, text_status, error_thrown) {
+
[Freeipa-devel] [PATCH] 098 Forms based authentication UI
Support for forms based authentication was added to UI.
It consist of:
1) new login page
Page url is [ipa server]/ipa/ui/login.html
Page contains a login form. For authentication it sends ajax request at
[ipa server]/session/json/login_password. If authentication is
successfull page is redirected to [ipa server]/ipa/ui if it fails from
whatever reason a message is shown.
2) new enhanced error dialog - authorization_dialog.
This dialog is displayed when user is not authorized to perform action -
usually when ticket and session expires.
It is a standard error dialog which shows kerberos ticket related error
message and newly offers (as a link) to use form based authentication.
If user click on the link, the dialog content and buttons switch to
login dialog which has same functionality as 'new login page'. User is
able to return back to the error message by clicking on a back button.
login.html uses same css styles as migration page - ipa-migration.css
was merged into ipa.css.
https://fedorahosted.org/freeipa/ticket/2450
Theoretically the login.html is not needed.
Sometime later we should come up with a method how to i18n static pages
and main page prior to authentication.
--
Petr Vobornik
From 17c4675f36c684563a259610a02e65c56e21aea6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Voborn=C3=ADk?= pvobo...@redhat.com
Date: Mon, 27 Feb 2012 15:31:20 +0100
Subject: [PATCH] Forms based authentication UI
Support for forms based authentication was added to UI.
It consist of:
1) new login page
Page url is [ipa server]/ipa/ui/login.html
Page contains a login form. For authentication it sends ajax request at [ipa server]/session/json/login_password. If authentication is successfull page is redirected to [ipa server]/ipa/ui if it fails from whatever reason a message is shown.
2) new enhanced error dialog - authorization_dialog.
This dialog is displayed when user is not authorized to perform action - usually when ticket and session expires.
It is a standard error dialog which shows kerberos ticket related error message and newly offers (as a link) to use form based authentication. If user click on the link, the dialog content and buttons switch to login dialog which has same functionality as 'new login page'. User is able to return back to the error message by clicking on a back button.
login.html uses same css styles as migration page - ipa-migration.css was merged into ipa.css.
https://fedorahosted.org/freeipa/ticket/2450
---
freeipa.spec.in |7 +-
install/migration/Makefile.am |1 -
install/migration/error.html|1 -
install/migration/index.html|1 -
install/migration/invalid.html |5 +-
install/migration/ipa_migration.css | 96
install/ui/Makefile.am |2 +
install/ui/dialog.js| 39 --
install/ui/field.js |6 +-
install/ui/ipa.css | 99 -
install/ui/ipa.js | 287 ++-
install/ui/jsl.conf |1 +
install/ui/login.html | 51 ++
install/ui/login.js | 76 +
install/ui/test/data/ipa_init.json |8 +-
install/ui/widget.js|1 -
ipalib/plugins/internal.py |6 +
17 files changed, 527 insertions(+), 160 deletions(-)
delete mode 100644 install/migration/ipa_migration.css
create mode 100644 install/ui/login.html
create mode 100644 install/ui/login.js
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 44e4828643da8ee4f4f715ffe4b7302e6c0b14f7..5047db938cbbd4728c90fa0d321a6bc8bb0e19ae 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -547,10 +547,10 @@ fi
%{_usr}/share/ipa/migration/error.html
%{_usr}/share/ipa/migration/index.html
%{_usr}/share/ipa/migration/invalid.html
-%{_usr}/share/ipa/migration/ipa_migration.css
%{_usr}/share/ipa/migration/migration.py*
%dir %{_usr}/share/ipa/ui
%{_usr}/share/ipa/ui/index.html
+%{_usr}/share/ipa/ui/login.html
%{_usr}/share/ipa/ui/logout.html
%{_usr}/share/ipa/ui/*.ico
%{_usr}/share/ipa/ui/*.css
@@ -668,6 +668,11 @@ fi
%ghost %attr(0644,root,apache) %config(noreplace) %{_sysconfdir}/ipa/ca.crt
%changelog
+
+* Wed Feb 29 2012 Petr Vobornik pvobo...@redhat.com - 2.2.0-14
+- Add Web UI form based login page
+- Removed ipa_migration.css
+
* Wed Feb 29 2012 Petr Vobornik pvobo...@redhat.com - 2.2.0-13
- Add Web UI logout page
diff --git a/install/migration/Makefile.am b/install/migration/Makefile.am
index aa571364084872a88eaea2410506e1c432e747af..b90578015060c65be8e2d60bf486fd2606394873 100644
--- a/install/migration/Makefile.am
+++ b/install/migration/Makefile.am
@@ -5,7 +5,6 @@ app_DATA = \
error.html \
index.html \
invalid.html \
- ipa_migration.css \
migration.py \
$(NULL)
diff --git a/install/migration/error.html b/install/migration/error.html
index
Re: [Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options
On 02/29/2012 03:53 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/29/2012 11:14 AM, Jan Cholasta wrote:
On 29.2.2012 11:09, Petr Viktorin wrote:
On 02/28/2012 03:19 PM, Jan Cholasta wrote:
On 28.2.2012 11:54, Petr Viktorin wrote:
On 02/27/2012 10:44 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/20/2012 08:51 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/2159 says various config
options
are not marked Required, so entering an empty value for it will
pass
validation (and IPA will blow up later when it expects a
string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute
'split'
There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to
mean
both No value supplied and Empty value supplied. The method
currently assumes the former, and skips validation entirely for
`None`
values to optional parameters.
For example, the following will delete membergroup, even though
it's a
required attribute :
$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1
Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to
LADP
objects, though. So I needed to tackle this on a lower level.
This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD
Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the nonempty flag.
The second patch fixes
https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.
This looks good but I think there are other things to protect in
config
as well such as the default e-mail domain. It is probably safe to
say
that everything in there is required.
rob
Let me just double-check this with you.
According to code in the user plugin (around line 330), if the
default
e-mail domain is not set, users don't get an address
auto-assigned. Do
we really want to require user e-mails?
ipaconfigstring (the password plugin flags) are a set (multivalue,
not
required).
The rest of the values I left as not required are for optional
features
or limits: search results time limit, max. username length,
password
expiry notification. Currently if these are missing, the
feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same
effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values
instead of
removing the config entry?
I think we want to enforce that these are defined. It will be
confusing
for users if these are not there at all. I don't think we need to
show
the special options, just declare that the attribute is required.
rob
Attaching updated patch 13.
Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring
are
still optional.
You have removed all the config-related defensive code in the
patch, is
this a good idea? What will happen if someone e.g. deletes a required
config attribute directly from LDAP?
Then IPA crashes. The defensive code wasn't there for all cases anyway,
as ticket #2159 shows.
If we want to protect against this it would probably be better to make
the config class itself give the default when a required value is
missing.
This, and raise an error in cases where no default is available (the
check should probably be done in ldap.get_ipa_config).
Honza
Would a better approach be to modify the LDAP schema to require these
values?
I think that may be a longer-term fix. I propose we keep the defensive
code in for now and correct it in the future.
rob
Here is an updated patch 13 that does that.
--
Petr³
From fc0261471398999816581765ce28004a068cdfa2 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Mon, 20 Feb 2012 04:03:27 -0500
Subject: [PATCH] Mark most config options as required
IPA assumes most config options are present, but allowed the user
to delete them. This patch marks them as required.
https://fedorahosted.org/freeipa/ticket/2159
---
ipalib/plugins/config.py | 30 +++---
1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/ipalib/plugins/config.py b/ipalib/plugins/config.py
index c4615e3d1843a848e65090d64fd50fa833d81220..df960f4c0117e453ffb79ae7469476b5ff234f0d 100644
--- a/ipalib/plugins/config.py
+++ b/ipalib/plugins/config.py
@@ -97,22 +97,22 @@ class config(LDAPObject):
label_singular = _('Configuration')
takes_params = (
-Int('ipamaxusernamelength?',
+Int('ipamaxusernamelength',
cli_name='maxusername',
[Freeipa-devel] More types of replica in FreeIPA
Hi everyone, I'm currently working on my thesis. It's objective is $SUBJ and we already have ticket for that: #194 https://fedorahosted.org/freeipa/ticket/194. The task is to create two more replica types - the HUB and Consumer. In 389-DS both the HUB and Consumer are read-only. Additionally the HUB can push the data to the Consumers. In case of FreeIPA the server is not only providing data, but also services like CA, NTP, DNS, Kerberos. Therefore I'm kindly asking you for advices and opinions on that: 1. What should be the position of HUB? I mean should it be used as an interconnection between Masters and Consumers only, so that it will be 'hidden' in the topology and only forwards the updates, or should the HUB be also used as a regular Consumer which has additional ability to push the updates further to Consumers/HUBS? 2. Which services should be available on HUB and Consumer? I think, the priority of these replicas would be to answer to data request by ipa whatever-(find|show) commands or to provide some LDAP data for email addressing etc. Also it shouldn't cause much trouble to run NTP on Consumer, but what about Kerberos or CA? Is it a good solution to let users authenticate against these replicas? Is it correct to leave classified data like passwords on these replicas? Thanks in advance for your reactions Ondra -- Regards, Ondrej Hamada FreeIPA team jabber:oh...@jabbim.cz IRC: ohamada ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 968 don't allow reconnection to deleted master
On Wed, 2012-02-29 at 09:13 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-02-28 at 16:36 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Sat, 2012-02-25 at 17:43 -0500, Rob Crittenden wrote: This patch does two things: 1. Prompts when deleting a master to make clear that this is irreversible 2. Does not allow a deleted master to be reconnected. Reconnecting to a deleted master causes all heck to break loose because we delete principals as part of deletion process. If you reconnect to a deleted master then we replicate those deletes and the connected master is now unusable (no principals). A simple test is: Install master Install replica ipa-replica-manage del replica ipa-replica-manage connect replica ipa-server-uninstall -U on replica re-install replica The re-install should be successful. rob Generally, it looks and works well. I just miss some unattended way to deleted a replica, from other script for example. I think we may either re-use --force flag for this purpose or introduce an --unattended flag. I also found an issue with S4U2Proxy memberPrincipal added for each replica. Since the memberPrincipal values for deleted replica are not removed when a replica is being deleted, ipa-replica-install reports a (benign) error when it tries to add a duplicate value afterwards. I filed a ticket for this one: https://fedorahosted.org/freeipa/ticket/2451 Martin OK, went with --force. rob The approach should be OK, but the patch you included is wrong. Martin OK, this should be right. rob Yup, that's better. ACK. Pushed to master, ipa-2-2. I raised Affects Tests flag in Trac, --force flag need to be added to ipa-replica-manage del $REPLICA tests. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Adding Debian support to the FreeIPA code
On Tue, 2012-02-28 at 23:45 +0200, Alexander Bokovoy wrote: On Tue, 28 Feb 2012, Krzysztof Klimonda wrote: - __setup_autoconfig modifies files in /usr/share/ and that seems to be non-compliant with FHS. It may slip through checks at first but I'd expect people reporting bugs at some point. I'll comment on this one as we'll need to get away from modifying files in /usr/share in Fedora as well by Fedora 18 or 19 according to the upcoming changes to simplify file system layout. So I'd rather move those /usr/share/ipa files to /var/lib/ipa/DOMAIN like PKI setup does, also during ipa-server-install time. That means it will be one less difference soon as well. Yeah I've been wanting to do that for a while too, but it always got deferred. Do we have a ticket that tracks this? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] discussion needed: 0009 Support for IPv6 elements in idnsForwarders attribute
Either way looks ok to me. I agree that using a space may be less confusing if this syntax never allows to specify multiple addresses. If multiple address can be specified than it may be less ideal to use spaces. Simo. On Wed, 2012-02-29 at 15:14 +0100, Petr Spacek wrote: And there is the patch, sorry. Petr^2 On 02/29/2012 03:10 PM, Petr Spacek wrote: Hello, this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/49 , but I want to discuss one (unimplemented) change: I propose a change in (currently very strange) forwarders syntax. Current syntax: IP[.port] examples: 1.2.3.4 (without optional port) 1.2.3.4.5553 (optional port 5553) A::B (IPv6, without optional port) A::B.5553 :::1.2.3.4 (6to4, without optional port) :::1.2.3.4.5553 (6to4, with optional port 5553) I find this syntax confusing, non-standard and not-typo-proof. IMHO better choice is to follow BIND forwarders syntax: IP [port ip_port] (port is string delimited with spaces) (From: http://www.zytrax.com/books/dns/ch7/queries.html#forwarders) *Current syntax is not documented*, so probably is not used anywhere. (And DNS server on non-standard port is probably useful only for testing purposes, but it's another story.) What is you opinion? ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 972 fix migration
Martin Kosek wrote:
On Tue, 2012-02-28 at 17:36 -0500, Rob Crittenden wrote:
We were setting the GID of migrated users to that of the default user's
group (ipausers) when it should have been the same as the UID unless UPG
was disabled.
This does the right thing and fixes migration which was broken when we
made ipausers a non-posix group.
rob
NACK
This is a good start, but you missed a case when UPGs are disabled. We
crash in that case:
# ipa-managed-entries -e 'UPG Definition' disable
Disabling Plugin
# ipa migrate-ds --user-container=ou=People --group-container=ou=Groups
ldap://vm-054.idm.lab.bos.redhat.com --bind-dn=cn=Directory Manager
Password:
ipa: ERROR: an internal error has occurred
/var/log/httpd/error_log:
[Wed Feb 29 09:15:36 2012] [error] ipa: ERROR: non-public: KeyError: 'gidnumber'
[Wed Feb 29 09:15:36 2012] [error] Traceback (most recent call last):
[Wed Feb 29 09:15:36 2012] [error] File
/usr/lib/python2.7/site-packages/ipaserver/rpcserver.py, line 314, in
wsgi_execute
[Wed Feb 29 09:15:36 2012] [error] result = self.Command[name](*args,
**options)
[Wed Feb 29 09:15:36 2012] [error] File
/usr/lib/python2.7/site-packages/ipalib/frontend.py, line 443, in __call__
[Wed Feb 29 09:15:36 2012] [error] ret = self.run(*args, **options)
[Wed Feb 29 09:15:36 2012] [error] File
/usr/lib/python2.7/site-packages/ipalib/frontend.py, line 721, in run
[Wed Feb 29 09:15:36 2012] [error] return self.execute(*args, **options)
[Wed Feb 29 09:15:36 2012] [error] File
/usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 667, in
execute
[Wed Feb 29 09:15:36 2012] [error] ldap, config, ds_ldap, ds_base_dn,
options
[Wed Feb 29 09:15:36 2012] [error] File
/usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 605, in
migrate
[Wed Feb 29 09:15:36 2012] [error] **blacklists
[Wed Feb 29 09:15:36 2012] [error] File
/usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 125, in
_pre_migrate_user
[Wed Feb 29 09:15:36 2012] [error] ctx['def_group_gid'] =
g_attrs['gidnumber'][0]
[Wed Feb 29 09:15:36 2012] [error] KeyError: 'gidnumber'
[Wed Feb 29 09:15:36 2012] [error] ipa: INFO: ad...@idm.lab.bos.redhat.com:
migrate_ds(u'ldap://vm-054.idm.lab.bos.redhat.com', u'',
binddn=u'cn=Directory Manager', usercontainer=u'ou=People',
groupcontainer=u'ou=Groups', userobjectclass=(u'person',),
groupobjectclass=(u'groupOfUniqueNames',u'groupOfNames'),
userignoreobjectclass=None, userignoreattribute=None,
groupignoreobjectclass=None, groupignoreattribute=None,
groupoverwritegid=False, schema=u'RFC2307bis', continue=False,
exclude_groups=None, exclude_users=None): KeyError
Martin
Updated. Will now report an error if the default group is not POSIX and
UPG is disabled.
rob
From 5dc9c28a5dbaa5595ccce0567574b088b54c8f46 Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Tue, 28 Feb 2012 17:34:14 -0500
Subject: [PATCH] Don't set migrated user's GID to that of default users
group.
The GID should be the UID unless UPG is disabled.
https://fedorahosted.org/freeipa/ticket/2430
---
ipalib/plugins/migration.py | 11 ---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index ab4e523e5b505577f83be4f95724bd9a9a50f8b6..a3724abd650a5e098b987798fe259e1149a434bb 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -126,9 +126,13 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs
try:
(g_dn, g_attrs) = ldap.get_entry(ctx['def_group_dn'], ['gidnumber'])
except errors.NotFound:
-error_msg = 'Default group for new users not found.'
+error_msg = _('Default group for new users not found.')
raise errors.NotFound(reason=error_msg)
-ctx['def_group_gid'] = g_attrs['gidnumber'][0]
+if not ldap.has_upg():
+if 'gidnumber' in g_attrs:
+ctx['def_group_gid'] = g_attrs['gidnumber'][0]
+else:
+raise errors.NotFound(reason=_('User Private Groups are disabled and the default users group is not POSIX'))
# fill in required attributes by IPA
entry_attrs['ipauniqueid'] = 'autogenerate'
@@ -137,7 +141,8 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs
home_dir = '%s/%s' % (homes_root, pkey)
home_dir = home_dir.replace('//', '/').rstrip('/')
entry_attrs['homedirectory'] = home_dir
-entry_attrs.setdefault('gidnumber', ctx['def_group_gid'])
+if 'def_group_gid' in ctx:
+entry_attrs.setdefault('gidnumber', ctx['def_group_gid'])
# do not migrate all attributes
for attr in entry_attrs.keys():
--
1.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
Re: [Freeipa-devel] [PATCHES] 0016 Fixes for{add, set, del}attr with managed attributes
On 02/29/2012 03:50 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/27/2012 11:03 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
Patch 16 defers validation conversion until after
{add,del,set}attr is
processed, so that we don't search for an integer in a list of strings
(this caused ticket #2405), and so that the end result of these
operations is validated (#2407).
Patch 17 makes these options honor params marked no_create and
no_update.
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
NACK on Patch 17 which breaks patch 16.
How is patch 16 broken? It works for me.
My point is they rely on one another, IMHO, so without 17 the reported
problem still exists.
*attr is specifically made to be powerful. We don't want to arbitrarily
block updating certain values.
Noted
Not having patch 17 means that the problem reported in 2408 still
occurs. It should probably check both the schema and the param to
determine if something can have multiple values and reject that way.
I see the problem now: the certificate subject base is defined as a
multi-value attribute in the LDAP schema. If it's changed to
single-value the existing validation should catch it.
Also, most of the config attributes should probably be required in the
schema. Am I right?
I'm a newbie here; what do I need to do when changing the schema? Is
there a patch that does something similar I could use as an example?
The framework should be able to impose its own single-value will as
well. If a Param is designated as single-value the *attr should honor it.
Here is the updated patch.
Since *attr is powerful enough to modify 'no_update' Params, which
CRUDUpdate forgets about, I need to check the params of the LDAPObject
itself.
Updating schema is a bit of a nasty business right now. See
10-selinuxusermap.update for an example.
I'll leave schema changes for after the release, then.
--
Petr³
From 38a30c9215d0d708e1c0a4c9ba92eafffbde1f74 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 24 Feb 2012 12:26:28 -0500
Subject: [PATCH] Defer conversion and validation until after
--{add,del,set}attr are handled
--addattr friends that modified attributes known to Python sometimes
used converted and validated Python values instead of LDAP strings.
This caused a problem for --delattr, which searched for a converted
integer in a list of raw strings (ticket 2407).
With this patch we work on raw strings, converting only when done.
Deferring validation ensures the end result is valid, so proper errors
are raised instead of failing later (ticket 2405).
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
---
ipalib/plugins/baseldap.py | 37 +++--
1 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 725704ee0e2d784a1f5ad8691d90888366f8efec..cdfe0fe2b127b886e1889a68fa263208e055db4b 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -759,8 +759,6 @@ last, after all sets and adds.),
Convert a string in the form of name/value pairs into a dictionary.
The incoming attribute may be a string or a list.
-Any attribute found that is also a param is validated.
-
:param attrs: A list of name/value pairs
:param append: controls whether this returns a list of values or a single
@@ -776,12 +774,6 @@ last, after all sets and adds.),
if len(value) == 0:
# None means delete this attribute
value = None
-if attr in self.params:
-try:
- value = self.params[attr](value)
-except errors.ValidationError, err:
-(name, error) = str(err.strerror).split(':')
-raise errors.ValidationError(name=attr, error=error)
if append and attr in newdict:
if type(value) in (tuple,):
newdict[attr] += list(value)
@@ -885,12 +877,29 @@ last, after all sets and adds.),
# normalize all values
changedattrs = setattrs | addattrs | delattrs
for attr in changedattrs:
-# remove duplicite and invalid values
-entry_attrs[attr] = list(set([val for val in entry_attrs[attr] if val]))
-if not entry_attrs[attr]:
-entry_attrs[attr] = None
-elif isinstance(entry_attrs[attr], (tuple, list)) and len(entry_attrs[attr]) == 1:
-entry_attrs[attr] = entry_attrs[attr][0]
+if attr in self.obj.params:
+# convert single-value params to scalars
+# Need to use the LDAPObject's params, not self's, because the
+# CRUD classes filter their disallowed parameters out.
+
Re: [Freeipa-devel] More types of replica in FreeIPA
On Wed, 2012-02-29 at 16:19 +0100, Ondrej Hamada wrote: Hi everyone, I'm currently working on my thesis. It's objective is $SUBJ and we already have ticket for that: #194. The task is to create two more replica types - the HUB and Consumer. In 389-DS both the HUB and Consumer are read-only. Additionally the HUB can push the data to the Consumers. In case of FreeIPA the server is not only providing data, but also services like CA, NTP, DNS, Kerberos. Therefore I'm kindly asking you for advices and opinions on that: 1. What should be the position of HUB? I mean should it be used as an interconnection between Masters and Consumers only, so that it will be 'hidden' in the topology and only forwards the updates, or should the HUB be also used as a regular Consumer which has additional ability to push the updates further to Consumers/HUBS? I would think that having shadow HUBs would make things more reliable. 2. Which services should be available on HUB and Consumer? I think, the priority of these replicas would be to answer to data request by ipa whatever-(find|show) commands or to provide some LDAP data for email addressing etc. Also it shouldn't cause much trouble to run NTP on Consumer, but what about Kerberos or CA? Is it a good solution to let users authenticate against these replicas? Is it correct to leave classified data like passwords on these replicas? CA's clearly have no place in a read-only replica in my mind. Kerberos stuff is different. The problem with a read-only replica and Kerberos is that krb needs to write data for local user lockouts. Password changes can be avoided by allowing kadmind only on full masters. There is also another angle to consider and that is the Rad-Only Domain Controller (RODC) available in the Microsoft world. This kind of server is even more limited than a read-only replica as it does not contain the full data. To do that it requires quite some tweaking on the KDC component too, so maybe it is out of scope, but it may make sense reading up on what they do to have a sense of the kind of services they enable/disable on read only servers. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 940 apply some validation to some classes only
On 20.2.2012 22:56, Rob Crittenden wrote: Rob Crittenden wrote: The variable name rdnattr can be misleading. It is only used to give the name of hte RDN in something that can be renamed. Compare this to something like netgroups where the DN has no visible relationship to the content of the object (ipaUniqueId). Only those objects that define rdnattr get a rename option (look at netgroups, for example). rdnattr is always the primary key but not always defined. It should probably be a boolean instead, rdn_is_primary_key or something a bit more obvious. I can make that change here. rob Updated patch. It seems I broke query a few months ago trying to enforce no white spaces in some names. I did the rdnattr variable rename. Looking back at the changelog this was meant to always match the primary key so lets remove the possibility that it doesn't. By doing it this way we get the pattern for free. rob This is certainly better, but I still have a few concerns: 1. --rename is tied to the RDN change operation. I think this is wrong. --rename should be available for all objects, not only when the object's RDN attribute and primary key attribute are the same (so that we can change the primary key of any object). Similarly, modrdn should be triggered for all objects every time the RDN attribute changes, not only when the object's RDN attribute and primary key attribute are the same (so the DN is properly updated for all objects). I don't know if this is in the scope of this patch, though. 2. In crud.Create/crud.Update, query is set for params which have the ask_create/ask_update flag set. This is an error, as we are obviously not querying LDAP with these params (it seems someone forgot to remove the query=True keyword argument after copy-pasting from crud.Search). Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] discussion needed: 0009 Support for IPv6 elements in idnsForwarders attribute
On 02/29/2012 04:30 PM, Simo Sorce wrote: Either way looks ok to me. I agree that using a space may be less confusing if this syntax never allows to specify multiple addresses. If multiple address can be specified than it may be less ideal to use spaces. Simo. idnsForwarders is multi-value attribute, so each value contain single forwarder address. Petr^2 Spacek On Wed, 2012-02-29 at 15:14 +0100, Petr Spacek wrote: And there is the patch, sorry. Petr^2 On 02/29/2012 03:10 PM, Petr Spacek wrote: Hello, this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/49 , but I want to discuss one (unimplemented) change: I propose a change in (currently very strange) forwarders syntax. Current syntax: IP[.port] examples: 1.2.3.4 (without optional port) 1.2.3.4.5553 (optional port 5553) A::B (IPv6, without optional port) A::B.5553 :::1.2.3.4 (6to4, without optional port) :::1.2.3.4.5553 (6to4, with optional port 5553) I find this syntax confusing, non-standard and not-typo-proof. IMHO better choice is to follow BIND forwarders syntax: IP [port ip_port] (port is string delimited with spaces) (From: http://www.zytrax.com/books/dns/ch7/queries.html#forwarders) *Current syntax is not documented*, so probably is not used anywhere. (And DNS server on non-standard port is probably useful only for testing purposes, but it's another story.) What is you opinion? ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 12 When migrating warn user if compat is enabled
Ondrej Hamada wrote: On 02/28/2012 10:52 PM, Rob Crittenden wrote: Ondrej Hamada wrote: On 02/27/2012 09:47 PM, Rob Crittenden wrote: Ondrej Hamada wrote: On 02/21/2012 02:32 PM, Ondrej Hamada wrote: On 02/20/2012 06:53 PM, Rob Crittenden wrote: Ondrej Hamada wrote: https://fedorahosted.org/freeipa/ticket/2274 Added check into migration plugin to warn user when compat is enabled. If compat is enabled, the migration fails and user is warned that he must turn the compat off or run the script with (the newly introduced) option '--compat'. '--compat' is just a flag, by default set to false. If it is set, the compat check is skipped. Interesting approach. I think this is probably good, preventing migration when the compat plugin is enabled unless you specifically decide to. I think the option may need another name, maybe --with-compat or something. I think in the message we should use enabled instead of on. That is the language of ipa-compat-manage. The migration help should have a discussion of why this is a problem too, and what compat really is (provides a different view of the data to be compatible with non RFC2703bis systems). rob corrected Ondra ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel I forget to update the commit message about the change of flag name. Corrected patch attached. This works ok it just seems to be making an assumption on the client when to print this. I think a similar value like enabled needs to be created to explicitly say why we are returning. rob sorry for that, value created Ondra I think you need to define beter what compat means in the output, it coudl be very confusing. You can return a value for it without testing whether it is actually a problem or not. I think what compat is supposed to mean is Am I failing because of compat and not an indication of whether compat is enabled or not. Some documentation at a minimum should be added. It otherwise seems to work ok. rob You could return a value for compat here without I've updated the description of 'compat' value in output and also changed the condition when this value is set to False. Now it is set to False only when the migration fails because of compatibility plugin. Code looks good. I think the error language needs some tweaking. I think the help text should read: The schema compat feature allows IPA to reformat data for systems that do not support RFC2307bis. It is recommended that this be disabled during migration to reduce system overhead. It can be re-enabled after migration. To migrate with it enabled use the --with-compat option. I think the client-side error should read: The compat plug-in is enabled. This can increase the memory requirements during migration. Disable the compat plug-in with \'ipa-compat-manage disable\' or re-run this script with \'--with-compat\' option. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 14 ipa permission-add does not fail if using invalid attribute
Ondrej Hamada wrote: On 02/28/2012 09:57 PM, Rob Crittenden wrote: Ondrej Hamada wrote: On 02/27/2012 03:22 PM, Rob Crittenden wrote: Ondrej Hamada wrote: When adding or modifying permission with both type and attributes specified, check whether the attributes are allowed for specified type. In case of disallowed attributes the InvalidSyntax error is raised. New tests were also added to the unit-tests. https://fedorahosted.org/freeipa/ticket/2293 https://www.redhat.com/mailman/listinfo/freeipa-devel NACK. You should use obj.object_class_config to determine if the default list of objectclasses comes from LDAP. I think that may be it, otherwise the patch reads ok. I'm very glad to see unit tests! rob Corrected Sorry, found a couple of more things I should have found the first review. Please use the dn module to construct dn_ipaconfig. Or you can also get the DN on-the-fly since the config object using get_dn(). Probably just as safe to call: if obj.object_class_config: ... rather than hasattr. I suppose its just a style thing. Done. I wonder if ObjectclassViolation is a better exception. SyntaxError means the data type is wrong, not that it isn't allowed. I agree that it makes more sense and I've updated the patch that way, but the documentation says: permission operation fails with schema syntax errors - maybe we should also update the documentation. rob I'll open a ticket on clarifying SyntaxError. ACK, pushed to master and ipa-2-2 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] discussion needed: 0009 Support for IPv6 elements in idnsForwarders attribute
I agree that we should keep the BIND syntax and separate port and IP address with a space. We will at least avoid possible issues with IP address decoding in the future. Since this is a new attribute we have a good chance to do changes now so that it is used correctly. I created an upstream ticket to change the behavior and validators in FreeIPA: https://fedorahosted.org/freeipa/ticket/2462 Martin On Wed, 2012-02-29 at 16:44 +0100, Petr Spacek wrote: On 02/29/2012 04:30 PM, Simo Sorce wrote: Either way looks ok to me. I agree that using a space may be less confusing if this syntax never allows to specify multiple addresses. If multiple address can be specified than it may be less ideal to use spaces. Simo. idnsForwarders is multi-value attribute, so each value contain single forwarder address. Petr^2 Spacek On Wed, 2012-02-29 at 15:14 +0100, Petr Spacek wrote: And there is the patch, sorry. Petr^2 On 02/29/2012 03:10 PM, Petr Spacek wrote: Hello, this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/49 , but I want to discuss one (unimplemented) change: I propose a change in (currently very strange) forwarders syntax. Current syntax: IP[.port] examples: 1.2.3.4 (without optional port) 1.2.3.4.5553 (optional port 5553) A::B (IPv6, without optional port) A::B.5553 :::1.2.3.4 (6to4, without optional port) :::1.2.3.4.5553 (6to4, with optional port 5553) I find this syntax confusing, non-standard and not-typo-proof. IMHO better choice is to follow BIND forwarders syntax: IP [port ip_port] (port is string delimited with spaces) (From: http://www.zytrax.com/books/dns/ch7/queries.html#forwarders) *Current syntax is not documented*, so probably is not used anywhere. (And DNS server on non-standard port is probably useful only for testing purposes, but it's another story.) What is you opinion? ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 972 fix migration
On Wed, 2012-02-29 at 10:31 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-02-28 at 17:36 -0500, Rob Crittenden wrote: We were setting the GID of migrated users to that of the default user's group (ipausers) when it should have been the same as the UID unless UPG was disabled. This does the right thing and fixes migration which was broken when we made ipausers a non-posix group. rob NACK This is a good start, but you missed a case when UPGs are disabled. We crash in that case: # ipa-managed-entries -e 'UPG Definition' disable Disabling Plugin # ipa migrate-ds --user-container=ou=People --group-container=ou=Groups ldap://vm-054.idm.lab.bos.redhat.com --bind-dn=cn=Directory Manager Password: ipa: ERROR: an internal error has occurred /var/log/httpd/error_log: [Wed Feb 29 09:15:36 2012] [error] ipa: ERROR: non-public: KeyError: 'gidnumber' [Wed Feb 29 09:15:36 2012] [error] Traceback (most recent call last): [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipaserver/rpcserver.py, line 314, in wsgi_execute [Wed Feb 29 09:15:36 2012] [error] result = self.Command[name](*args, **options) [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line 443, in __call__ [Wed Feb 29 09:15:36 2012] [error] ret = self.run(*args, **options) [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line 721, in run [Wed Feb 29 09:15:36 2012] [error] return self.execute(*args, **options) [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 667, in execute [Wed Feb 29 09:15:36 2012] [error] ldap, config, ds_ldap, ds_base_dn, options [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 605, in migrate [Wed Feb 29 09:15:36 2012] [error] **blacklists [Wed Feb 29 09:15:36 2012] [error] File /usr/lib/python2.7/site-packages/ipalib/plugins/migration. py, line 125, in _pre_migrate_user [Wed Feb 29 09:15:36 2012] [error] ctx['def_group_gid'] = g_attrs['gidnumber'][0] [Wed Feb 29 09:15:36 2012] [error] KeyError: 'gidnumber' [Wed Feb 29 09:15:36 2012] [error] ipa: INFO: ad...@idm.lab.bos.redhat.com: migrate_ds(u'ldap://vm-054.idm.lab.bos.redhat.com', u'', binddn=u'cn=Directory Manager', usercontainer=u'ou=People', groupcontainer=u'ou=Groups', userobjectclass=(u'person',), groupobjectclass=(u'groupOfUniqueNames',u'groupOfNames'), userignoreobjectclass=None, userignoreattribute=None, groupignoreobjectclass=None, groupignoreattribute=None, groupoverwritegid=False, schema=u'RFC2307bis', continue=False, exclude_groups=None, exclude_users=None): KeyError Martin Updated. Will now report an error if the default group is not POSIX and UPG is disabled. rob Yup, that should be enough. ACK. Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Move install script error handling to a common function
On Mon, 2012-02-27 at 17:51 +0100, Petr Viktorin wrote: On 02/22/2012 10:41 AM, Petr Viktorin wrote: This fixes https://fedorahosted.org/freeipa/ticket/2071 (Add final debug message in installers). The try/except blocks at the end of installers/management scripts are replaced by a call to a common function, which includes the final message. Obviously the installers still need some more love. This is as far as I got before Martin stopped me, saying I shouldn't change too much before a release :) If it's still too many changes to test, I could just wrap the blocks in some `with add_final_message` block for now, and resubmit this patch after the release. Yeah, this is exactly the kind of changes that can have yet-unseen consequences and I don't like pushing this close to the release. The original ticket just asks for a debug message when the install script ends. If possible, I would really prefer to have some low-risk patch adding just those and leave install script refactoring for next big release, like 3.x. Rob, what's your opinion on this? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 217-220 minor DNS fixes and improved validation
Martin Kosek wrote: On Mon, 2012-02-27 at 15:15 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-02-21 at 17:27 +0100, Martin Kosek wrote: This set of 3 DNS patches fixes 2 minor issues found during DNS test day (217, 218) and there is slightly longer patch (219) which improves and consolidates hostname/domain name validation. The testing should be pretty straightforward in case of these 3 tickets. Martin I forgot to generate API.txt in patch 219. Fixed version is included. While at it I made another patch (220) to DNS/host plugins built on 217-219. It fixes the way we handle FQDNs with ending . in both plugins. It will also fix bug that Marco Pizzoli was experiencing. Some clean ups in host plugin procedures for finding DNS zone for a host were needed so that host's IP address is added correctly to DNS zones with trailing dot. Martin A bit of rebasing is required on most of these. Thanks. I rebased all of those and fixed reported errors and also a misformatted except clause. 217 ACK Pushed to master, ipa-2-2. 218. NACK. The invalid values are still accepted and you get the same error message No options to add This is different than entering an invalid type like XYZ. I think we should instead either not allow entry at all or recognize that it is unsupported and say so. Good catch. I just fixed the error message, not the actual check. Fixed. 219. NACK. The error message doesn't include _ in the list of allowed characters but it accepts it (it probably shouldn't). IPA hostnames do not. Error message fixed. _ need to be allowed for DNS record names, they are used for example for SRV records. They are just not allowed for hostnames. 220 NACK. The ticket number is wrong and the patch doesn't apply. rob Fixed. ACK 218, 219, 220 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 975 don't delete system users we add
On Tue, 2012-02-28 at 23:07 -0500, Rob Crittenden wrote: Don't call userdel during uninstall to delete any system users we create. If they are deleted and the system adds another user for some reason (package install, for example) then file ownership can get hosed. rob NACK There is still a groupdel for dirsrv group. This makes the whole uninstall to blow up: # ipa-server-install --uninstall --unattended Shutting down all IPA services Removing IPA client configuration Unconfiguring ntpd Unconfiguring CA directory server Unconfiguring CA Unconfiguring named Unconfiguring web server Unconfiguring krb5kdc Unconfiguring kadmin Unconfiguring directory server Unconfiguring ipa_memcached ipa : CRITICAL failed to delete group Command '/usr/sbin/groupdel dirsrv' returned non-zero exit status 8 # /usr/sbin/groupdel dirsrv groupdel: cannot remove the primary group of user 'pkisrv' Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 099 Removed CSV creation from UI
Creating CSV values in UI is unnecessary and error-prone because server
converts them back to list. Possible problems with values containing
commas may occur. All occurrences of CSV joining were therefore removed.
https://fedorahosted.org/freeipa/ticket/2227
--
Petr Vobornik
From 04a88c28ef64c3f56f17458b27b7381d3d476658 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 29 Feb 2012 18:53:11 +0100
Subject: [PATCH] Removed CSV creation from UI
Creating CSV values in UI is unnecessary and error-prone because server converts them back to list. Possible problems with values containing commas may occur. All occurrences of CSV joining were therefore removed.
https://fedorahosted.org/freeipa/ticket/2227
---
install/ui/aci.js | 14 --
install/ui/add.js | 12 +---
install/ui/association.js | 24 ++--
install/ui/details.js |2 --
install/ui/dns.js |2 +-
install/ui/field.js |1 -
install/ui/rule.js|2 +-
install/ui/widget.js |2 +-
8 files changed, 18 insertions(+), 41 deletions(-)
diff --git a/install/ui/aci.js b/install/ui/aci.js
index ec3c8065b8d13e7feede2947188504ec6919230e..ba4a22bf1f2fa4179f093c0bf9eb89a7c7e31c8b 100644
--- a/install/ui/aci.js
+++ b/install/ui/aci.js
@@ -46,7 +46,6 @@ IPA.aci.permission_entity = function(spec) {
{
type: 'rights',
name: 'permissions',
-join: true,
widget: 'rights.permissions'
},
{
@@ -142,7 +141,6 @@ IPA.aci.permission_entity = function(spec) {
{
type: 'rights',
name: 'permissions',
-join: true,
widget: 'general.permissions'
},
{
@@ -407,14 +405,12 @@ IPA.aci.delegation_entity = function(spec) {
type: 'entity_select',
name: 'memberof',
other_entity: that.group_entity,
-other_field: 'cn',
-join: true
+other_field: 'cn'
},
{
type: 'attributes',
name: 'attrs',
-object_type: 'user',
-join: true
+object_type: 'user'
}
]
}
@@ -434,14 +430,12 @@ IPA.aci.delegation_entity = function(spec) {
type: 'entity_select',
name: 'memberof',
other_entity: that.group_entity,
-other_field: 'cn',
-join: true
+other_field: 'cn'
},
{
type: 'attributes',
name: 'attrs',
-object_type: 'user',
-join: true
+object_type: 'user'
}
]
});
diff --git a/install/ui/add.js b/install/ui/add.js
index 9b473ccc28befceeb7d38ac675ec3019e724459c..671d3f1fcbd3c9be195db8d102a6d6c80bb5c041 100644
--- a/install/ui/add.js
+++ b/install/ui/add.js
@@ -123,16 +123,14 @@ IPA.entity_adder_dialog = function(spec) {
var field = fields[j];
var values = record[field.param];
-if (!values) continue;
-
-// TODO: Handle multi-valued attributes like in detail facet's update()
-var value = values.join(',');
-if (!value) continue;
+if (!values || values.length === 0) continue;
if (field.param === pkey_name) {
-command.add_arg(value);
+command.add_arg(values[0]);
+} else if (values.length === 1) {
+command.set_option(field.param, values[0]);
} else {
-command.set_option(field.param, value);
+command.set_option(field.param, values);
}
}
diff --git a/install/ui/association.js b/install/ui/association.js
index 72e1f0c0e380fbb0169eb852e41b8c0f587835e6..b170b39d231ef6ce22161a199b039390faca0a34 100644
--- a/install/ui/association.js
+++ b/install/ui/association.js
@@ -110,29 +110,17 @@ IPA.bulk_associator = function(spec) {
return;
}
-var value = that.values.shift();
-if (!value) {
-that.on_success();
-return;
-}
-
-while (that.values.length 0) {
-value += ',' + that.values.shift();
-}
-
-var args = [that.pkey];
-var options = { 'all': true };
-options[that.other_entity.name] = value;
-
var command = IPA.command({
entity: that.entity.name,
Re: [Freeipa-devel] [PATCH] 0015 Only split CSV strings once (updated)
On 02/27/2012 02:01 PM, Petr Viktorin wrote: It seems I didn't communicate the problem and my solution clearly enough, so let me try again. (Also, I learned from the discussions!) Currently, both the client and the server parse CSV options. The client does *not* re-encode the CSV before sending; the parsing is really done twice. This means e.g. that you need 3 backslashes to escape a literal comma: after the client-side split, '\\\,' becomes '\,'; which after the server-side split becomes ','. Since CSV is specific to the command-line, and the client is responsible for translating command-line input to XML-RPC (which has its own syntax for lists), the ideal fix will be to move CSV processing entirely to the client. This will be a rather invasive change, mainly because some parts of the UI now expect the server-side parsing (but they don't escape CSV, so values containing commas or backslashes are broken). So it won't make it to the upcoming release. My patch provides a quick fix: when a call comes from the command-line client, disable the server-side parsing. I investigated all occurrences of CSV creation in Web UI. I removed them and UI is working fine. The patch is on the list: pvoborni 099. So your patch shouldn't affect UI if my patch is applied. I can't get away from moving split_csv() (which is not idempotent) out of normalize() (which is, and gets called lots of times); this is the patch's major change in therms of LOC. I'll note again that this only affects values with backslashes or double quotes. Exactly these options are currently broken (=need double escaping). The normal uses of CSV are completely unaffected. Attaching updated patch; the change vs. the original is that the don't parse again flag is now only set at the server, when a XMLRPC call is received, making the client fully forward-compatible (the flag doesn't get sent through the wire). The ticket is https://fedorahosted.org/freeipa/ticket/2227, but this patch is only the first step in fixing it. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 217-220 minor DNS fixes and improved validation
On Wed, 2012-02-29 at 12:39 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Mon, 2012-02-27 at 15:15 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-02-21 at 17:27 +0100, Martin Kosek wrote: This set of 3 DNS patches fixes 2 minor issues found during DNS test day (217, 218) and there is slightly longer patch (219) which improves and consolidates hostname/domain name validation. The testing should be pretty straightforward in case of these 3 tickets. Martin I forgot to generate API.txt in patch 219. Fixed version is included. While at it I made another patch (220) to DNS/host plugins built on 217-219. It fixes the way we handle FQDNs with ending . in both plugins. It will also fix bug that Marco Pizzoli was experiencing. Some clean ups in host plugin procedures for finding DNS zone for a host were needed so that host's IP address is added correctly to DNS zones with trailing dot. Martin A bit of rebasing is required on most of these. Thanks. I rebased all of those and fixed reported errors and also a misformatted except clause. 217 ACK Pushed to master, ipa-2-2. 218. NACK. The invalid values are still accepted and you get the same error message No options to add This is different than entering an invalid type like XYZ. I think we should instead either not allow entry at all or recognize that it is unsupported and say so. Good catch. I just fixed the error message, not the actual check. Fixed. 219. NACK. The error message doesn't include _ in the list of allowed characters but it accepts it (it probably shouldn't). IPA hostnames do not. Error message fixed. _ need to be allowed for DNS record names, they are used for example for SRV records. They are just not allowed for hostnames. 220 NACK. The ticket number is wrong and the patch doesn't apply. rob Fixed. ACK 218, 219, 220 rob Thanks, pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 940 apply some validation to some classes only
Jan Cholasta wrote:
On 20.2.2012 22:56, Rob Crittenden wrote:
Rob Crittenden wrote:
The variable name rdnattr can be misleading. It is only used to give the
name of hte RDN in something that can be renamed. Compare this to
something like netgroups where the DN has no visible relationship to the
content of the object (ipaUniqueId). Only those objects that define
rdnattr get a rename option (look at netgroups, for example).
rdnattr is always the primary key but not always defined. It should
probably be a boolean instead, rdn_is_primary_key or something a bit
more obvious. I can make that change here.
rob
Updated patch. It seems I broke query a few months ago trying to enforce
no white spaces in some names.
I did the rdnattr variable rename. Looking back at the changelog this
was meant to always match the primary key so lets remove the possibility
that it doesn't. By doing it this way we get the pattern for free.
rob
This is certainly better, but I still have a few concerns:
1. --rename is tied to the RDN change operation. I think this is wrong.
--rename should be available for all objects, not only when the object's
RDN attribute and primary key attribute are the same (so that we can
change the primary key of any object). Similarly, modrdn should be
triggered for all objects every time the RDN attribute changes, not only
when the object's RDN attribute and primary key attribute are the same
(so the DN is properly updated for all objects).
I don't know if this is in the scope of this patch, though.
Right, not in this scope. An entry where RDN != primary key doesn't need
--rename to do a rename, just a mod on whatever its key is. We can
fake this to have a consistent interface though. Please open a ticket.
2. In crud.Create/crud.Update, query is set for params which have the
ask_create/ask_update flag set. This is an error, as we are obviously
not querying LDAP with these params (it seems someone forgot to remove
the query=True keyword argument after copy-pasting from crud.Search).
Honza
Updated
rob
From 81e5502265dab83d5cafc59aa1c80f68c1c1f35e Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Wed, 29 Feb 2012 13:31:20 -0500
Subject: [PATCH] Only apply validation rules when adding and updating.
There may be cases, for whatever reason, that an otherwise illegal
entry gets created that doesn't match the criteria for a valid
user/host/group name. If this happens (i.e. migration) there is no way
to remove this using the IPA tools because we always applied the name
pattern. So you can't, for example, delete a user with an illegal name.
Primary keys are cloned with query=True in PKQuery which causes no
rules to be applied on mod/show/find. This reverts a change from commit
3a5e26a0 which applies class rules when query=True (for enforcing no
white space).
Replace rdnattr with rdn_is_primary_key. This was meant to tell us when
an RDN change was necessary to do a rename. There could be a disconnect
where the rdnattr wasn't the primary key and in that case we don't
need to do an RDN change, so use a boolean instead so that it is
clear that RDN == primary key.
Add a test to ensure that nowhitespace is actually enforced.
https://fedorahosted.org/freeipa/ticket/2115
Related: https://fedorahosted.org/freeipa/ticket/2089
Whitespace tickets:
https://fedorahosted.org/freeipa/ticket/1285
https://fedorahosted.org/freeipa/ticket/1286
https://fedorahosted.org/freeipa/ticket/1287
---
API.txt| 24 +-
ipalib/crud.py |6 +++-
ipalib/parameters.py |3 +-
ipalib/plugins/automount.py|2 +-
ipalib/plugins/baseldap.py | 21 ---
ipalib/plugins/group.py|2 +-
ipalib/plugins/permission.py |2 +-
ipalib/plugins/privilege.py|2 +-
ipalib/plugins/role.py |2 +-
ipalib/plugins/user.py |2 +-
tests/test_xmlrpc/test_group_plugin.py | 22
tests/test_xmlrpc/test_host_plugin.py | 42
tests/test_xmlrpc/test_role_plugin.py | 10 +++
tests/test_xmlrpc/test_user_plugin.py | 36 +++
14 files changed, 145 insertions(+), 31 deletions(-)
diff --git a/API.txt b/API.txt
index b5f5774b58449dd5496d85117fa6f970ea34662d..dadd9c11d56d7d1e493a18d9d5218557b9d6099b 100644
--- a/API.txt
+++ b/API.txt
@@ -2036,12 +2036,12 @@ command: permission_add
args: 1,12,3
arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
option: Str('permissions', attribute=True, cli_name='permissions', csv=True, multivalue=True, required=True)
-option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=True, required=False)
-option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type',
Re: [Freeipa-devel] [PATCH] 0014 Move install script error handling to a common function
Martin Kosek wrote: On Mon, 2012-02-27 at 17:51 +0100, Petr Viktorin wrote: On 02/22/2012 10:41 AM, Petr Viktorin wrote: This fixes https://fedorahosted.org/freeipa/ticket/2071 (Add final debug message in installers). The try/except blocks at the end of installers/management scripts are replaced by a call to a common function, which includes the final message. Obviously the installers still need some more love. This is as far as I got before Martin stopped me, saying I shouldn't change too much before a release :) If it's still too many changes to test, I could just wrap the blocks in some `with add_final_message` block for now, and resubmit this patch after the release. Yeah, this is exactly the kind of changes that can have yet-unseen consequences and I don't like pushing this close to the release. The original ticket just asks for a debug message when the install script ends. If possible, I would really prefer to have some low-risk patch adding just those and leave install script refactoring for next big release, like 3.x. Rob, what's your opinion on this? Martin Yes, I agree. Simpler is better at this point. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 975 don't delete system users we add
Martin Kosek wrote: On Tue, 2012-02-28 at 23:07 -0500, Rob Crittenden wrote: Don't call userdel during uninstall to delete any system users we create. If they are deleted and the system adds another user for some reason (package install, for example) then file ownership can get hosed. rob NACK There is still a groupdel for dirsrv group. This makes the whole uninstall to blow up: Ouch, nice catch. It didn't blow up for me and I totally missed this. Updated patch. rob From e59464e42e72ddc380e95cde4634a4fb62b1cc4c Mon Sep 17 00:00:00 2001 From: Rob Crittenden rcrit...@redhat.com Date: Tue, 28 Feb 2012 23:05:06 -0500 Subject: [PATCH] Don't delete system users that are added during installation. We don't want to run the risk of adding a user, uninstalling it, the system adding a new user (for another package install for example) and then re-installing IPA. This wreaks havoc with file and directory ownership. https://fedorahosted.org/freeipa/ticket/2423 --- install/tools/ipa-server-install | 10 -- ipaserver/install/cainstance.py | 17 +++-- ipaserver/install/dsinstance.py | 10 +++--- 3 files changed, 10 insertions(+), 27 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 47f999b4e8b6bc6df54ba6e8a8fa92616d0a9416..29c831572b97e9fd8f6dab9a88c254cb8a4a9598 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -475,16 +475,6 @@ def uninstall(): sstore._load() group_exists = sstore.restore_state(install, group_exists) -if group_exists == False: -try: -grp.getgrnam(dsinstance.DS_GROUP) -try: -ipautil.run([/usr/sbin/groupdel, dsinstance.DS_GROUP]) -except ipautil.CalledProcessError, e: -root_logger.critical(failed to delete group %s % e) -rv = 1 -except KeyError: -root_logger.info(Group %s already removed, dsinstance.DS_GROUP) ipaservices.knownservices.ipa.disable() diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py index d2c8d0576fcc7a380bb9a303954d1d4503148a3b..97094c7f848172f210171ec4085df7570d92ed1d 100644 --- a/ipaserver/install/cainstance.py +++ b/ipaserver/install/cainstance.py @@ -406,11 +406,9 @@ class CADSInstance(service.Service): user_exists = self.restore_state(user_exists) -if user_exists == False: -try: -ipautil.run([/usr/sbin/userdel, PKI_DS_USER]) -except ipautil.CalledProcessError, e: -root_logger.critical(failed to delete user %s % e) +# At one time we removed this user on uninstall. That can potentially +# orphan files, or worse, if another useradd runs in the intermim, +# cause files to have a new owner. class CAInstance(service.Service): @@ -1065,11 +1063,10 @@ class CAInstance(service.Service): root_logger.critical(failed to uninstall CA instance %s % e) user_exists = self.restore_state(user_exists) -if user_exists == False: -try: -ipautil.run([/usr/sbin/userdel, PKI_USER]) -except ipautil.CalledProcessError, e: -root_logger.critical(failed to delete user %s % e) + +# At one time we removed this user on uninstall. That can potentially +# orphan files, or worse, if another useradd runs in the intermim, +# cause files to have a new owner. def publish_ca_cert(self, location): args = [-L, -n, self.canickname, -a] diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py index c66f2a7f11ad8b34ed8304a6465998f6d518a814..c62a07f1c32c7a043207b00e5ad4fdd5e8ef 100644 --- a/ipaserver/install/dsinstance.py +++ b/ipaserver/install/dsinstance.py @@ -626,13 +626,9 @@ class DsInstance(service.Service): user_exists = self.restore_state(user_exists) -if user_exists == False: -pent = pwd.getpwnam(DS_USER) -installutils.remove_file(/var/tmp/ldap_%d % pent.pw_uid) -try: -ipautil.run([/usr/sbin/userdel, DS_USER]) -except ipautil.CalledProcessError, e: -root_logger.critical(failed to delete user %s % e) +# At one time we removed this user on uninstall. That can potentially +# orphan files, or worse, if another useradd runs in the intermim, +# cause files to have a new owner. # Make sure some upgrade-related state is removed. This could cause # re-installation problems. -- 1.7.6 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 976 add tests for HTTP_Status
The tests for not_found were broken, this fixes it and adds tests for
the other statuses.
I changed the parent class of HTTP_Status because it calls self.info
which is provided by Plugable. This wasn't a problem at runtime because
Backend provides self.log.
rob
From 78c13cfdf81e8aa731f953bbc8293bb6dd2b89fb Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Wed, 29 Feb 2012 16:12:58 -0500
Subject: [PATCH] subclass HTTP_Status from plugable.Plugin, fix not_found
tests
HTTP_Status needs to subclass from Plugin because it does its own logging.
Add tests for other methods of HTTP_Status
---
ipaserver/rpcserver.py |3 +-
tests/test_ipaserver/test_rpcserver.py | 53 +++
2 files changed, 48 insertions(+), 8 deletions(-)
diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py
index c383f0482171e264c379aa594568f036feafe915..271f7717b6343c3f7569a6e955bb5ae814cc27e9 100644
--- a/ipaserver/rpcserver.py
+++ b/ipaserver/rpcserver.py
@@ -26,6 +26,7 @@ Also see the `ipalib.rpc` module.
from cgi import parse_qs
from xml.sax.saxutils import escape
from xmlrpclib import Fault
+from ipalib import plugable
from ipalib.backend import Executioner
from ipalib.errors import PublicError, InternalError, CommandError, JSONError, ConversionError, CCacheError, RefererError, InvalidSessionPassword
from ipalib.request import context, Connection, destroy_context
@@ -96,7 +97,7 @@ _unauthorized_template = html
/body
/html
-class HTTP_Status(object):
+class HTTP_Status(plugable.Plugin):
def not_found(self, environ, start_response, url, message):
Return a 404 Not Found error.
diff --git a/tests/test_ipaserver/test_rpcserver.py b/tests/test_ipaserver/test_rpcserver.py
index 15ca9dc08f77d5bed11d819c3297d17e5aeea2a4..97632b05d454b0ece60181ceeb6980b95e6909a1 100644
--- a/tests/test_ipaserver/test_rpcserver.py
+++ b/tests/test_ipaserver/test_rpcserver.py
@@ -46,28 +46,67 @@ class StartResponse(object):
def test_not_found():
-f = rpcserver.not_found
+f = rpcserver.HTTP_Status()
t = rpcserver._not_found_template
s = StartResponse()
# Test with an innocent URL:
-d = dict(SCRIPT_NAME='/ipa', PATH_INFO='/foo/stuff')
+url = '/ipa/foo/stuff'
assert_equal(
-f(d, s),
+f.not_found(None, s, url, None),
[t % dict(url='/ipa/foo/stuff')]
)
assert s.status == '404 Not Found'
-assert s.headers == [('Content-Type', 'text/html')]
+assert s.headers == [('Content-Type', 'text/html; charset=utf-8')]
# Test when URL contains any of ''
s.reset()
-d = dict(SCRIPT_NAME='nbsp;', PATH_INFO='scriptdo_bad_stuff();/script')
+url ='nbsp;' + 'scriptdo_bad_stuff();/script'
assert_equal(
-f(d, s),
+f.not_found(None, s, url, None),
[t % dict(url='amp;nbsp;lt;scriptgt;do_bad_stuff();lt;/scriptgt;')]
)
assert s.status == '404 Not Found'
-assert s.headers == [('Content-Type', 'text/html')]
+assert s.headers == [('Content-Type', 'text/html; charset=utf-8')]
+
+
+def test_bad_request():
+f = rpcserver.HTTP_Status()
+t = rpcserver._bad_request_template
+s = StartResponse()
+
+assert_equal(
+f.bad_request(None, s, 'illegal request'),
+[t % dict(message='illegal request')]
+)
+assert s.status == '400 Bad Request'
+assert s.headers == [('Content-Type', 'text/html; charset=utf-8')]
+
+
+def test_internal_error():
+f = rpcserver.HTTP_Status()
+t = rpcserver._internal_error_template
+s = StartResponse()
+
+assert_equal(
+f.internal_error(None, s, 'request failed'),
+[t % dict(message='request failed')]
+)
+assert s.status == '500 Internal Server Error'
+assert s.headers == [('Content-Type', 'text/html; charset=utf-8')]
+
+
+def test_internal_error():
+f = rpcserver.HTTP_Status()
+t = rpcserver._unauthorized_template
+s = StartResponse()
+
+assert_equal(
+f.unauthorized(None, s, 'unauthorized'),
+[t % dict(message='unauthorized')]
+)
+assert s.status == '401 Unauthorized'
+assert s.headers == [('Content-Type', 'text/html; charset=utf-8')]
--
1.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 65] Log a message when returning non-success HTTP result
John Dennis wrote: The routines used to return a non-success HTTP result from WSGI failed to log the aberrant event, this corrects that omission. ACK, pushed to master and ipa-2-2 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 096 Fixed content type check in login_password
Petr Vobornik wrote: login_password is expecting that request content_type will be 'application/x-www-form-urlencoded'. Current check is an equality check of content_type http header. RFC 3875 defines that content type can contain parameters separated by ';'. For example: when firefox is doing ajax call it sets the request header to 'application/x-www-form-urlencoded; charset=UTF-8' which leads to negative result. This patch makes the check more benevolent to allow such values. Patch is a fix-up for: https://fedorahosted.org/freeipa/ticket/2095 ACK, pushed to master and ipa-2-2 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 975 don't delete system users we add
On Wed, 2012-02-29 at 15:51 -0500, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-02-28 at 23:07 -0500, Rob Crittenden wrote: Don't call userdel during uninstall to delete any system users we create. If they are deleted and the system adds another user for some reason (package install, for example) then file ownership can get hosed. rob NACK There is still a groupdel for dirsrv group. This makes the whole uninstall to blow up: Ouch, nice catch. It didn't blow up for me and I totally missed this. Updated patch. rob Martin noticed that we were still saving the user/group existence state. We don't need to do that. I left in the restoring state so upgrades still have this removed on uninstall. rob Yeah, its OK now, thanks. ACK. Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 097 Added logout button
Petr Vobornik wrote: Logout button was added to Web UI. A click on logout button executes session_logout command. If command succeeds or xhr stutus is 401 (unauthorized - already logged out) page is redirected to logout.html. logout.html is a simple page with You have been logged out text and a link to return back to main page. https://fedorahosted.org/freeipa/ticket/2363 ACK, pushed to master and ipa-2-2 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Adding Debian support to the FreeIPA code
On Wed, 29 Feb 2012, Simo Sorce wrote: On Tue, 2012-02-28 at 23:45 +0200, Alexander Bokovoy wrote: On Tue, 28 Feb 2012, Krzysztof Klimonda wrote: - __setup_autoconfig modifies files in /usr/share/ and that seems to be non-compliant with FHS. It may slip through checks at first but I'd expect people reporting bugs at some point. I'll comment on this one as we'll need to get away from modifying files in /usr/share in Fedora as well by Fedora 18 or 19 according to the upcoming changes to simplify file system layout. So I'd rather move those /usr/share/ipa files to /var/lib/ipa/DOMAIN like PKI setup does, also during ipa-server-install time. That means it will be one less difference soon as well. Yeah I've been wanting to do that for a while too, but it always got deferred. Do we have a ticket that tracks this? Created https://fedorahosted.org/freeipa/ticket/2465 for this purpose. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] 43 Inherit nssldap security access settings during replica install
JR Aquino wrote: When making adjustments to increase the bind security settings of a FreeIPA server, it is best practice to inherit those settings when installing a new replica server. Inherit the following bind security settings when performing a replica install: 'nsslapd-allow-unauthenticated-binds', 'nsslapd-require-secure-binds', 'nsslapd-allow-anonymous-access', 'nsslapd-minssf' https://fedorahosted.org/freeipa/ticket/1930 NACK There is a connection helper in service.py you can use, ldap_connect(). Use it like: if not self.admin_conn: self.ldap_connect() x = self.conn.addEntry(foo) ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
