| Message ID | 20191001120652.31898-1-wardragon78@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] Insert client connection data into PAM environment | expand |
Hi Paolo, On 01/10/2019 14:06, Paolo Cerrito wrote: > From: paolo <paolo.cerrito@uniroma2.it> On June 27th another patch with the same subject was sent by you to this mailing list. Is this new patch any different? If so, it should bear a "v2" in the subject and the differences should be explicitly mentioned to ease the review. On top of that I commented your original patch with the following: > Why do we need this change? > What benefit does it give us? > How can it be used? > IMHO it would be nice to add these pieces of information to the commit > message (right now it feels .. "empty" ) But it seems that none of this information has been added to the commit message (and it is still empty). Could you please take care of that, so to make the review easier for who is not deep into those lines of code you have changed? Thanks a lot. Regards,
Hi, Its useful to set PAM_RHOSTS which will allow use of pam_access for access control etc. So feature ACK. I would like to see a more precise commit message header like: "Insert remote IP address into PAM environment" On Tue, Oct 1, 2019 at 8:25 AM Paolo Cerrito <wardragon78@gmail.com> wrote: > From: paolo <paolo.cerrito@uniroma2.it> > > --- > src/plugins/auth-pam/auth-pam.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/src/plugins/auth-pam/auth-pam.c > b/src/plugins/auth-pam/auth-pam.c > index 88b53204..9d8dfb95 100644 > --- a/src/plugins/auth-pam/auth-pam.c > +++ b/src/plugins/auth-pam/auth-pam.c > @@ -115,6 +115,7 @@ struct user_pass { > char password[128]; > char common_name[128]; > char response[128]; > + char remote[128]; > > const struct name_value_list *name_value_list; > }; > @@ -517,13 +518,15 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t > handle, const int type, const cha > const char *username = get_env("username", envp); > const char *password = get_env("password", envp); > const char *common_name = get_env("common_name", envp) ? > get_env("common_name", envp) : ""; > + const char *remote = get_env("untrusted_ip", envp) ? > get_env("untrusted_ip", envp) : get_env("untrusted_ip6", envp); > Ensure that remote can't be NULL, like: if (!remote) { remote = ""; } Though get_env() is unlikely to return NULL in this case, safer not to rely on that and risk a NULL dereferencing later. > if (username && strlen(username) > 0 && password) > { > if (send_control(context->foreground_fd, COMMAND_VERIFY) == -1 > || send_string(context->foreground_fd, username) == -1 > || send_string(context->foreground_fd, password) == -1 > - || send_string(context->foreground_fd, common_name) == -1) > + || send_string(context->foreground_fd, common_name) == -1 > + || send_string(context->foreground_fd, remote) == -1) > { > fprintf(stderr, "AUTH-PAM: Error sending auth info to > background process\n"); > } > @@ -750,8 +753,16 @@ pam_auth(const char *service, const struct user_pass > *up) > status = pam_start(service, name_value_list_provided ? NULL : > up->username, &conv, &pamh); > if (status == PAM_SUCCESS) > { > + /* Set PAM_RHOST environment variable */ > + if (*(up->remote)) > + { > + status = pam_set_item(pamh, PAM_RHOST, up->remote); > + } > /* Call PAM to verify username/password */ > - status = pam_authenticate(pamh, 0); > + if (status == PAM_SUCCESS) > + { > + status = pam_authenticate(pamh, 0); > + } > if (status == PAM_SUCCESS) > { > status = pam_acct_mgmt(pamh, 0); > @@ -839,7 +850,8 @@ pam_server(int fd, const char *service, int verb, > const struct name_value_list * > case COMMAND_VERIFY: > if (recv_string(fd, up.username, sizeof(up.username)) == > -1 > || recv_string(fd, up.password, sizeof(up.password)) > == -1 > - || recv_string(fd, up.common_name, > sizeof(up.common_name)) == -1) > + || recv_string(fd, up.common_name, > sizeof(up.common_name)) == -1 > + || recv_string(fd, up.remote, sizeof(up.remote)) == > -1) > { > fprintf(stderr, "AUTH-PAM: BACKGROUND: read error on > command channel: code=%d, exiting\n", > command); > @@ -853,6 +865,7 @@ pam_server(int fd, const char *service, int verb, > const struct name_value_list * > up.username, up.password); > #else > fprintf(stderr, "AUTH-PAM: BACKGROUND: USER: %s\n", > up.username); > + fprintf(stderr, "AUTH-PAM: BACKGROUND: REMOTE: %s\n", > up.remote); > Just for the record: we have to replace all these xprintf()'s by plugin_log/plugin_vlog callbacks but that's beyond the scope here. #endif > } > Looks good otherwise. Selva <div dir="ltr"><div dir="ltr"><div>Hi,</div><div><br></div><div>Its useful to set PAM_<span id="m_-8358678940876167294m_712506079045984063:8dn.1">RHOSTS</span> which will allow use of</div><div><span id="m_-8358678940876167294m_712506079045984063:8dn.2">pam</span>_access for access control etc. So feature <span id="m_-8358678940876167294m_712506079045984063:8dn.3">ACK</span>.<br></div></div><div><br></div><div>I would like to see a more precise commit message header like:</div><div>"Insert remote <span id="m_-8358678940876167294m_712506079045984063:8dn.4">IP</span> address into PAM environment"<br></div><div><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 1, 2019 at 8:25 AM Paolo <span id="m_-8358678940876167294m_712506079045984063:8dn.5">Cerrito</span> <<a href="mailto:wardragon78@gmail.com" target="_blank">wardragon78@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: paolo <<a href="mailto:paolo.cerrito@uniroma2.it" target="_blank">paolo.cerrito@uniroma2.it</a>><br> <br> ---<br> src/plugins/auth-pam/auth-pam.c | 19 ++++++++++++++++---<br> 1 file changed, 16 insertions(+), 3 deletions(-)<br> <br> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c<br> index 88b53204..9d8dfb95 100644<br> --- a/src/plugins/auth-pam/auth-pam.c<br> +++ b/src/plugins/auth-pam/auth-pam.c<br> @@ -115,6 +115,7 @@ struct user_pass {<br> char password[128];<br> char common_name[128];<br> char response[128];<br> + char remote[128];<br> <br> const struct name_value_list *name_value_list;<br> };<br> @@ -517,13 +518,15 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha<br> const char *username = get_env("username", envp);<br> const char *password = get_env("password", envp);<br> const char *common_name = get_env("common_name", envp) ? get_env("common_name", envp) : "";<br> + const char *remote = get_env("untrusted_ip", envp) ? get_env("untrusted_ip", envp) : get_env("untrusted_ip6", envp);<br></blockquote><br></div><div class="gmail_quote">Ensure that remote can't be NULL, like:<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">if (!remote) <br></div><div class="gmail_quote">{</div><div class="gmail_quote"> remote = "";</div><div class="gmail_quote">}</div><div class="gmail_quote"><br></div><div class="gmail_quote">Though get_<span id="m_-8358678940876167294m_712506079045984063:8dn.6">env</span>() is unlikely to return NULL in this case,</div>safer not to rely on that and risk a NULL <span id="m_-8358678940876167294m_712506079045984063:8dn.7">dereferencing</span> later.<div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> if (username && strlen(username) > 0 && password)<br> {<br> if (send_control(context->foreground_fd, COMMAND_VERIFY) == -1<br> || send_string(context->foreground_fd, username) == -1<br> || send_string(context->foreground_fd, password) == -1<br> - || send_string(context->foreground_fd, common_name) == -1)<br> + || send_string(context->foreground_fd, common_name) == -1<br> + || send_string(context->foreground_fd, remote) == -1)<br> {<br> fprintf(stderr, "AUTH-PAM: Error sending auth info to background process\n");<br> }<br> @@ -750,8 +753,16 @@ pam_auth(const char *service, const struct user_pass *up)<br> status = pam_start(service, name_value_list_provided ? NULL : up->username, &conv, &pamh);<br> if (status == PAM_SUCCESS)<br> {<br> + /* Set PAM_RHOST environment variable */<br> + if (*(up->remote))<br> + {<br> + status = pam_set_item(pamh, PAM_RHOST, up->remote);<br> + }<br> /* Call PAM to verify username/password */<br> - status = pam_authenticate(pamh, 0);<br> + if (status == PAM_SUCCESS)<br> + {<br> + status = pam_authenticate(pamh, 0);<br> + }<br> if (status == PAM_SUCCESS)<br> {<br> status = pam_acct_mgmt(pamh, 0);<br> @@ -839,7 +850,8 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *<br> case COMMAND_VERIFY:<br> if (recv_string(fd, up.username, sizeof(up.username)) == -1<br> || recv_string(fd, up.password, sizeof(up.password)) == -1<br> - || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1)<br> + || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1<br> + || recv_string(fd, up.remote, sizeof(up.remote)) == -1)<br> {<br> fprintf(stderr, "AUTH-PAM: BACKGROUND: read error on command channel: code=%d, exiting\n",<br> command);<br> @@ -853,6 +865,7 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *<br> up.username, up.password);<br> #else<br> fprintf(stderr, "AUTH-PAM: BACKGROUND: USER: %s\n", up.username);<br> + fprintf(stderr, "AUTH-PAM: BACKGROUND: REMOTE: %s\n", up.remote);<br></blockquote><div><br></div><div>Just for the record: we have to replace all these <span id="m_-8358678940876167294m_712506079045984063:8dn.8">xprintf</span>()'s by<br></div><div><span id="m_-8358678940876167294m_712506079045984063:8dn.9">plugin</span>_log/<span id="m_-8358678940876167294m_712506079045984063:8dn.10">plugin</span>_<span id="m_-8358678940876167294m_712506079045984063:8dn.11">vlog</span> callbacks but that's beyond the scope here.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> #endif<br> }<br></blockquote><div><br></div><div>Looks good otherwise.</div><div><br></div><div><span id="m_-8358678940876167294m_712506079045984063:8dn.12">Selva</span></div></div></div>
Hi, On Tue, Oct 1, 2019 at 1:02 PM Antonio Quartulli <a@unstable.cc> wrote: > Hi Paolo, > > On 01/10/2019 14:06, Paolo Cerrito wrote: > > From: paolo <paolo.cerrito@uniroma2.it> > > On June 27th another patch with the same subject was sent by you to this > mailing list. Is this new patch any different? > > If so, it should bear a "v2" in the subject and the differences should > be explicitly mentioned to ease the review. > > On top of that I commented your original patch with the following: > > > Why do we need this change? > > What benefit does it give us? > > How can it be used? > > > IMHO it would be nice to add these pieces of information to the commit > > message (right now it feels .. "empty" ) > > But it seems that none of this information has been added to the commit > message (and it is still empty). > Could you please take care of that, so to make the review easier for who > is not deep into those lines of code you have changed? > Aha, I missed the previous thread. Looks like this one is the same patch as the previous one. Paolo: please improve on the commit message, address the comments, and submit a v2 with the message-id of this one in --in-reply-to. Selva <div dir="ltr"><div dir="ltr"><br></div><div>Hi,</div><div><br></div><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 1, 2019 at 1:02 PM Antonio Quartulli <a@unstable.cc> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Paolo,<br> <br> On 01/10/2019 14:06, Paolo Cerrito wrote:<br> > From: paolo <<a href="mailto:paolo.cerrito@uniroma2.it" target="_blank">paolo.cerrito@uniroma2.it</a>><br> <br> On June 27th another patch with the same subject was sent by you to this<br> mailing list. Is this new patch any different?<br> <br> If so, it should bear a "v2" in the subject and the differences should<br> be explicitly mentioned to ease the review.<br> <br> On top of that I commented your original patch with the following:<br> <br> > Why do we need this change?<br> > What benefit does it give us?<br> > How can it be used?<br> <br> > IMHO it would be nice to add these pieces of information to the commit<br> > message (right now it feels .. "empty" )<br> <br> But it seems that none of this information has been added to the commit<br> message (and it is still empty).<br> Could you please take care of that, so to make the review easier for who<br> is not deep into those lines of code you have changed?<br></blockquote><div><br></div><div>Aha, I missed the previous thread. Looks like this one is the same</div><div>patch as the previous one.</div><div><br></div><div>Paolo: please improve on the commit message, address the comments,<br></div><div>and submit a v2 with the message-id of this one in --in-reply-to.</div><div><br></div><div>Selva<br></div></div></div></div>
Hi Paolo, Thank you very much for this patch. Selva has already given this a feature-ack, which I agree to. So lets continue here. First of all, it is really important that the commit message is good on patches. Please read this really good blog post on this topic: <https://chris.beams.io/posts/git-commit/> And as an example of a reasonably good commit message in OpenVPN is for example this one: <https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17870.html> The keypoints in a commit message are: - Subject line A straight to the point what this change is about - Commit message: Explain *why* this change is needed, *what* does change this give us. Don't describe how, as that need to be clear in the commit diff below the commit message. And having the "Signed-off-by: " line in the commit message is also really important. Secondly, when you next time submit a patch, please ensure it has a proper version identifier if you update a patch you have already sent. And now ... the review of your patch, please follow in between the diff lines below. On 01/10/2019 14:06, Paolo Cerrito wrote: > From: paolo <paolo.cerrito@uniroma2.it> > > --- > src/plugins/auth-pam/auth-pam.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c > index 88b53204..9d8dfb95 100644 > --- a/src/plugins/auth-pam/auth-pam.c > +++ b/src/plugins/auth-pam/auth-pam.c > @@ -115,6 +115,7 @@ struct user_pass { > char password[128]; > char common_name[128]; > char response[128]; > + char remote[128]; Why 128 bytes? Unless I'm mistaken, an IPv6 address can be up to 39 characters and an IPv4 will never be longer than 15 characters. So having 40 bytes should be reasonable enough, as that give space for a \0 terminator too. > const struct name_value_list *name_value_list; > }; > @@ -517,13 +518,15 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha > const char *username = get_env("username", envp); > const char *password = get_env("password", envp); > const char *common_name = get_env("common_name", envp) ? get_env("common_name", envp) : ""; > + const char *remote = get_env("untrusted_ip", envp) ? get_env("untrusted_ip", envp) : get_env("untrusted_ip6", envp); I know we don't do this in the get_env() lines below, but we should really try to restrict the number of bytes we extract from the env table. This isn't functionally important in this code here, but more a generic and common way. IPv6 is generally preferred over IPv4 by the OS, so you should check if untrusted_ip6 is present and then only extract the IPv4 address if there is no IPv6 address. > if (username && strlen(username) > 0 && password) > { > if (send_control(context->foreground_fd, COMMAND_VERIFY) == -1 > || send_string(context->foreground_fd, username) == -1 > || send_string(context->foreground_fd, password) == -1 > - || send_string(context->foreground_fd, common_name) == -1) > + || send_string(context->foreground_fd, common_name) == -1 > + || send_string(context->foreground_fd, remote) == -1) > { > fprintf(stderr, "AUTH-PAM: Error sending auth info to background process\n"); > } > @@ -750,8 +753,16 @@ pam_auth(const char *service, const struct user_pass *up) > status = pam_start(service, name_value_list_provided ? NULL : up->username, &conv, &pamh); > if (status == PAM_SUCCESS) > { > + /* Set PAM_RHOST environment variable */ > + if (*(up->remote)) > + { > + status = pam_set_item(pamh, PAM_RHOST, up->remote); > + } > /* Call PAM to verify username/password */ > - status = pam_authenticate(pamh, 0); > + if (status == PAM_SUCCESS) > + { > + status = pam_authenticate(pamh, 0); > + } I understand the importance of this call. But can this break anything for existing configurations already using auth-pam? I think this check should be enabled by an option given to the auth-pam module in the 'plugin' OpenVPN statement. You will see that argument in the char **argv array pointer found in struct openvpn_plugin_args_open_in in the openvpn_plugin_open_v3() function. I suggest an option as simple as "call-pam-auth" or something like that. > if (status == PAM_SUCCESS) > { > status = pam_acct_mgmt(pamh, 0); > @@ -839,7 +850,8 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list * > case COMMAND_VERIFY: > if (recv_string(fd, up.username, sizeof(up.username)) == -1 > || recv_string(fd, up.password, sizeof(up.password)) == -1 > - || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1) > + || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1 > + || recv_string(fd, up.remote, sizeof(up.remote)) == -1) > { > fprintf(stderr, "AUTH-PAM: BACKGROUND: read error on command channel: code=%d, exiting\n", > command); > @@ -853,6 +865,7 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list * > up.username, up.password); > #else > fprintf(stderr, "AUTH-PAM: BACKGROUND: USER: %s\n", up.username); > + fprintf(stderr, "AUTH-PAM: BACKGROUND: REMOTE: %s\n", up.remote); Two things. I think it would be good to have the up.remote in the same line as up.username. But you it should probably be empty ("") if up.remote is NULL. And I suggest using the same format as found in other parts of the OpenVPN logging ... "USERNAME/IP-ADDRESS". If IP address is not available, use just "USERNAME". If you have any questions or comments, feel free to reach out. And also feel free to join the #openvpn-devel IRC channel on FreeNode; there are several of us community and corporate developers there so it is a chance to get quicker replies there (most of us are in the EU time zones).
diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c index 88b53204..9d8dfb95 100644 --- a/src/plugins/auth-pam/auth-pam.c +++ b/src/plugins/auth-pam/auth-pam.c @@ -115,6 +115,7 @@ struct user_pass { char password[128]; char common_name[128]; char response[128]; + char remote[128]; const struct name_value_list *name_value_list; }; @@ -517,13 +518,15 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha const char *username = get_env("username", envp); const char *password = get_env("password", envp); const char *common_name = get_env("common_name", envp) ? get_env("common_name", envp) : ""; + const char *remote = get_env("untrusted_ip", envp) ? get_env("untrusted_ip", envp) : get_env("untrusted_ip6", envp); if (username && strlen(username) > 0 && password) { if (send_control(context->foreground_fd, COMMAND_VERIFY) == -1 || send_string(context->foreground_fd, username) == -1 || send_string(context->foreground_fd, password) == -1 - || send_string(context->foreground_fd, common_name) == -1) + || send_string(context->foreground_fd, common_name) == -1 + || send_string(context->foreground_fd, remote) == -1) { fprintf(stderr, "AUTH-PAM: Error sending auth info to background process\n"); } @@ -750,8 +753,16 @@ pam_auth(const char *service, const struct user_pass *up) status = pam_start(service, name_value_list_provided ? NULL : up->username, &conv, &pamh); if (status == PAM_SUCCESS) { + /* Set PAM_RHOST environment variable */ + if (*(up->remote)) + { + status = pam_set_item(pamh, PAM_RHOST, up->remote); + } /* Call PAM to verify username/password */ - status = pam_authenticate(pamh, 0); + if (status == PAM_SUCCESS) + { + status = pam_authenticate(pamh, 0); + } if (status == PAM_SUCCESS) { status = pam_acct_mgmt(pamh, 0); @@ -839,7 +850,8 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list * case COMMAND_VERIFY: if (recv_string(fd, up.username, sizeof(up.username)) == -1 || recv_string(fd, up.password, sizeof(up.password)) == -1 - || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1) + || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1 + || recv_string(fd, up.remote, sizeof(up.remote)) == -1) { fprintf(stderr, "AUTH-PAM: BACKGROUND: read error on command channel: code=%d, exiting\n", command); @@ -853,6 +865,7 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list * up.username, up.password); #else fprintf(stderr, "AUTH-PAM: BACKGROUND: USER: %s\n", up.username); + fprintf(stderr, "AUTH-PAM: BACKGROUND: REMOTE: %s\n", up.remote); #endif }
From: paolo <paolo.cerrito@uniroma2.it> --- src/plugins/auth-pam/auth-pam.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)