date:20150226

Re: [Freeipa-devel] [PATCH] 0039 Try continue ipa-client-automount even if nsslapd-minssf 0.

2015-02-26 Thread Rob Crittenden

Martin Basti wrote:
 On 26/02/15 10:57, David Kupka wrote:
 https://fedorahosted.org/freeipa/ticket/4902


 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel
 Works for me, ACK.

NACK.

If you simply pass in /etc/ipa/ca.crt as the cacert path then it will
use TLS.

rob

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

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

2015-02-26 Thread Gabe Alford

Yeah. That makes more sense. Updated patch attached.

Thanks,

Gabe

On Wed, Feb 25, 2015 at 3:55 PM, Tomas Babej tba...@redhat.com wrote:

  Hi Gabe,

 sorry for not being clear. This approach will not work:

 +class TestAdvice(BaseTestInvalidAdvice,
 + BaseTestFedoraAuthconfig,
 + BaseTestFreeBSDNSSPAM,
 + BaseTestGenericNSSPAM,
 + BaseTestGenericSSSDBefore19,
 + BaseTestRedHatNSS,
 + BaseTestRedHatNSSPAM,
 + BaseTestRedHatSSSDBefore19,
 + BaseTestAdvice):
 +pass

 By combining all the base classes into one, you will not get the desired
 effect (which is to run the test_advice method for each advice_id). Let me
 explain why:

 The test runner works in the following way: it inspects any discovered
 class which name begins with Test, and executes each its method, which
 names begins with test as a test case.

 If the test runner inspects the TestAdvice class, the only method
 beggining with test, which it will see, is the test_advice which was
 inherited back from BaseTestAdvice class. So we can safely conclude the
 test runner will only run 1 test case.

 Which one, you may ask? Well, since the test_advice behaviour is fully
 determined by the values of advice_id, advice_regex and raiseerr
 attributes, let's look at their values in TestAdvice class. This class does
 not define attirbutes with such names, so we move along the inheritance
 chain (also called MRO) - the first class from which we inherit is
 BaseTestInvalidAdvice, and this class defines all three mentioned
 attributes.

 Hence the only test method will be run the test for invalid advice :)

 Now, how to fix this? The easiest approach would be to abandon the
 approach with the separate classes, and map each class to a test method in
 the TestAdvice class, like this (from the top of my head):

 +class TestAdvice(IntegrationTest):
 +topology = 'line'
 +
 +def test_invalid_advice(self):
 +advice_id = 'invalid-advise-param'
 +advice_regex = invalid[\s]+\'advice\'.*
 +raiseerr = False
 +# Obtain the advice from the server
 +tasks.kinit_admin(self.master)
 +result = self.master.run_command(['ipa-advise', self.advice_id],
 + raiseonerr=self.raiseerr)
 +
 +if not result.stdout_text:
 +advice = result.stderr_text
 +else:
 +advice = result.stdout_text
 +
 +assert re.search(self.advice_regex, advice, re.S)
 +
 +def test_advice_fedora_authconfig(self):
 +advice_id = 'config-fedora-authconfig'
 +advice_regex = \#\!\/bin\/sh.* \
 +   authconfig[\s]+\-\-enableldap[\s]+ \
 +   \-\-ldapserver\=.*[\s]+\-\-enablerfc2307bis[\s]+
 \
 +   \-\-enablekrb5
 +raiseonerr = True
 +# Obtain the advice from the server
 +tasks.kinit_admin(self.master)
 +result = self.master.run_command(['ipa-advise', self.advice_id],
 + raiseonerr=self.raiseerr)
 +
 +if not result.stdout_text:
 +advice = result.stderr_text
 +else:
 +advice = result.stdout_text
 +
 +assert re.search(self.advice_regex, advice, re.S)

 ... the same for the remaining 6 cases

 Now, this pattern has lots of duplicated code which can be extracted to a
 helper method, I just thought it would help to be more explicit to get the
 idea across. In the end you can achieve the same level of conciseness than
 with the separate test classes. Good luck!

 HTH,

 Tomas




 On 02/25/2015 03:52 PM, Gabe Alford wrote:

  No worries about the delay. Thanks for taking the time! Updated patch
 attached.

  Thanks,

  Gabe

 On Tue, Feb 24, 2015 at 11:03 AM, Tomas Babej tba...@redhat.com wrote:

  Hi Gabe,

 sorry for the delay. Here comes the review!

  1.) All the tests fail, since the IPA master is not installed at all:

 def test_advice(self):
 # Obtain the advice from the server
tasks.kinit_admin(self.master)

 test_integration/test_advise.py:37:
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 _ _
 test_integration/tasks.py:484: in kinit_admin
 stdin_text=host.config.admin_password)
 ../pytest_multihost/host.py:222: in run_command
 command.wait(raiseonerr=raiseonerr)
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 _ _

 self = pytest_multihost.transport.SSHCommand object at 0x7f09c0530c90
 raiseonerr = True

 def wait(self, raiseonerr=True):
 Wait for the remote process to exit

 Raises an excption if the exit code is not 0, unless raiseonerr 
 is
 true.
 
 if self._done:
 return self.returncode

 self._end_process()

 self._done = True

 if raiseonerr and self.returncode:

Re: [Freeipa-devel] [PATCH 0190] DNSSEC: add support for CKM_RSA_PKCS_OAEP mechanism

2015-02-26 Thread Martin Basti


On 26/02/15 12:47, Petr Spacek wrote:

On 11.2.2015 14:10, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/4657#comment:13

Patch attached.

--
Martin Basti


freeipa-mbasti-0190-DNSSEC-add-support-for-CKM_RSA_PKCS_OAEP-mechanism.patch


 From 4d698a5adaa94eb854c75bd9bcaf3093f31a11e5 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Wed, 11 Feb 2015 14:05:46 +0100
Subject: [PATCH] DNSSEC add support for CKM_RSA_PKCS_OAEP mechanism

Ticket: https://fedorahosted.org/freeipa/ticket/4657#comment:13
---
  ipapython/ipap11helper/p11helper.c | 72 --
  1 file changed, 69 insertions(+), 3 deletions(-)

diff --git a/ipapython/ipap11helper/p11helper.c 
b/ipapython/ipap11helper/p11helper.c
index 
4e0f262057b377124793f1e3091a8c9df4794164..c638bbe849f1bbddc8004bd1c41128b1c9e7
 100644
--- a/ipapython/ipap11helper/p11helper.c
+++ b/ipapython/ipap11helper/p11helper.c
@@ -53,6 +53,22 @@
  // TODO
  #define CKA_COPYABLE   (0x0017)
  
+#define CKG_MGF1_SHA1 (0x0001)

+
+#define CKZ_DATA_SPECIFIED(0x0001)
+
+struct ck_rsa_pkcs_oaep_params {
+  CK_MECHANISM_TYPE hash_alg;
+  unsigned long mgf;
+  unsigned long source;
+  void *source_data;
+  unsigned long source_data_len;
+};
+
+typedef struct ck_rsa_pkcs_oaep_params CK_RSA_PKCS_OAEP_PARAMS;
+typedef struct ck_rsa_pkcs_oaep_params *CK_RSA_PKCS_OAEP_PARAMS_PTR;
+
+
  CK_BBOOL true = CK_TRUE;
  CK_BBOOL false = CK_FALSE;
  
@@ -118,6 +134,17 @@ CK_BBOOL* bool;

  } PyObj2Bool_mapping_t;
  
  /**

+ * Constants
+ */
+static const CK_RSA_PKCS_OAEP_PARAMS CONST_RSA_PKCS_OAEP_PARAMS = {
+.hash_alg = CKM_SHA_1,
+.mgf = CKG_MGF1_SHA1,
+.source = CKZ_DATA_SPECIFIED,
+.source_data = NULL,
+.source_data_len = 0
+};
+
+/**
   * ipap11helper Exceptions
   */
  static PyObject *ipap11helperException; //parent class for all exceptions
@@ -1359,17 +1386,36 @@ P11_Helper_export_wrapped_key(P11_Helper* self, 
PyObject *args, PyObject *kwds)
  CK_BYTE_PTR wrapped_key = NULL;
  CK_ULONG wrapped_key_len = 0;
  CK_MECHANISM wrapping_mech = { CKM_RSA_PKCS, NULL, 0 };
-CK_MECHANISM_TYPE wrapping_mech_type = CKM_RSA_PKCS;
  /* currently we don't support parameter in mechanism */
  
  static char *kwlist[] = { key, wrapping_key, wrapping_mech, NULL };

  //TODO check long overflow
  //TODO export method
  if (!PyArg_ParseTupleAndKeywords(args, kwds, kkk|, kwlist, object_key,
-object_wrapping_key, wrapping_mech_type)) {
+object_wrapping_key, wrapping_mech.mechanism)) {
  return NULL;
  }
-wrapping_mech.mechanism = wrapping_mech_type;
+
+// fill mech parameters
+switch(wrapping_mech.mechanism){
+case CKM_RSA_PKCS:
+case CKM_AES_KEY_WRAP:
+case CKM_AES_KEY_WRAP_PAD:
+//default params
+break;
+
+case CKM_RSA_PKCS_OAEP:
+/* Use the same configuration as openSSL
+ * https://www.openssl.org/docs/crypto/RSA_public_encrypt.html
+ */
+ wrapping_mech.pParameter = (void*) CONST_RSA_PKCS_OAEP_PARAMS;
+ wrapping_mech.ulParameterLen = sizeof(CONST_RSA_PKCS_OAEP_PARAMS);
+break;
+
+default:
+PyErr_SetString(ipap11helperError, Unsupported wrapping 
mechanism);
+return NULL;
+}
  
  rv = self-p11-C_WrapKey(self-session, wrapping_mech,

  object_wrapping_key, object_key, NULL, wrapped_key_len);
@@ -1452,6 +1498,26 @@ P11_Helper_import_wrapped_secret_key(P11_Helper* self, 
PyObject *args,
  return NULL;
  }
  
