Hello,
attached patches should fix https://fedorahosted.org/sssd/ticket/1830
First patch reuse existing function create_pam_data to creation structures pam_data everywhere in the source code. In second patch this function is extended to also initialize own members of type struct sss_auth_token *.
Second patch makes authtok structure really opaque.
Any comments are welcomed
LS
On 03/14/2013 12:58 PM, Lukas Slebodnik wrote:
@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost)));
- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok)));
- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok)));
- DEBUG(SSSDBG_CRIT_FAILURE,
("authtok type: %d\n", sss_authtok_get_type(pd->authtok)));- DEBUG(SSSDBG_CRIT_FAILURE,
("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok))); DEBUG(l, ("priv: %d\n", pd->priv)); DEBUG(l, ("cli_pid: %d\n", pd->cli_pid));
The patches look good to me, just please use the debug level variable here, not hardcoded level.
On (15/03/13 09:13), Ondrej Kos wrote:
On 03/14/2013 12:58 PM, Lukas Slebodnik wrote:
@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost)));
- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok)));
- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok)));
- DEBUG(SSSDBG_CRIT_FAILURE,
("authtok type: %d\n", sss_authtok_get_type(pd->authtok)));- DEBUG(SSSDBG_CRIT_FAILURE,
("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok)));}
The patches look good to me, just please use the debug level variable here, not hardcoded level.
-- Ondrej Kos
Thanks. I am attaching updated patches.
LS
On 03/15/2013 10:34 AM, Lukas Slebodnik wrote:
On (15/03/13 09:13), Ondrej Kos wrote:
On 03/14/2013 12:58 PM, Lukas Slebodnik wrote:
@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost)));
- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok)));
- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok)));
- DEBUG(SSSDBG_CRIT_FAILURE,
("authtok type: %d\n", sss_authtok_get_type(pd->authtok)));- DEBUG(SSSDBG_CRIT_FAILURE,
("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok))); DEBUG(l, ("priv: %d\n", pd->priv)); DEBUG(l, ("cli_pid: %d\n", pd->cli_pid));The patches look good to me, just please use the debug level variable here, not hardcoded level.
-- Ondrej Kos
Thanks. I am attaching updated patches.
LS
Ack from me
Ondra
On Fri, Mar 15, 2013 at 12:46:34PM +0100, Ondrej Kos wrote:
On 03/15/2013 10:34 AM, Lukas Slebodnik wrote:
On (15/03/13 09:13), Ondrej Kos wrote:
On 03/14/2013 12:58 PM, Lukas Slebodnik wrote:
@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost)));
- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok)));
- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok)));
- DEBUG(SSSDBG_CRIT_FAILURE,
("authtok type: %d\n", sss_authtok_get_type(pd->authtok)));- DEBUG(SSSDBG_CRIT_FAILURE,
("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok)));}
The patches look good to me, just please use the debug level variable here, not hardcoded level.
-- Ondrej Kos
Thanks. I am attaching updated patches.
LS
Ack from me
Ondra
The second patch no longer applies on the current master, can you rebase it?
On (20/03/13 13:58), Jakub Hrozek wrote:
On Fri, Mar 15, 2013 at 12:46:34PM +0100, Ondrej Kos wrote:
On 03/15/2013 10:34 AM, Lukas Slebodnik wrote:
On (15/03/13 09:13), Ondrej Kos wrote:
On 03/14/2013 12:58 PM, Lukas Slebodnik wrote:
@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost)));
- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok)));
- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok)));
- DEBUG(SSSDBG_CRIT_FAILURE,
("authtok type: %d\n", sss_authtok_get_type(pd->authtok)));- DEBUG(SSSDBG_CRIT_FAILURE,
("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok)));}
The patches look good to me, just please use the debug level variable here, not hardcoded level.
-- Ondrej Kos
Thanks. I am attaching updated patches.
LS
Ack from me
Ondra
The second patch no longer applies on the current master, can you rebase it?
I am attaching rebased patches.
LS
On Wed, Mar 20, 2013 at 07:17:28PM +0100, Lukas Slebodnik wrote:
On (20/03/13 13:58), Jakub Hrozek wrote:
On Fri, Mar 15, 2013 at 12:46:34PM +0100, Ondrej Kos wrote:
On 03/15/2013 10:34 AM, Lukas Slebodnik wrote:
On (15/03/13 09:13), Ondrej Kos wrote:
On 03/14/2013 12:58 PM, Lukas Slebodnik wrote:
@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost)));
- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok)));
- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok)));
- DEBUG(SSSDBG_CRIT_FAILURE,
("authtok type: %d\n", sss_authtok_get_type(pd->authtok)));- DEBUG(SSSDBG_CRIT_FAILURE,
("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok)));}
The patches look good to me, just please use the debug level variable here, not hardcoded level.
-- Ondrej Kos
Thanks. I am attaching updated patches.
LS
Ack from me
Ondra
The second patch no longer applies on the current master, can you rebase it?
I am attaching rebased patches.
LS
+struct sss_auth_token *sss_authtok_new_empty(TALLOC_CTX *mem_ctx) +{
- struct sss_auth_token *token;
- token = talloc_zero(mem_ctx, struct sss_auth_token);
^^^ tabs instead of spaces.
- if (token == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));- }
- return token;
+}
The rest looks good to me.
On (21/03/13 20:18), Jakub Hrozek wrote:
On Wed, Mar 20, 2013 at 07:17:28PM +0100, Lukas Slebodnik wrote:
On (20/03/13 13:58), Jakub Hrozek wrote:
On Fri, Mar 15, 2013 at 12:46:34PM +0100, Ondrej Kos wrote:
On 03/15/2013 10:34 AM, Lukas Slebodnik wrote:
On (15/03/13 09:13), Ondrej Kos wrote:
On 03/14/2013 12:58 PM, Lukas Slebodnik wrote: >@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) > DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); > DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); > DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost))); >- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok))); >- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok))); >+ DEBUG(SSSDBG_CRIT_FAILURE, >+ ("authtok type: %d\n", sss_authtok_get_type(pd->authtok))); >+ DEBUG(SSSDBG_CRIT_FAILURE, >+ ("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok))); > DEBUG(l, ("priv: %d\n", pd->priv)); > DEBUG(l, ("cli_pid: %d\n", pd->cli_pid)); > }
The patches look good to me, just please use the debug level variable here, not hardcoded level.
-- Ondrej Kos
Thanks. I am attaching updated patches.
LS
Ack from me
Ondra
The second patch no longer applies on the current master, can you rebase it?
I am attaching rebased patches.
LS
+struct sss_auth_token *sss_authtok_new_empty(TALLOC_CTX *mem_ctx) +{
- struct sss_auth_token *token;
- token = talloc_zero(mem_ctx, struct sss_auth_token);
^^^ tabs instead of spaces.
- if (token == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));- }
- return token;
+}
The rest looks good to me.
Thanks. I'm attaching updated patches.
LS
On Fri, Mar 22, 2013 at 09:19:13AM +0100, Lukas Slebodnik wrote:
On (21/03/13 20:18), Jakub Hrozek wrote:
On Wed, Mar 20, 2013 at 07:17:28PM +0100, Lukas Slebodnik wrote:
On (20/03/13 13:58), Jakub Hrozek wrote:
On Fri, Mar 15, 2013 at 12:46:34PM +0100, Ondrej Kos wrote:
On 03/15/2013 10:34 AM, Lukas Slebodnik wrote:
On (15/03/13 09:13), Ondrej Kos wrote: >On 03/14/2013 12:58 PM, Lukas Slebodnik wrote: >>@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) >> DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); >> DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); >> DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost))); >>- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok))); >>- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok))); >>+ DEBUG(SSSDBG_CRIT_FAILURE, >>+ ("authtok type: %d\n", sss_authtok_get_type(pd->authtok))); >>+ DEBUG(SSSDBG_CRIT_FAILURE, >>+ ("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok))); >> DEBUG(l, ("priv: %d\n", pd->priv)); >> DEBUG(l, ("cli_pid: %d\n", pd->cli_pid)); >> } > >The patches look good to me, just please use the debug level variable >here, not hardcoded level. > >-- >Ondrej Kos
Thanks. I am attaching updated patches.
LS
Ack from me
Ondra
The second patch no longer applies on the current master, can you rebase it?
I am attaching rebased patches.
LS
+struct sss_auth_token *sss_authtok_new_empty(TALLOC_CTX *mem_ctx) +{
- struct sss_auth_token *token;
- token = talloc_zero(mem_ctx, struct sss_auth_token);
^^^ tabs instead of spaces.
- if (token == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));- }
- return token;
+}
The rest looks good to me.
Thanks. I'm attaching updated patches.
LS
- ret = sss_authtok_set_password(pd, &pd->authtok, password, keysize);
- ret = sss_authtok_set_password(pd, pd->authtok, password, keysize);
We agreed with Lukas off-list that it would be safer to use pd->authtok as the memory context here, because the memory blob should be owned by the authtok structure, not the pam_data structure. It's unlikely we'll ever steal or move the memory, but it's better to be safe.
On (22/03/13 14:42), Jakub Hrozek wrote:
On Fri, Mar 22, 2013 at 09:19:13AM +0100, Lukas Slebodnik wrote:
On (21/03/13 20:18), Jakub Hrozek wrote:
On Wed, Mar 20, 2013 at 07:17:28PM +0100, Lukas Slebodnik wrote:
On (20/03/13 13:58), Jakub Hrozek wrote:
On Fri, Mar 15, 2013 at 12:46:34PM +0100, Ondrej Kos wrote:
On 03/15/2013 10:34 AM, Lukas Slebodnik wrote: >On (15/03/13 09:13), Ondrej Kos wrote: >>On 03/14/2013 12:58 PM, Lukas Slebodnik wrote: >>>@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) >>> DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); >>> DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); >>> DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost))); >>>- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok))); >>>- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok))); >>>+ DEBUG(SSSDBG_CRIT_FAILURE, >>>+ ("authtok type: %d\n", sss_authtok_get_type(pd->authtok))); >>>+ DEBUG(SSSDBG_CRIT_FAILURE, >>>+ ("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok))); >>> DEBUG(l, ("priv: %d\n", pd->priv)); >>> DEBUG(l, ("cli_pid: %d\n", pd->cli_pid)); >>> } >> >>The patches look good to me, just please use the debug level variable >>here, not hardcoded level. >> >>-- >>Ondrej Kos > >Thanks. >I am attaching updated patches. > >LS > > > Ack from me
Ondra
The second patch no longer applies on the current master, can you rebase it?
I am attaching rebased patches.
LS
+struct sss_auth_token *sss_authtok_new_empty(TALLOC_CTX *mem_ctx) +{
- struct sss_auth_token *token;
- token = talloc_zero(mem_ctx, struct sss_auth_token);
^^^ tabs instead of spaces.
- if (token == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));- }
- return token;
+}
The rest looks good to me.
Thanks. I'm attaching updated patches.
LS
- ret = sss_authtok_set_password(pd, &pd->authtok, password, keysize);
- ret = sss_authtok_set_password(pd, pd->authtok, password, keysize);
We agreed with Lukas off-list that it would be safer to use pd->authtok as the memory context here, because the memory blob should be owned by the authtok structure, not the pam_data structure. It's unlikely we'll ever steal or move the memory, but it's better to be safe.
Talloc contex updated. I am attaching updated patches
LS
On Mon, Mar 25, 2013 at 08:45:38AM +0100, Lukas Slebodnik wrote:
On (22/03/13 14:42), Jakub Hrozek wrote:
On Fri, Mar 22, 2013 at 09:19:13AM +0100, Lukas Slebodnik wrote:
On (21/03/13 20:18), Jakub Hrozek wrote:
On Wed, Mar 20, 2013 at 07:17:28PM +0100, Lukas Slebodnik wrote:
On (20/03/13 13:58), Jakub Hrozek wrote:
On Fri, Mar 15, 2013 at 12:46:34PM +0100, Ondrej Kos wrote: > On 03/15/2013 10:34 AM, Lukas Slebodnik wrote: > >On (15/03/13 09:13), Ondrej Kos wrote: > >>On 03/14/2013 12:58 PM, Lukas Slebodnik wrote: > >>>@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) > >>> DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); > >>> DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); > >>> DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost))); > >>>- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok))); > >>>- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok))); > >>>+ DEBUG(SSSDBG_CRIT_FAILURE, > >>>+ ("authtok type: %d\n", sss_authtok_get_type(pd->authtok))); > >>>+ DEBUG(SSSDBG_CRIT_FAILURE, > >>>+ ("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok))); > >>> DEBUG(l, ("priv: %d\n", pd->priv)); > >>> DEBUG(l, ("cli_pid: %d\n", pd->cli_pid)); > >>> } > >> > >>The patches look good to me, just please use the debug level variable > >>here, not hardcoded level. > >> > >>-- > >>Ondrej Kos > > > >Thanks. > >I am attaching updated patches. > > > >LS > > > > > > > Ack from me > > Ondra
The second patch no longer applies on the current master, can you rebase it?
I am attaching rebased patches.
LS
+struct sss_auth_token *sss_authtok_new_empty(TALLOC_CTX *mem_ctx) +{
- struct sss_auth_token *token;
- token = talloc_zero(mem_ctx, struct sss_auth_token);
^^^ tabs instead of spaces.
- if (token == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));- }
- return token;
+}
The rest looks good to me.
Thanks. I'm attaching updated patches.
LS
- ret = sss_authtok_set_password(pd, &pd->authtok, password, keysize);
- ret = sss_authtok_set_password(pd, pd->authtok, password, keysize);
We agreed with Lukas off-list that it would be safer to use pd->authtok as the memory context here, because the memory blob should be owned by the authtok structure, not the pam_data structure. It's unlikely we'll ever steal or move the memory, but it's better to be safe.
Talloc contex updated. I am attaching updated patches
LS
Nack, I'm getting segfault on login with these patches, backtrace is pointing to sss_authtok_copy
On (25/03/13 12:44), Jakub Hrozek wrote:
On Mon, Mar 25, 2013 at 08:45:38AM +0100, Lukas Slebodnik wrote:
On (22/03/13 14:42), Jakub Hrozek wrote:
On Fri, Mar 22, 2013 at 09:19:13AM +0100, Lukas Slebodnik wrote:
On (21/03/13 20:18), Jakub Hrozek wrote:
On Wed, Mar 20, 2013 at 07:17:28PM +0100, Lukas Slebodnik wrote:
On (20/03/13 13:58), Jakub Hrozek wrote: >On Fri, Mar 15, 2013 at 12:46:34PM +0100, Ondrej Kos wrote: >> On 03/15/2013 10:34 AM, Lukas Slebodnik wrote: >> >On (15/03/13 09:13), Ondrej Kos wrote: >> >>On 03/14/2013 12:58 PM, Lukas Slebodnik wrote: >> >>>@@ -151,8 +167,10 @@ void pam_print_data(int l, struct pam_data *pd) >> >>> DEBUG(l, ("tty: %s\n", PAM_SAFE_ITEM(pd->tty))); >> >>> DEBUG(l, ("ruser: %s\n", PAM_SAFE_ITEM(pd->ruser))); >> >>> DEBUG(l, ("rhost: %s\n", PAM_SAFE_ITEM(pd->rhost))); >> >>>- DEBUG(l, ("authtok type: %d\n", sss_authtok_get_type(&pd->authtok))); >> >>>- DEBUG(l, ("newauthtok type: %d\n", sss_authtok_get_type(&pd->newauthtok))); >> >>>+ DEBUG(SSSDBG_CRIT_FAILURE, >> >>>+ ("authtok type: %d\n", sss_authtok_get_type(pd->authtok))); >> >>>+ DEBUG(SSSDBG_CRIT_FAILURE, >> >>>+ ("newauthtok type: %d\n", sss_authtok_get_type(pd->newauthtok))); >> >>> DEBUG(l, ("priv: %d\n", pd->priv)); >> >>> DEBUG(l, ("cli_pid: %d\n", pd->cli_pid)); >> >>> } >> >> >> >>The patches look good to me, just please use the debug level variable >> >>here, not hardcoded level. >> >> >> >>-- >> >>Ondrej Kos >> > >> >Thanks. >> >I am attaching updated patches. >> > >> >LS >> > >> > >> > >> Ack from me >> >> Ondra > >The second patch no longer applies on the current master, can you rebase >it?
I am attaching rebased patches.
LS
+struct sss_auth_token *sss_authtok_new_empty(TALLOC_CTX *mem_ctx) +{
- struct sss_auth_token *token;
- token = talloc_zero(mem_ctx, struct sss_auth_token);
^^^ tabs instead of spaces.
- if (token == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));- }
- return token;
+}
The rest looks good to me.
Thanks. I'm attaching updated patches.
LS
- ret = sss_authtok_set_password(pd, &pd->authtok, password, keysize);
- ret = sss_authtok_set_password(pd, pd->authtok, password, keysize);
We agreed with Lukas off-list that it would be safer to use pd->authtok as the memory context here, because the memory blob should be owned by the authtok structure, not the pam_data structure. It's unlikely we'll ever steal or move the memory, but it's better to be safe.
Talloc contex updated. I am attaching updated patches
LS
Nack, I'm getting segfault on login with these patches, backtrace is pointing to sss_authtok_copy
Problem was that "struct pam_data" was allocated on stack and zero initialized. Now this case is correctly handled in copy_pam_data.
Changes to previous verion of patches: --only in patch 0002-Making-the-authtok-structure-really-opaque.patch in file src/providers/dp_pam_data_util.c in function copy_pam_data
Patches attached.
LS
On Fri, 2013-03-29 at 11:29 +0100, Lukas Slebodnik wrote:
On (25/03/13 12:44), Jakub Hrozek wrote:
Nack, I'm getting segfault on login with these patches, backtrace is pointing to sss_authtok_copy
Problem was that "struct pam_data" was allocated on stack and zero initialized. Now this case is correctly handled in copy_pam_data.
Changes to previous verion of patches: --only in patch 0002-Making-the-authtok-structure-really-opaque.patch in file src/providers/dp_pam_data_util.c in function copy_pam_data
Patches attached.
Hey Lukas, given you now made authtok an actual talloc context in all cases, I think it would make sense to drop the mem_ctx argument from all the sss_authtok_set_*() calls. and pass only a sss_auth_tok pointer, and document it must be allocated with sss_authotk_new in authtok.h
Also I would rename sss_authtok_new_empty, to just sss_authotk_new, whether by default it is empty or not is an internal detail and the suffix '_new' is generally explicit enough.
Simo.
On (29/03/13 09:59), Simo Sorce wrote:
On Fri, 2013-03-29 at 11:29 +0100, Lukas Slebodnik wrote:
On (25/03/13 12:44), Jakub Hrozek wrote:
Nack, I'm getting segfault on login with these patches, backtrace is pointing to sss_authtok_copy
Problem was that "struct pam_data" was allocated on stack and zero initialized. Now this case is correctly handled in copy_pam_data.
Changes to previous verion of patches: --only in patch 0002-Making-the-authtok-structure-really-opaque.patch in file src/providers/dp_pam_data_util.c in function copy_pam_data
Patches attached.
Hey Lukas, given you now made authtok an actual talloc context in all cases, I think it would make sense to drop the mem_ctx argument from all the sss_authtok_set_*() calls. and pass only a sss_auth_tok pointer, and document it must be allocated with sss_authotk_new in authtok.h
TALLOC context removed from copy and setter functions.
There isn't other way how to create new authtok like using function sss_authtok_new and it has already been documented.
Also I would rename sss_authtok_new_empty, to just sss_authotk_new, whether by default it is empty or not is an internal detail and the suffix '_new' is generally explicit enough.
Renamed.
Simo.
New patches attached.
LS
On Tue, 2013-04-02 at 10:21 +0200, Lukas Slebodnik wrote:
On (29/03/13 09:59), Simo Sorce wrote:
On Fri, 2013-03-29 at 11:29 +0100, Lukas Slebodnik wrote:
On (25/03/13 12:44), Jakub Hrozek wrote:
Nack, I'm getting segfault on login with these patches, backtrace is pointing to sss_authtok_copy
Problem was that "struct pam_data" was allocated on stack and zero initialized. Now this case is correctly handled in copy_pam_data.
Changes to previous verion of patches: --only in patch 0002-Making-the-authtok-structure-really-opaque.patch in file src/providers/dp_pam_data_util.c in function copy_pam_data
Patches attached.
Hey Lukas, given you now made authtok an actual talloc context in all cases, I think it would make sense to drop the mem_ctx argument from all the sss_authtok_set_*() calls. and pass only a sss_auth_tok pointer, and document it must be allocated with sss_authotk_new in authtok.h
TALLOC context removed from copy and setter functions.
There isn't other way how to create new authtok like using function sss_authtok_new and it has already been documented.
Also I would rename sss_authtok_new_empty, to just sss_authotk_new, whether by default it is empty or not is an internal detail and the suffix '_new' is generally explicit enough.
Renamed.
Simo.
New patches attached.
Looks good to me, if it passes all tests it's an ACK.
Simo.
On Tue, Apr 02, 2013 at 08:39:22AM -0400, Simo Sorce wrote:
On Tue, 2013-04-02 at 10:21 +0200, Lukas Slebodnik wrote:
On (29/03/13 09:59), Simo Sorce wrote:
On Fri, 2013-03-29 at 11:29 +0100, Lukas Slebodnik wrote:
On (25/03/13 12:44), Jakub Hrozek wrote:
Nack, I'm getting segfault on login with these patches, backtrace is pointing to sss_authtok_copy
Problem was that "struct pam_data" was allocated on stack and zero initialized. Now this case is correctly handled in copy_pam_data.
Changes to previous verion of patches: --only in patch 0002-Making-the-authtok-structure-really-opaque.patch in file src/providers/dp_pam_data_util.c in function copy_pam_data
Patches attached.
Hey Lukas, given you now made authtok an actual talloc context in all cases, I think it would make sense to drop the mem_ctx argument from all the sss_authtok_set_*() calls. and pass only a sss_auth_tok pointer, and document it must be allocated with sss_authotk_new in authtok.h
TALLOC context removed from copy and setter functions.
There isn't other way how to create new authtok like using function sss_authtok_new and it has already been documented.
Also I would rename sss_authtok_new_empty, to just sss_authotk_new, whether by default it is empty or not is an internal detail and the suffix '_new' is generally explicit enough.
Renamed.
Simo.
New patches attached.
Looks good to me, if it passes all tests it's an ACK.
Simo.
I tested LDAP auth, LDAP binding with a DN and password, Kerberos auth and Kerberos chpass. All work fine for me.
Ack.
On Tue, Apr 02, 2013 at 04:44:07PM +0200, Jakub Hrozek wrote:
On Tue, Apr 02, 2013 at 08:39:22AM -0400, Simo Sorce wrote:
On Tue, 2013-04-02 at 10:21 +0200, Lukas Slebodnik wrote:
On (29/03/13 09:59), Simo Sorce wrote:
On Fri, 2013-03-29 at 11:29 +0100, Lukas Slebodnik wrote:
On (25/03/13 12:44), Jakub Hrozek wrote:
Nack, I'm getting segfault on login with these patches, backtrace is pointing to sss_authtok_copy
Problem was that "struct pam_data" was allocated on stack and zero initialized. Now this case is correctly handled in copy_pam_data.
Changes to previous verion of patches: --only in patch 0002-Making-the-authtok-structure-really-opaque.patch in file src/providers/dp_pam_data_util.c in function copy_pam_data
Patches attached.
Hey Lukas, given you now made authtok an actual talloc context in all cases, I think it would make sense to drop the mem_ctx argument from all the sss_authtok_set_*() calls. and pass only a sss_auth_tok pointer, and document it must be allocated with sss_authotk_new in authtok.h
TALLOC context removed from copy and setter functions.
There isn't other way how to create new authtok like using function sss_authtok_new and it has already been documented.
Also I would rename sss_authtok_new_empty, to just sss_authotk_new, whether by default it is empty or not is an internal detail and the suffix '_new' is generally explicit enough.
Renamed.
Simo.
New patches attached.
Looks good to me, if it passes all tests it's an ACK.
Simo.
I tested LDAP auth, LDAP binding with a DN and password, Kerberos auth and Kerberos chpass. All work fine for me.
Ack.
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Ondrej Kos -
Simo Sorce