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.cerr...@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). -- kind regards, David Sommerseth OpenVPN Inc _______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