+switch(wrapping_mech.mechanism){

+case CKM_RSA_PKCS:
+case CKM_AES_KEY_WRAP:
+case CKM_AES_KEY_WRAP_PAD:
+//default params
+break;

NACK. This switch is duplicate of the previous one. Please split it into an
auxiliary function and call it twice.

Thank you!


Thanks. Updated patch attached.


--
Martin Basti

From e10fab710c7fd820fd05f5c1990df5b02eb28862 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Wed, 11 Feb 2015 14:05:46 +0100
Subject: [PATCH] DNSSEC add support for CKM_RSA_PKCS_OAEP mechanism

Ticket: https://fedorahosted.org/freeipa/ticket/4657#comment:13
---
 ipapython/ipap11helper/p11helper.c | 76 --
 1 file changed, 73 insertions(+), 3 deletions(-)

diff --git a/ipapython/ipap11helper/p11helper.c b/ipapython/ipap11helper/p11helper.c
index 9172e720d69aab82ab55a41b43b16145dad730f8..9a7b3ce56b4a40c23c461e40a8e1ded28a2d7c49 100644
--- a/ipapython/ipap11helper/p11helper.c
+++ b/ipapython/ipap11helper/p11helper.c
@@ -56,6 +56,22 @@
 // TODO
 #define CKA_COPYABLE   (0x0017)
 
+#define CKG_MGF1_SHA1 (0x0001)
+
+#define CKZ_DATA_SPECIFIED(0x0001)
+
+struct ck_rsa_pkcs_oaep_params {
+  CK_MECHANISM_TYPE hash_alg;
+  unsigned long mgf;
+  unsigned long source;
+  void *source_data;
+  unsigned long

Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

2015-02-26 Thread Martin Basti


On 26/02/15 13:06, Petr Spacek wrote:

Hello Martin,

thank you for patch! This NACK is only aesthetic :-)

On 25.2.2015 14:21, Martin Basti wrote:

  if (!check_return_value(rv, import_wrapped_key: key unwrapping)) {
+error = 1;
+goto final;
+}


This exact sequence is repeated many times in the code.

I would prefer a C macro like this:
#define GOTO_FAIL   \
 do {   \
 error = 1; \
 goto final;\
 } while(0)

This allows more dense code like:
if (!test)
GOTO_FAIL;

and does not have the risk of missing error = 1 somewhere.


+final:
  if (pkey != NULL)
  EVP_PKEY_free(pkey);
+if (label != NULL) PyMem_Free(label);
+if (error){
+return NULL;
+}
  return ret;
  }

Apparently, this is inconsistent with itself.

Please pick one style and use it, e.g.
if (label != NULL)
PyMem_Free(label)

... and do not add curly braces when unnecessary.

If you want, we can try running $ indent on current sources and committing
changes separately so you do not have to make changes like this by hand.


Thanks. Updated patch attached.

--
Martin Basti

From a92e2b4cc7826c9bcaf61dc422fe5108e1a19c1c Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Tue, 24 Feb 2015 19:25:31 +0100
Subject: [PATCH] Fix memory leaks in ipap11helper

Ticket: https://fedorahosted.org/freeipa/ticket/4657
---
 ipapython/ipap11helper/p11helper.c | 311 +++--
 1 file changed, 194 insertions(+), 117 deletions(-)

diff --git a/ipapython/ipap11helper/p11helper.c b/ipapython/ipap11helper/p11helper.c
index 9a7b3ce56b4a40c23c461e40a8e1ded28a2d7c49..99534b16d68e71e79693a9f5c40b49a94aeff7dd 100644
--- a/ipapython/ipap11helper/p11helper.c
+++ b/ipapython/ipap11helper/p11helper.c
@@ -160,6 +160,12 @@ static PyObject *ipap11helperDuplicationError; //key already exists
  * Support functions
  */
 
