iscsi target: Let initiator decide whether it wants to authenticate target

iscsi target: Let initiator decide whether it wants to authenticate target

[Vincent Pelletier]

Do not fail authentication after target is happy with initiator's
credentials, when target is configured to authenticate itself to an
initiator but current initiator did not provide required values.
Also, downgrade "Could not find CHAP_I." to a debug level message, as it
will happen normally in such case.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 drivers/target/iscsi/iscsi_target_auth.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_auth.c b/drivers/target/iscsi/iscsi_target_auth.c
index 4e680d753941..8598eb00830a 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -290,18 +290,21 @@ static int chap_server_compute_md5(
 		pr_debug("[server] MD5 Digests match, CHAP connection"
 				" successful.\n\n");
 	/*
-	 * One way authentication has succeeded, return now if mutual
-	 * authentication is not enabled.
+	 * One way authentication has succeeded
+	 */
+	auth_ret = 0;
+	*nr_out_len = 0;
+	/*
+	 * Return now if mutual authentication is not enabled.
 	 */
 	if (!auth->authenticate_target) {
-		auth_ret = 0;
 		goto out;
 	}
 	/*
 	 * Get CHAP_I.
 	 */
 	if (extract_param(nr_in_ptr, "CHAP_I", 10, identifier, &type) < 0) {
-		pr_err("Could not find CHAP_I.\n");
+		pr_debug("Could not find CHAP_I.\n");
 		goto out;
 	}
 
@@ -407,7 +410,7 @@ static int chap_server_compute_md5(
 			response);
 	*nr_out_len += 1;
 	pr_debug("[server] Sending CHAP_R=0x%s\n", response);
-	auth_ret = 0;
+	chap->chap_state = CHAP_STAGE_SERVER_NR;
 out:
 	kzfree(desc);
 	if (tfm)
@@ -462,10 +465,6 @@ u32 chap_main_loop(
 			chap_close(conn);
 			return 2;
 		}
-		if (auth->authenticate_target)
-			chap->chap_state = CHAP_STAGE_SERVER_NR;
-		else
-			*out_len = 0;
 		chap_close(conn);
 		return 1;
 	}
-- 
2.18.0

Re: iscsi target: Let initiator decide whether it wants to authenticate target

[Mike Christie]

On 08/15/2018 07:08 AM, Vincent Pelletier wrote:
> Do not fail authentication after target is happy with initiator's
> credentials, when target is configured to authenticate itself to an
> initiator but current initiator did not provide required values.
> Also, downgrade "Could not find CHAP_I." to a debug level message, as it
> will happen normally in such case.
> 

I just worry some users have set this and expect the extra layer of
checks. I can see how it is more convenient though. I think this is
something I really am not sure about because I have not worked on the
code for a long time. It is better if Nick were around.

I saw some targets/initiators allow you to configure this type of thing
as optional where in that mode it works like in your patch. What about that?

I guess you can wait for other reviewers or maybe some distro packagers
to chime in too.

Re: iscsi target: Let initiator decide whether it wants to authenticate target

[Vincent Pelletier]

On Thu, 16 Aug 2018 00:00:35 -0500, Mike Christie <mchristi@redhat.com>
wrote:
> I just worry some users have set this and expect the extra layer of
> checks.

Now that you mention it, it is very possible indeed. The original code
could help spot an initiator misconfiguration.

> I can see how it is more convenient though. I think this is
> something I really am not sure about because I have not worked on the
> code for a long time. It is better if Nick were around.

FWIW, I do not have a noteworthy use-case behind this - I merely have a
home NAS with 2 LUNs exported. It is rather from a "I just noticed this
going over the code and the RFC does not seem to require this" and a "I
guess this confused/will confuse someone trying it out" point of view.

> I saw some targets/initiators allow you to configure this type of thing
> as optional where in that mode it works like in your patch. What about that?

That would indeed be nicer than my patch. And then it can remain
enforced by default.

> I guess you can wait for other reviewers or maybe some distro packagers
> to chime in too.

I certainly can wait, yes. No worries.
-- 
Vincent Pelletier