diff mbox series

[3/3] target: iscsi: control authentication per ACL

Message ID 20210914100314.492-4-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
Add acls/{ACL}/attrib/authentication attribute that controls authentication
for the particular ACL. By default, this attribute inherits a value of
authentication attribute of the target port group to keep backward
compatibility.

authentication attribute has 3 states:
 "0" - authentication is turned off for this ACL
 "1" - authentication is required for this ACL
 "" - authentication is inherited from TPG

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_configfs.c  | 41 +++++++++++++++++++
 drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
 .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
 include/target/iscsi/iscsi_target_core.h      |  2 +
 4 files changed, 51 insertions(+), 1 deletion(-)

Comments

Mike Christie Sept. 30, 2021, 6:52 p.m. UTC | #1
On 9/14/21 5:03 AM, Dmitry Bogdanov wrote:
> Add acls/{ACL}/attrib/authentication attribute that controls authentication
> for the particular ACL. By default, this attribute inherits a value of
> authentication attribute of the target port group to keep backward
> compatibility.
> 
> authentication attribute has 3 states:
>  "0" - authentication is turned off for this ACL
>  "1" - authentication is required for this ACL
>  "" - authentication is inherited from TPG


Why the empty string for this value? Maybe 2 or -1?


> 
> 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_configfs.c  | 41 +++++++++++++++++++
>  drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
>  .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
>  include/target/iscsi/iscsi_target_core.h      |  2 +
>  4 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index e3750b64cc0c..2d70de342408 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
>  ISCSI_NACL_ATTR(random_datain_seq_offsets);
>  ISCSI_NACL_ATTR(random_r2t_offsets);
>  
> +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
> +		char *page)
> +{
> +	struct se_node_acl *se_nacl = attrib_to_nacl(item);
> +	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
> +
> +	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
> +		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
> +
> +		return sprintf(page, "%u (inherited)\n",
> +				tpg->tpg_attrib.authentication);


I think we want a value of -1 or 2 for inherited then here it should print
that value.
Dmitry Bogdanov Oct. 4, 2021, 7:56 a.m. UTC | #2
Hi Mike,

> > Add acls/{ACL}/attrib/authentication attribute that controls authentication
> > for the particular ACL. By default, this attribute inherits a value of
> > authentication attribute of the target port group to keep backward
> > compatibility.
> > 
> > authentication attribute has 3 states:
> >  "0" - authentication is turned off for this ACL
> >  "1" - authentication is required for this ACL
> >  "" - authentication is inherited from TPG
>
>
> Why the empty string for this value? Maybe 2 or -1?
That was design decision by logic that since that attribute has a precedence 
to clear that precedence we must clear the attribute, i.e. set to the empty value.

> > 
> > 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_configfs.c  | 41 +++++++++++++++++++
> >  drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
> >  .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
> >  include/target/iscsi/iscsi_target_core.h      |  2 +
> >  4 files changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> > index e3750b64cc0c..2d70de342408 100644
> > --- a/drivers/target/iscsi/iscsi_target_configfs.c
> > +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> > @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
> >  ISCSI_NACL_ATTR(random_datain_seq_offsets);
> >  ISCSI_NACL_ATTR(random_r2t_offsets);
> >  
> > +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
> > +		char *page)
> > +{
> > +	struct se_node_acl *se_nacl = attrib_to_nacl(item);
> > +	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
> > +
> > +	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
> > +		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
> > +
> > +		return sprintf(page, "%u (inherited)\n",
> > +				tpg->tpg_attrib.authentication);
>
>
> I think we want a value of -1 or 2 for inherited then here it should print
> that value.

We decided to hide the internal value from userspace and represent it similar to
tpg.authentication to have the same handling there.

BR,
 Dmitry
Mike Christie Oct. 5, 2021, 4:04 p.m. UTC | #3
On 10/4/21 2:56 AM, Dmitriy Bogdanov wrote:
> Hi Mike,
> 
>>> Add acls/{ACL}/attrib/authentication attribute that controls authentication
>>> for the particular ACL. By default, this attribute inherits a value of
>>> authentication attribute of the target port group to keep backward
>>> compatibility.
>>>
>>> authentication attribute has 3 states:
>>>  "0" - authentication is turned off for this ACL
>>>  "1" - authentication is required for this ACL
>>>  "" - authentication is inherited from TPG
>>
>>
>> Why the empty string for this value? Maybe 2 or -1?
> That was design decision by logic that since that attribute has a precedence 
> to clear that precedence we must clear the attribute, i.e. set to the empty value.
> 
>>>
>>> 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_configfs.c  | 41 +++++++++++++++++++
>>>  drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
>>>  .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
>>>  include/target/iscsi/iscsi_target_core.h      |  2 +
>>>  4 files changed, 51 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
>>> index e3750b64cc0c..2d70de342408 100644
>>> --- a/drivers/target/iscsi/iscsi_target_configfs.c
>>> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
>>> @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
>>>  ISCSI_NACL_ATTR(random_datain_seq_offsets);
>>>  ISCSI_NACL_ATTR(random_r2t_offsets);
>>>  
>>> +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
>>> +		char *page)
>>> +{
>>> +	struct se_node_acl *se_nacl = attrib_to_nacl(item);
>>> +	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
>>> +
>>> +	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
>>> +		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
>>> +
>>> +		return sprintf(page, "%u (inherited)\n",
>>> +				tpg->tpg_attrib.authentication);
>>
>>
>> I think we want a value of -1 or 2 for inherited then here it should print
>> that value.
> 
> We decided to hide the internal value from userspace and represent it similar to
> tpg.authentication to have the same handling there.

I'm not sure what you meant by representing it similar to tpg.authentication. That
attrib, and I think every attrib, prints 1 value.

The problem with above is that this works by accident for rtslib based apps which
read in the attribs, stores them, then on restore writes them to the kernel. On the
read/save stage we get "0 (inherited)". Then on the restore stage we try to write
that back to the kernel and get an error. rtslib/targetcli just spits out an error
and ignores it, so it still works since the kernel used the default. We don't
really want the error spit out and I don't think we want it to work by accident like
this.
Dmitry Bogdanov Oct. 18, 2021, 11:48 a.m. UTC | #4
Hi Mike,

>>>> Add acls/{ACL}/attrib/authentication attribute that controls authentication
>>>> for the particular ACL. By default, this attribute inherits a value of
>>>> authentication attribute of the target port group to keep backward
>>>> compatibility.
>>>>
>>>> authentication attribute has 3 states:
>>>>  "0" - authentication is turned off for this ACL
>>>>  "1" - authentication is required for this ACL
>>>>  "" - authentication is inherited from TPG
>>>
>>>
>>> Why the empty string for this value? Maybe 2 or -1?
>> That was design decision by logic that since that attribute has a precedence 
>> to clear that precedence we must clear the attribute, i.e. set to the empty value.
>> 
>>>>
>>>> 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_configfs.c  | 41 +++++++++++++++++++
>>>>  drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
>>>>  .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
>>>>  include/target/iscsi/iscsi_target_core.h      |  2 +
>>>>  4 files changed, 51 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
>>>> index e3750b64cc0c..2d70de342408 100644
>>>> --- a/drivers/target/iscsi/iscsi_target_configfs.c
>>>> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
>>>> @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
>>>>  ISCSI_NACL_ATTR(random_datain_seq_offsets);
>>>>  ISCSI_NACL_ATTR(random_r2t_offsets);
>>>>  
>>>> +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
>>>> +		char *page)
>>>> +{
>>>> +	struct se_node_acl *se_nacl = attrib_to_nacl(item);
>>>> +	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
>>>> +
>>>> +	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
>>>> +		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
>>>> +
>>>> +		return sprintf(page, "%u (inherited)\n",
>>>> +				tpg->tpg_attrib.authentication);
>>>
>>>
>>> I think we want a value of -1 or 2 for inherited then here it should print
>>> that value.
>> 
>> We decided to hide the internal value from userspace and represent it similar to
>> tpg.authentication to have the same handling there.
>
> I'm not sure what you meant by representing it similar to tpg.authentication. That
> attrib, and I think every attrib, prints 1 value.
>
> The problem with above is that this works by accident for rtslib based apps which
> read in the attribs, stores them, then on restore writes them to the kernel. On the
> read/save stage we get "0 (inherited)". Then on the restore stage we try to write
> that back to the kernel and get an error. rtslib/targetcli just spits out an error
> and ignores it, so it still works since the kernel used the default. We don't
> really want the error spit out and I don't think we want it to work by accident like
> this.

I missed that fact that rtslib saves/restores all attributes. You are right then, I need to
report some effective value (I think '-1'). Will send new patchset soon.

BR,
 Dmitry
diff mbox series

Patch

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index e3750b64cc0c..2d70de342408 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -314,6 +314,46 @@  ISCSI_NACL_ATTR(random_datain_pdu_offsets);
 ISCSI_NACL_ATTR(random_datain_seq_offsets);
 ISCSI_NACL_ATTR(random_r2t_offsets);
 
+static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
+		char *page)
+{
+	struct se_node_acl *se_nacl = attrib_to_nacl(item);
+	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
+
+	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
+		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
+
+		return sprintf(page, "%u (inherited)\n",
+				tpg->tpg_attrib.authentication);
+	}
+	return sprintf(page, "%u\n", nacl->node_attrib.authentication);
+}
+
+static ssize_t iscsi_nacl_attrib_authentication_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct se_node_acl *se_nacl = attrib_to_nacl(item);
+	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
+	s32 val;
+	int ret;
+
+	if (sysfs_streq(page, "")) {
+		val = NA_AUTHENTICATION_INHERITED;
+	} else {
+		ret = kstrtos32(page, 0, &val);
+		if (ret)
+			return ret;
+		if (val != 0 && val != 1)
+			return -EINVAL;
+	}
+
+	nacl->node_attrib.authentication = val;
+
+	return count;
+}
+
+CONFIGFS_ATTR(iscsi_nacl_attrib_, authentication);
+
 static struct configfs_attribute *lio_target_nacl_attrib_attrs[] = {
 	&iscsi_nacl_attrib_attr_dataout_timeout,
 	&iscsi_nacl_attrib_attr_dataout_timeout_retries,
@@ -323,6 +363,7 @@  static struct configfs_attribute *lio_target_nacl_attrib_attrs[] = {
 	&iscsi_nacl_attrib_attr_random_datain_pdu_offsets,
 	&iscsi_nacl_attrib_attr_random_datain_seq_offsets,
 	&iscsi_nacl_attrib_attr_random_r2t_offsets,
+	&iscsi_nacl_attrib_attr_authentication,
 	NULL,
 };
 
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 006fa679517a..9873c5e34206 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -813,6 +813,7 @@  static int iscsi_target_do_authentication(
 
 bool iscsi_conn_auth_required(struct iscsi_conn *conn)
 {
+	struct iscsi_node_acl *nacl;
 	struct se_node_acl *se_nacl;
 
 	if (conn->sess->sess_ops->SessionType) {
@@ -839,7 +840,12 @@  bool iscsi_conn_auth_required(struct iscsi_conn *conn)
 
 	pr_debug("Known ACL %s is trying to connect\n",
 		 se_nacl->initiatorname);
-	return conn->tpg->tpg_attrib.authentication;
+
+	nacl = to_iscsi_nacl(se_nacl);
+	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED)
+		return conn->tpg->tpg_attrib.authentication;
+
+	return nacl->node_attrib.authentication;
 }
 
 static int iscsi_target_handle_csg_zero(
diff --git a/drivers/target/iscsi/iscsi_target_nodeattrib.c b/drivers/target/iscsi/iscsi_target_nodeattrib.c
index e3ac247bffe8..baf1c93fa1e3 100644
--- a/drivers/target/iscsi/iscsi_target_nodeattrib.c
+++ b/drivers/target/iscsi/iscsi_target_nodeattrib.c
@@ -30,6 +30,7 @@  void iscsit_set_default_node_attribues(
 {
 	struct iscsi_node_attrib *a = &acl->node_attrib;
 
+	a->authentication = NA_AUTHENTICATION_INHERITED;
 	a->dataout_timeout = NA_DATAOUT_TIMEOUT;
 	a->dataout_timeout_retries = NA_DATAOUT_TIMEOUT_RETRIES;
 	a->nopin_timeout = NA_NOPIN_TIMEOUT;
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 21c1aaa6dae2..0913909fa765 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -26,6 +26,7 @@  struct sock;
 #define ISCSI_RX_THREAD_NAME		"iscsi_trx"
 #define ISCSI_TX_THREAD_NAME		"iscsi_ttx"
 #define ISCSI_IQN_LEN			224
+#define NA_AUTHENTICATION_INHERITED	-1
 
 /* struct iscsi_node_attrib sanity values */
 #define NA_DATAOUT_TIMEOUT		3
@@ -714,6 +715,7 @@  struct iscsi_login {
 } ____cacheline_aligned;
 
 struct iscsi_node_attrib {
+	s32			authentication;
 	u32			dataout_timeout;
 	u32			dataout_timeout_retries;
 	u32			default_erl;