+#define GOTO_FAIL \
+do {  \
+error = 1;\
+goto final;   \
+} while(0);
+
 CK_BBOOL* pyobj_to_bool(PyObject* pyobj) {
 if (PyObject_IsTrue(pyobj))
 return true;
@@ -200,9 +206,11 @@ unsigned char* unicode_to_char_array(PyObject *unicode, Py_ssize_t *l) {
 /* Copy string first, then DECREF
  * https://docs.python.org/2/c-api/string.html#c.PyString_AS_STRING
  */
-result = (unsigned char *) malloc((size_t) *l);
+result = (unsigned char *) PyMem_Malloc((size_t) *l);
 if (result == NULL){
-PyErr_SetString(ipap11helperError, Memory allocation error);
+Py_DECREF(utf8_str);
+PyErr_NoMemory();
+return NULL;
 } else {
 memcpy(result, bytes, *l);
 }
@@ -689,7 +697,9 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 
 PyObject *label_unicode = NULL;
 Py_ssize_t label_length = 0;
+CK_BYTE *label = NULL;
 int r;
+int error = 0;
 static char *kwlist[] = { subject, id, key_length, cka_copyable,
 cka_decrypt, cka_derive, cka_encrypt, cka_extractable,
 cka_modifiable, cka_private, cka_sensitive, cka_sign,
@@ -711,26 +721,26 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 return NULL;
 }
 
-CK_BYTE *label = (unsigned char*) unicode_to_char_array(label_unicode,
+label = (unsigned char*) unicode_to_char_array(label_unicode,
 label_length);
+if (label == NULL) GOTO_FAIL;
+
 CK_MECHANISM mechanism = { //TODO param?
 CKM_AES_KEY_GEN, NULL_PTR, 0 };
 
 if ((key_length != 16)  (key_length != 24)  (key_length != 32)) {
 PyErr_SetString(ipap11helperError,
 generate_master_key: key length allowed values are: 16, 24 and 32);
-return NULL;
+GOTO_FAIL;
 }
 
-//TODO free label if check failed
-//TODO is label freed inside dont we use freed value later
 r = _id_exists(self, id, id_length, CKO_SECRET_KEY);
 if (r == 1) {
 PyErr_SetString(ipap11helperDuplicationError,
 Master key with same ID already exists);
-return NULL;
+GOTO_FAIL;
 } else if (r == -1) {
-return NULL;
+GOTO_FAIL;
 }
 
 /* Process keyword boolean arguments */
@@ -758,9 +768,13 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 
 rv = self-p11-C_GenerateKey(self-session, mechanism, symKeyTemplate,
 sizeof(symKeyTemplate) / sizeof(CK_ATTRIBUTE), master_key);
-if (!check_return_value(rv, generate master key))
-return NULL;
+if (!check_return_value(rv, generate master key)){
+GOTO_FAIL;
+}
+final:
+if (label != NULL) PyMem_Free(label);
 
+if

Re: [Freeipa-devel] [PATCH] 0039 Try continue ipa-client-automount even if nsslapd-minssf 0.

2015-02-26 Thread David Kupka


On 02/26/2015 02:55 PM, Rob Crittenden wrote:

Martin Basti wrote:

On 26/02/15 10:57, David Kupka wrote:

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


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

Works for me, ACK.


NACK.

If you simply pass in /etc/ipa/ca.crt as the cacert path then it will
use TLS.

rob

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



Thanks for the catch Rob. Updated patch attached.

--
David Kupka
From 4811019508953c197a77aac081732f0af4cce614 Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Thu, 26 Feb 2015 04:44:26 -0500
Subject: [PATCH] Use IPA CA certificate when available and ignore NO_TLS_LDAP
 when not.

ipa-client-automount is run after ipa-client-install so the CA certificate
should be available. If the certificate is not available and ipadiscovery.ipacheckldap
returns NO_TLS_LDAP warn user and try to continue.

https://fedorahosted.org/freeipa/ticket/4902
---
 ipa-client/ipa-install/ipa-client-automount | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-automount b/ipa-client/ipa-install/ipa-client-automount
index 110e0ba13287e8c3061864b2e6c7b27d0ca83a6c..9e38412c8893c5d5b7459627a29aadee1c876f3a 100755
--- a/ipa-client/ipa-install/ipa-client-automount
+++ b/ipa-client/ipa-install/ipa-client-automount
@@ -379,6 +379,10 @@ def main():
 api.bootstrap(**cfg)
 api.finalize()
 
+ca_cert_path = None
+if os.path.exists(paths.IPA_CA_CRT):
+ca_cert_path = paths.IPA_CA_CRT
+
 if options.uninstall:
 return uninstall(fstore, statestore)
 
@@ -390,7 +394,7 @@ def main():
 ds = ipadiscovery.IPADiscovery()
 if not options.server:
 print Searching for IPA server...
-ret = ds.search()
+ret = ds.search(ca_cert_path=ca_cert_path)
 root_logger.debug('Executing DNS discovery')
 if ret == ipadiscovery.NO_LDAP_SERVER:
 root_logger.debug('Autodiscovery did not find LDAP server')
@@ -406,11 +410,13 @@ def main():
 else:
 server = options.server
 root_logger.debug(Verifying that %s is an IPA server % server)
-ldapret = ds.ipacheckldap(server, api.env.realm)
+ldapret = ds.ipacheckldap(server, api.env.realm, ca_cert_path)
 if ldapret[0] == ipadiscovery.NO_ACCESS_TO_LDAP:
 print Anonymous access to the LDAP server is disabled.
 print Proceeding without strict verification.
 print Note: This is not an error if anonymous access has been explicitly restricted.
+elif ldapret[0] == ipadiscovery.NO_TLS_LDAP:
+root_logger.warning(Unencrypted access to LDAP is not supported.)
 elif ldapret[0] != 0:
 sys.exit('Unable to confirm that %s is an IPA server' % server)
 
-- 
2.1.0

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

Re: [Freeipa-devel] IPA Server upgrade 4.2 design

2015-02-26 Thread Martin Basti


On 26/02/15 10:45, Petr Spacek wrote:

On 25.2.2015 17:49, Martin Basti wrote:

On 25/02/15 17:15, Petr Spacek wrote:

On 24.2.2015 19:10, Martin Basti wrote:

Hello all,

please read the design page, any objections/suggestions appreciated
http://www.freeipa.org/page/V4/Server_Upgrade_Refactoring

Thank you for the design, I have only few nitpicks.


Increase update files numbers range
Update files number will be extended into 4 digits values.

IMHO the dependency on particular number format should be removed altogether.
It should be perfectly enough to say that updates are executed in ASCII
lexicographic order and be done with it.

4.1.3-2  4.1.3-12 in lexicographic order, this will not fit.

Well, sure, but it allows you to use
00-a
01-b

and renumber it to

001-a
002-b

at will without changes to code. (Lexicographic order is what 'ls' uses by
default so you can see the real ordering at any time very easily.)

Also, as you pointed out, it allows you to do things like
12.345-a
12.666-bbb
without changing code, again :-)

Oh stupid me, I read it wrong, I replied with IPA version compare.

sounds good to me, any objections anyone?


Petr^2 Spacek


To resolve issues mentioned above only one command should do upgrade:
ipa-server-upgrade.

I very much agree with this.



ipa-server-upgrade characteristics

...

4. LDAP data update (+ update plugins)
5. upgrade configuration

At this point I would appreciate explanatory text what is 'LDAP data update'
and what is 'upgrade configuration'. Maybe some examples could be enough.

LDAP data update == upgrading data stored in LDAP (user data + cn=config)
upgrade configuration == upgrading configuration of services in filesystem
(apache, named)

I will add some explanation there.

ipactl checks if installed version and version stored in LDAP are the same:

...

ipactl start|restart option --force overrides this check.

I would like to see a separate option. --force currently skips rollback if
some services could not start but this is fundamentally different from
version/upgrade checks.

Ohh, good catch thank you, maybe --skip-version-check ?

Sounds fine to me.


ipa-server-upgrade
--version show program's version number and exit

Maybe it could print code version + data version (if available). It could be
handy debugging tool.

Good idea thanks

ipa-server-upgrade
--test Note: for developing only

Is it really worth the effort to keep the option and invest more time in it?


I do not expect any extra effort (except fixing 3 plugins - 6 lines of code
approx), so if it will help to develop updates it could stay there (personally
I do not use it, usually updates broke during write to server on some
constraints)

Okay, I thought that it is broken significantly.




--
Martin Basti

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

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

2015-02-26 Thread Tomas Babej


ACK.

Pushed to:
ipa-4-1: ddd7fb6a68fd413b1561eab9c29bac18882e5efd
master: ae4ee6b53376bb7f3d1b4707c4e105c91b5cd8ab

On 02/26/2015 05:58 PM, Gabe Alford wrote:

Yeah. That makes more sense. Updated patch attached.

Thanks,

Gabe

On Wed, Feb 25, 2015 at 3:55 PM, Tomas Babej tba...@redhat.com 
mailto:tba...@redhat.com wrote:


Hi Gabe,

sorry for not being clear. This approach will not work:

+class TestAdvice(BaseTestInvalidAdvice,
+ BaseTestFedoraAuthconfig,
+ BaseTestFreeBSDNSSPAM,
+ BaseTestGenericNSSPAM,
+ BaseTestGenericSSSDBefore19,
+ BaseTestRedHatNSS,
+ BaseTestRedHatNSSPAM,
+ BaseTestRedHatSSSDBefore19,
+ BaseTestAdvice):
+pass

By combining all the base classes into one, you will not get the
desired effect (which is to run the test_advice method for each
advice_id). Let me explain why:

The test runner works in the following way: it inspects any
discovered class which name begins with Test, and executes each
its method, which names begins with test as a test case.

If the test runner inspects the TestAdvice class, the only method
beggining with test, which it will see, is the test_advice
which was inherited back from BaseTestAdvice class. So we can
safely conclude the test runner will only run 1 test case.

Which one, you may ask? Well, since the test_advice behaviour is
fully determined by the values of advice_id, advice_regex and
raiseerr attributes, let's look at their values in TestAdvice
class. This class does not define attirbutes with such names, so
we move along the inheritance chain (also called MRO) - the first
class from which we inherit is BaseTestInvalidAdvice, and this
class defines all three mentioned attributes.

Hence the only test method will be run the test for invalid advice :)

Now, how to fix this? The easiest approach would be to abandon the
approach with the separate classes, and map each class to a test
method in the TestAdvice class, like this (from the top of my head):

+class TestAdvice(IntegrationTest):
+topology = 'line'
+
+def test_invalid_advice(self):
+advice_id = 'invalid-advise-param'
+advice_regex = invalid[\s]+\'advice\'.*
+raiseerr = False
+# Obtain the advice from the server
+tasks.kinit_admin(self.master)
+result = self.master.run_command(['ipa-advise',
self.advice_id],
+ raiseonerr=self.raiseerr)
+
+if not result.stdout_text:
+advice = result.stderr_text
+else:
+advice = result.stdout_text
+
+assert re.search(self.advice_regex, advice, re.S)
+
+def test_advice_fedora_authconfig(self):
+advice_id = 'config-fedora-authconfig'
+advice_regex = \#\!\/bin\/sh.* \
+ authconfig[\s]+\-\-enableldap[\s]+ \
+ \-\-ldapserver\=.*[\s]+\-\-enablerfc2307bis[\s]+ \
+   \-\-enablekrb5
+raiseonerr = True
+# Obtain the advice from the server
+tasks.kinit_admin(self.master)
+result = self.master.run_command(['ipa-advise',
self.advice_id],
+ raiseonerr=self.raiseerr)
+
+if not result.stdout_text:
+advice = result.stderr_text
+else:
+advice = result.stdout_text
+
+assert re.search(self.advice_regex, advice, re.S)

... the same for the remaining 6 cases

Now, this pattern has lots of duplicated code which can be
extracted to a helper method, I just thought it would help to be
more explicit to get the idea across. In the end you can achieve
the same level of conciseness than with the separate test classes.
Good luck!

HTH,

Tomas




On 02/25/2015 03:52 PM, Gabe Alford wrote:

No worries about the delay. Thanks for taking the time! Updated
patch attached.

Thanks,

Gabe

On Tue, Feb 24, 2015 at 11:03 AM, Tomas Babej tba...@redhat.com
mailto:tba...@redhat.com wrote:

Hi Gabe,

sorry for the delay. Here comes the review!

1.) All the tests fail, since the IPA master is not installed
at all:

 def test_advice(self):
 # Obtain the advice from the server
   tasks.kinit_admin(self.master)

