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

Allow connect to use configured TLS keys #2532

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

igaw
Copy link
Collaborator

@igaw igaw commented Oct 10, 2024

Extend the --tls_key and --keyring argument to accept also configured keys.

Depends on linux-nvme/libnvme#894

@hreinecke
Copy link
Collaborator

Weelll ... there was a reason why I did use the key IDs to reference the keys. If you specify the key on the commandline it will be present in the commandline args for that process for everyone to see (just do a 'cat /proc//cmdline and you can read the entire commandline argument details), with not even basic security restrictions.
So it'll be really easy for even unprivileged users to get to arbitrary keys.
If you want to go that route then please use a filename as commandline argument. But not the key itself.

@hreinecke
Copy link
Collaborator

Which incidentally was the reasons why I want to rework DH-CHAP key handling to use the key store.

@igaw
Copy link
Collaborator Author

igaw commented Oct 10, 2024

Yep, I thought the same. Thus I am not sure if we really want this and I got here due the work on linux-nvme/libnvme#890 where @martin-gpy ask for the JSON output generated by the 'connect' command is showing the 'configured key'. The key in the keystore is obviously not the 'configured key' thus we can't produce a 'correct' JSON configuration file.

I think we should keep the discussion in place, thus let's have it here linux-nvme/libnvme#890

@igaw
Copy link
Collaborator Author

igaw commented Oct 10, 2024

And there is also nvme gen-tls-key and nvme check-tls-key which accepts/operate with the configured key.

@hreinecke
Copy link
Collaborator

We could insert the key description in the json output. With that we can lookup the matching key upon reload.
That would avoid us to have to specify the actual key data anywhere, and would neatly tie in with existing mechanisms
(ie we would either query the key store for a key ID or a key description).

@igaw igaw force-pushed the tls-keys branch 6 times, most recently from 26c0fdc to b9c74e0 Compare October 24, 2024 09:45
@igaw igaw marked this pull request as ready for review October 24, 2024 09:45
Fetch TLS API changes.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
The spec is limiting the size of both variables to one byte, thus there
is no need to use wider types.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
The connect-all command accepts JSON configuration but not
the connect command. connect-all will try to connect to all
possible resources, which includes creating discovery controllers.
This might not always needed or wanted.

The connect command will only connect to the controllers
listed in the configuration file and doesn't to any
additional discovery at all.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
Extend the connect command also to accept the pre-shared key via command
line. Obviously, this is not recommended to use for a production system
but for testing this is a simple way to get a setup working.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
With the added support to also accept the key via the command line
update the documentation accordingly.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
Load keys to the keyring when the nvme-tcp module is loaded.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
Export the keys with the correct encoding scheme.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
Since this file contains secret enforce the read/write permission
limited to the owner only.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use the variable name consistently. The rest of the code base names this
variable as version.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
When creating a new key and it is inserted into keystore also support to
append it to a keyfile.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
@igaw
Copy link
Collaborator Author

igaw commented Oct 24, 2024

During some more testing of various use cases, I found some more issues.

The last patch adds a new feature which allows to export a generated key to a keyfile. This should help the use case to maintain a /etc/nvme/tls-keys files. This patch is still WIP, needs some more love.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants