-
Notifications
You must be signed in to change notification settings - Fork 77
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
nvme: testcases for TLS support #158
base: master
Are you sure you want to change the base?
Conversation
return 1 | ||
fi | ||
|
||
systemctl start tlshd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to check that it exists as a dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also check the version of ktls-utils?
Or just explain in a comment if you have any expectations from it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point. Will check what we can do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to "man systemctl" "EXIT STATUS" section, systemctl command returns exit status "4" for "no such unit". So it would work to check if "systemctl status tlshd" command's exist status is 4 or not.
I use Fedora, and needed to install "ktls-utils" package to run the test case. It would be the better to mention the word "ktls-utils" in the SKIP_REASONS message to help users to understand what is missing.
_nvmet_target_setup --blkdev file --tls | ||
|
||
# Test unencrypted connection | ||
echo "Test unencrypted connection w/ tls not required" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
umm, looks pretty useless...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think so. This is testing the 'not required' setting in nvmet, which should accept both TLS and non-TLS connections even if TLS is enabled on the target.
echo "WARNING: connection is not encrypted" | ||
fi | ||
|
||
_nvme_disconnect_subsys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any room to test passing explicit keys and private keyrings to this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not do that here. This is for testing the 'default' case, where PSKs are pre-populated in the keyring and the connection picks up the keys automatically. Explicit keys and keyrings are really just for testing.
But we should have a separate testcase for that, true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit 320b9b6 does not look adding value. The helper function requires more types than direct call of "_require_nvme_trtype tcp".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit bc544f8 introduces the --concat option of _nvme_connect_subsys(), but it is not used anywhere. Do we need this commit in this PR? If it is a preparation for the next PR, I suggest to move this commit to that PR.
@hreinecke Thanks for rebasing the series. I ran the test case in my environment using the kernel v6.13 and the latest nvme-cli (2.10.2-77-gb4628c3, with libnvme 1.11.1-48-gacc19fc), but it fails.
059.full file left logs as follows:
kernel message was as follows:
I'm not sure if this catches a kernel bug. Still the test case may need improvement, or I may be missing something. If you have any insights about this failure, please let me know. |
Okay, I'll fix it up. |
It would if I had pushed the testcase for secure concatenation... |
_check_ctrl_tls() need to redirect stderr to /dev/null, not stdout (as it does now on two occasions). Will be fixing up the testcase. |
To start TLS-encrypted connections. Signed-off-by: Hannes Reinecke <hare@suse.de>
Add --tls option to _create_nvmet_subsystem and allow to specify the tls requirements in _create_nvmet_port. Signed-off-by: Hannes Reinecke <hare@suse.de>
@hreinecke Thanks for updating the patches. Question, which kernel should I use to run the test case? nvme/059 failure looks like this.
Also, nvme/060 run terminated in the middle. Kernel reported a BUG.
|
TCP connections can be encrypted using in-kernel TLS, so add a testcase to exercise the various combinations. Signed-off-by: Hannes Reinecke <hare@suse.de>
To start secure concatenation the option '--concat' has to be passed to the 'nvme connect' command. Signed-off-by: Hannes Reinecke <hare@suse.de>
NVMe-TCP has a 'secure concatenation' mode, where the TLS PSK is generated from the secret negotiated by the DH-HMAC-CHAP authentication, and the TLS connection is started after authentication. Signed-off-by: Hannes Reinecke <hare@kernel.org>
This pull request adds two new testcases for nvme TLS support, one for 'plain' TLS with TLS PSKs, and the other one for testing 'secure concatenation' where TLS is started after DH-HMAC-CHAP authentication.