test_integration/test_advise.py:37:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _
test_integration/tasks.py:484: in kinit_admin
 stdin_text=host.config.admin_password)
../pytest_multihost/host.py:222: in run_command
 command.wait(raiseonerr=raiseonerr)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _

Re: [Freeipa-devel] IPA Server upgrade 4.2 design

2015-02-26 Thread Petr Spacek

On 25.2.2015 17:49, Martin Basti wrote:
 On 25/02/15 17:15, Petr Spacek wrote:
 On 24.2.2015 19:10, Martin Basti wrote:
 Hello all,

 please read the design page, any objections/suggestions appreciated
 http://www.freeipa.org/page/V4/Server_Upgrade_Refactoring
 Thank you for the design, I have only few nitpicks.

 Increase update files numbers range
 Update files number will be extended into 4 digits values.
 IMHO the dependency on particular number format should be removed altogether.
 It should be perfectly enough to say that updates are executed in ASCII
 lexicographic order and be done with it.
 4.1.3-2  4.1.3-12 in lexicographic order, this will not fit.

Well, sure, but it allows you to use
00-a
01-b

and renumber it to

001-a
002-b

at will without changes to code. (Lexicographic order is what 'ls' uses by
default so you can see the real ordering at any time very easily.)

Also, as you pointed out, it allows you to do things like
12.345-a
12.666-bbb
without changing code, again :-)

