Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions

2013-05-14 Thread Tomas Babej

On 05/10/2013 04:57 PM, Petr Spacek wrote:

On 6.5.2013 17:40, Tomas Hozza wrote:

On 04/08/2013 07:45 PM, Petr Spacek wrote:

Generalize attribute_name-rdata_type conversions.

Attribute names are generated on-the-fly: String Record is appended
to textual representation of DNS RDATA type.

String Record is cut down from the attribute name during
attribute name to rdata type conversion.

 From now, the plugin doesn't add artificial limitation to supported
record types.


ACK.

The patch looks good. (I didn't do functional test)

Cosmetic issue:
I think it would be good to dynamically allocate mod_type in LDAPMod
in every case and include the mod_type memory freeing in
free_ldapmod() function. Now one has to be be careful when it is
statically or dynamically allocated. Before it was static in every case.


It is good idea. This version of the patch contains ldap_mod_create() 
function which allocates the whole structure including mod_type of 
fixed size. All writes to mod_type checks the array length, so it 
should not cause any harm.


The function modify_soa_record() still uses statically allocated 
LDAPMod structure with statically allocated strings for mod_type, but 
the LDAPMod structure never leave this function. There are no calls to 
ldap_mod_create() and ldap_mod_free(), so I think it is obvious.


Tbabej, please try to dynamically update some A records with sync_ptr 
enabled. (And of course the support for some new type, like TLSA.)

For the existing record types, the patch works fine.

For any new types, a schema change is still required, since record types 
are still hardcoded in LDAP schema:


 LDAP error: Object class violation: attribute tlsarecord not allowed



Thank you!



___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Tomas
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0142] Improve LDAP error logging

2013-05-14 Thread Tomas Babej

On 05/07/2013 09:36 AM, Tomas Hozza wrote:

On 04/09/2013 03:27 PM, Petr Spacek wrote:

Hello,

Improve LDAP error logging.

Diagnostic error message is logged when it is available.


Plugin with this patch produces messages like:

LDAP error: Server is unwilling to perform: Minimum SSF not met.: bind
to LDAP server failed

intead of

bind to LDAP server failed: Server is unwilling to perform


Second example is:

LDAP error: Object class violation: attribute mgrecord not allowed
: while modifying(add) entry 'idnsName=pspacek,
idnsname=example.com,cn=dns,dc=e,dc=test'

instead of



:-D


snip

diff --git a/src/log.h b/src/log.h
index 
312f24322fd0c6f9943c6beb810ac0bcd8f3896c..cbf1a3faaaccea7391d65d018e80d8ec688fc111
 100644

--- a/src/log.h

+++ b/src/log.h

@@ -55,16 +55,30 @@

