[PATCH 09/18] nvme-tcp: add connect option 'tls'

[Sagi Grimberg]

>>> Add a connect option 'tls' to request TLS1.3 in-band encryption, and
>>> abort the connection attempt if TLS could not be established.
>>>
>>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>>> ---
>>>   drivers/nvme/host/fabrics.c | 5 +++++
>>>   drivers/nvme/host/fabrics.h | 2 ++
>>>   drivers/nvme/host/tcp.c     | 7 ++++++-
>>>   3 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>>> index bbaa04a0c502..fdff7cdff029 100644
>>> --- a/drivers/nvme/host/fabrics.c
>>> +++ b/drivers/nvme/host/fabrics.c
>>> @@ -609,6 +609,7 @@ static const match_table_t opt_tokens = {
>>>       { NVMF_OPT_DISCOVERY,        "discovery"        },
>>>       { NVMF_OPT_DHCHAP_SECRET,    "dhchap_secret=%s"    },
>>>       { NVMF_OPT_DHCHAP_CTRL_SECRET,    "dhchap_ctrl_secret=%s"    },
>>> +    { NVMF_OPT_TLS,            "tls"            },
>>>       { NVMF_OPT_ERR,            NULL            }
>>>   };
>>> @@ -632,6 +633,7 @@ static int nvmf_parse_options(struct 
>>> nvmf_ctrl_options *opts,
>>>       opts->hdr_digest = false;
>>>       opts->data_digest = false;
>>>       opts->tos = -1; /* < 0 == use transport default */
>>> +    opts->tls = false;
>>>       options = o = kstrdup(buf, GFP_KERNEL);
>>>       if (!options)
>>> @@ -918,6 +920,9 @@ static int nvmf_parse_options(struct 
>>> nvmf_ctrl_options *opts,
>>>               kfree(opts->dhchap_ctrl_secret);
>>>               opts->dhchap_ctrl_secret = p;
>>>               break;
>>> +        case NVMF_OPT_TLS:
>>> +            opts->tls = true;
>>> +            break;
>>>           default:
>>>               pr_warn("unknown parameter or missing value '%s' in 
>>> ctrl creation request\n",
>>>                   p);
>>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>>> index dcac3df8a5f7..c4538a9d437c 100644
>>> --- a/drivers/nvme/host/fabrics.h
>>> +++ b/drivers/nvme/host/fabrics.h
>>> @@ -70,6 +70,7 @@ enum {
>>>       NVMF_OPT_DISCOVERY    = 1 << 22,
>>>       NVMF_OPT_DHCHAP_SECRET    = 1 << 23,
>>>       NVMF_OPT_DHCHAP_CTRL_SECRET = 1 << 24,
>>> +    NVMF_OPT_TLS        = 1 << 25,
>>>   };
>>>   /**
>>> @@ -128,6 +129,7 @@ struct nvmf_ctrl_options {
>>>       int            max_reconnects;
>>>       char            *dhchap_secret;
>>>       char            *dhchap_ctrl_secret;
>>> +    bool            tls;
>>>       bool            disable_sqflow;
>>>       bool            hdr_digest;
>>>       bool            data_digest;
>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>> index bcf24e9a08e1..bbff1f52a167 100644
>>> --- a/drivers/nvme/host/tcp.c
>>> +++ b/drivers/nvme/host/tcp.c
>>> @@ -1902,6 +1902,9 @@ static int nvme_tcp_alloc_admin_queue(struct 
>>> nvme_ctrl *ctrl)
>>>               break;
>>>       }
>>>       if (ret) {
>>> +        /* Abort if TLS is requested */
>>> +        if (num_keys && ctrl->opts->tls)
>>> +            goto out_free_queue;
>>>           /* Try without TLS */
>>>           ret = nvme_tcp_alloc_queue(ctrl, 0, 0);
>>>           if (ret)
>>> @@ -1934,6 +1937,8 @@ static int __nvme_tcp_alloc_io_queues(struct 
>>> nvme_ctrl *ctrl)
>>>                   break;
>>>           }
>>>           if (ret) {
>>> +            if (num_keys && ctrl->opts->tls)
>>> +                goto out_free_queues;
>>
>> I don't see why we even attempt tls if we're not explicitly told to.
> 
> Because we don't know.

Exactly why we should do what we've been told.

> It's all easy if we do a discovery before connect, as the discovery log 
> page tells us what to do.
> But if we do _not_ do a discovery, how would we know what to do?

The initiator of the connect needs to know that.

> We could try to start off with no TLS, but if the server requires TLS 
> then all we get is a 'connection refused' error, leaving us none the wiser.
> So really we have to start off with TLS (if we have a matching 
> identity). The server then can always refuse the connection (with the 
> same error), in which case we should re-try without TLS.
> That's where the 'tls' option comes in: to avoid having a fallback 
> without TLS.

I don't think we should do any type of fallback in the driver.
I think that if we want a fallback we need to put it in userspace.

> So in the end we have three modes for the client:
> 
> 1) TLS not supported
> 2) TLS allowed (with fallback to non-TLS)
> 3) TLS required (no fallback to non-TLS)
4) TLS allowed, but not desired.

> For modes 2) and 3) a PSK has to be present, and
> the 'tls' option is used to switch from mode 2) to mode 3)

I think that tls option should tell the driver exactly what
it needs to do. It seems wrong to me that now every tcp connection
would unconditionally start with tls and fallback to normal.

But let me think about it some more...