Petr^2 Spacek

 To resolve issues mentioned above only one command should do upgrade:
 ipa-server-upgrade.
 I very much agree with this.


 ipa-server-upgrade characteristics
 ...
 4. LDAP data update (+ update plugins)
 5. upgrade configuration
 At this point I would appreciate explanatory text what is 'LDAP data update'
 and what is 'upgrade configuration'. Maybe some examples could be enough.
 LDAP data update == upgrading data stored in LDAP (user data + cn=config)
 upgrade configuration == upgrading configuration of services in filesystem
 (apache, named)
 
 I will add some explanation there.

 ipactl checks if installed version and version stored in LDAP are the same:
 ...
 ipactl start|restart option --force overrides this check.
 I would like to see a separate option. --force currently skips rollback if
 some services could not start but this is fundamentally different from
 version/upgrade checks.
 Ohh, good catch thank you, maybe --skip-version-check ?
Sounds fine to me.


 ipa-server-upgrade
 --version show program's version number and exit
 Maybe it could print code version + data version (if available). It could be
 handy debugging tool.
 Good idea thanks
 ipa-server-upgrade
 --test Note: for developing only
 Is it really worth the effort to keep the option and invest more time in it?

 I do not expect any extra effort (except fixing 3 plugins - 6 lines of code
 approx), so if it will help to develop updates it could stay there (personally
 I do not use it, usually updates broke during write to server on some
 constraints)

Okay, I thought that it is broken significantly.

-- 
Petr^2 Spacek

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

[Freeipa-devel] [PATCH] 0039 Try continue ipa-client-automount even if nsslapd-minssf 0.

2015-02-26 Thread David Kupka


https://fedorahosted.org/freeipa/ticket/4902
--
David Kupka
From 06f268e0c6435f3ba421787cf57e49c2ef2ac00d Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Thu, 26 Feb 2015 04:44:26 -0500
Subject: [PATCH] Try continue ipa-client-automount even if nsslapd-minssf  0.

If ipadiscovery.ipacheckldap returned NO_TLS_LDAP warn user and
try to continue.

https://fedorahosted.org/freeipa/ticket/4902
---
 ipa-client/ipa-install/ipa-client-automount | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ipa-client/ipa-install/ipa-client-automount b/ipa-client/ipa-install/ipa-client-automount
