| Message ID | 20220917140818.6155-1-a@unstable.cc |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Openvpn-devel] Add common_name to the conv method. This allows the common_name to be accessible in PAM. | expand |
Hi, On Sat, Sep 17, 2022 at 04:08:18PM +0200, Antonio Quartulli wrote: > From: Michael Karvan <Michael.Karvan@gmail.com> > > Signed-off-by: Michael Karvan <Michael.Karvan@gmail.com> > --- > src/plugins/auth-pam/auth-pam.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c > index 70339445..9f37c8c0 100644 > --- a/src/plugins/auth-pam/auth-pam.c > +++ b/src/plugins/auth-pam/auth-pam.c > @@ -746,8 +746,15 @@ my_conv(int n, const struct pam_message **msg_array, > } > break; > > - case PAM_ERROR_MSG: > case PAM_TEXT_INFO: > + aresp[i].resp = strdup(up->common_name); > + if (aresp[i].resp == NULL) > + { > + ret = PAM_CONV_ERR; > + } > + break; > + Not sure I understand the intricaticies of PAM enough, but this seems hackish and not really correct to me. Linux' "man pam_conv" suggests that PAM_TEXT_INFO is to "Display some text", but just having the common_name there only makes sense if you have a PAM module that actually knows what to do with it, and displays this in a nice way. No? Can you elaborate more on this patch, how it is interacting with specific PAM modules, and what is happening where? gert
On Sat, Sep 17, 2022 at 10:09 AM Antonio Quartulli <a@unstable.cc> wrote: > From: Michael Karvan <Michael.Karvan@gmail.com> > > Signed-off-by: Michael Karvan <Michael.Karvan@gmail.com> > --- > src/plugins/auth-pam/auth-pam.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/plugins/auth-pam/auth-pam.c > b/src/plugins/auth-pam/auth-pam.c > index 70339445..9f37c8c0 100644 > --- a/src/plugins/auth-pam/auth-pam.c > +++ b/src/plugins/auth-pam/auth-pam.c > @@ -746,8 +746,15 @@ my_conv(int n, const struct pam_message **msg_array, > } > break; > > - case PAM_ERROR_MSG: > case PAM_TEXT_INFO: > + aresp[i].resp = strdup(up->common_name); > + if (aresp[i].resp == NULL) > + { > + ret = PAM_CONV_ERR; > + } > + break; > + > + case PAM_ERROR_MSG: > break; > > To add to what Gert's comment, we already support COMMONNAME in addition to USERNAME, PASSWORD and OTP via text replacements for pam prompts specified in the config file. Like: plugin openvpn-auth-pam.so "openvpn user USERNAME password PASSWORD cn COMMONNAME" and have the PAM module prompt for "user', "password" and "cn", for example. So why do we need this non-standard stuff ? In fact, IMO, we should be getting rid of this whole "else {}" clause starting line 728 that tries to guess the prompts based on echo-off ( to mean password) echo-on (to mean username) etc. Instead, require that the plugin line in the config file must specify expected prompts and replacement strings as above. Selva
Hi, On 18/09/2022 20:30, Selva Nair wrote: > > > On Sat, Sep 17, 2022 at 10:09 AM Antonio Quartulli <a@unstable.cc > <mailto:a@unstable.cc>> wrote: > > From: Michael Karvan <Michael.Karvan@gmail.com > <mailto:Michael.Karvan@gmail.com>> > > Signed-off-by: Michael Karvan <Michael.Karvan@gmail.com > <mailto:Michael.Karvan@gmail.com>> > --- > src/plugins/auth-pam/auth-pam.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/plugins/auth-pam/auth-pam.c > b/src/plugins/auth-pam/auth-pam.c > index 70339445..9f37c8c0 100644 > --- a/src/plugins/auth-pam/auth-pam.c > +++ b/src/plugins/auth-pam/auth-pam.c > @@ -746,8 +746,15 @@ my_conv(int n, const struct pam_message > **msg_array, > } > break; > > - case PAM_ERROR_MSG: > case PAM_TEXT_INFO: > + aresp[i].resp = strdup(up->common_name); > + if (aresp[i].resp == NULL) > + { > + ret = PAM_CONV_ERR; > + } > + break; > + > + case PAM_ERROR_MSG: > break; > > > To add to what Gert's comment, we already support COMMONNAME in addition > to USERNAME, PASSWORD and > OTP via text replacements for pam prompts specified in the config file. > Like: > > plugin openvpn-auth-pam.so "openvpn user USERNAME password PASSWORD cn > COMMONNAME" > > and have the PAM module prompt for "user', "password" and "cn", for example. > > So why do we need this non-standard stuff ? Right. This means that this patch is *obsolete*, as we already have a way to convey the common name. > > In fact, IMO, we should be getting rid of this whole "else {}" clause > starting line 728 that tries to guess the prompts based on echo-off ( to > mean password) echo-on (to mean username) etc. Instead, require that the > plugin line in the config file must specify expected prompts and > replacement strings as above. Agreed. If we have a "clean" way to gather those details, we should get rid of the hack. Cheers,
diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c index 70339445..9f37c8c0 100644 --- a/src/plugins/auth-pam/auth-pam.c +++ b/src/plugins/auth-pam/auth-pam.c @@ -746,8 +746,15 @@ my_conv(int n, const struct pam_message **msg_array, } break; - case PAM_ERROR_MSG: case PAM_TEXT_INFO: + aresp[i].resp = strdup(up->common_name); + if (aresp[i].resp == NULL) + { + ret = PAM_CONV_ERR; + } + break; + + case PAM_ERROR_MSG: break; default: