Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment, upgraded

[Antonio Quartulli]

Hi Paolo,

On 20/06/2022 14:21, Paolo Cerrito wrote:

From: paolo 


Is this a new version of your previous patch having subject "Insert 
client connection data into PAM environment"?


If yes, you should send it as a v2 (i.e. with subject starting with 
"[PATCH v2]") instead of appending ", upgraded".


You can easily create a v2 patch by adding the argument "-v2" when 
running "git format-patch".


On top of that, when sending a v2 it'd be nice if you could specify what 
has changed compared to the original version. Something like:


"Changes from v1:
* this and that
* here and there"

Also, when signing a patch you should put your name and surname, as if 
you are signing a public document.


Last, but not least, please add some text to your commit message, 
explaining why you need to implement this change, what it does (high 
level) and how you are going to do it.



Best Regards,



---
  src/plugins/auth-pam/auth-pam.c | 25 +
  1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index 70339445..c2e66e5c 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -49,7 +49,7 @@
  #include 
  #include 
  #include "utils.h"
-
+#include 
  #include 
  
  #define DEBUG(verb) ((verb) >= 4)

@@ -121,6 +121,7 @@ struct user_pass {
  char password[128];
  char common_name[128];
  char response[128];
+char remote[INET6_ADDRSTRLEN];
  
  const struct name_value_list *name_value_list;

  };
@@ -529,6 +530,11 @@ 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_ip6", envp);
+
+if (remote == NULL){
+remote = get_env("untrusted_ip", envp); //if Null, try to take 
ipv4 if not set ipv6
+}
  
  /* should we do deferred auth?

   *  yes, if there is "auth_control_file" and "deferred_auth_pam" env
@@ -554,7 +560,8 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, 
const int type, const cha
  || 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, auth_control_file) == 
-1)
+|| send_string(context->foreground_fd, auth_control_file) == -1
+|| send_string(context->foreground_fd, remote) == -1)
  {
  plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error sending auth info 
to background process");
  }
@@ -789,8 +796,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);
@@ -956,7 +971,8 @@ pam_server(int fd, const char *service, int verb, const 
struct name_value_list *
  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, ac_file_name, sizeof(ac_file_name)) == 
-1)
+|| recv_string(fd, ac_file_name, sizeof(ac_file_name)) == 
-1
+|| recv_string(fd, up.remote, sizeof(up.remote)) == -1)
  {
  plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: read 
error on command channel: code=%d, exiting",
 command);
@@ -970,6 +986,7 @@ pam_server(int fd, const char *service, int verb, const 
struct name_value_list *
 up.username, up.password);
  #else
  plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: USER: %s", 
up.username);
+plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: REMOTE: %s", 
up.remote);
  #endif
  }
  


--
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[David Sommerseth]

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:


And as an example of a reasonably good commit message in OpenVPN is for
example this one:


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 
> 
> ---
>  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(u

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[Selva Nair]

Hi,

On Tue, Oct 1, 2019 at 1:02 PM Antonio Quartulli  wrote:

> Hi Paolo,
>
> On 01/10/2019 14:06, Paolo Cerrito wrote:
> > From: paolo 
>
> 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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[Selva Nair]

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  wrote:

> From: paolo 
>
> ---
>  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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[Antonio Quartulli]

Hi Paolo,

On 01/10/2019 14:06, Paolo Cerrito wrote:
> From: paolo 

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,


-- 
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[Илья Шипицин]

Do not pay attention to osx. I will fix it soon

On Fri, Jun 28, 2019, 4:29 PM Paolo  wrote:

> Hi,
>
> after rebasing my fork on current master, the are no conflicts with
> current source code. Travis error on osx are not releated to my code,
> they are errors about configuration peace not working on osx.
>
> --
> -***-
> Paolo Cerrito
> -***-
>
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[Paolo]

Hi,

after rebasing my fork on current master, the are no conflicts with
current source code. Travis error on osx are not releated to my code,
they are errors about configuration peace not working on osx.

-- 
-***-
Paolo Cerrito
-***-



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[Paolo]

Hi,

another example (and the simplest case) is:

support to set remote client data into PAM environment, in turn
correctly allow PAM logging the client address to syslog

Paolo Cerrito

Il 27/06/19 11:07, Antonio Quartulli ha scritto:
> Hi,
>
> On 27/06/2019 10:26, Paolo Cerrito wrote:
>> From: paolo 
>>
>> Signed-off-by: Paolo Cerrito 
> 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" ;-) )
>
> Regards,
>
>> ---
>>  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);
>>  
>>  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
>>  }
>>  
>>

-- 
-***-
Paolo Cerrito
-***-



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[Paolo]

Hi,

this change is needed to pass remote ip address to pam environment. I
try to explain by an example, so this is the case for us.

I would use pam_recent module to make dynamic ip firewalling. User can
try to login some times, after for example 3 times, pam_recent could
block ip using iptables dynamic rules for about an hour. But pam module
need not only username for logging, but remote ip for firewalling.

This is just an example about the use. But other modules can make other
things using remote ip.




Il 27/06/19 11:07, Antonio Quartulli ha scritto:
> Hi,
>
> On 27/06/2019 10:26, Paolo Cerrito wrote:
>> From: paolo 
>>
>> Signed-off-by: Paolo Cerrito 
> 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" ;-) )
>
> Regards,
>
>> ---
>>  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);
>>  
>>  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
>>  }
>>  
>>

-- 
-***-
Paolo Cerrito
-***-




___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment

[Antonio Quartulli]

Hi,

On 27/06/2019 10:26, Paolo Cerrito wrote:
> From: paolo 
> 
> Signed-off-by: Paolo Cerrito 

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" ;-) )

Regards,

> ---
>  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);
>  
>  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
>  }
>  
> 

-- 
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel