| Message ID | 97479a0ccf17d8ad7a8ba7a0f7e8190da6ddc72e.1534334933.git.plr.vincent@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iscsi target: Let initiator decide whether it wants to authenticate target | expand |
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.
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.
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; }
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(-)