Skip to contentSkip to conversation

Support reading the challenge-response from console

PATCH In review Selva Nair 3 participants 4 messages Dec 20, 2015 – Apr 18, 2016

lore
Share & tools
lore

Selva Nair

author
#0 lore raw

PATCH 1/1 Support reading the challenge-response from console

Trying to keep the footrpint small, this patch adds to the convoluted code-flow in get_user_pass_cr(). Cleanup left for later. -----8<-----

Currently prompting for a response to static-challenge gets skipped when the username and passowrd are read from a file. Further, dynamic challenge gets wrongly handled as if its a username/password request.

The Fix:
- Add yet another flag in get_user_pass_cr() to set when prompting of response from console is needed.
- In receive_auth_failed(), the challenge text received from server _always_ copied to the auth_challenge
buffer: this is needed to trigger prompting from console when required.
- Also show the challenge text instead of an opaque "Response:" at the prompt.

While at it, also remove the special treatment of authfile == "management" in get_user_pass_cr(). The feature implied by that test does not exist.

Tested:
- username and optionally password from file, rest from console
- the above with a static challenge
- the above with a dynamic challenge
- all of the above with systemd in place of console
- all from management with and without static/dynamic challenge.

Thanks to Wayne Davison <[email protected]...> for pointing out the issue with challenge-response, and an initial patch.

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/misc.c |   22 +++++++++++++++-------
 src/openvpn/push.c |    5 ++++-
 2 files changed, 19 insertions(+), 8 deletions(-)
src/openvpn/misc.c +15 -7
index 05ed073..363b8f7 100644
@@ -1044,6 +1044,7 @@ get_user_pass_cr (struct user_pass *up,
       bool from_authfile = (auth_file && !streq (auth_file, "stdin"));
       bool username_from_stdin = false;
       bool password_from_stdin = false;
+      bool response_from_stdin = true;
 
       if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
 	msg (M_WARN, "Note: previous '%s' credentials failed", prefix);

@@ -1053,10 +1054,11 @@ get_user_pass_cr (struct user_pass *up,
        * Get username/password from management interface?
        */
       if (management
-	  && ((auth_file && streq (auth_file, "management")) || (!from_authfile && (flags & GET_USER_PASS_MANAGEMENT)))
+          && (!from_authfile && (flags & GET_USER_PASS_MANAGEMENT))
 	  && management_query_user_pass_enabled (management))
 	{
 	  const char *sc = NULL;
+          response_from_stdin = false;
 
 	  if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
 	    management_auth_failure (management, prefix, "previous auth credentials failed");

@@ -1090,7 +1092,10 @@ get_user_pass_cr (struct user_pass *up,
 	  if (!strlen (up->password))
 	    strcpy (up->password, "ok");
 	}
-      else if (from_authfile)
+      /*
+       * Read from auth file unless this is a dynamic challenge request.
+       */
+      else if (from_authfile && !(flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
         {
           /*
            * Try to get username/password from a file.

@@ -1141,10 +1146,10 @@ get_user_pass_cr (struct user_pass *up,
       /*
        * Get username/password from standard input?
        */
-      if (username_from_stdin || password_from_stdin)
+      if (username_from_stdin || password_from_stdin || response_from_stdin)
 	{
 #ifdef ENABLE_CLIENT_CR
-	  if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
+          if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE) && response_from_stdin)
 	    {
 	      struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
 	      if (ac)

@@ -1154,7 +1159,8 @@ get_user_pass_cr (struct user_pass *up,
 
 		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
 		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
-		  if (!get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
+                  if (!get_console_input (ac->challenge_text, BOOL_CAST(ac->flags&CR_ECHO),
+                                          response, USER_PASS_LEN))
 		    msg (M_FATAL, "ERROR: could not read challenge response from stdin");
 		  strncpynt (up->username, ac->user, USER_PASS_LEN);
 		  buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response);

@@ -1185,14 +1191,16 @@ get_user_pass_cr (struct user_pass *up,
 		msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
 
 #ifdef ENABLE_CLIENT_CR
-	      if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+              if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE) && response_from_stdin)
 		{
 		  char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
 		  struct buffer packed_resp;
 		  char *pw64=NULL, *resp64=NULL;
 
 		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
-		  if (!get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response, USER_PASS_LEN))
+
+                  if (!get_console_input (auth_challenge, BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO),
+                                          response, USER_PASS_LEN))
 		    msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
 		  if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
 		      || openvpn_base64_encode(response, strlen(response), &resp64) == -1)
src/openvpn/push.c +4 -1
index d4f3cb6..ba13881 100644
@@ -76,8 +76,11 @@ receive_auth_failed (struct context *c, const struct buffer *buffer)
 	  if (buf_string_compare_advance (&buf, "AUTH_FAILED,") && BLEN (&buf))
 	    reason = BSTR (&buf);
 	  management_auth_failure (management, UP_TYPE_AUTH, reason);
-	} else
+	}
 #endif
+      /*
+       * Save the dynamic-challenge text even when management is defined
+       */
 	{
 #ifdef ENABLE_CLIENT_CR
 	  struct buffer buf = *buffer;
Signature · 1 line
1.7.10.4
#2 lore raw
In reply to Selva Nair

Hi,

Quoted 23 lines · Selva Nair

Trying to keep the footrpint small, this patch adds to the convoluted code-flow in get_user_pass_cr(). Cleanup left for later. -----8<-----

Currently prompting for a response to static-challenge gets skipped when the username and passowrd are read from a file. Further, dynamic challenge gets wrongly handled as if its a username/password request.

The Fix:
- Add yet another flag in get_user_pass_cr() to set when prompting of response from console is needed.
- In receive_auth_failed(), the challenge text received from server _always_ copied to the auth_challenge
buffer: this is needed to trigger prompting from console when required.
- Also show the challenge text instead of an opaque "Response:" at the prompt.

While at it, also remove the special treatment of authfile == "management" in get_user_pass_cr(). The feature implied by that test does not exist.

Quoted 7 lines

Tested:
- username and optionally password from file, rest from console
- the above with a static challenge
- the above with a dynamic challenge
- all of the above with systemd in place of console
- all from management with and without static/dynamic challenge.

Sorry for taking so long to review.

ACK. These fixes make sense.

-Steffan

#3 lore raw
In reply to Selva Nair

Your patch has been applied to the master and release/2.3 branch.

Thanks for your patience with us :-)

commit 9b0f1df2560441ab5ea80f053acd0161de8b6c7a (master) commit 6f31a83827627bbef58aaf073911e331369919b7 (release/2.3)

Author: Selva Nair
Date:   Sun Dec 20 14:12:53 2015 -0500
     Support reading the challenge-response from console

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10868
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering