-
Notifications
You must be signed in to change notification settings - Fork 106
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
tang: default JWK thumbprint is now SHA-256 / deprecate SHA-1 #264
Conversation
This is related to latchset/jose#86 |
src/pins/tang/clevis-decrypt-tang
Outdated
# `kid' thumprint not in the advertised keys, but it's possible jose used | ||
# a different default algorithm for calculating its thumbprint, so let's | ||
# try out the supported algorithms to make sure `kid' really is not part | ||
# of the advertised keys. |
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.
Would it make sense to derive the algorithm from the hash length and limit to sensible hashes as SHA1, SHA256 for now? This could limit the amount of attempts here (if jose supports more hash algorithms) and speed up things.
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, something like this would work, but why would you want to limit it to SHA1 and SHA256?
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 am not sure what all hash types you are planning to support nor what hash types does jwk support at all, but as you are not transferring the hash type alongside of the hash and just trying all of them, it makes it prone to collisions especially if an attacker could modify the thumbprints and jwk supports md5 or other ancient stuff.
Limiting to known list of good algorithm can give you assurance what is actually being used. Using SHA1 is the current behavior, which is good enough for short future, SHA256 is something that is still possible by users to compare manually and good for quite long future, but anything bigger is hard to compare by users. I see this is automated comparison so it should not be an issue, but other uses might require manual comparison.
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.
These are good points, thanks for elaborating. I agree it is a good idea to limit it to SHA1 and SHA256 for now.
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.
Would you mind taking another look at the updated version?
During encryption using tang, the clevis metadata stores the thumbprint of the exchange key in the `kid' attribute. This thumbprint used jose default thumbprint hash algorithm -- SHA-1 as of now, even though RFC 7638 recommends at least SHA-256. During decryption with tang, we then validate the key with the thumbprint represented by `kid' is present in the advertisement. At this point, we used again jose default thumbprint hash for this check. We do have the JWK, though, so we are able to recalculate the thumbprint with different algorithms to make sure it matches. clevis now moved to use SHA-256 for this thumbprint, and deprecated SHA-1 use. It still supports SHA-1 so that previously encrypted data can still be decrypted, but a warning will be issued when decryption finds a thumbprint using a deprecated hash.
09705e3
to
5f7348f
Compare
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.
Does make sense to have some same parts of clevis-encrypt-tang and clevis-decrypt-tang detached to some common functions file like in clevis-luks scripts? Only think I see the same are the constants and cycle through the CLEVIS_ALTERNATIVE_THP_ALGS - this is definitely unnecessary. It's just minor comment.
It probably would make sense to do that, at some point. As you pointed out, the common parts are currently minimal, so I think it would be fine to keep it as is for now. |
After latchset#264, we have started using SHA-256 for the JWK thumbprints, however an issue was introduced causing clevis to output data to stdout in certain situations which in turn would render the output of the encryption malformed. This commit fixes this issue and updates the tests to catch this scenario.
After #264, we have started using SHA-256 for the JWK thumbprints, however an issue was introduced causing clevis to output data to stdout in certain situations which in turn would render the output of the encryption malformed. This commit fixes this issue and updates the tests to catch this scenario.
During the encryption using tang, the clevis metadata stores the
thumbprint of the exchange key in the `kid' attribute. This thumbprint
uses jose default thumbprint hash algorithm -- SHA1 as of now.
When decrypting, we validate that the key with thumbprint represented by
kid' is present in the advertisement. At this point, jose then uses its default hash algorithm to do the checking. If jose default thumbprint hash algorithm changes and we have a different default than what was used to calculate
kid' at encryption time, this verification will fail, andso does the decryption itself.
To handle a possible change in jose default thumbprint hash algorithm,
this verification was extended to try out every jose supported hash
algorithms, if the validation of `kid' fails with the default one.