Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tcp: cannot connect to target that advertises TLS secure channel support #612

Closed
cleech opened this issue Apr 4, 2023 · 7 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@cleech
Copy link

cleech commented Apr 4, 2023

With the early addition of the TLS related connect arguments to libnvme, a Linux kernel cannot connect to a target that advertises (but does not require) secure channel operation.

While doing a connect-all, if the transport requirements (TREQ) field of the discovery log page entry has bits 1:0 set to 1 (required) or 2 (not required) then the tls config option will be added to the connect command passed to the kernel.

We're starting to see targets that support some version of secure channel, and connect-all with nvme-cli 2.x fails because the kernel does not recognize the tls config option.

I realize that the kernel TLS patches are currently under review, but as they will be able to be configured out, libnvme should allow a fallback to an unsecured connection when in not-required mode.

libnvme may also need to check TSAS SECTYPE to see that the TLS version supported by the target matches that supported by the kernel.

@igaw igaw added the bug Something isn't working label Apr 5, 2023
@igaw
Copy link
Collaborator

igaw commented Apr 5, 2023

So one thing we should do is read from /dev/nvme-fabrics and check which option is supported before setting it. I thought we have added this code but I can't find it... still searching

@cleech
Copy link
Author

cleech commented Apr 5, 2023

I was able to look at a full discovery log page from one target that hit this, and it's worse than I thought.

The target isn't even advertising any TLS support, it's just setting the secure channel requirement field to 2 "not required" instead of 0 "not specified"

@igaw
Copy link
Collaborator

igaw commented Apr 12, 2023

In nvmf_connect_disc_entry() there is the logic which enables the TLS:

libnvme/src/nvme/fabrics.c

Lines 787 to 790 in 5ac91b7

if (e->trtype == NVMF_TRTYPE_TCP &&
(e->treq & NVMF_TREQ_REQUIRED ||
e->treq & NVMF_TREQ_NOT_REQUIRED))
c->cfg.tls = true;

I think the idea here is, if the target support TLS we going to use it. The 'Not required' is kind of useless from security perspective, no? Need to check the specs first.

Anyway, we need to add the kernel option filter independent. Working on it.

@igaw
Copy link
Collaborator

igaw commented Apr 12, 2023

With the PR #618 I get:

# nvme connect-all  -t tcp -a 192.168.154.10 
option "keyring" ignored
option "tls_key" ignored
option "tls" ignored

192.168.154.10 isn't running so this just the attempt to connect to the discovery controller. The log level for these messages is on info. I suppose for the time being we should make them debug until we figured out all the other details. @hreinecke what are you thoughts on this?

@hreinecke
Copy link
Collaborator

Yes, we should reduce it to 'debug' so as not to confuse users (or scripts).

@igaw
Copy link
Collaborator

igaw commented Apr 12, 2023

I reduced the warn level now. Though I am pondering if we might want a more visible warning when the user ask explicitly for an encrypted connection? Anyway, I'll go ahead and apply the libnvme PR.

@igaw
Copy link
Collaborator

igaw commented Nov 17, 2023

This should now be addressed by 1f5db47 ("fabrics: use SECTYPE to determine whether to use TLS")

@igaw igaw closed this as completed Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants