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

tang: default JWK thumbprint is now SHA-256 / deprecate SHA-1 #264

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

sergio-correia
Copy link
Collaborator

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, and
so 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.

@sergio-correia
Copy link
Collaborator Author

This is related to latchset/jose#86

# `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.
Copy link

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.

Copy link
Collaborator Author

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?

Copy link

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.
@sergio-correia sergio-correia changed the title pins: update tang decrypt to handle different jose default thp hash tang: default JWK thumbprint is now SHA-256 / deprecate SHA-1 Apr 12, 2021
Copy link

@ZelenyMartin ZelenyMartin left a 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.

@sergio-correia
Copy link
Collaborator Author

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.

@sergio-correia sergio-correia merged commit f4ed006 into latchset:master Apr 14, 2021
@sergio-correia sergio-correia deleted the jose-thp-alg branch April 14, 2021 12:38
sergio-correia added a commit to sergio-correia/clevis that referenced this pull request Apr 15, 2021
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.
sergio-correia added a commit that referenced this pull request Apr 15, 2021
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.
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.

3 participants