KRB5: Fixing FQ name of user in krb5_setup()

[celestian]

This patch fixes creation of FQ username if krb5_map_user option
ise used.

Resolves:
https://fedorahosted.org/sssd/ticket/3188

[jhrozek]

Why not do it the other way around and qualify all names in the mapping? That needs to be done only once unlike this approach that needs to unqualify the names every time they are used.

[celestian]

I pushed patch to our CI, we will see result soon.

[celestian]

http://sssd-ci.duckdns.org/logs/job/53/43/summary.html
So, it is problem with cflags in test. I will send patch soon.

[celestian]

Link to our CI:
http://sssd-ci.duckdns.org/logs/job/53/46/summary.html

[jhrozek]

Both these calls should have their return value checked. Unless you're using strdup to get around a const-issue, you can also directly assign to id_name.

[celestian]

I added conditions on sss_create_internal_fqname() calls.

I use strdup for two reason:

• is const, of course,
• without it the CI test fails -- it is because of need more room for FQ name.

[jhrozek]

You also need to use the output name for kr->kuserok_user, currently it's using qualified name, which is not going to work (see the branch just below..)

[lslebodn]

The similar issue is also in other tests:
refcount_tests_CFLAGS
fail_over_tests_CFLAGS
sbus_tests_CFLAGS
sbus_codegen_tests_CFLAGS

and you will need to use discard_const in sbus* test to fix warnings

[celestian]

Addressed. Thanks.

[lslebodn]

Bump, there are users waiting for this fix. LS

[celestian]

Hi,
I push new version. All comments are addressed.
Petr

[lslebodn]

the following change looks werd to me
- dbus_free_string_array(arr_object_path);
+ dbus_free_string_array(discard_const(arr_object_path));

What about following change
@@ -673,8 +674,8 @@ START_TEST(test_marshal_basic_types)
dbus_int64_t v_int64[] = { INT64_C(-6666666666666666), INT64_C(7777777777777777) };
dbus_uint64_t v_uint64[] = { UINT64_C(7777777777777777), INT64_C(888888888888888888) };
double v_double[] = { 1.1, 2.2, 3.3 };
- char *v_string[] = { "bears", "bears", "bears" };
- char *v_object_path[] = { "/original", "/original" };
+ const char *v_string[] = { "bears", "bears", "bears" };
+ const char *v_object_path[] = { "/original", "/original" };

unsigned char *arr_byte = v_byte;
dbus_int16_t *arr_int16 = v_int16;
@@ -684,8 +685,8 @@ START_TEST(test_marshal_basic_types)
dbus_int64_t *arr_int64 = v_int64;
dbus_uint64_t *arr_uint64 = v_uint64;
double *arr_double = v_double;
- char **arr_string = v_string;
- char **arr_object_path = v_object_path;
+ char **arr_string = discard_const_p(char *, v_string);
+ char **arr_object_path = discard_const_p(char *, v_object_path);

BTW the current order of patches would break bisec.
the 1st patch should contain fixes in sbus testa
and then shoudl be added AM_CFLAGS

LS

[celestian]

I change the order of patches, thanks Lukas.

[lslebodn]

On (19/09/16 04:07), celestian wrote:

I change the order of patches, thanks Lukas.

Thank you for changing the order but I cannot see requested changes
in src/tests/sbus_codegen_tests.c

LS

[celestian]

Lukas,
sorry,I missed it. New version pushed.

I have a question, is there any way how to reply to your comment? This is new one, it is not direct answer.

[lslebodn]

On (19/09/16 04:39), celestian wrote:

Lukas,
sorry,I missed it. New version pushed.

I have question, is there any way how to reply to your comment? This is new one, it is not direct answer.

The first 2 patches pushed to master:

• 4f2509f
• 626d821

They were not very related to the ticket #3188

LS

[jhrozek]

ret is undefined here..

[celestian]

Thanks, I added ENOENT for this case in both conditions.

[jhrozek]

I think ENOMEM would be better, I don't see why we should return "No such file or directory".

[jhrozek]

You forgot to check the result of strdup. And why do we need to duplicate the name here after all? Can't we just steal it or assign directly to name_to_primary[i].id_name?

[celestian]

Now I am stealing it. It is allocated on tmp_ctx.

[celestian]

New version is pushed.

[lslebodn]

Bump for review. Users are waiting.

[jhrozek]

I think this is just not needed, why not assign to the id_name/krb5_primary and remove the fq_name variable?

Something like:
name_to_primary[i].krb_primary = sss_create_internal_fqname(name_to_primary,

[jhrozek]

Also please s/ENOENT/ENOMEM/

[celestian]

s/ENOENT/ENOMEM/ addressed.

[celestian]

I will take a look at fq_name stuff.

[celestian]

So, all notes addressed (fq_name too).
I pushed new version.

[lslebodn]

Bump for review. Users are waiting.

[jhrozek]

Code-wise ACK, the code works and looks good. I will push once Coverity and CI finish.

[jhrozek]

On Thu, Sep 22, 2016 at 02:57:27AM -0700, Jakub Hrozek wrote:

Code-wise ACK, the code works and looks good. I will push once Coverity and CI finish.

CI: http://sssd-ci.duckdns.org/logs/job/53/87/summary.html

ACK

[lslebodn]

On (22/09/16 04:48), Jakub Hrozek wrote:

On Thu, Sep 22, 2016 at 02:57:27AM -0700, Jakub Hrozek wrote:

Code-wise ACK, the code works and looks good. I will push once Coverity and CI finish.

CI: http://sssd-ci.duckdns.org/logs/job/53/87/summary.html

ACK

master:

• b34ffbf

LS