index 110e0ba13287e8c3061864b2e6c7b27d0ca83a6c..bbf9b916b02106dffe26dc592959abfa4712be5b 100755
--- a/ipa-client/ipa-install/ipa-client-automount
+++ b/ipa-client/ipa-install/ipa-client-automount
@@ -411,6 +411,8 @@ def main():
 print Anonymous access to the LDAP server is disabled.
 print Proceeding without strict verification.
 print Note: This is not an error if anonymous access has been explicitly restricted.
+elif ldapret[0] == ipadiscovery.NO_TLS_LDAP:
+root_logger.warning(Unencrypted access to LDAP is not supported.)
 elif ldapret[0] != 0:
 sys.exit('Unable to confirm that %s is an IPA server' % server)
 
-- 
2.1.0

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

Re: [Freeipa-devel] [PATCH 0190] DNSSEC: add support for CKM_RSA_PKCS_OAEP mechanism

2015-02-26 Thread Petr Spacek

On 11.2.2015 14:10, Martin Basti wrote:
 https://fedorahosted.org/freeipa/ticket/4657#comment:13
 
 Patch attached.
 
 -- 
 Martin Basti
 
 
 freeipa-mbasti-0190-DNSSEC-add-support-for-CKM_RSA_PKCS_OAEP-mechanism.patch
 
 
 From 4d698a5adaa94eb854c75bd9bcaf3093f31a11e5 Mon Sep 17 00:00:00 2001
 From: Martin Basti mba...@redhat.com
 Date: Wed, 11 Feb 2015 14:05:46 +0100
 Subject: [PATCH] DNSSEC add support for CKM_RSA_PKCS_OAEP mechanism
 
 Ticket: https://fedorahosted.org/freeipa/ticket/4657#comment:13
 ---
  ipapython/ipap11helper/p11helper.c | 72 
 --
  1 file changed, 69 insertions(+), 3 deletions(-)
 
 diff --git a/ipapython/ipap11helper/p11helper.c 
 b/ipapython/ipap11helper/p11helper.c
 index 
 4e0f262057b377124793f1e3091a8c9df4794164..c638bbe849f1bbddc8004bd1c41128b1c9e7
  100644
 --- a/ipapython/ipap11helper/p11helper.c
 +++ b/ipapython/ipap11helper/p11helper.c
 @@ -53,6 +53,22 @@
  // TODO
  #define CKA_COPYABLE   (0x0017)
  
 +#define CKG_MGF1_SHA1 (0x0001)
 +
 +#define CKZ_DATA_SPECIFIED(0x0001)
 +
 +struct ck_rsa_pkcs_oaep_params {
 +  CK_MECHANISM_TYPE hash_alg;
 +  unsigned long mgf;
 +  unsigned long source;
 +  void *source_data;
 +  unsigned long source_data_len;
 +};
 +
 +typedef struct ck_rsa_pkcs_oaep_params CK_RSA_PKCS_OAEP_PARAMS;
 +typedef struct ck_rsa_pkcs_oaep_params *CK_RSA_PKCS_OAEP_PARAMS_PTR;
 +
 +
  CK_BBOOL true = CK_TRUE;
  CK_BBOOL false = CK_FALSE;
  
 @@ -118,6 +134,17 @@ CK_BBOOL* bool;
  } PyObj2Bool_mapping_t;
  
  /**
 + * Constants
 + */
 +static const CK_RSA_PKCS_OAEP_PARAMS CONST_RSA_PKCS_OAEP_PARAMS = {
 +.hash_alg = CKM_SHA_1,
 +.mgf = CKG_MGF1_SHA1,
 +.source = CKZ_DATA_SPECIFIED,
 +.source_data = NULL,
 +.source_data_len = 0
 +};
 +
 +/**
   * ipap11helper Exceptions
   */
  static PyObject *ipap11helperException; //parent class for all exceptions
 @@ -1359,17 +1386,36 @@ P11_Helper_export_wrapped_key(P11_Helper* self, 
 PyObject *args, PyObject *kwds)
  CK_BYTE_PTR wrapped_key = NULL;
  CK_ULONG wrapped_key_len = 0;
  CK_MECHANISM wrapping_mech = { CKM_RSA_PKCS, NULL, 0 };
 -CK_MECHANISM_TYPE wrapping_mech_type = CKM_RSA_PKCS;
  /* currently we don't support parameter in mechanism */
  
  static char *kwlist[] = { key, wrapping_key, wrapping_mech, NULL };
  //TODO check long overflow
  //TODO export method
  if (!PyArg_ParseTupleAndKeywords(args, kwds, kkk|, kwlist, object_key,
 -object_wrapping_key, wrapping_mech_type)) {
 +object_wrapping_key, wrapping_mech.mechanism)) {
  return NULL;
  }
 -wrapping_mech.mechanism = wrapping_mech_type;
 +
 +// fill mech parameters
 +switch(wrapping_mech.mechanism){
 +case CKM_RSA_PKCS:
 +case CKM_AES_KEY_WRAP:
 +case CKM_AES_KEY_WRAP_PAD:
 +//default params
 +break;
 +
 +case CKM_RSA_PKCS_OAEP:
 +/* Use the same configuration as openSSL
 + * https://www.openssl.org/docs/crypto/RSA_public_encrypt.html
 + */
 + wrapping_mech.pParameter = (void*) CONST_RSA_PKCS_OAEP_PARAMS;
 + wrapping_mech.ulParameterLen = 
 sizeof(CONST_RSA_PKCS_OAEP_PARAMS);
 +break;
 +
 +default:
 +PyErr_SetString(ipap11helperError, Unsupported wrapping 
 mechanism);
 +return NULL;
 +}
  
  rv = self-p11-C_WrapKey(self-session, wrapping_mech,
  object_wrapping_key, object_key, NULL, wrapped_key_len);
 @@ -1452,6 +1498,26 @@ P11_Helper_import_wrapped_secret_key(P11_Helper* self, 
 PyObject *args,
  return NULL;
  }
  
 +switch(wrapping_mech.mechanism){
 +case CKM_RSA_PKCS:
 +case CKM_AES_KEY_WRAP:
 +case CKM_AES_KEY_WRAP_PAD:
 +//default params
 +break;

NACK. This switch is duplicate of the previous one. Please split it into an
auxiliary function and call it twice.

Thank you!

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCHES 0001-0002] ipa-client-install NTP fixes

