Skip to content

Libnvme derived tls psk fix#975

Closed
chiranjeb-wdc wants to merge 4 commits intolinux-nvme:masterfrom
chiranjeb-wdc:libnvme_derived_tls_psk_fix
Closed

Libnvme derived tls psk fix#975
chiranjeb-wdc wants to merge 4 commits intolinux-nvme:masterfrom
chiranjeb-wdc:libnvme_derived_tls_psk_fix

Conversation

@chiranjeb-wdc
Copy link

Issue(s):

  1. Bug: Derived TLS PSK implementation is not specification compliant
  2. Improvement(s):
    • hkdf function implementations do not support ability to run known-answer test (KAT) as algorithm verification
    • Thus, no KAT is run as an algorithm verification before using the hkdf algorithms as a robustness

Solution(s):

  1. Bug Fix: Introduce and use compliant HKDF-Extract, HKDF-Expand, and HKDF-Expand-Label crypto services to derive TLS PSK
  2. Enhancement(s):
    • Implement KAT for HKDF-Extract, HKDF-Expand, and HKDF-Expand-Label
    • Introduce KAT to before using the algorithm to ensure accidentally wrong keys are not generated
    • The new crypto services can be used to address similar issues in the other part of the livnvme code

derived tls psk derivation fix
This reverts commit 0b686f5, reversing
changes made to e9dae79.
1. Bug: Derived TLS PSK implementation is not specification compliant
2. Improvement(s):
      - hkdf function implementations do not support ability to run known-answer test (KAT) as algorithm verification
      - Thus, no KAT is run as an algorithm verification before using the hkdf algorithms as a robustness

Solution(s):
1. Bug Fix: Introduce and use compliant HKDF-Extract, HKDF-Expand, and HKDF-Expand-Label crypto services to derive TLS PSK
2. Enhancement(s):
    - Implement KAT for HKDF-Extract, HKDF-Expand, and HKDF-Expand-Label
    - Introduce KAT to before using the algorithm to ensure accidentally wrong keys are not generated
    - The new crypto services can be used to address similar issues in the other part of the livnvme code
@hreinecke
Copy link
Collaborator

Thanks for the submission, the changes look reasonable. However, there only is a KAT for SHA-256, but in NVMe we support both SHA-256 and SHA-384. So we really should have another KAT for SHA-384 to test this path, too.

@chiranjeb
Copy link

Thanks for the submission, the changes look reasonable. However, there only is a KAT for SHA-256, but in NVMe we support both SHA-256 and SHA-384. So we really should have another KAT for SHA-384 to test this path, too.

Hello @hreinecke :

Thank you so much for the feedback. I can certainly add that. However, would it be possible to accept this change as is and then allow me to add the KAT for SHA-384 in a new pull request? By the way, we have tested the nvme-connect with no transformation for both SHA-256 and SHA-384, and that is passing. My first goal was to fix the HKDF implementation, but I totally get your point. Please let me know your thoughts.

@igaw
Copy link
Collaborator

igaw commented Apr 1, 2025

Sorry for the late feedback. Just back from LSF/MM/BPF and catching up with the inbox.

This PR contains merges. But PRs should contain commits and not any other type of git objects. We have some documentation how you can resolve this ('git rebase'): https://github.com/linux-nvme/nvme-cli?tab=readme-ov-file#how-to-cleanup-your-series-before-creating-pr

Also write a commit message which follows the Linux Kernel Coding style guide, see: https://docs.kernel.org/process/submitting-patches.html# e.g.

linux: derived tls psk derivation fix

[Explain what is broken and how you are fixing it]

Signed-off-by: [your email address]

Thanks!

@igaw
Copy link
Collaborator

igaw commented May 7, 2025

This PR went stale. Please reopen it when you got an update. Thanks.

@igaw igaw closed this May 7, 2025
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.

4 participants