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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sha1 builtin #3126

Merged
merged 3 commits into from Jun 20, 2022
Merged

Add Sha1 builtin #3126

merged 3 commits into from Jun 20, 2022

Conversation

dfreeman
Copy link
Contributor

Overview

We discussed this briefly in Slack last weekend. While SHA-1 is a broken algorithm from a cryptographic perspective, it's still useful to have available for existing protocols that require its use (e.g. the WebSocket handshake).

Implementation notes

This introduces a new builtin crypto.HashAlgorithm.Sha1 using the existing declareHashAlgorithm machinery in parser-typechecker.

Interesting/controversial decisions

In principle there's no reason this couldn't be implemented in pure Unison (and in fact I did just that as an exercise), but that's likely significantly slower, and there's already a fairly established pattern of exposing well-known cryptographic primitives as builtins.

Test coverage

I've mirrored the transcript coverage that the other hash builtins have using test vectors from the same source as the existing ones.

Loose ends

I'm not 100% clear about the relationship between .base and .builtin, and whether there are any additional steps necessary to integrate this. If so, I'd welcome guidance 馃檪

Copy link
Contributor

@dolio dolio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty straight forward to me.

@dolio
Copy link
Contributor

dolio commented Jun 17, 2022

For reference, .builtin is where these sorts of things get auto-added when running the builtins.merge command in UCM.

To get them in base, you should submit a PR to it. The simplest way I know how to prepare that is to run builtins.merge locally, then move.term the added thing into the appropriate place in a forked base. I think there might be a way to directly create a term alias for a new builtin without builtins.merge, but I don't actually know how.

@pchiusano
Copy link
Member

Awesome, thanks @dfreeman !

Could you add yourself to CONTRIBUTORS.markdown and we'll get this merged!

@pchiusano
Copy link
Member

pchiusano commented Jun 17, 2022

@dfreeman once this is merged you can add an alias to base or at least file a ticket at https://github.com/unisonweb/base so we don't lose track of adding it for the next release.

But basically, for builtins like this, you can do what @dolio said above, or do -

.base> fork trunk sha1
.base.sha1> alias.term ##Sha1 crypto.HashAlgorithm.Sha1

Also you can create a doc called crypto.HashAlgorithm.Sha1.doc as well.

And then push that somewhere and open a ticket on base that says where to pull from.

@dfreeman
Copy link
Contributor Author

Could you add yourself to CONTRIBUTORS.markdown and we'll get this merged!

Done! Sorry for the delay.

Once this is merged and I'm back home from traveling later this week I'll plan to open a ticket for base as well. Thanks for the guidance!

@pchiusano
Copy link
Member

@dfreeman sounds good, I created unisonweb/base#95 to track.

@pchiusano pchiusano added the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Jun 20, 2022
@pchiusano
Copy link
Member

Applied ready-to-merge, it should merge after CI completes. Thanks again @dfreeman!

@mergify mergify bot merged commit c88b4f3 into unisonweb:trunk Jun 20, 2022
@mergify mergify bot removed the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Jun 20, 2022
@dfreeman dfreeman deleted the df/sha1 branch June 23, 2022 17:44
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.

None yet

3 participants