log_write(GET_LOG_LEVEL(level), format, ##__VA_ARGS__)
/* LDAP logging functions */
-#define log_ldap_error(ld) \
-   do {\
-   int err;\
-   char *errmsg = UNKNOWN; \
-   if (ldap_get_option(ld, LDAP_OPT_RESULT_CODE, err) \
-   == LDAP_OPT_SUCCESS)\
-   errmsg = ldap_err2string(err);  \
-   log_error_position(LDAP error: %s, errmsg); \
-   } while (0);\
+#define LOG_LDAP_ERR_PREFIX LDAP error: 
+#define log_ldap_error(ld, desc, ...)  
\
+   do {
\
+   int err;
\
+   char *errmsg = NULL;
\
+   char *diagmsg = NULL;   
\
+   if (ldap_get_option(ld, LDAP_OPT_RESULT_CODE, err) 
\
+   == LDAP_OPT_SUCCESS) {  
\
+   errmsg = ldap_err2string(err);  
\

Getting error msg for the first time here.


+   if (ldap_get_option(ld, 
LDAP_OPT_DIAGNOSTIC_MESSAGE, diagmsg)  \
+   == LDAP_OPT_SUCCESS  diagmsg != NULL) 
{   \
+   errmsg = ldap_err2string(err);  
\

Again getting error msg with the same err. Maybe a copy-paste error?


+   log_error(LOG_LDAP_ERR_PREFIX %s: 
%s:  desc,\
+   errmsg, diagmsg, 
##__VA_ARGS__);\
+   ldap_memfree(diagmsg);  
\
+   } else  
\
+   log_error(LOG_LDAP_ERR_PREFIX %s: 
 desc,\
+   errmsg, ##__VA_ARGS__); 
\
+   } else {
\
+   log_error(LOG_LDAP_ERR_PREFIX   
\
+   unable to obtain LDAP error code:   
\
+   desc, ##__VA_ARGS__);   
\
+   }   
\
+   } while (0);
void
log_write(int level, const char *format, ...) ISC_FORMAT_PRINTF(2, 3);


Regards,

Tomas Hozza

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

ACK, provides the desired info.

Tomas

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0143] Treat syntax errors in LDAP filters as fatal

2013-05-14 Thread Tomas Babej

On 05/07/2013 10:16 AM, Tomas Hozza wrote:

On 04/09/2013 03:39 PM, Petr Spacek wrote:

Hello,

Treat syntax errors in LDAP filters as fatal.

Filters are hardcoded at the moment, this is preventive action.


ACK.

The patch looks good. (I didn't do functional test)


Regards,

Tomas Hozza

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

ACK. It does not break anything :)

Tomas

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] 0027 Prompt for nameserver IP address in dnszone-add

2013-05-14 Thread Ana Krivokapic
On 05/13/2013 02:50 PM, Petr Vobornik wrote:
 On 05/12/2013 03:10 PM, Ana Krivokapic wrote:
 On 05/10/2013 10:37 PM, Endi Sukma Dewata wrote:
 On 5/10/2013 9:38 AM, Petr Viktorin wrote:
 On 05/10/2013 03:57 PM, Ana Krivokapic wrote:
 [...]
 Thanks for catching the bugs. Updated patches are attached.

 Thanks! It works nicely.
 Endi is doing a quick check of the Javascript, if he doesn't find an
 issue then ACK.

 If this still makes it into 3.2.0, please only push the first patch
 there.

 I tried this in the UI:

Zone name: test.com
Authoritative nameserver: ns.sometest.com.

 The 'Nameserver IP address' field is still enabled. This is because
 the name
 server is considered in the zone although it's actually not.

 The CLI seems to work fine, it didn't ask for IP address.

 The UI probably could be fixed using endsWith(ns, '.' + zone).
 Everything else
 looks fine. ACK with the fix.


 Fixed, updated patch attached.


 A nitpick for UI part which is not a blocker(nack) because we don't
 have any strict rules for following topic:

 We should avoid depending on widget's html output outside of the
 widget code.

 So we should use:
zone_w.save()[0]
 instead of:
   $('input', zone_w.container).val();

 same for `ns`.
Thanks, fixed.

 Unfortunately there is no text_widget.is_enabled() method  so
 `zone_w.input.prop('disabled')` can't be replaced.
I implemented the `text_widget.is_enabled()` method, and replaced
`zone_w.input.prop('disabled')` with `!zone_w.is_enabled()`.

Updated patch attached.


-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From 89bfc2047cd6e832c36245e1a183b9269b6d8b86 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Thu, 9 May 2013 18:47:12 +0200
Subject: [PATCH] Prompt for nameserver IP address in dnszone-add

Prompt for nameserver IP address in interactive mode of dnszone-add.

Add a corresponding field to dnszone creation dialog in the web UI.

This parameter is required if and only if:
* New zone is a forward zone
* Nameserver is defined inside the new zone

Add a new unit test to cover this functionality.

https://fedorahosted.org/freeipa/ticket/3603
---
 install/ui/src/freeipa/dns.js   | 58 +
 install/ui/src/freeipa/widget.js|  4 ++
 install/ui/test/data/ipa_init_commands.json | 11 +
 ipalib/plugins/dns.py   | 21 +
 tests/test_cmdline/test_cli.py  | 67 +
 5 files changed, 161 insertions(+)

diff --git a/install/ui/src/freeipa/dns.js b/install/ui/src/freeipa/dns.js
index 5024e8b768ea46c86eb4db5901e71b02866432ff..897ea7114fe8e520f446f0525a039d689dcddf64 100644
--- a/install/ui/src/freeipa/dns.js
+++ b/install/ui/src/freeipa/dns.js
@@ -300,6 +300,11 @@ return {
 fields: [
 'idnssoamname',
 {
+name: 'ip_address',
+validators: [ 'ip_address' ],
+metadata: '@mc-opt:dnszone_add:ip_address'
+},
+{
 name: 'idnssoarname',
 required: false
 },
@@ -576,11 +581,64 @@ IPA.dnszone_adder_dialog = function(spec) {
 
 var that = IPA.entity_adder_dialog(spec);
 
+function endsWith(str, suffix) {
+return str.indexOf(suffix, str.length - suffix.length) !== -1;
+}
+
+var init = function() {
+var zone_w = that.fields.get_field('idnsname').widget;
+var reverse_zone_w = that.fields.get_field('name_from_ip').widget;
+var ns_w = that.fields.get_field('idnssoamname').widget;
+
+zone_w.value_changed.attach(that.check_ns_ip);
+reverse_zone_w.value_changed.attach(that.check_ns_ip);
+ns_w.value_changed.attach(that.check_ns_ip);
+};
+
+that.check_ns_ip = function() {
+var ip_address_f = that.fields.get_field('ip_address');
+var zone_w = that.fields.get_field('idnsname').widget;
+var ns_w = that.fields.get_field('idnssoamname').widget;
+
+var zone = zone_w.save()[0];
+var ns = ns_w.save()[0];
+
+var zone_is_reverse = !zone_w.is_enabled() ||
+  endsWith(zone, '.in-addr.arpa.') ||
+  endsWith(zone, '.ip6.arpa.');
+var relative_ns = true;
+var ns_in_zone = false;
+
+if (ns  ns[ns.length-1] === '.') {
+relative_ns = false;
+ns = ns.slice(0, -1);
+}
+
+if (zone  zone[zone.length-1] === '.') {
+zone = zone.slice(0, -1);
+}
+
+if (ns  zone  endsWith(ns, '.' + zone)) {
+ns_in_zone = true;
+}
+
+if (!zone_is_reverse  (relative_ns || ns_in_zone)) {
+ip_address_f.set_enabled(true);
+ip_address_f.set_required(true);
+} else {
+ip_address_f.reset();
+

Re: [Freeipa-devel] [PATCH] 0027 Prompt for nameserver IP address in dnszone-add

2013-05-14 Thread Ana Krivokapic
On 05/14/2013 12:05 PM, Ana Krivokapic wrote:
 On 05/13/2013 02:50 PM, Petr Vobornik wrote:
 On 05/12/2013 03:10 PM, Ana Krivokapic wrote:
 On 05/10/2013 10:37 PM, Endi Sukma Dewata wrote:
 On 5/10/2013 9:38 AM, Petr Viktorin wrote:
 On 05/10/2013 03:57 PM, Ana Krivokapic wrote:
 [...]
 Thanks for catching the bugs. Updated patches are attached.
 Thanks! It works nicely.
 Endi is doing a quick check of the Javascript, if he doesn't find an
 issue then ACK.

 If this still makes it into 3.2.0, please only push the first patch
 there.
 I tried this in the UI:

Zone name: test.com
Authoritative nameserver: ns.sometest.com.

 The 'Nameserver IP address' field is still enabled. This is because
 the name
 server is considered in the zone although it's actually not.

 The CLI seems to work fine, it didn't ask for IP address.

 The UI probably could be fixed using endsWith(ns, '.' + zone).
 Everything else
 looks fine. ACK with the fix.

 Fixed, updated patch attached.

 A nitpick for UI part which is not a blocker(nack) because we don't
 have any strict rules for following topic:

 We should avoid depending on widget's html output outside of the
 widget code.

 So we should use:
zone_w.save()[0]
 instead of:
   $('input', zone_w.container).val();

 same for `ns`.
 Thanks, fixed.
 Unfortunately there is no text_widget.is_enabled() method  so
 `zone_w.input.prop('disabled')` can't be replaced.
 I implemented the `text_widget.is_enabled()` method, and replaced
 `zone_w.input.prop('disabled')` with `!zone_w.is_enabled()`.

 Updated patch attached.

Petr caught another bug: due to the return value of
`text_widget.save()`, an exception was raised in the case of empty zone.
This has been fixed in the attached patch.

I also changed the name of the endsWith() function to ends_with(), to
conform to our coding standard.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From 160e5a46e9a21c702d61c457343e2f0408ede2d4 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Thu, 9 May 2013 18:47:12 +0200
Subject: [PATCH] Prompt for nameserver IP address in dnszone-add

Prompt for nameserver IP address in interactive mode of dnszone-add.

Add a corresponding field to dnszone creation dialog in the web UI.

This parameter is required if and only if:
* New zone is a forward zone
* Nameserver is defined inside the new zone

Add a new unit test to cover this functionality.

https://fedorahosted.org/freeipa/ticket/3603
---
 install/ui/src/freeipa/dns.js   | 58 +
 install/ui/src/freeipa/widget.js|  4 ++
 install/ui/test/data/ipa_init_commands.json | 11 +
 ipalib/plugins/dns.py   | 21 +
 tests/test_cmdline/test_cli.py  | 67 +
 5 files changed, 161 insertions(+)

diff --git a/install/ui/src/freeipa/dns.js b/install/ui/src/freeipa/dns.js
index 5024e8b768ea46c86eb4db5901e71b02866432ff..52cbb81f36f863afd61bf3b6cc04affe59809a48 100644
--- a/install/ui/src/freeipa/dns.js
+++ b/install/ui/src/freeipa/dns.js
@@ -300,6 +300,11 @@ return {
 fields: [
 'idnssoamname',
 {
+name: 'ip_address',
+validators: [ 'ip_address' ],
+metadata: '@mc-opt:dnszone_add:ip_address'
+},
+{
 name: 'idnssoarname',
 required: false
 },
@@ -576,11 +581,64 @@ IPA.dnszone_adder_dialog = function(spec) {
 
 var that = IPA.entity_adder_dialog(spec);
 
+function ends_with(str, suffix) {
+return str.indexOf(suffix, str.length - suffix.length) !== -1;
+}
+
+var init = function() {
+var zone_w = that.fields.get_field('idnsname').widget;
+var reverse_zone_w = that.fields.get_field('name_from_ip').widget;
+var ns_w = that.fields.get_field('idnssoamname').widget;
+
+zone_w.value_changed.attach(that.check_ns_ip);
+reverse_zone_w.value_changed.attach(that.check_ns_ip);
+ns_w.value_changed.attach(that.check_ns_ip);
+};
+
+that.check_ns_ip = function() {
+var ip_address_f = that.fields.get_field('ip_address');
+var zone_w = that.fields.get_field('idnsname').widget;
+var ns_w = that.fields.get_field('idnssoamname').widget;
+
+var zone = zone_w.save()[0] || '';
+var ns = ns_w.save()[0] || '';
+
+var zone_is_reverse = !zone_w.is_enabled() ||
+  ends_with(zone, '.in-addr.arpa.') ||
+  ends_with(zone, '.ip6.arpa.');
+var relative_ns = true;
+var ns_in_zone = false;
+
+if (ns  ns[ns.length-1] === '.') {
+relative_ns = false;
+ns = ns.slice(0, -1);
+}
+
+if (zone  zone[zone.length-1] === '.') {
+zone = zone.slice(0, -1);
+ 

Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions

2013-05-14 Thread Petr Spacek

On 14.5.2013 11:45, Tomas Babej wrote:

On 05/10/2013 04:57 PM, Petr Spacek wrote:

On 6.5.2013 17:40, Tomas Hozza wrote:

On 04/08/2013 07:45 PM, Petr Spacek wrote:

Generalize attribute_name-rdata_type conversions.

Attribute names are generated on-the-fly: String Record is appended
to textual representation of DNS RDATA type.

String Record is cut down from the attribute name during
attribute name to rdata type conversion.

 From now, the plugin doesn't add artificial limitation to supported
record types.


ACK.

The patch looks good. (I didn't do functional test)

Cosmetic issue:
I think it would be good to dynamically allocate mod_type in LDAPMod
in every case and include the mod_type memory freeing in
free_ldapmod() function. Now one has to be be careful when it is
statically or dynamically allocated. Before it was static in every case.


It is good idea. This version of the patch contains ldap_mod_create()
function which allocates the whole structure including mod_type of fixed
size. All writes to mod_type checks the array length, so it should not cause
any harm.

The function modify_soa_record() still uses statically allocated LDAPMod
structure with statically allocated strings for mod_type, but the LDAPMod
structure never leave this function. There are no calls to ldap_mod_create()
and ldap_mod_free(), so I think it is obvious.

Tbabej, please try to dynamically update some A records with sync_ptr
enabled. (And of course the support for some new type, like TLSA.)

For the existing record types, the patch works fine.

For any new types, a schema change is still required, since record types are
still hardcoded in LDAP schema:

  LDAP error: Object class violation: attribute tlsarecord not allowed


That is expected behaviour. The point is that schema change is much simpler 
than recompiling the bind-dyndb-ldap (and can be done at run-time).


BTW schema file contains instructions how to add support for any record type 
in a way compatible with rest of the world:

http://drift.uninett.no/nett/ip-nett/dnsattributes.schema

Was it ACK?

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0148] Explicitly return SERVFAIL if PTR synchronization is misconfigured.

2013-05-14 Thread Tomas Babej

On 05/09/2013 05:23 PM, Petr Spacek wrote:

On 9.5.2013 14:53, Petr Spacek wrote:

On 9.5.2013 10:59, Tomas Hozza wrote:

On 04/16/2013 12:45 PM, Petr Spacek wrote:

Hello,

Explicitly return SERVFAIL if PTR synchronization is misconfigured.

SERVFAIL will be returned if PTR synchronization is enabled
in forward zone but reverse zone has dynamic updates disabled.



What the patch does little bit differs from what the commit
message says. Explanation follows:

Snip from ldap_helper.c (starting line 2959):

/* Get attribute idnsAllowDynUpdate for reverse zone or use 
default. */

dns_name_free(zone_name, mctx);
dns_name_init(zone_name, NULL);
CHECK(dn_to_dnsname(mctx, owner_zone_dn_ptr, zone_name, NULL));

zone_settings = NULL;
result = zr_get_zone_settings(ldap_inst-zone_register, zone_name,
  zone_settings);
if (result != ISC_R_SUCCESS) {
if (result == ISC_R_NOTFOUND)
log_debug(3, active zone '%s' not found, zone_dn);
goto cleanup;
^
You replaced this goto with CLEANUP_WITH(DNS_R_SERVFAIL) but
the check if dynamic updates in reverse zone are enabled
is done in the following IF statement
}

CHECK(setting_get_bool(dyn_update, zone_settings, zone_dyn_update));
if (!zone_dyn_update) {
log_debug(3, dynamic update is not allowed in zone 
 '%s', zone_dn);
CLEANUP_WITH(ISC_R_NOPERM);
}


The patch modifies the plugin to explicitly return SERVFAIL if there 
was

some error while getting settings of PTR zone (the zone does not exist,
etc).

Maybe it would be good to explicitly return SERVFAIL also if dynamic
updates in PTR zone are disabled and modify the commit message to
better express what this patch does.


You are right. Revised patch is attached.


I sent a bad patch by mistake...



___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

I tested the patch. Works ok, ACK.

Tomas
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] Final OTP Review

2013-05-14 Thread Nathaniel McCallum
On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote:
 On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote:
  On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote:
   On 05/02/2013 10:27 PM, Nathaniel McCallum wrote:
All issues fixed unless noted below... The attached patches are tested
to work.
   
On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote:
   
- (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
(although I know slapi_ch_malloc() currently just aborts on failure, I
think that is plain wrong which is why I would prefer using
malloc/strdup, but well, I guess this is not something I am going to
care too much about for now).
Not fixed.
   
- Is the logic with auth_type_used correct ?
At least the name of the variable sounds misleading, because you set it
only if the authentication was successful but not set if it was 'used'
but was unsuccessful. Made me look at it multiple times to reconstuct
the logic. The var name could be better, however I also want a comment
that explain exactly how we are going to manage authentication and
fallbacks here as that logic is critical and is useful for anyone that
is going to have to change this code later in order not to break it.
The variable is now gone. I re-factored the section to make the logic
clearer and put a nice big comment up top.
   
- General question: how does this PRE_BIND plugin interact with
ipapwd_pre_bind() in the ipa-pwd-extop() plugin ?
Are you going to cause that plugin not to run by returning a result
directly in this function ?
Or is this plugin configured to run only after the previous one went
through ?
I ask because I do not see any ordering information in the cn=config
plugin configuration so it is not immediately clear to me.
That is a good question for Nathan since he wrote this part of the
code...
   We would need to set the precedence if you want a predictable/guaranteed 
   execution order.  If a pre-BIND plug-in callback returns non-zero (which 
   you should do when the plug-in sends the result to the client directly), 
   it will cause other pre-bind plug-ins to not be called.  This is 
   actually how all pre-op plug-ins work.  If a pre-op callback returns an 
   error, we don't call the rest of the pre-op plug-ins in the list.
  
  Ok, but this does not answer my question.
  We definitely need to *always* run our other preop plugin as we do
  sanity checks like verifying if the user is enabled/disabled etc...
  Also we need to understand how to deal with migrating password auth when
  OTP is enabeld.
  
  TBH I think we should not have a separate OTP-auth plugin but we should
  probably have a single plugin that handles authentication and the 2
  should be merged. Keeping them separate is going to cause more harm than
  good with unexpected interactions.
  
  We could still have 2 plugins and simply move the prebind action
  currently don in ipa-pwd-extop to the otp plugin by making some more
  code common. But it is probably easier to just merge OTP into
  ipa-pwd-extop right now than try to do a huge refactoring. We can always
  refactor the ipa-pwd-extop plugin later.
 
 The attached patches encompass an initial review of the companion daemon
 and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp
 into ipa-pwd-extop appears to have broken something during install, but
 I don't have enough familiarity with 389 to understand what I've broken.
 If I upgrade after an install, it appears to work.
 
 An RPM with the patches is available here:
 http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935
 
 Nathan / Rob / Simo, could you take a look and see what might be broken
 in ipa-pwd-extop?

While I'm not quite sure what the problem was, I do know it appeared on
the stock 3.2 F19 RPMs. I also fixed it by accident. I am certain it is
unrelated to these patches.

I have now tested install and upgrade with the six patches in the
previous email and everything is in order, including permissions. At
this point, we just need reviews/ACKs.

Nathaniel


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] DNSSEC support design considerations

2013-05-14 Thread Petr Spacek

Hello list,

I investigated various scenarios for DNSSEC integration and I would like to 
hear your opinions about proposed approach and it's effects.



The most important finding is that bind-dyndb-ldap can't support DNSSEC 
without rewrite of the 'in-memory database' component. Fortunately, it seems 
that we can drop our own implementation of the internal DNS database 
(ldap_driver.c and cache.c) and re-use the database from BIND (so called RBTDB).


I'm trying to reach Adam Tkac with the question Why we decided to implement 
it again rather than re-use BIND's code?.



Re-usage of BIND's implementation will have following properties:


== Advantages ==
- Big part of DNSSEC implementation from BIND9 can be reused.
- Overall plugin implementation will be simpler - we can drop many lines of 
our code and bugs.

- Run-time performance could be much much better.

- We will get implementation for these tickets for free:
-- #95  wildcard CNAME does NOT work
-- #64  IXFR support (IMHO this is important!)
-- #6   Cache non-existing records

And partially:
-- #7   Allow limiting of the cache


== Disadvantages ==
- Support for configurations without persistent search will complicate things 
a lot.
-- Proposal = Make persistent search obligatory. OpenLDAP supports LDAP 
SyncRepl, so it should be possible to make plugin compatible with 389 and 
OpenLDAP at the same time. I would defer this to somebody from users/OpenLDAP 
community.


- Data from LDAP have to be dumped to memory (or to file) before the server 
will start replying to queries.
= This is not nice, but servers usually are not restarted often. IMHO it is a 
good compromise between complexity and performance.
= It should be possible to save old database to disk (during BIND shutdown or 
periodically) and re-use this old database during server startup. I.e. server 
will start replying immediately from 'old' database and then the server will 
switch to the new database when dump from LDAP is finished.
= As a side effect, BIND can start even if connection to LDAP server is down 
- this can improve infrastructure resiliency a lot!



== Uncertain effects ==
- Memory consumption will change, but I'm not sure in which direction.
- SOA serial number maintenance is a open question.


Decision if persistent search is a 'requirement' or not will have significant 
impact on the design, so I will write the design document when this decision 
is made.


--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCH 0153] Update NEWS file for upcoming 3.2 release

2013-05-14 Thread Petr Spacek

Hello,

Update NEWS file for upcoming 3.2 release.

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCH 0154] Bump NVR to 3.2

2013-05-14 Thread Petr Spacek

Hello,

Bump NVR to 3.2.

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] 0027 Prompt for nameserver IP address in dnszone-add

2013-05-14 Thread Petr Vobornik

On 05/14/2013 01:36 PM, Ana Krivokapic wrote:

On 05/14/2013 12:05 PM, Ana Krivokapic wrote:

On 05/13/2013 02:50 PM, Petr Vobornik wrote:

A nitpick for UI part which is not a blocker(nack) because we don't
have any strict rules for following topic:

We should avoid depending on widget's html output outside of the
widget code.

So we should use:
zone_w.save()[0]
instead of:
   $('input', zone_w.container).val();

same for `ns`.

Thanks, fixed.

Unfortunately there is no text_widget.is_enabled() method  so
`zone_w.input.prop('disabled')` can't be replaced.

I implemented the `text_widget.is_enabled()` method, and replaced
`zone_w.input.prop('disabled')` with `!zone_w.is_enabled()`.

Updated patch attached.


Petr caught another bug: due to the return value of
`text_widget.save()`, an exception was raised in the case of empty zone.
This has been fixed in the attached patch.

I also changed the name of the endsWith() function to ends_with(), to
conform to our coding standard.



ACK

--
Petr Vobornik

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0153] Update NEWS file for upcoming 3.2 release

2013-05-14 Thread Petr Spacek

On 14.5.2013 16:20, Petr Spacek wrote:

Update NEWS file for upcoming 3.2 release.


--
Petr^2 Spacek
From 44a27e5cda14cee7e22efa6bbf23e2ef2f9c1e7d Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Tue, 14 May 2013 16:18:31 +0200
Subject: [PATCH] Update NEWS file for upcoming 3.2 release.

Signed-off-by: Petr Spacek pspa...@redhat.com
---
 NEWS | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/NEWS b/NEWS
index 878b667633bf067124a7c05c27ae949f7983db18..e54b47194f127d77bc90eee52ea10a6fdabcf826 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,25 @@
+3.2
+=
+[1] An error in dynamic update/transfer/query policy is interpreted as
+most restrictive policy, i.e. nobody is allowed to update/transfer/query
+the zone.
+https://fedorahosted.org/bind-dyndb-ldap/ticket/116
+
+[2] Attempts to update zones with idnsAllowDynUpdate == FALSE are logged.
+
+[3] TTL values  2^31-1 are interpreted as 0.
+https://fedorahosted.org/bind-dyndb-ldap/ticket/117
+
+[4] All RR types supported by BIND are automatically supported by plugin.
+From now it is enough to add new attribute type to LDAP schema,
+no recompilation is required.
+
+[5] PTR record synchronization deletes only PTR records, but no other records
+(e.g. TXT) under names in the reverse zone.
+
+[6] Various improvements related to logging (dynamic updates, PTR record
+synchronization, LDAP error handling).
+
 3.1
 =
 [1] Crash caused by zone deletion introduced by 
-- 
1.7.11.7

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0154] Bump NVR to 3.2

2013-05-14 Thread Petr Spacek

On 14.5.2013 16:20, Petr Spacek wrote:

Bump NVR to 3.2.


--
Petr^2 Spacek
From 6b5d5adde697ead38cff9199f6e3b126f1fcd488 Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Tue, 14 May 2013 16:18:52 +0200
Subject: [PATCH] Bump NVR to 3.2.

Signed-off-by: Petr Spacek pspa...@redhat.com
---
 configure.ac | 2 +-
 contrib/bind-dyndb-ldap.spec | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 984de44b1c456d8105e42920c6d158bae4676c29..f49ae583f0407fad69e7cd5e5c123f8a867183ec 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,5 +1,5 @@
 AC_PREREQ([2.59])
-AC_INIT([bind-dyndb-ldap], [3.1], [freeipa-devel@redhat.com])
+AC_INIT([bind-dyndb-ldap], [3.2], [freeipa-devel@redhat.com])
 
 AM_INIT_AUTOMAKE([-Wall foreign dist-bzip2])
 
diff --git a/contrib/bind-dyndb-ldap.spec b/contrib/bind-dyndb-ldap.spec
index e1b4f423fe2c315e33f3f92b91cc3e6445e483de..99fd26e13045aee0ec372bb739fc2e6eeccfe3d4 100644
--- a/contrib/bind-dyndb-ldap.spec
+++ b/contrib/bind-dyndb-ldap.spec
@@ -1,7 +1,7 @@
 %define VERSION %{version}
 
 Name:   bind-dyndb-ldap
-Version:3.1
+Version:3.2
 Release:0%{?dist}
 Summary:LDAP back-end plug-in for BIND
 
-- 
1.7.11.7

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0149] Clean up PTR record synchronization code and make it more robust

2013-05-14 Thread Tomas Babej

On 05/13/2013 05:22 PM, Petr Spacek wrote:

On 13.5.2013 16:49, Simo Sorce wrote:

On Mon, 2013-05-13 at 16:32 +0200, Petr Spacek wrote:

+   if ((ip = inet_addr(ip_str)) == INADDR_NONE) {



This kind of construct is hard to read and debug in gdb
I would suggest you do:
ip = inet_addr(ip_str);
if (ip == INADDER_NONE) {


I agree, done.



___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

I tested the patch, works fine. ACK

Tomas
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0149] Clean up PTR record synchronization code and make it more robust

2013-05-14 Thread Petr Spacek

On 14.5.2013 16:41, Tomas Babej wrote:

On 05/13/2013 05:22 PM, Petr Spacek wrote:

On 13.5.2013 16:49, Simo Sorce wrote:

On Mon, 2013-05-13 at 16:32 +0200, Petr Spacek wrote:

+   if ((ip = inet_addr(ip_str)) == INADDR_NONE) {



This kind of construct is hard to read and debug in gdb
I would suggest you do:
ip = inet_addr(ip_str);
if (ip == INADDER_NONE) {


I agree, done.


I tested the patch, works fine. ACK


Pushed to master: 8d12512d0eb4445f4966fd0e326cde9823f6a0bb

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0151] Disallow all zone transfers/queries if transfer/query policy configuration failed

2013-05-14 Thread Petr Spacek

On 9.5.2013 14:20, Tomas Hozza wrote:

On 04/19/2013 12:44 PM, Petr Spacek wrote:

Hello,

Disallow all zone transfers/queries if transfer/query policy
configuration failed.

Without this patch the old policy stays in effect
if re-configuration with the new policy failed.

https://fedorahosted.org/bind-dyndb-ldap/ticket/116



ACK.

Patch looks OK!


Pushed to master: 0a5051392e218702a37073823101cbb6553b9445

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0152] Replace TTL values 2^31-1 with 0.

2013-05-14 Thread Petr Spacek

On 3.5.2013 15:19, Tomas Hozza wrote:

- Original Message -

On 3.5.2013 14:35, Tomas Babej wrote:

On 04/30/2013 03:45 PM, Petr Spacek wrote:

Hello,

Replace TTL values  2^31-1 with 0.

The rule comes from RFC 2181 section 8.

https://fedorahosted.org/bind-dyndb-ldap/ticket/117



___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


ACK, works fine.

Just one question though, the patch as it is leaves the invalid TTL value
in
the tree,
even though it is never interpreted as one (thanks to this patch).

$ ipa dnsrecord-show ipa.example.com skuska --all
dn:
idnsname=skuska,idnsname=ipa.example.com,cn=dns,dc=ipa,dc=example,dc=com
Record name: skuska
Time to live: 2147483648
A record: 192.168.0.1
objectclass: top, idnsrecord

from /var/log/messages:
named[18275]: entry
'idnsname=skuska,idnsname=ipa.example.com,cn=dns,dc=ipa,dc=example,dc=com':
entry TTL 2147483648  MAXTTL, setting TTL to 0

Wouldn't that be confusing to the user? Shouldn't we fix the TTL value set
in
the entry as well?


It is exactly what original BIND does. I would like to imitate the same
behaviour if you are not against it strongly.

I think that:
1) Somebody could use bind-dyndb-ldap with read-only access to LDAP.
2) It will unnecessarily complicate the code.

--
Petr^2 Spacek


Review ACK.

The patch looks good. I also agree with Peter's reasoning. There is also
an error logged when the TTL has MSB set, so one can notice there is a bad
TTL value set in LDAP.


Pushed to master: ccc439e5a5d8d2e0e6dbcb85351f48c501fdad03

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0148] Explicitly return SERVFAIL if PTR synchronization is misconfigured.

2013-05-14 Thread Petr Spacek

On 14.5.2013 15:07, Tomas Babej wrote:

On 05/09/2013 05:23 PM, Petr Spacek wrote:

On 9.5.2013 14:53, Petr Spacek wrote:

On 9.5.2013 10:59, Tomas Hozza wrote:

On 04/16/2013 12:45 PM, Petr Spacek wrote:

Hello,

Explicitly return SERVFAIL if PTR synchronization is misconfigured.

SERVFAIL will be returned if PTR synchronization is enabled
in forward zone but reverse zone has dynamic updates disabled.



What the patch does little bit differs from what the commit
message says. Explanation follows:

Snip from ldap_helper.c (starting line 2959):

/* Get attribute idnsAllowDynUpdate for reverse zone or use default. */
dns_name_free(zone_name, mctx);
dns_name_init(zone_name, NULL);
CHECK(dn_to_dnsname(mctx, owner_zone_dn_ptr, zone_name, NULL));

zone_settings = NULL;
result = zr_get_zone_settings(ldap_inst-zone_register, zone_name,
  zone_settings);
if (result != ISC_R_SUCCESS) {
if (result == ISC_R_NOTFOUND)
log_debug(3, active zone '%s' not found, zone_dn);
goto cleanup;
^
You replaced this goto with CLEANUP_WITH(DNS_R_SERVFAIL) but
the check if dynamic updates in reverse zone are enabled
is done in the following IF statement
}

CHECK(setting_get_bool(dyn_update, zone_settings, zone_dyn_update));
if (!zone_dyn_update) {
log_debug(3, dynamic update is not allowed in zone 
 '%s', zone_dn);
CLEANUP_WITH(ISC_R_NOPERM);
}


The patch modifies the plugin to explicitly return SERVFAIL if there was
some error while getting settings of PTR zone (the zone does not exist,
etc).

Maybe it would be good to explicitly return SERVFAIL also if dynamic
updates in PTR zone are disabled and modify the commit message to
better express what this patch does.


You are right. Revised patch is attached.


I sent a bad patch by mistake...


Pushed to master: 04b48143f592541d3c98e06229987e36dbaf6ec8

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0150] Do not delete whole node during PTR record synchronization.

2013-05-14 Thread Petr Spacek

On 9.5.2013 14:06, Tomas Hozza wrote:

On 04/18/2013 04:58 PM, Petr Spacek wrote:

Hello,

Do not delete whole node during PTR record synchronization.

https://fedorahosted.org/bind-dyndb-ldap/ticket/115



ACK.

The patch looks good.


Pushed to master: 1c63c045b5238fb675b7a517876869bcace2cdab

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions

2013-05-14 Thread Tomas Babej

On 05/14/2013 02:01 PM, Petr Spacek wrote:

On 14.5.2013 11:45, Tomas Babej wrote:

On 05/10/2013 04:57 PM, Petr Spacek wrote:

On 6.5.2013 17:40, Tomas Hozza wrote:

On 04/08/2013 07:45 PM, Petr Spacek wrote:

Generalize attribute_name-rdata_type conversions.

Attribute names are generated on-the-fly: String Record is appended
to textual representation of DNS RDATA type.

String Record is cut down from the attribute name during
attribute name to rdata type conversion.

 From now, the plugin doesn't add artificial limitation to supported
record types.


ACK.

The patch looks good. (I didn't do functional test)

Cosmetic issue:
I think it would be good to dynamically allocate mod_type in LDAPMod
in every case and include the mod_type memory freeing in
free_ldapmod() function. Now one has to be be careful when it is
statically or dynamically allocated. Before it was static in every 
case.


It is good idea. This version of the patch contains ldap_mod_create()
function which allocates the whole structure including mod_type of 
fixed
size. All writes to mod_type checks the array length, so it should 
not cause

any harm.

The function modify_soa_record() still uses statically allocated 
LDAPMod
structure with statically allocated strings for mod_type, but the 
LDAPMod
structure never leave this function. There are no calls to 
ldap_mod_create()

and ldap_mod_free(), so I think it is obvious.

Tbabej, please try to dynamically update some A records with sync_ptr
enabled. (And of course the support for some new type, like TLSA.)

For the existing record types, the patch works fine.

For any new types, a schema change is still required, since record 
types are

still hardcoded in LDAP schema:

  LDAP error: Object class violation: attribute tlsarecord not allowed


That is expected behaviour. The point is that schema change is much 
simpler than recompiling the bind-dyndb-ldap (and can be done at 
run-time).


BTW schema file contains instructions how to add support for any 
record type in a way compatible with rest of the world:

http://drift.uninett.no/nett/ip-nett/dnsattributes.schema

Was it ACK?

I was not nacking, just pointing out. Tested with changed schema, works 
as expected.


Here, have my ACK.

Tomas

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions

2013-05-14 Thread Petr Spacek

On 14.5.2013 17:42, Tomas Babej wrote:

On 05/14/2013 02:01 PM, Petr Spacek wrote:

On 14.5.2013 11:45, Tomas Babej wrote:

On 05/10/2013 04:57 PM, Petr Spacek wrote:

On 6.5.2013 17:40, Tomas Hozza wrote:

On 04/08/2013 07:45 PM, Petr Spacek wrote:

Generalize attribute_name-rdata_type conversions.

Attribute names are generated on-the-fly: String Record is appended
to textual representation of DNS RDATA type.

String Record is cut down from the attribute name during
attribute name to rdata type conversion.

 From now, the plugin doesn't add artificial limitation to supported
record types.


ACK.

The patch looks good. (I didn't do functional test)

Cosmetic issue:
I think it would be good to dynamically allocate mod_type in LDAPMod
in every case and include the mod_type memory freeing in
free_ldapmod() function. Now one has to be be careful when it is
statically or dynamically allocated. Before it was static in every case.


It is good idea. This version of the patch contains ldap_mod_create()
function which allocates the whole structure including mod_type of fixed
size. All writes to mod_type checks the array length, so it should not cause
any harm.

The function modify_soa_record() still uses statically allocated LDAPMod
structure with statically allocated strings for mod_type, but the LDAPMod
structure never leave this function. There are no calls to ldap_mod_create()
and ldap_mod_free(), so I think it is obvious.

Tbabej, please try to dynamically update some A records with sync_ptr
enabled. (And of course the support for some new type, like TLSA.)

For the existing record types, the patch works fine.

For any new types, a schema change is still required, since record types are
still hardcoded in LDAP schema:

  LDAP error: Object class violation: attribute tlsarecord not allowed


That is expected behaviour. The point is that schema change is much simpler
than recompiling the bind-dyndb-ldap (and can be done at run-time).

BTW schema file contains instructions how to add support for any record type
in a way compatible with rest of the world:
http://drift.uninett.no/nett/ip-nett/dnsattributes.schema

Was it ACK?


I was not nacking, just pointing out. Tested with changed schema, works as
expected.

Here, have my ACK.


Pushed to master: 35ac3e422376cc28040d03c7550c2c68a967a0cf

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0143] Treat syntax errors in LDAP filters as fatal

2013-05-14 Thread Petr Spacek

On 14.5.2013 11:47, Tomas Babej wrote:

On 05/07/2013 10:16 AM, Tomas Hozza wrote:

On 04/09/2013 03:39 PM, Petr Spacek wrote:

Hello,

Treat syntax errors in LDAP filters as fatal.

Filters are hardcoded at the moment, this is preventive action.


ACK.

The patch looks good. (I didn't do functional test)


Regards,

Tomas Hozza


ACK. It does not break anything :)


Pushed to master: 33133d9eaf757aef520e334e7e2eacce959f929c

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0142] Improve LDAP error logging

2013-05-14 Thread Petr Spacek

On 14.5.2013 11:46, Tomas Babej wrote:

On 05/07/2013 09:36 AM, Tomas Hozza wrote:

On 04/09/2013 03:27 PM, Petr Spacek wrote:

Hello,

Improve LDAP error logging.

Diagnostic error message is logged when it is available.


Plugin with this patch produces messages like:

LDAP error: Server is unwilling to perform: Minimum SSF not met.: bind
to LDAP server failed

intead of

bind to LDAP server failed: Server is unwilling to perform


Second example is:

LDAP error: Object class violation: attribute mgrecord not allowed
: while modifying(add) entry 'idnsName=pspacek,
idnsname=example.com,cn=dns,dc=e,dc=test'

instead of



:-D


snip

diff --git a/src/log.h b/src/log.h
index
312f24322fd0c6f9943c6beb810ac0bcd8f3896c..cbf1a3faaaccea7391d65d018e80d8ec688fc111
100644

--- a/src/log.h

+++ b/src/log.h

@@ -55,16 +55,30 @@

log_write(GET_LOG_LEVEL(level), format, ##__VA_ARGS__)
/* LDAP logging functions */
-#define log_ldap_error(ld) \
- do { \
- int err; \
- char *errmsg = UNKNOWN; \
- if (ldap_get_option(ld, LDAP_OPT_RESULT_CODE, err) \
- == LDAP_OPT_SUCCESS) \
- errmsg = ldap_err2string(err); \
- log_error_position(LDAP error: %s, errmsg); \
- } while (0); \
+#define LOG_LDAP_ERR_PREFIX LDAP error: 
+#define log_ldap_error(ld, desc, ...) \
+ do { \
+ int err; \
+ char *errmsg = NULL; \
+ char *diagmsg = NULL; \
+ if (ldap_get_option(ld, LDAP_OPT_RESULT_CODE,
err) \
+ == LDAP_OPT_SUCCESS) { \
+ errmsg = ldap_err2string(err); \

Getting error msg for the first time here.


+ if (ldap_get_option(ld, LDAP_OPT_DIAGNOSTIC_MESSAGE,
diagmsg) \
+ == LDAP_OPT_SUCCESS  diagmsg != NULL) { \
+ errmsg = ldap_err2string(err);\

Again getting error msg with the same err. Maybe a copy-paste error?


+ log_error(LOG_LDAP_ERR_PREFIX %s: %s:  desc, \
+ errmsg, diagmsg, ##__VA_ARGS__); \
+ ldap_memfree(diagmsg); \
+ } else \
+ log_error(LOG_LDAP_ERR_PREFIX %s:  desc, \
+ errmsg, ##__VA_ARGS__); \
+ } else { \
+ log_error(LOG_LDAP_ERR_PREFIX \
+ unable to obtain LDAP error code:  \
+ desc, ##__VA_ARGS__); \
+ } \
+ } while (0);
void
log_write(int level, const char *format, ...) ISC_FORMAT_PRINTF(2, 3);


Regards,

Tomas Hozza


ACK, provides the desired info.


Pushed to master: af83758cb3f91129399494c95a1847814b1d71a8

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0153] Update NEWS file for upcoming 3.2 release

2013-05-14 Thread Petr Spacek

On 14.5.2013 16:36, Petr Spacek wrote:

On 14.5.2013 16:20, Petr Spacek wrote:

Update NEWS file for upcoming 3.2 release.


Pushed to master: bc647206677f54ecf14eb7f77984ec25070ddfc2

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0154] Bump NVR to 3.2

2013-05-14 Thread Petr Spacek

On 14.5.2013 16:37, Petr Spacek wrote:

On 14.5.2013 16:20, Petr Spacek wrote:

Bump NVR to 3.2.


Pushed to master: eebb0679be611e325ce6c2938ac6c8ca5e734ea6

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] Build fixes for F19 for bind-dyndb-ldap

2013-05-14 Thread Tomas Babej

Hi,

This patch contains various (gcc-related) fixes to make build
on F19 possible.

Tomas
From 4374791563602f026b870b0c37979506f02683ae Mon Sep 17 00:00:00 2001
From: Tomas Babej tba...@redhat.com
Date: Mon, 6 May 2013 07:23:08 -0400
Subject: [PATCH] Build fixes for Fedora 19

This patch contains various (gcc-related) fixes to make build
on F19 possible.
---
 src/acl.c | 4 ++--
 src/ldap_helper.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/acl.c b/src/acl.c
index 754cd53dc3d31b99d0954836feafbd46747c48c2..2d4f54db89c73b7e9387f21bbd94eedc364c152b 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -179,7 +179,7 @@ parse(cfg_parser_t *parser, const char *string, cfg_type_t **type,
 	RUNTIME_CHECK(isc_once_do(once, init_cfgtypes) == ISC_R_SUCCESS);
 
 	string_len = strlen(string);
-	isc_buffer_init(buffer, string, string_len);
+	isc_buffer_init(buffer, (char *)string, string_len);
 	isc_buffer_add(buffer, string_len);
 
 	result = cfg_parse_buffer(parser, buffer, *type, ret);
@@ -296,7 +296,7 @@ get_fixed_name(const cfg_obj_t *obj, const char *name, dns_fixedname_t *fname)
 	str = cfg_obj_asstring(obj);
 
 	len = strlen(str);
-	isc_buffer_init(buf, str, len);
+	isc_buffer_init(buf, (char *)str, len);
 
 	/*
 	 * Workaround for https://bugzilla.redhat.com/show_bug.cgi?id=728925
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 037629f3e50fdba2c17c1a180736ed3b64169aae..eda17fa8fed63e43d702c7ad9984b176e0b27065 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -2037,7 +2037,7 @@ parse_rdata(isc_mem_t *mctx, ldap_qresult_t *qresult,
 	text.base = rdata_text;
 	text.length = strlen(text.base);
 
-	isc_buffer_init(lex_buffer, text.base, text.length);
+	isc_buffer_init(lex_buffer, (char *)text.base, text.length);
 	isc_buffer_add(lex_buffer, text.length);
 	isc_buffer_setactive(lex_buffer, text.length);
 
-- 
1.8.1.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] Build fixes for F19 for bind-dyndb-ldap

2013-05-14 Thread Petr Spacek

On 14.5.2013 18:16, Tomas Babej wrote:

This patch contains various (gcc-related) fixes to make build
on F19 possible.


Congratulations to your first patch for bind-dyndb-ldap! :-)

ACK

Pushed to master: c5716e660e145ebec6ff091e2479f93e4c54d464

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCH] 413 Fix: HBAC Test tab is missing

2013-05-14 Thread Petr Vobornik

Fix for really stupid mistake with quite big impact...

Caused by typo in metadata provider source path.

No metadata - no HBAC test entity - no tab

https://fedorahosted.org/freeipa/ticket/3627
--
Petr Vobornik
From 5b1fc24ae1e28ce8a775fb7923cedfba397a210f Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Tue, 14 May 2013 18:28:49 +0200
Subject: [PATCH] Fix: HBAC Test tab is missing

Caused by typo in metadata provider source path.

No metadata - no HBAC test entity - no tab

https://fedorahosted.org/freeipa/ticket/3627
---
 install/ui/src/freeipa/_base/metadata_provider.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/_base/metadata_provider.js b/install/ui/src/freeipa/_base/metadata_provider.js
index fcb072395c1cf92982cc1bec66df045bb0a94abe..9a332b55600bfe0189733987edf4e1aa50f6d72c 100644
--- a/install/ui/src/freeipa/_base/metadata_provider.js
+++ b/install/ui/src/freeipa/_base/metadata_provider.js
@@ -32,7 +32,7 @@ define(['dojo/_base/lang', './Provider', './Search_provider'],
 var commmads = new Provider({
 code: '@mc:',
 source: metadata,
-path: 'commmads'
+path: 'commands'
 });
 var object_param = new Search_provider({
 code: '@mo-param:',
-- 
1.8.1.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH] 407 Set KRB5CCNAME so that dirsrv can work with newer krb5-server

2013-05-14 Thread Martin Kosek
The DIR ccache format is now the default in krb5-server 1.11.2-4
but /run/user/uid isn't created for Apache by anything so it
has no ccache (and it doesn't have SELinux permissions to write here
either).

Use KRB5CCNAME to set a file path instead in /etc/sysconfig/dirsrv.

https://fedorahosted.org/freeipa/ticket/3628



Without this patch, replication on F19 is broken.

Martin
From 1be93108c4c1506ea50879d645c47ab6843a6ee1 Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Tue, 14 May 2013 18:36:50 +0200
Subject: [PATCH] Set KRB5CCNAME so that dirsrv can work with newer krb5-server

The DIR ccache format is now the default in krb5-server 1.11.2-4
but /run/user/uid isn't created for Apache by anything so it
has no ccache (and it doesn't have SELinux permissions to write here
either).

Use KRB5CCNAME to set a file path instead in /etc/sysconfig/dirsrv.

https://fedorahosted.org/freeipa/ticket/3628
---
 install/tools/ipa-upgradeconfig |  1 +
 ipaserver/install/dsinstance.py | 18 ++
 2 files changed, 19 insertions(+)

diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig
index 8fa9b189a2dc207e2d90ab32131e65fac0f1f9e0..8e9357f20fe7c9a88908def6a2e3b2104f07d73a 100644
--- a/install/tools/ipa-upgradeconfig
+++ b/install/tools/ipa-upgradeconfig
@@ -919,6 +919,7 @@ def main():
 http.configure_httpd_ccache()
 
 ds = dsinstance.DsInstance()
+ds.configure_dirsrv_ccache()
 
 fix_schema_file_syntax(ds)
 
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index e6bb054ddad4a0d91d76d4c79eb477913e8776aa..3b841417e717587675d3ac748ec02182b3e14672 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -26,6 +26,7 @@
 import time
 import tempfile
 import base64
+import stat
 
 from ipapython.ipa_log_manager import *
 from ipapython import ipautil, sysrestore, dogtag, ipaldap
@@ -213,6 +214,7 @@ def __common_setup(self, enable_ssl=False):
 self.step(configuring certmap.conf, self.__certmap_conf)
 self.step(configure autobind for root, self.__root_autobind)
 self.step(configure new location for managed entries, self.__repoint_managed_entries)
+self.step(configure dirsrv ccache, self.configure_dirsrv_ccache)
 self.step(restarting directory server, self.__restart_instance)
 
 def __common_post_setup(self):
@@ -515,6 +517,22 @@ def __config_lockout_module(self):
 def __repoint_managed_entries(self):
 self._ldap_mod(repoint-managed-entries.ldif, self.sub_dict)
 
+def configure_dirsrv_ccache(self):
+pent = pwd.getpwnam(dirsrv)
+ccache = '/tmp/krb5cc_%d' % pent.pw_uid
+filepath = '/etc/sysconfig/dirsrv'
+if not os.path.exists(filepath):
+# file doesn't exist; create it with correct ownership  mode
+open(filepath, 'a').close()
+os.chmod(filepath,
+stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
+os.chown(filepath, 0, 0)
+
+replacevars = {'KRB5CCNAME': ccache}
+old_values = ipautil.backup_config_and_replace_variables(
+self.fstore, filepath, replacevars=replacevars)
+ipaservices.restore_context(filepath)
+
 def __managed_entries(self):
 self._ldap_mod(managed-entries.ldif, self.sub_dict)
 
-- 
1.8.1.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 413 Fix: HBAC Test tab is missing

2013-05-14 Thread Martin Kosek
On 05/14/2013 06:37 PM, Petr Vobornik wrote:
 Fix for really stupid mistake with quite big impact...
 
 Caused by typo in metadata provider source path.
 
 No metadata - no HBAC test entity - no tab
 
 https://fedorahosted.org/freeipa/ticket/3627
 

ACK. Pushed to master.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] 225 Remove leading zero from IPA_NUM_VERSION

2013-05-14 Thread Martin Kosek
On 05/13/2013 03:40 PM, Petr Viktorin wrote:
 On 05/13/2013 02:58 PM, Rob Crittenden wrote:
 Petr Viktorin wrote:
 Due to my mistake in commit 9125285, the IPA_NUM_VERSION variable
 contained a leading zero, so it was treated as octal in Python code.
 Bumping the development version to 3.2.99 resulted in 030299, an invalid
 value.
 Luckily, 320  030200  30200 so the ordering is not broken.

 Sorry for the mistake.


 I think at least a comment somewhere is required too, probably in
 version.h, describing which versions were affected by this octal problem.

 rob
 
 Added.
 

This helps, thanks. ACK, pushed to master, ipa-3-1.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] Final OTP Review

2013-05-14 Thread Martin Kosek
On 05/14/2013 03:53 PM, Nathaniel McCallum wrote:
 On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote:
 On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote:
 On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote:
 On 05/02/2013 10:27 PM, Nathaniel McCallum wrote:
 All issues fixed unless noted below... The attached patches are tested
 to work.

 On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote:

 - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
 (although I know slapi_ch_malloc() currently just aborts on failure, I
 think that is plain wrong which is why I would prefer using
 malloc/strdup, but well, I guess this is not something I am going to
 care too much about for now).
 Not fixed.

 - Is the logic with auth_type_used correct ?
 At least the name of the variable sounds misleading, because you set it
 only if the authentication was successful but not set if it was 'used'
 but was unsuccessful. Made me look at it multiple times to reconstuct
 the logic. The var name could be better, however I also want a comment
 that explain exactly how we are going to manage authentication and
 fallbacks here as that logic is critical and is useful for anyone that
 is going to have to change this code later in order not to break it.
 The variable is now gone. I re-factored the section to make the logic
 clearer and put a nice big comment up top.

 - General question: how does this PRE_BIND plugin interact with
 ipapwd_pre_bind() in the ipa-pwd-extop() plugin ?
 Are you going to cause that plugin not to run by returning a result
 directly in this function ?
 Or is this plugin configured to run only after the previous one went
 through ?
 I ask because I do not see any ordering information in the cn=config
 plugin configuration so it is not immediately clear to me.
 That is a good question for Nathan since he wrote this part of the
 code...
 We would need to set the precedence if you want a predictable/guaranteed 
 execution order.  If a pre-BIND plug-in callback returns non-zero (which 
 you should do when the plug-in sends the result to the client directly), 
 it will cause other pre-bind plug-ins to not be called.  This is 
 actually how all pre-op plug-ins work.  If a pre-op callback returns an 
 error, we don't call the rest of the pre-op plug-ins in the list.

 Ok, but this does not answer my question.
 We definitely need to *always* run our other preop plugin as we do
 sanity checks like verifying if the user is enabled/disabled etc...
 Also we need to understand how to deal with migrating password auth when
 OTP is enabeld.

 TBH I think we should not have a separate OTP-auth plugin but we should
 probably have a single plugin that handles authentication and the 2
 should be merged. Keeping them separate is going to cause more harm than
 good with unexpected interactions.

 We could still have 2 plugins and simply move the prebind action
 currently don in ipa-pwd-extop to the otp plugin by making some more
 code common. But it is probably easier to just merge OTP into
 ipa-pwd-extop right now than try to do a huge refactoring. We can always
 refactor the ipa-pwd-extop plugin later.

 The attached patches encompass an initial review of the companion daemon
 and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp
 into ipa-pwd-extop appears to have broken something during install, but
 I don't have enough familiarity with 389 to understand what I've broken.
 If I upgrade after an install, it appears to work.

 An RPM with the patches is available here:
 http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935

 Nathan / Rob / Simo, could you take a look and see what might be broken
 in ipa-pwd-extop?
 
 While I'm not quite sure what the problem was, I do know it appeared on
 the stock 3.2 F19 RPMs. I also fixed it by accident. I am certain it is
 unrelated to these patches.
 
 I have now tested install and upgrade with the six patches in the
 previous email and everything is in order, including permissions. At
 this point, we just need reviews/ACKs.
 
 Nathaniel
 

I tested IPA server upgrades, new installs and also adding 3.2+OTP replica for
F18 3.1.4 IPA master. Everything seemed to work fine (when I added my patch 407
fixing the replication), I did not see any breakage.

Issues I found with too much logging I reported should now be fixed on github,
so this should be OK.

So it is an ACK from my side if Rob does not discover some blocking issue.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] 407 Set KRB5CCNAME so that dirsrv can work with newer krb5-server

2013-05-14 Thread Rob Crittenden

Martin Kosek wrote:

The DIR ccache format is now the default in krb5-server 1.11.2-4
but /run/user/uid isn't created for Apache by anything so it
has no ccache (and it doesn't have SELinux permissions to write here
either).

Use KRB5CCNAME to set a file path instead in /etc/sysconfig/dirsrv.

https://fedorahosted.org/freeipa/ticket/3628



Without this patch, replication on F19 is broken.

Martin



ACK, pushed to master and ipa-3-2

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel