Add fido2 pin #399
Add fido2 pin #399
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hello @olastor . Thanks for the PR. Why did you not include a man page for the decryption part? |
src/pins/fido2/clevis-decrypt-fido2
Outdated
| exit 1 | ||
| fi | ||
| if ! hmac_salt="$(jose fmt --json="$hdr" --get clevis --get fido2 --get hmac_salt --unquote=-)" ; then | ||
| echo 'JWE missing 'hmac_salt' header parameter!' >&2 |
There was a problem hiding this comment.
As you are using simple quotes, you could do:
echo "JWE .... 'hmac_salt' .... ! "
src/pins/fido2/clevis-decrypt-fido2
Outdated
| exit 1 | ||
| fi | ||
| if ! uv="$(jose fmt --json="$hdr" --get clevis --get fido2 --get uv --unquote=-)" ; then | ||
| echo 'JWE missing 'uv' header parameter!' >&2 |
There was a problem hiding this comment.
As you are using simple quotes, you could do:
echo "JWE .... 'uv' .... ! "
This is repeated in different lines, so please, review them if possible.
Shellcheck complains about it this way:
In clevis-decrypt-fido2 line 44:
echo 'JWE missing 'uv' header parameter!' >&2
^-- SC2026 (info): This word is outside of quotes. Did you intend to 'nest '"'single quotes'"' instead'?
There was a problem hiding this comment.
Right, thanks for the hint. I fixed that and the above comment in the latest commit 👌
Hi @sarroutbi , thank you for taking a look at the PR! The man clevis decrypt man page seems to be "pin-agnostic". None of the other pins has one, so I think this pin wouldn't need one, either. Only for encrypting, there are specific options that need to be documented. |
src/pins/fido2/clevis-encrypt-fido2
Outdated
| fi | ||
|
|
||
| # use the secret in a key wrapping key | ||
| jwk='{"kty":"oct", "alg":"PBES2-HS512+A256KW"}' |
There was a problem hiding this comment.
If anyone is reviewing this, I'd like to know your opinion on my choice of this "alg" here (if any).
There was a problem hiding this comment.
Sorry, I don't have a strong opinion regarding algorithm selection.
There was a problem hiding this comment.
Update: I changed this to AES256GCM. The thinking behind the exotic first choice of algorithm here was that it would be better to additionally do some key derivation from the hmac secret retrieved from the fido2 token, and also because I experienced some wrapping errors when using the raw value returned. Coming back to this, I changed my mind. I don't think an additional key derivation might be needed here as the hmac over the random salt should be sufficient (but I am happy to be proven wrong about it). The wrapping errors I experienced with the raw value were likely due to libfido2 returning values encoded in base64, and I probably didn't get the conversion to base64url right, which should now be fixed. So now the hmac output is used as a simple AES256GCM type key.
Hello @olastor. Thanks for clarifying. You are completely right. I did not realize it was "pin-agnostic" |
Thanks, I think adding unit tests is unfortunately non-trivial because there would need to be some emulator of a fido2 token involved. |
There was a problem hiding this comment.
Thank you very much for your PR. Changes LGTM. Let's wait for a second review by @sergio-correia, as this is an important update.
|
@sergio-correia : can you please provide your feedback regarding this PR when possible? |
|
@sarroutbi @sergio-correia It's been a while since I opened this PR and I am wondering how big the chances of merging this in the future are? I am willing (given I find the time) to look into how to create tests or improve the docs/code if necessary, or add a notice about it being an experimental pin in the beginning. There are for example some possible UX improvements like waiting for the key to be inserted or more informative messages for the user. However, if this PR stays "stale", then I will probably move forward to maintaining this pin in a separate personal repo instead. |
|
Hello @olastor. As I explained before, I prefer to have a double check on this PR, taking into account this is a big change, I would like a second opinion. @sergio-correia : can you please provide your feedback regarding this PR when possible? |
|
I am going to maintain this pin in a separate repository (https://github.com/olastor/clevis-pin-fido2) for now. Maybe I'll reopen the PR in the future if this project has more active maintainers. |
Implementation proposal for #332
fido2-credandfido2-assert"PBES2-HS512+A256KW"AES256GCMExample usage:
Would appreciate any reviews, especially by people familiar with FIDO2/WebAuthn/CTAP to make sure the design is reasonably secure. Afaik, creating a non-discoverable credential and storing it as public metadata is similar as systemd-cryptenroll implements it.