2015-02-26 Thread Martin Kosek

On 02/26/2015 03:28 AM, Nathan Kinder wrote:
 Hi,
 
 The two attached patches address some issues that affect
 ipa-client-install when syncing time from the NTP server.  Now that we
 use ntpd to perform the time sync, the client install can end up hanging
 forever when the server is not reachable (firewall issues, etc.).  These
 patches address the issues in two different ways:
 
 1 - Don't attempt to sync time when --no-ntp is specified.
 
 2 - Implement a timeout capability that is used when we run ntpd to
 perform the time sync to prevent indefinite hanging.
 
 The one potentially contentious issue is that this introduces a new
 dependency on python-subprocess32 to allow us to have timeout support
 when using Python 2.x.  This is packaged for Fedora, but I don't see it
 on RHEL or CentOS currently.  It would need to be packaged there.
 
 https://fedorahosted.org/freeipa/ticket/4842
 
 Thanks,
 -NGK

Thanks for Patches. For the second patch, I would really prefer to avoid new
dependency, especially if it's not packaged in RHEL/CentOS. Maybe we could use
some workaround instead, as in:

http://stackoverflow.com/questions/3733270/python-subprocess-timeout

?

Martin

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

Re: [Freeipa-devel] [PATCH] 0039 Try continue ipa-client-automount even if nsslapd-minssf 0.

2015-02-26 Thread Martin Basti


On 26/02/15 10:57, David Kupka wrote:

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


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

Works for me, ACK.

--
Martin Basti

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

Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

2015-02-26 Thread Petr Spacek

Hello Martin,

thank you for patch! This NACK is only aesthetic :-)

On 25.2.2015 14:21, Martin Basti wrote:
  if (!check_return_value(rv, import_wrapped_key: key unwrapping)) {
 +error = 1;
 +goto final;
 +}


This exact sequence is repeated many times in the code.

I would prefer a C macro like this:
#define GOTO_FAIL   \
do {\
error = 1;  \
goto final; \
} while(0)

This allows more dense code like:
if (!test)
GOTO_FAIL;

and does not have the risk of missing error = 1 somewhere.

 +final:
  if (pkey != NULL)
  EVP_PKEY_free(pkey);
 +if (label != NULL) PyMem_Free(label);
 +if (error){
 +return NULL;
 +}
  return ret;
  }

Apparently, this is inconsistent with itself.

Please pick one style and use it, e.g.
if (label != NULL)
PyMem_Free(label)

... and do not add curly braces when unnecessary.

If you want, we can try running $ indent on current sources and committing
changes separately so you do not have to make changes like this by hand.

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH] 0039 Try continue ipa-client-automount even if nsslapd-minssf 0.

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

Re: [Freeipa-devel] [PATCH 0190] DNSSEC: add support for CKM_RSA_PKCS_OAEP mechanism

Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

Re: [Freeipa-devel] [PATCH] 0039 Try continue ipa-client-automount even if nsslapd-minssf 0.

Re: [Freeipa-devel] IPA Server upgrade 4.2 design

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

Re: [Freeipa-devel] IPA Server upgrade 4.2 design

[Freeipa-devel] [PATCH] 0039 Try continue ipa-client-automount even if nsslapd-minssf 0.

Re: [Freeipa-devel] [PATCH 0190] DNSSEC: add support for CKM_RSA_PKCS_OAEP mechanism

Re: [Freeipa-devel] [PATCHES 0001-0002] ipa-client-install NTP fixes

Re: [Freeipa-devel] [PATCH] 0039 Try continue ipa-client-automount even if nsslapd-minssf 0.

Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

13 matches

Site Navigation

Mail list logo

Footer information