diff mbox series

[2/3] scsi: target: iscsi: extract auth functions

Message ID 20210914100314.492-3-d.bogdanov@yadro.com (mailing list archive)
State Changes Requested
Headers show
Series target: iscsi: control authentication per ACL | expand

Commit Message

Dmitry Bogdanov Sept. 14, 2021, 10:03 a.m. UTC 
Create functions that answers simple questions:
whether authentication is required, what credentials, whether
connection is autenticated.

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/iscsi/iscsi_target_nego.c | 140 +++++++++++++++--------
 1 file changed, 92 insertions(+), 48 deletions(-)

Comments

Mike Christie Sept. 30, 2021, 6:46 p.m. UTC | #1 
On 9/14/21 5:03 AM, Dmitry Bogdanov wrote:
> +bool iscsi_conn_auth_required(struct iscsi_conn *conn)
> +{
> +	struct se_node_acl *se_nacl;
> +
> +	if (conn->sess->sess_ops->SessionType) {
> +		/*
> +		 * For SessionType=Discovery
> +		 */
> +		return conn->tpg->tpg_attrib.authentication;
> +	}
> +	/*
> +	 * For SessionType=Normal
> +	 */
> +	se_nacl = conn->sess->se_sess->se_node_acl;
> +	if (!se_nacl) {
> +		pr_debug("Unknown ACL %s is trying to connect\n",
> +			 se_nacl->initiatorname);
> +		return true;

Before the patch, if we didn't have an ACL we would go by
conn->tpg->tpg_attrib.authentication. But with the patch, if
we don't have an ACL, then it looks like we always require authentication
which I don't think is right.

Is the code above supposed to return the value of 
conn->tpg->tpg_attrib.authentication?


> +	}
> +
> +	if (se_nacl->dynamic_node_acl) {
> +		pr_debug("Dynamic ACL %s is trying to connect\n",
> +			 se_nacl->initiatorname);
> +		return conn->tpg->tpg_attrib.authentication;
> +	}
> +
> +	pr_debug("Known ACL %s is trying to connect\n",
> +		 se_nacl->initiatorname);
> +	return conn->tpg->tpg_attrib.authentication;
> +}
> +
>  static int iscsi_target_handle_csg_zero(
>  	struct iscsi_conn *conn,
>  	struct iscsi_login *login)
> @@ -874,22 +903,26 @@ static int iscsi_target_handle_csg_zero(
>  		return -1;
>  
>  	if (!iscsi_check_negotiated_keys(conn->param_list)) {
> -		if (conn->tpg->tpg_attrib.authentication &&
> -		    !strncmp(param->value, NONE, 4)) {
> -			pr_err("Initiator sent AuthMethod=None but"
> -				" Target is enforcing iSCSI Authentication,"
> -					" login failed.\n");
> -			iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
> -					ISCSI_LOGIN_STATUS_AUTH_FAILED);
> -			return -1;
> -		}
> +		bool auth_required = iscsi_conn_auth_required(conn);
> +

In __iscsi_target_login_thread we have:

        if (conn->sess)
                conn->sess->se_sess->sup_prot_ops =
                        conn->conn_transport->iscsit_get_sup_prot_ops(conn);

before we call:

iscsi_target_start_negotiation -> iscsi_target_do_login- > iscsi_target_handle_csg_zero
and hit the code above.

If conn->sess can be NULL then iscsi_conn_auth_required will crash.

However, I can't tell how conn->sess can be NULL in that code path. Is the conn->sess
check in __iscsi_target_login_thread not needed?
Dmitry Bogdanov Oct. 4, 2021, 7:41 a.m. UTC | #2 
Hi Mike,

> > +{
> > +	struct se_node_acl *se_nacl;
> > +
> > +	if (conn->sess->sess_ops->SessionType) {
> > +		/*
> > +		 * For SessionType=Discovery
> > +		 */
> > +		return conn->tpg->tpg_attrib.authentication;
> > +	}
> > +	/*
> > +	 * For SessionType=Normal
> > +	 */
> > +	se_nacl = conn->sess->se_sess->se_node_acl;
> > +	if (!se_nacl) {
> > +		pr_debug("Unknown ACL %s is trying to connect\n",
> > +			 se_nacl->initiatorname);
> > +		return true;
>
> Before the patch, if we didn't have an ACL we would go by
> conn->tpg->tpg_attrib.authentication. But with the patch, if
> we don't have an ACL, then it looks like we always require authentication
> which I don't think is right.
>
> Is the code above supposed to return the value of 
> conn->tpg->tpg_attrib.authentication?

No, no. This piece of code is the same as it was.
An absence of ACL is some erroneous situation because the login must be
rejected earlier in __iscsi_target_login_thread -> iscsi_target_locate_portal

> > +	}
> > +
> > +	if (se_nacl->dynamic_node_acl) {
> > +		pr_debug("Dynamic ACL %s is trying to connect\n",
> > +			 se_nacl->initiatorname);
> > +		return conn->tpg->tpg_attrib.authentication;
> > +	}
> > +
> > +	pr_debug("Known ACL %s is trying to connect\n",
> > +		 se_nacl->initiatorname);
> > +	return conn->tpg->tpg_attrib.authentication;
> > +}
> > +
> >  static int iscsi_target_handle_csg_zero(
> >  	struct iscsi_conn *conn,
> >  	struct iscsi_login *login)
> > @@ -874,22 +903,26 @@ static int iscsi_target_handle_csg_zero(
> >  		return -1;
> >  
> >  	if (!iscsi_check_negotiated_keys(conn->param_list)) {
> > -		if (conn->tpg->tpg_attrib.authentication &&
> > -		    !strncmp(param->value, NONE, 4)) {
> > -			pr_err("Initiator sent AuthMethod=None but"
> > -				" Target is enforcing iSCSI Authentication,"
> > -					" login failed.\n");
> > -			iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
> > -					ISCSI_LOGIN_STATUS_AUTH_FAILED);
> > -			return -1;
> > -		}
> > +		bool auth_required = iscsi_conn_auth_required(conn);
> > +
>
> In __iscsi_target_login_thread we have:
>
>         if (conn->sess)
>                 conn->sess->se_sess->sup_prot_ops =
>                         conn->conn_transport->iscsit_get_sup_prot_ops(conn);
>
> before we call:
>
> iscsi_target_start_negotiation -> iscsi_target_do_login- > iscsi_target_handle_csg_zero
> and hit the code above.
>
> If conn->sess can be NULL then iscsi_conn_auth_required will crash.
>
> However, I can't tell how conn->sess can be NULL in that code path. Is the conn->sess
> check in __iscsi_target_login_thread not needed?

conn->sess is set to NULL in iscsi_login_non_zero_tsih_s1 and  new session is created
in iscsi_login_non_zero_tsih_s2 which is before iscsi_target_start_negotiation, so
we are safe.

BR,
 Dmitry
diff mbox series

Patch

diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index f0769708e4fb..006fa679517a 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -94,6 +94,31 @@  int extract_param(
 	return 0;
 }
 
+static struct iscsi_node_auth *iscsi_get_node_auth(struct iscsi_conn *conn)
+{
+	struct iscsi_portal_group *tpg;
+	struct iscsi_node_acl *nacl;
+	struct se_node_acl *se_nacl;
+
+	if (conn->sess->sess_ops->SessionType)
+		return &iscsit_global->discovery_acl.node_auth;
+
+	se_nacl = conn->sess->se_sess->se_node_acl;
+	if (!se_nacl) {
+		pr_err("Unable to locate struct se_node_acl for CHAP auth\n");
+		return NULL;
+	}
+
+	if (se_nacl->dynamic_node_acl) {
+		tpg = to_iscsi_tpg(se_nacl->se_tpg);
+		return &tpg->tpg_demo_auth;
+	}
+
+	nacl = to_iscsi_nacl(se_nacl);
+
+	return &nacl->node_auth;
+}
+
 static u32 iscsi_handle_authentication(
 	struct iscsi_conn *conn,
 	char *in_buf,
@@ -102,38 +127,11 @@  static u32 iscsi_handle_authentication(
 	int *out_length,
 	unsigned char *authtype)
 {
-	struct iscsi_session *sess = conn->sess;
 	struct iscsi_node_auth *auth;
-	struct iscsi_node_acl *nacl;
-	struct iscsi_portal_group *tpg;
-	struct se_node_acl *se_nacl;
-
-	if (!sess->sess_ops->SessionType) {
-		/*
-		 * For SessionType=Normal
-		 */
-		se_nacl = conn->sess->se_sess->se_node_acl;
-		if (!se_nacl) {
-			pr_err("Unable to locate struct se_node_acl for"
-					" CHAP auth\n");
-			return -1;
-		}
-
-		if (se_nacl->dynamic_node_acl) {
-			tpg = to_iscsi_tpg(se_nacl->se_tpg);
-
-			auth = &tpg->tpg_demo_auth;
-		} else {
-			nacl = to_iscsi_nacl(se_nacl);
 
-			auth = &nacl->node_auth;
-		}
-	} else {
-		/*
-		 * For SessionType=Discovery
-		 */
-		auth = &iscsit_global->discovery_acl.node_auth;
-	}
+	auth = iscsi_get_node_auth(conn);
+	if (!auth)
+		return -1;
 
 	if (strstr("CHAP", authtype))
 		strcpy(conn->sess->auth_type, "CHAP");
@@ -813,6 +811,37 @@  static int iscsi_target_do_authentication(
 	return 0;
 }
 
+bool iscsi_conn_auth_required(struct iscsi_conn *conn)
+{
+	struct se_node_acl *se_nacl;
+
+	if (conn->sess->sess_ops->SessionType) {
+		/*
+		 * For SessionType=Discovery
+		 */
+		return conn->tpg->tpg_attrib.authentication;
+	}
+	/*
+	 * For SessionType=Normal
+	 */
+	se_nacl = conn->sess->se_sess->se_node_acl;
+	if (!se_nacl) {
+		pr_debug("Unknown ACL %s is trying to connect\n",
+			 se_nacl->initiatorname);
+		return true;
+	}
+
+	if (se_nacl->dynamic_node_acl) {
+		pr_debug("Dynamic ACL %s is trying to connect\n",
+			 se_nacl->initiatorname);
+		return conn->tpg->tpg_attrib.authentication;
+	}
+
+	pr_debug("Known ACL %s is trying to connect\n",
+		 se_nacl->initiatorname);
+	return conn->tpg->tpg_attrib.authentication;
+}
+
 static int iscsi_target_handle_csg_zero(
 	struct iscsi_conn *conn,
 	struct iscsi_login *login)
@@ -874,22 +903,26 @@  static int iscsi_target_handle_csg_zero(
 		return -1;
 
 	if (!iscsi_check_negotiated_keys(conn->param_list)) {
-		if (conn->tpg->tpg_attrib.authentication &&
-		    !strncmp(param->value, NONE, 4)) {
-			pr_err("Initiator sent AuthMethod=None but"
-				" Target is enforcing iSCSI Authentication,"
-					" login failed.\n");
-			iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
-					ISCSI_LOGIN_STATUS_AUTH_FAILED);
-			return -1;
-		}
+		bool auth_required = iscsi_conn_auth_required(conn);
+
+		if (auth_required) {
+			if (!strncmp(param->value, NONE, 4)) {
+				pr_err("Initiator sent AuthMethod=None but"
+				       " Target is enforcing iSCSI Authentication,"
+				       " login failed.\n");
+				iscsit_tx_login_rsp(conn,
+						ISCSI_STATUS_CLS_INITIATOR_ERR,
+						ISCSI_LOGIN_STATUS_AUTH_FAILED);
+				return -1;
+			}
 
-		if (conn->tpg->tpg_attrib.authentication &&
-		    !login->auth_complete)
-			return 0;
+			if (!login->auth_complete)
+				return 0;
 
-		if (strncmp(param->value, NONE, 4) && !login->auth_complete)
-			return 0;
+			if (strncmp(param->value, NONE, 4) &&
+			    !login->auth_complete)
+				return 0;
+		}
 
 		if ((login_req->flags & ISCSI_FLAG_LOGIN_NEXT_STAGE1) &&
 		    (login_req->flags & ISCSI_FLAG_LOGIN_TRANSIT)) {
@@ -904,6 +937,18 @@  static int iscsi_target_handle_csg_zero(
 	return iscsi_target_do_authentication(conn, login);
 }
 
+static bool iscsi_conn_authenticated(struct iscsi_conn *conn,
+				     struct iscsi_login *login)
+{
+	if (!iscsi_conn_auth_required(conn))
+		return true;
+
+	if (login->auth_complete)
+		return true;
+
+	return false;
+}
+
 static int iscsi_target_handle_csg_one(struct iscsi_conn *conn, struct iscsi_login *login)
 {
 	int ret;
@@ -947,11 +992,10 @@  static int iscsi_target_handle_csg_one(struct iscsi_conn *conn, struct iscsi_log
 		return -1;
 	}
 
-	if (!login->auth_complete &&
-	     conn->tpg->tpg_attrib.authentication) {
+	if (!iscsi_conn_authenticated(conn, login)) {
 		pr_err("Initiator is requesting CSG: 1, has not been"
-			 " successfully authenticated, and the Target is"
-			" enforcing iSCSI Authentication, login failed.\n");
+		       " successfully authenticated, and the Target is"
+		       " enforcing iSCSI Authentication, login failed.\n");
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
 				ISCSI_LOGIN_STATUS_AUTH_FAILED);
 		return -1;