Re: [Freeipa-devel] Continuous Integration dependency tree testing
On Wed, Sep 24, 2014 at 08:47:57AM +0200, Martin Kosek wrote: There is a good question though how the test should be triggered when our dependency changes and the code does not (especially in stable branches). This may be the part where the mechanism you propose would come in play. Can't you just run the tests once per day, no matter what? -- Jan Pazdziora Principal Software Engineer, Identity Management Engineering, Red Hat ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Continuous Integration dependency tree testing
On 09/29/2014 08:31 AM, Jan Pazdziora wrote: On Wed, Sep 24, 2014 at 08:47:57AM +0200, Martin Kosek wrote: There is a good question though how the test should be triggered when our dependency changes and the code does not (especially in stable branches). This may be the part where the mechanism you propose would come in play. Can't you just run the tests once per day, no matter what? Sure, I hope Jenkins would make that easy. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 757 webui: do not offer ipa-ad-winsync and ipa-ipa-trust range types
On 24.9.2014 12:17, Petr Vobornik wrote:
webui: do not offer ipa-ad-winsync and ipa-ipa-trust range types
They are not supported by API.
Forgot to attach patch...
--
Petr Vobornik
From b9edc41752d173b36c52923634b33028064180ed Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 3 Sep 2014 16:57:40 +0200
Subject: [PATCH] webui: do not offer ipa-ad-winsync and ipa-ipa-trust range
types
They are not supported by API.
---
install/ui/src/freeipa/idrange.js | 8
1 file changed, 8 deletions(-)
diff --git a/install/ui/src/freeipa/idrange.js b/install/ui/src/freeipa/idrange.js
index 4cc6fb11d71b8d6ff369c814f6fe5632830baf33..3bf913ab40bb6fd288612343befc03e19d7d1801 100644
--- a/install/ui/src/freeipa/idrange.js
+++ b/install/ui/src/freeipa/idrange.js
@@ -121,14 +121,6 @@ return {
{
value: 'ipa-ad-trust-posix',
label: '@i18n:objects.idrange.type_ad_posix'
-},
-{
-value: 'ipa-ad-winsync',
-label: '@i18n:objects.idrange.type_winsync'
-},
-{
-value: 'ipa-ipa-trust',
-label: '@i18n:objects.idrange.type_ipa'
}
]
},
--
1.9.3
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Continuous Integration dependency tree testing
On 29.9.2014 08:31, Jan Pazdziora wrote: On Wed, Sep 24, 2014 at 08:47:57AM +0200, Martin Kosek wrote: There is a good question though how the test should be triggered when our dependency changes and the code does not (especially in stable branches). This may be the part where the mechanism you propose would come in play. Can't you just run the tests once per day, no matter what? You can but then it would be harder to detect which particular update broke IPA. (Imagine that there was 10 different package updates in the same day. You would have to bisect the find the broken package.) -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0017 Do not require description in UI.
https://fedorahosted.org/freeipa/ticket/4387
--
David Kupka
From 8a0ac7417e904c21946e08bbdd759550bffab5ad Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Fri, 26 Sep 2014 02:54:28 -0400
Subject: [PATCH] Do not require description in UI.
Description attribute is not required in LDAP schema so there is no reason to
require it in UI. Modified tests to reflect this change.
https://fedorahosted.org/freeipa/ticket/4387
---
API.txt | 14 +++---
ipalib/plugins/group.py | 2 +-
ipalib/plugins/hbacsvcgroup.py| 2 +-
ipalib/plugins/hostgroup.py | 2 +-
ipalib/plugins/netgroup.py| 2 +-
ipalib/plugins/privilege.py | 2 +-
ipalib/plugins/role.py| 2 +-
ipalib/plugins/sudocmdgroup.py| 2 +-
ipatests/test_cmdline/test_cli.py | 1 -
ipatests/test_xmlrpc/test_batch_plugin.py | 9 +
10 files changed, 15 insertions(+), 23 deletions(-)
diff --git a/API.txt b/API.txt
index bbd0f507b2faeec0239920cdcff28fe25d618e02..d8078b98a7ea7f2e51e4eec7f04b7321781318c7 100644
--- a/API.txt
+++ b/API.txt
@@ -1296,7 +1296,7 @@ args: 1,10,3
arg: Str('cn', attribute=True, cli_name='group_name', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Flag('external', autofill=True, cli_name='external', default=False)
option: Int('gidnumber', attribute=True, cli_name='gid', minvalue=1, multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
@@ -1681,7 +1681,7 @@ args: 1,7,3
arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('setattr*', cli_name='setattr', exclude='webui')
@@ -1931,7 +1931,7 @@ args: 1,7,3
arg: Str('cn', attribute=True, cli_name='hostgroup_name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('setattr*', cli_name='setattr', exclude='webui')
@@ -2180,7 +2180,7 @@ args: 1,11,3
arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Str('externalhost', attribute=True, cli_name='externalhost', multivalue=True, required=False)
option: StrEnum('hostcategory', attribute=True, cli_name='hostcat', multivalue=False, required=False, values=(u'all',))
option: Str('nisdomainname', attribute=True, cli_name='nisdomain', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', required=False)
@@ -2608,7 +2608,7 @@ args: 1,7,3
arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('setattr*', cli_name='setattr', exclude='webui')
@@
Re: [Freeipa-devel] [PATCH] 0017 Do not require description in UI.
On 09/29/2014 10:09 AM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4387 The changes look OK so far, except the test fix. The test_batch_plugin.py test is apparently testing that batch command behaves well in RequirementError for options. Thus, we should not remove it, we should just pick different option. Like filling uid+first options with user-add, but missing the --last option. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0017 Do not require description in UI.
On 09/29/2014 10:22 AM, Martin Kosek wrote:
On 09/29/2014 10:09 AM, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/4387
The changes look OK so far, except the test fix.
The test_batch_plugin.py test is apparently testing that batch command behaves
well in RequirementError for options. Thus, we should not remove it, we should
just pick different option. Like filling uid+first options with user-add, but
missing the --last option.
Martin
Ok, but the test is bit redundant as there is already test for missing
givenname that should behave the same way.
--
David Kupka
From 1e661dc28aaec2d9e1cffb873c193da7221eefd9 Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Fri, 26 Sep 2014 02:54:28 -0400
Subject: [PATCH] Do not require description in UI.
Description attribute is not required in LDAP schema so there is no reason to
require it in UI. Modified tests to reflect this change.
https://fedorahosted.org/freeipa/ticket/4387
---
API.txt | 14 +++---
ipalib/plugins/group.py | 2 +-
ipalib/plugins/hbacsvcgroup.py| 2 +-
ipalib/plugins/hostgroup.py | 2 +-
ipalib/plugins/netgroup.py| 2 +-
ipalib/plugins/privilege.py | 2 +-
ipalib/plugins/role.py| 2 +-
ipalib/plugins/sudocmdgroup.py| 2 +-
ipatests/test_cmdline/test_cli.py | 1 -
ipatests/test_xmlrpc/test_batch_plugin.py | 5 +++--
10 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/API.txt b/API.txt
index bbd0f507b2faeec0239920cdcff28fe25d618e02..d8078b98a7ea7f2e51e4eec7f04b7321781318c7 100644
--- a/API.txt
+++ b/API.txt
@@ -1296,7 +1296,7 @@ args: 1,10,3
arg: Str('cn', attribute=True, cli_name='group_name', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Flag('external', autofill=True, cli_name='external', default=False)
option: Int('gidnumber', attribute=True, cli_name='gid', minvalue=1, multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
@@ -1681,7 +1681,7 @@ args: 1,7,3
arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('setattr*', cli_name='setattr', exclude='webui')
@@ -1931,7 +1931,7 @@ args: 1,7,3
arg: Str('cn', attribute=True, cli_name='hostgroup_name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('setattr*', cli_name='setattr', exclude='webui')
@@ -2180,7 +2180,7 @@ args: 1,11,3
arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
-option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True)
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Str('externalhost', attribute=True, cli_name='externalhost', multivalue=True, required=False)
option: StrEnum('hostcategory', attribute=True, cli_name='hostcat', multivalue=False, required=False, values=(u'all',))
option: Str('nisdomainname', attribute=True, cli_name='nisdomain', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', required=False)
@@ -2608,7 +2608,7 @@ args: 1,7,3
arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
option: Str('addattr*',
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
Dne 29.9.2014 v 05:16 Fraser Tweedale napsal(a): On Fri, Sep 26, 2014 at 10:44:16AM -0400, Simo Sorce wrote: On Fri, 26 Sep 2014 13:54:34 +0200 Martin Kosek mko...@redhat.com wrote: I tested the patch (it works fine with Dogtag 10), but I got very confused. What CA option are we setting? Signing algorithm or Key Algorithm? I thought we are only setting Signing algorithm, but in that case: We are setting key algorithm for the CA signing key. That did not made me any less confused... If I check for example fields from certificate details from my browser, I see 2 algorithms names: * Public Key Algorithm (RSA, ECC, ...) * Certificate Signature Algorithm (SHA-1 with RSA, SHA-256 with RSA, something with ECC) In that world, key algorithm should really refer to the key PKI algorithm, i.e. RSA, ECC, ... Signature algorithms is where hashes come to play. - --ca-key-algorithm option should rather read --ca-signing-key-algorithm If you want to emphasize that it is actually the algorithm used to sign the CA certificate, the option should read --ca-certificate-signature-algorithm, but I would rather stick to Dogtag terminology and keep the string key algorithm in the name. I still think for most people key algorithm refers to Public Key algorithm. Rob or Simo, what is your take on this? If we are defining the signing algorithm the signing string should be somewhere in the option. Having just --key-algorithm is indeed confusing. Simo. My take is that the terminology should be chosen in line with standards. The X.509 field is called `signatureAlgorithm' so `--ca-certificate-signature-algorithm' makes sense to me. Consistency with Dogtag terminology is a secondary consideration considering FreeIPA users are unlikely to interact directly with Dogtag much (especially during installation). Fraser I think it actually sets both the key algorithm and the signature algorithm (you can't do a RSA signature with a EC key, etc.), that's probably why it is called key algorithm in Dogtag. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
On 09/29/2014 11:11 AM, Jan Cholasta wrote: Dne 29.9.2014 v 05:16 Fraser Tweedale napsal(a): On Fri, Sep 26, 2014 at 10:44:16AM -0400, Simo Sorce wrote: On Fri, 26 Sep 2014 13:54:34 +0200 Martin Kosek mko...@redhat.com wrote: I tested the patch (it works fine with Dogtag 10), but I got very confused. What CA option are we setting? Signing algorithm or Key Algorithm? I thought we are only setting Signing algorithm, but in that case: We are setting key algorithm for the CA signing key. That did not made me any less confused... If I check for example fields from certificate details from my browser, I see 2 algorithms names: * Public Key Algorithm (RSA, ECC, ...) * Certificate Signature Algorithm (SHA-1 with RSA, SHA-256 with RSA, something with ECC) In that world, key algorithm should really refer to the key PKI algorithm, i.e. RSA, ECC, ... Signature algorithms is where hashes come to play. - --ca-key-algorithm option should rather read --ca-signing-key-algorithm If you want to emphasize that it is actually the algorithm used to sign the CA certificate, the option should read --ca-certificate-signature-algorithm, but I would rather stick to Dogtag terminology and keep the string key algorithm in the name. I still think for most people key algorithm refers to Public Key algorithm. Rob or Simo, what is your take on this? If we are defining the signing algorithm the signing string should be somewhere in the option. Having just --key-algorithm is indeed confusing. Simo. My take is that the terminology should be chosen in line with standards. The X.509 field is called `signatureAlgorithm' so `--ca-certificate-signature-algorithm' makes sense to me. Consistency with Dogtag terminology is a secondary consideration considering FreeIPA users are unlikely to interact directly with Dogtag much (especially during installation). Fraser I think it actually sets both the key algorithm and the signature algorithm (you can't do a RSA signature with a EC key, etc.), that's probably why it is called key algorithm in Dogtag. Hm, you are right that the key algorithm is implied during signature algorithm selection. But still, values SHA256withRSA and friends really denote just a signature algorithm and the option should be named accordingly. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0017 Do not require description in UI.
On 09/29/2014 11:04 AM, David Kupka wrote: On 09/29/2014 10:22 AM, Martin Kosek wrote: On 09/29/2014 10:09 AM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4387 The changes look OK so far, except the test fix. The test_batch_plugin.py test is apparently testing that batch command behaves well in RequirementError for options. Thus, we should not remove it, we should just pick different option. Like filling uid+first options with user-add, but missing the --last option. Martin Ok, but the test is bit redundant as there is already test for missing givenname that should behave the same way. I think it is useful to keep the test here, in the previous one no option was added. Anyway, this should not stop this patch from going in. I checked the test results + Web UI and all looks OK. ACK. Pushed to: master: cd9a4cca1fe17998a342fde000ece5bf46d13d27 ipa-4-1: b69510b9bf8216d52707968bf520fd2dfa6e1ba7 Thanks, Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
Dne 29.9.2014 v 12:20 Martin Kosek napsal(a):
On 09/29/2014 11:11 AM, Jan Cholasta wrote:
Dne 29.9.2014 v 05:16 Fraser Tweedale napsal(a):
On Fri, Sep 26, 2014 at 10:44:16AM -0400, Simo Sorce wrote:
On Fri, 26 Sep 2014 13:54:34 +0200
Martin Kosek mko...@redhat.com wrote:
I tested the patch (it works fine with Dogtag 10), but I got very
confused.
What CA option are we setting? Signing algorithm or Key Algorithm?
I thought we are only setting Signing algorithm, but in that
case:
We are setting key algorithm for the CA signing key.
That did not made me any less confused... If I check for example
fields from certificate details from my browser, I see 2 algorithms
names:
* Public Key Algorithm (RSA, ECC, ...)
* Certificate Signature Algorithm (SHA-1 with RSA, SHA-256 with RSA,
something with ECC)
In that world, key algorithm should really refer to the key PKI
algorithm, i.e. RSA, ECC, ... Signature algorithms is where hashes
come to play.
- --ca-key-algorithm option should rather read
--ca-signing-key-algorithm
If you want to emphasize that it is actually the algorithm used to
sign the CA certificate, the option should read
--ca-certificate-signature-algorithm, but I would rather stick to
Dogtag terminology and keep the string key algorithm in the
name.
I still think for most people key algorithm refers to Public Key
algorithm. Rob or Simo, what is your take on this?
If we are defining the signing algorithm the signing string should be
somewhere in the option.
Having just --key-algorithm is indeed confusing.
Simo.
My take is that the terminology should be chosen in line with
standards. The X.509 field is called `signatureAlgorithm' so
`--ca-certificate-signature-algorithm' makes sense to me.
Consistency with Dogtag terminology is a secondary consideration
considering FreeIPA users are unlikely to interact directly with
Dogtag much (especially during installation).
Fraser
I think it actually sets both the key algorithm and the signature algorithm
(you can't do a RSA signature with a EC key, etc.), that's probably why it is
called key algorithm in Dogtag.
Hm, you are right that the key algorithm is implied during signature algorithm
selection. But still, values SHA256withRSA and friends really denote just a
signature algorithm and the option should be named accordingly.
Martin
Updated patch attached.
--
Jan Cholasta
From 631edd794fd1418db785e780054ecc7ae1b03d8f Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Wed, 6 Aug 2014 09:43:19 +0200
Subject: [PATCH] Allow specifying signing algorithm of the IPA CA cert in
ipa-server-install.
This is especially useful for external CA install, as the algorithm is also
used for the CSR signature.
https://fedorahosted.org/freeipa/ticket/4447
---
install/tools/ipa-server-install | 13 ++---
install/tools/man/ipa-server-install.1 | 3 +++
ipaserver/install/cainstance.py| 12 ++--
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index 7d60d27..4ec430e 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -226,6 +226,10 @@ def parse_options():
cert_group.add_option(--subject, action=callback, callback=subject_callback,
type=string,
help=The certificate subject base (default O=realm-name))
+cert_group.add_option(--ca-signing-algorithm, dest=ca_signing_algorithm,
+ type=choice,
+ choices=('SHA1withRSA', 'SHA256withRSA', 'SHA512withRSA'),
+ help=Signing algorithm of the IPA CA certificate)
parser.add_option_group(cert_group)
dns_group = OptionGroup(parser, DNS options)
@@ -1075,7 +1079,8 @@ def main():
dogtag_constants=dogtag.install_constants)
if external == 0:
ca.configure_instance(host_name, domain_name, dm_password,
- dm_password, subject_base=options.subject)
+ dm_password, subject_base=options.subject,
+ ca_signing_algorithm=options.ca_signing_algorithm)
elif external == 1:
# stage 1 of external CA installation
options.realm_name = realm_name
@@ -1090,14 +1095,16 @@ def main():
write_cache(vars(options))
ca.configure_instance(host_name, domain_name, dm_password,
dm_password, csr_file=paths.ROOT_IPA_CSR,
- subject_base=options.subject)
+ subject_base=options.subject,
+ ca_signing_algorithm=options.ca_signing_algorithm)
else:
# stage 2 of external CA installation
ca.configure_instance(host_name, domain_name, dm_password,
dm_password,
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
On 09/29/2014 01:13 PM, Jan Cholasta wrote: Dne 29.9.2014 v 12:20 Martin Kosek napsal(a): On 09/29/2014 11:11 AM, Jan Cholasta wrote: Dne 29.9.2014 v 05:16 Fraser Tweedale napsal(a): On Fri, Sep 26, 2014 at 10:44:16AM -0400, Simo Sorce wrote: On Fri, 26 Sep 2014 13:54:34 +0200 Martin Kosek mko...@redhat.com wrote: I tested the patch (it works fine with Dogtag 10), but I got very confused. What CA option are we setting? Signing algorithm or Key Algorithm? I thought we are only setting Signing algorithm, but in that case: We are setting key algorithm for the CA signing key. That did not made me any less confused... If I check for example fields from certificate details from my browser, I see 2 algorithms names: * Public Key Algorithm (RSA, ECC, ...) * Certificate Signature Algorithm (SHA-1 with RSA, SHA-256 with RSA, something with ECC) In that world, key algorithm should really refer to the key PKI algorithm, i.e. RSA, ECC, ... Signature algorithms is where hashes come to play. - --ca-key-algorithm option should rather read --ca-signing-key-algorithm If you want to emphasize that it is actually the algorithm used to sign the CA certificate, the option should read --ca-certificate-signature-algorithm, but I would rather stick to Dogtag terminology and keep the string key algorithm in the name. I still think for most people key algorithm refers to Public Key algorithm. Rob or Simo, what is your take on this? If we are defining the signing algorithm the signing string should be somewhere in the option. Having just --key-algorithm is indeed confusing. Simo. My take is that the terminology should be chosen in line with standards. The X.509 field is called `signatureAlgorithm' so `--ca-certificate-signature-algorithm' makes sense to me. Consistency with Dogtag terminology is a secondary consideration considering FreeIPA users are unlikely to interact directly with Dogtag much (especially during installation). Fraser I think it actually sets both the key algorithm and the signature algorithm (you can't do a RSA signature with a EC key, etc.), that's probably why it is called key algorithm in Dogtag. Hm, you are right that the key algorithm is implied during signature algorithm selection. But still, values SHA256withRSA and friends really denote just a signature algorithm and the option should be named accordingly. Martin Updated patch attached. Looks good to me (and works good as well) - ACK. Pushed to master, ipa-4-1. (I just had to do a minor conflict resolution on master branch) Thanks, Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0271] baseldap: Properly handle the case of renaming object to the
On 09/18/2014 02:10 AM, Rob Crittenden wrote:
Tomas Babej wrote:
Hi,
When renaming a object to the same name, errors.EmptyModList is raised.
This is not properly handled, and can cause other modifications in the
LDAPUpdate command to be ignored.
https://fedorahosted.org/freeipa/ticket/4548
Needs some tests...
rob
Tests added. This patch is necessary for 4.1.
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
From 78f15ddcb0e3023fac964a7e770adda8a6362a93 Mon Sep 17 00:00:00 2001
From: Tomas Babej tba...@redhat.com
Date: Wed, 17 Sep 2014 17:17:54 +0200
Subject: [PATCH] baseldap: Properly handle the case of renaming object to the
same name
When renaming a object to the same name, errors.EmptyModList is raised.
This is not properly handled, and can cause other modifications in the
LDAPUpdate command to be ignored.
https://fedorahosted.org/freeipa/ticket/4548
---
ipalib/plugins/baseldap.py | 27 +--
ipatests/test_xmlrpc/test_user_plugin.py | 19 ++-
2 files changed, 35 insertions(+), 11 deletions(-)
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index a3cfe3ce33ffd999ea725cdf72bcf82cb11d5c84..13487aa9bc9d2e780fe8afbac86c25c822d23216 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -1398,16 +1398,23 @@ class LDAPUpdate(LDAPQuery, crud.Update):
entry_attrs[self.obj.primary_key.name] = options['rename']
if self.obj.rdn_is_primary_key and self.obj.primary_key.name in entry_attrs:
-# RDN change
-self._exc_wrapper(keys, options, ldap.update_entry_rdn)(
-entry_attrs.dn,
-RDN((self.obj.primary_key.name,
- entry_attrs[self.obj.primary_key.name])))
-rdnkeys = keys[:-1] + (entry_attrs[self.obj.primary_key.name], )
-entry_attrs.dn = self.obj.get_dn(*rdnkeys)
-del entry_attrs[self.obj.primary_key.name]
-options['rdnupdate'] = True
-rdnupdate = True
+try:
+# RDN change
+self._exc_wrapper(keys, options, ldap.update_entry_rdn)(
+entry_attrs.dn,
+RDN((self.obj.primary_key.name,
+ entry_attrs[self.obj.primary_key.name])))
+
+rdnkeys = keys[:-1] + (entry_attrs[self.obj.primary_key.name], )
+entry_attrs.dn = self.obj.get_dn(*rdnkeys)
+options['rdnupdate'] = True
+rdnupdate = True
+except errors.EmptyModlist:
+# Attempt to rename to the current name, ignore
+pass
+finally:
+# Delete the primary_key from entry_attrs either way
+del entry_attrs[self.obj.primary_key.name]
# Exception callbacks will need to test for options['rdnupdate']
# to decide what to do. An EmptyModlist in this context doesn't
diff --git a/ipatests/test_xmlrpc/test_user_plugin.py b/ipatests/test_xmlrpc/test_user_plugin.py
index e4c06145e2658f6c1684a60d12817a90d866056d..dfca104ebf39beaf7d42afb6b06c2100981df75e 100644
--- a/ipatests/test_xmlrpc/test_user_plugin.py
+++ b/ipatests/test_xmlrpc/test_user_plugin.py
@@ -527,11 +527,28 @@ class test_user(Declarative):
expected=errors.EmptyModlist(),
),
+dict(
+desc='Rename %s to same value, check that other modifications '
+ 'are performed' % renameduser1,
+command=('user_mod', [renameduser1],
+ dict(setattr=u'uid=%s' % renameduser1,
+ loginshell=u'/bin/bash')),
+expected=dict(
+result=get_user_result(
+renameduser1, u'Finkle', u'User1', 'mod',
+mail=[u'%s@%s' % (user1, api.env.domain)],
+homedirectory=[u'/home/%s' % user1],
+loginshell=[u'/bin/bash']),
+summary=u'Modified user %s' % renameduser1,
+value=renameduser1,
+),
+),
+
dict(
desc='Rename back %s' % renameduser1,
command=('user_mod', [renameduser1],
- dict(setattr=u'uid=%s' % user1)),
+ dict(setattr=u'uid=%s' % user1, loginshell=u'/bin/sh')),
expected=dict(
result=get_user_result(user1, u'Finkle', u'User1', 'mod'),
summary=u'Modified user %s' % renameduser1,
--
1.9.3
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
Dne 26.9.2014 v 19:54 Jan Cholasta napsal(a): Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... When adding the CA certificates to the temporary database it will report that a failure occurred, but not the exception: +except CalledProcessError, e: +root_logger.info(Failed to add CA to temporary NSS database.) +return CLIENT_INSTALL_ERROR Granted, NSS errors are often obtuse, but should it at least DEBUG log it? It is already logged in ipautil.run. The exception only says that the command failed, there's no point in logging it. 324, 325, 326: ACK 327: So the idea is to just mirror the certs and us the new db to know what was added? Exactly. What if someone has the same nickname, different cert in the two databases? Is that too much of a corner case? IMO it is too much of a corner case, but I plan on adding a better diff/patch algorithm in the future if it turns out to be a problem. 328, 329, 330, 331: ACK As an aside, do you know why the global certs are seen by mod_nss? libnssckbi.so is symlinked into the directory (certutil -L -d /etc/httpd/alias -h all will show all the certs). I'm not sure why it is this way, but the module is linked to the NSS database: $ sudo modutil -list -dbdir /etc/httpd/alias Listing of PKCS #11 Modules --- 1. NSS Internal PKCS #11 Module slots: 2 slots attached status: loaded slot: NSS Internal Cryptographic Services token: NSS Generic Crypto Services slot: NSS User Private Key and Certificate Services token: NSS Certificate DB 2. Root Certs library name: /etc/httpd/alias/libnssckbi.so slots: 2 slots attached status: loaded slot: /etc/pki/ca-trust/source token: System Trust slot: /usr/share/pki/ca-trust-source token: Default Trust --- 332-335 I think the naming and/or comments can be improved. For example, there are now 3 *retrieve_cert commands, all of which do slightly different things. All have external handlers (via the oddly named profile), but some are called internally as well. I have spent quite some time trying to come up with good names for them, but I was not able to do so. Suggestions are welcome. This is rather complex code: a command passes a call onto certmonger which then exuecutes the IPA CA helper... More documentation would definitely help a newcomer figure out how renewal, CA retrieval, etc. works. OK, I'll try to add some. Not to be too pedantic but there is a lot of this in dogtag-ipa-ca-renew-agent-submit: if variable: OR if not variable: Where variable defaults to None. Shouldn't the test be: if variable is [not] None: This doesn't catch empty strings, which may occur in some of these checks. 340: ACK Through some combination of ipa-certupdate and ipa-cacert-manage I seem to have hosed things up: ipa: DEBUG: certmonger request is in state dbus.String(u'CA_UNREACHABLE', variant_level=1) ipa.ipaserver.install.ipa_cacert_manage.CACertManage: DEBUG: File /usr/lib/python2.7/site-packages/ipapython/admintool.py, line 171, in execute return_value = self.run() File /usr/lib/python2.7/site-packages/ipaserver/install/ipa_cacert_manage.py, line
Re: [Freeipa-devel] [PATCH 0271] baseldap: Properly handle the case of renaming object to the
Dne 29.9.2014 v 13:52 Tomas Babej napsal(a): On 09/18/2014 02:10 AM, Rob Crittenden wrote: Tomas Babej wrote: Hi, When renaming a object to the same name, errors.EmptyModList is raised. This is not properly handled, and can cause other modifications in the LDAPUpdate command to be ignored. https://fedorahosted.org/freeipa/ticket/4548 Needs some tests... rob Tests added. This patch is necessary for 4.1. ACK. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
On Mon, 29 Sep 2014 13:16:07 +1000 Fraser Tweedale ftwee...@redhat.com wrote: On Fri, Sep 26, 2014 at 10:44:16AM -0400, Simo Sorce wrote: On Fri, 26 Sep 2014 13:54:34 +0200 Martin Kosek mko...@redhat.com wrote: I tested the patch (it works fine with Dogtag 10), but I got very confused. What CA option are we setting? Signing algorithm or Key Algorithm? I thought we are only setting Signing algorithm, but in that case: We are setting key algorithm for the CA signing key. That did not made me any less confused... If I check for example fields from certificate details from my browser, I see 2 algorithms names: * Public Key Algorithm (RSA, ECC, ...) * Certificate Signature Algorithm (SHA-1 with RSA, SHA-256 with RSA, something with ECC) In that world, key algorithm should really refer to the key PKI algorithm, i.e. RSA, ECC, ... Signature algorithms is where hashes come to play. - --ca-key-algorithm option should rather read --ca-signing-key-algorithm If you want to emphasize that it is actually the algorithm used to sign the CA certificate, the option should read --ca-certificate-signature-algorithm, but I would rather stick to Dogtag terminology and keep the string key algorithm in the name. I still think for most people key algorithm refers to Public Key algorithm. Rob or Simo, what is your take on this? If we are defining the signing algorithm the signing string should be somewhere in the option. Having just --key-algorithm is indeed confusing. Simo. My take is that the terminology should be chosen in line with standards. The X.509 field is called `signatureAlgorithm' so `--ca-certificate-signature-algorithm' makes sense to me. Consistency with Dogtag terminology is a secondary consideration considering FreeIPA users are unlikely to interact directly with Dogtag much (especially during installation). +1 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] [PATCHES] 319, 324-335 CA management and renewal fixes
Jan Cholasta wrote: Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... What's the point of a database missing certificates? When adding the CA certificates to the temporary database it will report that a failure occurred, but not the exception: +except CalledProcessError, e: +root_logger.info(Failed to add CA to temporary NSS database.) +return CLIENT_INSTALL_ERROR Granted, NSS errors are often obtuse, but should it at least DEBUG log it? It is already logged in ipautil.run. The exception only says that the command failed, there's no point in logging it. 324, 325, 326: ACK 327: So the idea is to just mirror the certs and us the new db to know what was added? Exactly. What if someone has the same nickname, different cert in the two databases? Is that too much of a corner case? IMO it is too much of a corner case, but I plan on adding a better diff/patch algorithm in the future if it turns out to be a problem. The result could be a deleted cert, that was my concern. It does seem to be a rather slim case. 328, 329, 330, 331: ACK As an aside, do you know why the global certs are seen by mod_nss? libnssckbi.so is symlinked into the directory (certutil -L -d /etc/httpd/alias -h all will show all the certs). I'm not sure why it is this way, but the module is linked to the NSS database: $ sudo modutil -list -dbdir /etc/httpd/alias Listing of PKCS #11 Modules --- 1. NSS Internal PKCS #11 Module slots: 2 slots attached status: loaded slot: NSS Internal Cryptographic Services token: NSS Generic Crypto Services slot: NSS User Private Key and Certificate Services token: NSS Certificate DB 2. Root Certs library name: /etc/httpd/alias/libnssckbi.so slots: 2 slots attached status: loaded slot: /etc/pki/ca-trust/source token: System Trust slot: /usr/share/pki/ca-trust-source token: Default Trust --- Sorry, that was a rhetorical question. Presumably NSS hunts around for libnssckbi.so but it doesn't seem to look very hard, except that it does seem to find it if it exists in the same directory as the DB, so I symlink it in. I think mod_nss is probably the only thing that does this, but it can make things rather interesting (for good and bad) dealing with global certs. 332-335 I think the naming and/or comments can be improved. For example, there are now 3 *retrieve_cert commands, all of which do slightly different things. All have external handlers (via the oddly named profile), but some are called internally as well. I have spent quite some time trying to come up with good names for them, but I was not able to do so. Suggestions are welcome. This is rather complex code: a command passes a call onto certmonger which then exuecutes the IPA CA helper... More documentation would definitely help a newcomer figure out how renewal, CA retrieval, etc. works. OK, I'll try to add some. Not to be too pedantic but there is a lot of this in dogtag-ipa-ca-renew-agent-submit: if variable: OR if not variable:
Re: [Freeipa-devel] [PATCH 0271] baseldap: Properly handle the case of renaming object to the
On 09/29/2014 02:44 PM, Jan Cholasta wrote: Dne 29.9.2014 v 13:52 Tomas Babej napsal(a): On 09/18/2014 02:10 AM, Rob Crittenden wrote: Tomas Babej wrote: Hi, When renaming a object to the same name, errors.EmptyModList is raised. This is not properly handled, and can cause other modifications in the LDAPUpdate command to be ignored. https://fedorahosted.org/freeipa/ticket/4548 Needs some tests... rob Tests added. This patch is necessary for 4.1. ACK. Pushed to: master: d83af7d38d65dfc50c70cb4ecf48f5c4c57d6979 ipa-4-1: 0cf2dbc44575e03cb630ac8d18ed59464abfd2b3 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
Dne 29.9.2014 v 15:05 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... What's the point of a database missing certificates? It won't be missing any certificates if /etc/pki/nssdb was not missing any certificates before the update. As I said, the update will not break anything. It will not fix anything either, but I don't think this kind of fixing should be done during client RPM upgrade. It is not consistent with anything else we do during client RPM upgrade, it does not scale well and it just does not feel right to me in overall. When adding the CA certificates to the temporary database it will report that a failure occurred, but not the exception: +except CalledProcessError, e: +root_logger.info(Failed to add CA to temporary NSS database.) +return CLIENT_INSTALL_ERROR Granted, NSS errors are often obtuse, but should it at least DEBUG log it? It is already logged in ipautil.run. The exception only says that the command failed, there's no point in logging it. 324, 325, 326: ACK 327: So the idea is to just mirror the certs and us the new db to know what was added? Exactly. What if someone has the same nickname, different cert in the two databases? Is that too much of a corner case? IMO it is too much of a corner case, but I plan on adding a better diff/patch algorithm in the future if it turns out to be a problem. The result could be a deleted cert, that was my concern. It does seem to be a rather slim case. 328, 329, 330, 331: ACK As an aside, do you know why the global certs are seen by mod_nss? libnssckbi.so is symlinked into the directory (certutil -L -d /etc/httpd/alias -h all will show all the certs). I'm not sure why it is this way, but the module is linked to the NSS database: $ sudo modutil -list -dbdir /etc/httpd/alias Listing of PKCS #11 Modules --- 1. NSS Internal PKCS #11 Module slots: 2 slots attached status: loaded slot: NSS Internal Cryptographic Services token: NSS Generic Crypto Services slot: NSS User Private Key and Certificate Services token: NSS Certificate DB 2. Root Certs library name: /etc/httpd/alias/libnssckbi.so slots: 2 slots attached status: loaded slot: /etc/pki/ca-trust/source token: System Trust slot: /usr/share/pki/ca-trust-source token: Default Trust --- Sorry, that was a rhetorical question. Presumably NSS hunts around for libnssckbi.so but it doesn't seem to look very hard, except that it does seem to find it if it exists in the same directory as the DB, so I symlink it in. I think mod_nss is probably the only thing that does this, but it can make things rather interesting (for good and bad) dealing with global certs. 332-335 I think the naming and/or comments can be improved. For example, there are now 3 *retrieve_cert commands, all of which do slightly different things. All have external handlers (via the oddly named profile), but some are called internally as well. I have spent quite some time trying to come up with good names for them, but I was not able to do so. Suggestions are welcome. This is rather
Re: [Freeipa-devel] [PATCH] 757 webui: do not offer ipa-ad-winsync and ipa-ipa-trust range types
On 9/29/2014 2:40 AM, Petr Vobornik wrote: On 24.9.2014 12:17, Petr Vobornik wrote: webui: do not offer ipa-ad-winsync and ipa-ipa-trust range types They are not supported by API. Forgot to attach patch... ACK. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
Dne 26.9.2014 v 19:40 Jan Cholasta napsal(a):
Dne 26.9.2014 v 17:37 Rob Crittenden napsal(a):
Petr Viktorin wrote:
On 09/24/2014 06:13 PM, Jan Cholasta wrote:
Hi,
the attached patches fix https://fedorahosted.org/freeipa/ticket/4480
and https://fedorahosted.org/freeipa/ticket/4489.
(Note that design page for this is TBD.)
Isn't this backwards then?
336:
Instead of
len(data[:match.start() + 1].splitlines())
you can do
data.count('\n', 0, match.start()) + 1
Unfortunately '\n' is not good enough, we have to check for '\r\n' and
'\r' as well, hence the use of splitlines.
337:
The --external_cert_file and --external_ca_file options for
ipa-ca-install are removed, do we really want to do that? Shouldn't they
be deprecated instead?
+1
Same for --external-ca-file in ipa-cacert-manage.
+1
IMO it's OK to just remove them, as they were added during 4.1
development and are not available in any released version of IPA.
I can't say I'm a fan of forcing users to concatenate cert files.
All the --*-cert-file options may be given multiple times.
338: Looks OK
339: Looks OK
Could you add some docstrings to the functions you add? Sometimes it's
harder than necessary to decipher what they do and what the
arguments/return values mean exactly.
Sure.
There is no user-visible documentation on what file types are
expected/supported. It would be good to add this to the man pages, or
the --help.
Added.
I also wonder if the detection code should be changed. It basically now
tries a slew of different mechanisms one at a time rather than trying to
identify the type of file and using that one. It may not be possible in
all cases but you could at least start by looking for ^- to know it
is a text file and go from there, otherwise step through the binary
formats.
Rearranged the code so that text files are tried first.
In external CA, the error message when specifying a certificate but not
the CA could be improved:
$ ipa-server-install --external_cert_file
~/p/Certificate_Authority_8.cer
...
CA certificate CN=Certificate Authority,O=IDM.LAB.ENG.BRQ.REDHAT.COM in
/home/pviktori/p/Certificate_Authority_8.cer is not valid:
(SEC_ERROR_UNKNOWN_ISSUER) Peer's Certificate issuer is not recognized.
Fixed.
For CA-less, I used a combination of files with which server
installation went well, but replica-install failed halfway through:
Console:
...
[16/36]: creating indices
[17/36]: enabling referential integrity plugin
[18/36]: configuring ssl for ds instance
[error] RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
Log tail:
2014-09-26T15:05:43Z DEBUG Starting external process
2014-09-26T15:05:43Z DEBUG args='/usr/bin/pk12util' '-d'
'/etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM/' '-i'
'/tmp/tmp2vEWX_ipa/realm_info/dscert.p12' '-k'
'/etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM//pwdfile.txt' '-v' '-w'
'/dev/stdin'
2014-09-26T15:05:43Z DEBUG Process finished, return code=17
2014-09-26T15:05:43Z DEBUG stdout=
2014-09-26T15:05:43Z DEBUG stderr=pk12util: PKCS12 decode not verified:
SEC_ERROR_BAD_PASSWORD: The security password entered is incorrect.
2014-09-26T15:05:43Z DEBUG Traceback (most recent call last):
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 370, in start_creation
run_step(full_msg, method)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 360, in run_step
method()
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
600, in __enable_ssl
trust_flags=trust_flags)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 1030, in create_from_pkcs12
self.import_pkcs12(pkcs12_fname, pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 971, in import_pkcs12
pkcs12_passwd=pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 191, in import_pkcs12
pkcs12_filename)
RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
2014-09-26T15:05:43Z DEBUG [error] RuntimeError: incorrect password
for pkcs#12 file /tmp/tmp2vEWX_ipa/realm_info/dscert.p12
2014-09-26T15:05:43Z DEBUG File
/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py,
line 644, in run_script
return_value = main_function()
File /sbin/ipa-replica-install, line 677, in main
ds = install_replica_ds(config)
File /sbin/ipa-replica-install, line 190, in install_replica_ds
ca_file=config.dir + /ca.crt,
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
354, in create_replica
self.start_creation(runtime=60)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 370, in start_creation
run_step(full_msg, method)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 360, in run_step
method()
File
Re: [Freeipa-devel] [PATCH] 757 webui: do not offer ipa-ad-winsync and ipa-ipa-trust range types
On 29.9.2014 16:19, Endi Sukma Dewata wrote: On 9/29/2014 2:40 AM, Petr Vobornik wrote: On 24.9.2014 12:17, Petr Vobornik wrote: webui: do not offer ipa-ad-winsync and ipa-ipa-trust range types They are not supported by API. Forgot to attach patch... ACK. Pushed to: ipa-4-1: d84b8fe8e3eb597d05201731faf1a0894f42aee0 master: fcce15d0bd0205c4fcd95a4057602da2e29b7bb8 -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0018 Check that port 8443 is available when installing PKI.
https://fedorahosted.org/freeipa/ticket/4564 -- David Kupka From d5748822b8fac3cde01670507f80bfa9c4c04ede Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Mon, 29 Sep 2014 04:27:30 -0400 Subject: [PATCH] Check that port 8443 is available when installing PKI. https://fedorahosted.org/freeipa/ticket/4564 --- install/tools/ipa-ca-install | 9 + install/tools/ipa-server-install | 5 + ipaserver/install/cainstance.py | 8 3 files changed, 22 insertions(+) diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install index fc89412486a288568de85761742dbf32e8a63c65..8ace1cd5d5600f1406f1fdd4ddf37e784ed27270 100755 --- a/install/tools/ipa-ca-install +++ b/install/tools/ipa-ca-install @@ -98,6 +98,11 @@ def parse_options(): def get_dirman_password(): return installutils.read_password(Directory Manager (existing master), confirm=False, validate=False) +def check_ca(): +if not cainstance.check_port(): +print IPA requires port 8443 for PKI but it is currently in use. +sys.exit(1) + def install_dns_records(config, options): if not bindinstance.dns_container_exists(config.master_host_name, @@ -198,6 +203,8 @@ def install_replica(safe_options, options, filename): else: cainstance.replica_ca_install_check(config) +check_ca() + # Configure the CA if necessary CA = cainstance.install_replica_ca(config, postinstall=True) @@ -291,6 +298,8 @@ def install_master(safe_options, options): domain_name = api.env.domain host_name = api.env.host +check_ca() + dirname = dsinstance.config_dirname( dsinstance.realm_to_serverid(realm_name)) cadb = certs.CertDB(realm_name, subject_base=subject_base) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 7d60d27bcfae9a89ad7c5d811d3f9d8a9fda60cb..520cfda43cc0bf2a85fab3db0c23aa60cc2d7372 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -795,6 +795,11 @@ def main(): # Make sure the 389-ds ports are available check_dirsrv(options.unattended) +if setup_ca: +if not cainstance.check_port(): +print IPA requires port 8443 for PKI but it is currently in use. +sys.exit(Aborting installation) + if options.conf_ntp: try: ipaclient.ntpconf.check_timedate_services() diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py index 04968d411fc5bc1073e86fab42743fc65f7b828a..164d67b69a489af077ef4929958fb4027761e96a 100644 --- a/ipaserver/install/cainstance.py +++ b/ipaserver/install/cainstance.py @@ -105,6 +105,14 @@ def check_inst(): return True +def check_port(): + +Check that dogtag port (8443) is available. + +Returns True when the port is free, False if it's taken. + +return not ipautil.host_port_open(None, 8443) + def get_preop_pin(instance_root, instance_name): # Only used for Dogtag 9 preop_pin = None -- 1.9.3 From a4c20f41f289bfb7b338790637089608bd80f2cd Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Mon, 29 Sep 2014 04:27:30 -0400 Subject: [PATCH] Check that port 8443 is available when installing PKI. https://fedorahosted.org/freeipa/ticket/4564 --- install/tools/ipa-ca-install | 9 + install/tools/ipa-server-install | 5 + ipaserver/install/cainstance.py | 8 3 files changed, 22 insertions(+) diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install index 475794bb6186725ad5ab079adfb98849c589e67e..96950efd7c68a4646deb7e90e0394d8c3dde2e77 100755 --- a/install/tools/ipa-ca-install +++ b/install/tools/ipa-ca-install @@ -98,6 +98,11 @@ def get_dirman_password(): Directory Manager (existing master), confirm=False, validate=False) +def check_ca(): +if not cainstance.check_port(): +print IPA requires port 8443 for PKI but it is currently in use. +sys.exit(1) + def install_dns_records(config, options): if not bindinstance.dns_container_exists(config.master_host_name, @@ -175,6 +180,8 @@ def install_replica(safe_options, options, filename): else: cainstance.replica_ca_install_check(config) +check_ca() + # Configure the CA if necessary CA = cainstance.install_replica_ca(config, postinstall=True) @@ -269,6 +276,8 @@ def install_master(safe_options, options): domain_name = api.env.domain host_name = api.env.host +check_ca() + dirname = dsinstance.config_dirname( dsinstance.realm_to_serverid(realm_name)) cadb = certs.CertDB(realm_name, subject_base=subject_base) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index e73a098df3c34794639f75460baac70b4b49480a..5321e2694992815e1bc93fe49772f11b82256f22 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -816,6 +816,11 @@ def main(): # Make sure the
Re: [Freeipa-devel] [PATCH 0067] Use stack allocation when writing values during otp auth
On Thu, 2014-09-25 at 13:45 +0200, thierry bordaz wrote: On 09/19/2014 07:49 PM, Nathaniel McCallum wrote: This is an optimization from patch 0062 (rescinded) which I think is worth keeping. There is no ticket for this. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello, That is exact that slapi* are doing a intensive usage of the of alloc/free. Using a stack allocated mods would reduce the number of alloc/free but I afraid it will not have a significant impact. For example further slapi_modify_internal is doing tons of alloc/free. Also I think it is not possible to use stack allocated mods. In fact mods will be modified by internal_mod (for example to add 'modifytimestamp', 'modifiersname') and mods will be reallocated. This could also occurs from plugins. Finally if a modify retry occurs, the original mods are freeed. So does slapi_modify_internal_set_pb() steal ownership of the LDAPMod array? I wish this were better documented. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0067] Use stack allocation when writing values during otp auth
On Thu, 2014-09-25 at 13:45 +0200, thierry bordaz wrote: On 09/19/2014 07:49 PM, Nathaniel McCallum wrote: This is an optimization from patch 0062 (rescinded) which I think is worth keeping. There is no ticket for this. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello, That is exact that slapi* are doing a intensive usage of the of alloc/free. Using a stack allocated mods would reduce the number of alloc/free but I afraid it will not have a significant impact. For example further slapi_modify_internal is doing tons of alloc/free. Also I think it is not possible to use stack allocated mods. In fact mods will be modified by internal_mod (for example to add 'modifytimestamp', 'modifiersname') and mods will be reallocated. This could also occurs from plugins. Finally if a modify retry occurs, the original mods are freeed. See ldap/servers/slapd/task.c. In this file, everything is stack allocated except for the value itself. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0067] Use stack allocation when writing values during otp auth
On 09/29/2014 05:45 PM, Nathaniel McCallum wrote: On Thu, 2014-09-25 at 13:45 +0200, thierry bordaz wrote: On 09/19/2014 07:49 PM, Nathaniel McCallum wrote: This is an optimization from patch 0062 (rescinded) which I think is worth keeping. There is no ticket for this. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello, That is exact that slapi* are doing a intensive usage of the of alloc/free. Using a stack allocated mods would reduce the number of alloc/free but I afraid it will not have a significant impact. For example further slapi_modify_internal is doing tons of alloc/free. Also I think it is not possible to use stack allocated mods. In fact mods will be modified by internal_mod (for example to add 'modifytimestamp', 'modifiersname') and mods will be reallocated. This could also occurs from plugins. Finally if a modify retry occurs, the original mods are freeed. See ldap/servers/slapd/task.c. In this file, everything is stack allocated except for the value itself. Hi Nathaniel, Yes you are correct. This is even a common method :-[ In fact the mods are duplicated/replaced by normalized one at the begining of modify_internal_pb. This is that duplicated ones that can later be extended/freed. Sorry for the noise. The fix is valid. Ack. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 130] extdom: add support for new version
On Thu, Sep 25, 2014 at 01:46:00PM +0200, Sumit Bose wrote:
On Wed, Sep 24, 2014 at 03:23:54PM +0200, Jakub Hrozek wrote:
On Tue, Sep 23, 2014 at 05:11:01PM +0200, Sumit Bose wrote:
Hi,
this patch should fix https://fedorahosted.org/freeipa/ticket/4031 and
with the corresponding SSSD part it would be possible to get the full
list of group memberships with the id command even for user who didn't
log in before.
bye,
Sumit
So far I only read the patch, no testing was done yet (I'm installing a
separate VM where I'll keep this new plugin for easy comparison and
backwards-compatibility testing)
Thank you for the review, please see comments below.
First, there are some Coverity warnings:
Error: USE_AFTER_FREE (CWE-825):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:242:
alias: Assigning: groups = new_groups. Now both point to the same
storage.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:246:
freed_arg: free(void *) frees groups.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:252:
use_after_free: Using freed pointer groups.
fixed
Error: CONSTANT_EXPRESSION_RESULT (CWE-398):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:596:
missing_parentheses: !id_type != SSS_ID_TYPE_GID is always true
regardless of the values of its operands. Did you intend to either negate
the entire comparison expression, in which case parentheses would be
required around the entire comparison expression to force that
interpretation, or negate the sense of the comparison (that is, use '=='
rather than '!=')? This occurs as the logical second operand of '||'.
fixed
Error: DEADCODE (CWE-561):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594:
cond_cannot_single: Condition request_type == 1U, taking false branch.
Now the value of request_type cannot be equal to 1.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594:
cond_cannot_set: Condition request_type == 3U, taking false branch. Now
the value of request_type cannot be equal to any of {1, 3}.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606:
cannot_set: At condition request_type == 1U, the value of request_type
cannot be equal to any of {1, 3}.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606:
dead_error_condition: The condition request_type == 1U cannot be true.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:607:
dead_error_line: Execution cannot reach this statement ret =
pack_ber_sid(sid_str,
I thik this is a result of the CONSTANT_EXPRESSION_RESULT issue, since I
fixed it this warning should be gone as well.
See some comments inline.
From 23ff38cdea85995b211e73f474bcb4b0d7fb8039 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 23 Sep 2014 15:55:43 +0200
Subject: [PATCH] extdom: add support for new version
Currently the extdom plugin is basically used to translate SIDs of AD
users and groups to names and POSIX IDs.
With this patch a new version is added which will return the full member
list for groups and the full list of group memberships for a user.
Additionally the gecos field, the home directory and the login shell of a
user are returned and an optional list of key-value pairs which
currently will contain the SID of the requested object if available.
https://fedorahosted.org/freeipa/ticket/4031
---
.../ipa-extdom-extop/ipa_extdom.h | 29 +-
.../ipa-extdom-extop/ipa_extdom_common.c | 850
+++--
.../ipa-extdom-extop/ipa_extdom_extop.c| 28 +-
3 files changed, 640 insertions(+), 267 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
index
5f834a047a579104cd2589ce417c580c1c5388d3..548ee74f561c474854c049726c4c3e71da5cbbea
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
@@ -64,6 +64,7 @@
#include sss_nss_idmap.h
#define EXOP_EXTDOM_OID 2.16.840.1.113730.3.8.10.4
+#define EXOP_EXTDOM_V2_OID 2.16.840.1.113730.3.8.10.4.1
It's a bit odd that this control is called V1 in the SSSD tree and V2 in
the IPA tree. It's not wrong, just strange maybe.
you are right, I renamed the versions here.
-int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req,
- struct extdom_res **res)
+int check_request(struct extdom_req *req, enum extdom_version version)
+{
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
Jan Cholasta wrote: Dne 29.9.2014 v 15:05 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... What's the point of a database missing certificates? It won't be missing any certificates if /etc/pki/nssdb was not missing any certificates before the update. As I said, the update will not break anything. It will not fix anything either, but I don't think this kind of fixing should be done during client RPM upgrade. It is not consistent with anything else we do during client RPM upgrade, it does not scale well and it just does not feel right to me in overall. Ok, I'll concede the point that there is no difference post-update, but it doesn't do what ipa-certupdate does. You can potentially end up with a completely diffferent set of certificates. So why the difference? Post install of new client bits: # certutil -L -d /etc/ipa/nssdb/ IPA CA CT,C,C After running ipa-certupdate: # certutil -L -d /etc/ipa/nssdb/ CN=Primary CA,O=example.com,C=US ,, CN=Secondary CA,O=example.com,C=US ,, GREYOAK.COM IPA CA CT,C,C Quite the difference. I'll ACK for now since this doesn't materially change anything and shouldn't break any installs but it begs the question of why it is acceptable now but not acceptable to make ipa-certupdate do the same. So ACK for 319, 324-331, 340. The LDAP update happens in renew_ca_cert. Are there any relevant errors in /var/log/messages? Is there caRenewalMaster in ipaConfigString of the master entry of the master where you run ipa-cacert-manage renew? Sep 25 16:06:44 grindle renew_ca_cert: Traceback (most recent call last): File /usr/lib64/ipa/certmonger/renew_ca_cert, line 214, in module main() File /usr/lib64/ipa/certmonger/renew_ca_cert, line 79, in main ca = cainstance.CAInstance(host_name=api.env.host, ldapi=False) File /usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py, line 357, in __init__ self.ra_agent_pwd = self.ra_agent_db + /pwdfile.txt TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' Sep 25 16:06:44 grindle certmonger: Certificate named caSigningCert cert-pki-ca in token NSS Certificate DB in database /etc/pki/pki-tomcat/alias issued by CA and saved. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 130] extdom: add support for new version
On Mon, Sep 29, 2014 at 06:15:21PM +0200, Sumit Bose wrote:
On Thu, Sep 25, 2014 at 01:46:00PM +0200, Sumit Bose wrote:
On Wed, Sep 24, 2014 at 03:23:54PM +0200, Jakub Hrozek wrote:
On Tue, Sep 23, 2014 at 05:11:01PM +0200, Sumit Bose wrote:
Hi,
this patch should fix https://fedorahosted.org/freeipa/ticket/4031 and
with the corresponding SSSD part it would be possible to get the full
list of group memberships with the id command even for user who didn't
log in before.
bye,
Sumit
So far I only read the patch, no testing was done yet (I'm installing a
separate VM where I'll keep this new plugin for easy comparison and
backwards-compatibility testing)
Thank you for the review, please see comments below.
First, there are some Coverity warnings:
Error: USE_AFTER_FREE (CWE-825):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:242:
alias: Assigning: groups = new_groups. Now both point to the same
storage.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:246:
freed_arg: free(void *) frees groups.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:252:
use_after_free: Using freed pointer groups.
fixed
Error: CONSTANT_EXPRESSION_RESULT (CWE-398):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:596:
missing_parentheses: !id_type != SSS_ID_TYPE_GID is always true
regardless of the values of its operands. Did you intend to either negate
the entire comparison expression, in which case parentheses would be
required around the entire comparison expression to force that
interpretation, or negate the sense of the comparison (that is, use '=='
rather than '!=')? This occurs as the logical second operand of '||'.
fixed
Error: DEADCODE (CWE-561):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594:
cond_cannot_single: Condition request_type == 1U, taking false branch.
Now the value of request_type cannot be equal to 1.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594:
cond_cannot_set: Condition request_type == 3U, taking false branch.
Now the value of request_type cannot be equal to any of {1, 3}.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606:
cannot_set: At condition request_type == 1U, the value of
request_type cannot be equal to any of {1, 3}.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606:
dead_error_condition: The condition request_type == 1U cannot be true.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:607:
dead_error_line: Execution cannot reach this statement ret =
pack_ber_sid(sid_str,
I thik this is a result of the CONSTANT_EXPRESSION_RESULT issue, since I
fixed it this warning should be gone as well.
See some comments inline.
From 23ff38cdea85995b211e73f474bcb4b0d7fb8039 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 23 Sep 2014 15:55:43 +0200
Subject: [PATCH] extdom: add support for new version
Currently the extdom plugin is basically used to translate SIDs of AD
users and groups to names and POSIX IDs.
With this patch a new version is added which will return the full member
list for groups and the full list of group memberships for a user.
Additionally the gecos field, the home directory and the login shell of
a
user are returned and an optional list of key-value pairs which
currently will contain the SID of the requested object if available.
https://fedorahosted.org/freeipa/ticket/4031
---
.../ipa-extdom-extop/ipa_extdom.h | 29 +-
.../ipa-extdom-extop/ipa_extdom_common.c | 850
+++--
.../ipa-extdom-extop/ipa_extdom_extop.c| 28 +-
3 files changed, 640 insertions(+), 267 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
index
5f834a047a579104cd2589ce417c580c1c5388d3..548ee74f561c474854c049726c4c3e71da5cbbea
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
@@ -64,6 +64,7 @@
#include sss_nss_idmap.h
#define EXOP_EXTDOM_OID 2.16.840.1.113730.3.8.10.4
+#define EXOP_EXTDOM_V2_OID 2.16.840.1.113730.3.8.10.4.1
It's a bit odd that this control is called V1 in the SSSD tree and V2 in
the IPA tree. It's not wrong, just strange maybe.
you are right, I renamed the versions here.
-int handle_request(struct
Re: [Freeipa-devel] [PATCH 130] extdom: add support for new version
On Mon, Sep 29, 2014 at 06:16:30PM +0200, Sumit Bose wrote: Hi, Jakub found another issue which is fixed with this new version. bye, Sumit and now with patch ... Thank you, ACK ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
+is_replication(Slapi_PBlock *pb)
+{
+int repl = 0;
+slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, repl);
+return repl != 0;
+}
+
+static const char *
+get_attribute(Slapi_Entry *entry)
+{
+static struct {
+const char *clss;
+const char *attr;
+} table[] = {
+{ ipatokenHOTP, ipatokenHOTPcounter },
+{ ipatokenTOTP, ipatokenTOTPwatermark },
+{ NULL, NULL }
+};
+
+const char *attr = NULL;
+char **clsses = NULL;
+
+clsses =
Re: [Freeipa-devel] [PATCH 0068] Move OTP synchronization step to after counter writeback
On Thu, 2014-09-25 at 15:15 +0200, thierry bordaz wrote: On 09/19/2014 07:53 PM, Nathaniel McCallum wrote: This prevents synchronization when an authentication collision occurs. https://fedorahosted.org/freeipa/ticket/4493 NOTE: this patch is related to the above ticket, but does not solve it. For the solution, please see patch 0064. This behavior fix is from patch 0062 (rescinded) and is worth keeping. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, . My understanding is that during a pre_bind, the plugins validates token codes (for example HOTP) checking that step ranges [-25..+25]. As soon as the token is valid, the new HOTPcounter is written in the entry. But in case of negative steps,I believe the otp-decrement plugin will reject this update. HOTP never goes backwards. See line 188 in libotp.c. Even if this check weren't present, we would *want* the decrement plugin to reject the update. If TOTPwatermark is updated and there is a second token code, then clockOffset is also updated. This update is done during a pre_bind, so if there are parallel binds on the server, there is a possibility that TOTPwatermark is updated from a bind and 'clockOffset' updated from an other bind. An option is to do a single internal modify to update both. We don't care about atomicity in this case. If two TOTP synchronizations occur at almost the same time, the value of clockOffset will be written twice with the same value. Since the values are the same, we don't care which value gets written first. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
On 09/29/2014 04:32 PM, Jan Cholasta wrote: Dne 26.9.2014 v 19:40 Jan Cholasta napsal(a): Dne 26.9.2014 v 17:37 Rob Crittenden napsal(a): Petr Viktorin wrote: On 09/24/2014 06:13 PM, Jan Cholasta wrote: Hi, the attached patches fix https://fedorahosted.org/freeipa/ticket/4480 and https://fedorahosted.org/freeipa/ticket/4489. (Note that design page for this is TBD.) IMO it's OK to just remove them, as they were added during 4.1 development and are not available in any released version of IPA. Ah, OK Added patch 341 for stricter CA certificate validation which fixes https://fedorahosted.org/freeipa/ticket/4477. Updated patches attached. I didn't find any more issues, except of course the missing design page and tests. Any ETA on those? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
Updated patch to fix merge conflicts from recent changes.
On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com wrote:
Hello,
Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I
missed any.
Thanks,
Gabe
From 51ea42d4ecb16a425ea4ff6871ed9e0d5ce67327 Mon Sep 17 00:00:00 2001
From: Gabe redhatri...@gmail.com
Date: Mon, 29 Sep 2014 17:23:25 -0600
Subject: [PATCH] Remove trivial path constants from modules
https://fedorahosted.org/freeipa/ticket/4399
---
.../certmonger/dogtag-ipa-ca-renew-agent-submit| 8 +--
install/tools/ipa-adtrust-install | 8 +--
install/tools/ipa-ca-install | 5 +-
install/tools/ipa-dns-install | 8 +--
install/tools/ipa-replica-conncheck| 9 ++-
install/tools/ipa-replica-install | 6 +-
install/tools/ipa-server-install | 39 +---
install/tools/ipa-upgradeconfig| 30 -
install/wsgi/plugins.py| 6 +-
ipa-client/ipa-install/ipa-client-automount| 62 +--
ipa-client/ipa-install/ipa-client-install | 72 ++
ipa-client/ipaclient/ntpconf.py| 28 -
ipalib/session.py | 5 +-
ipaplatform/fedora/tasks.py| 44 ++---
ipapython/certmonger.py| 8 +--
ipapython/ipautil.py | 3 -
ipapython/sysrestore.py| 5 +-
ipaserver/dcerpc.py| 3 +-
ipaserver/install/adtrustinstance.py | 13 ++--
ipaserver/install/bindinstance.py | 23 +++
ipaserver/install/certs.py | 6 +-
ipaserver/install/dsinstance.py| 35 +--
ipaserver/install/httpinstance.py | 32 --
ipaserver/install/ipa_backup.py| 7 +--
ipaserver/install/ipa_replica_prepare.py | 15 ++---
ipaserver/install/ldapupdate.py| 3 -
ipaserver/install/sysupgrade.py| 5 +-
ipaserver/install/upgradeinstance.py | 5 +-
ipaserver/rpcserver.py | 5 +-
29 files changed, 203 insertions(+), 295 deletions(-)
diff --git a/install/certmonger/dogtag-ipa-ca-renew-agent-submit b/install/certmonger/dogtag-ipa-ca-renew-agent-submit
index 4f0b78accac6840471f8b2e9f17288b3b4e82105..942ffec65d7b041fc6f9d3b2c19d3596fae79d31 100755
--- a/install/certmonger/dogtag-ipa-ca-renew-agent-submit
+++ b/install/certmonger/dogtag-ipa-ca-renew-agent-submit
@@ -71,8 +71,7 @@ def request_cert():
syslog.syslog(syslog.LOG_NOTICE,
Forwarding request to dogtag-ipa-renew-agent)
-path = paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT
-args = [path] + sys.argv[1:]
+args = [paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT] + sys.argv[1:]
stdout, stderr, rc = ipautil.run(args, raiseonerr=False, env=os.environ)
sys.stderr.write(stderr)
sys.stderr.flush()
@@ -282,12 +281,11 @@ def export_csr():
if not cert:
return (REJECTED, New certificate requests not supported)
-csr_file = paths.IPA_CA_CSR
try:
-with open(csr_file, 'wb') as f:
+with open(paths.IPA_CA_CSR, 'wb') as f:
f.write(csr)
except Exception, e:
-return (UNREACHABLE, Failed to write %s: %s % (csr_file, e))
+return (UNREACHABLE, Failed to write %s: %s % (paths.IPA_CA_CSR, e))
return (ISSUED, cert)
diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 6e55bbe3e57f1c609398dc571e90cb8677d91a33..aabd695b8c63856875a5daa2a3b2aef70980973d 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -33,8 +33,6 @@ from ipaplatform.paths import paths
from ipapython.ipa_log_manager import *
from ipapython.dn import DN
-log_file_name = paths.IPASERVER_INSTALL_LOG
-
def parse_options():
parser = IPAOptionParser(version=version.VERSION)
parser.add_option(-d, --debug, dest=debug, action=store_true,
@@ -213,8 +211,8 @@ def main():
if os.getegid() != 0:
sys.exit(Must be root to setup AD trusts on server)
-standard_logging_setup(log_file_name, debug=options.debug, filemode='a')
-print \nThe log file for this installation can be found in %s % log_file_name
+standard_logging_setup(paths.IPASERVER_INSTALL_LOG, debug=options.debug, filemode='a')
+print \nThe log file for this installation can be found in %s % paths.IPASERVER_INSTALL_LOG
root_logger.debug('%s was invoked with options: %s' % (sys.argv[0], safe_options))
root_logger.debug(missing options might be asked for interactively later\n)
@@ -452,5 +450,5 @@ information
return 0
if __name__ == '__main__':
-run_script(main, log_file_name=log_file_name,
+
