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

Add SHA384 #34

Merged
merged 5 commits into from
Jan 7, 2021
Merged

Add SHA384 #34

merged 5 commits into from
Jan 7, 2021

Conversation

l1h3r
Copy link
Contributor

@l1h3r l1h3r commented Dec 17, 2020

Description of change

Adds SHA384 for JSON web tokens used in identity.rs

Type of change

  • Enhancement (a non-breaking change which adds functionality)

How the change has been tested

Copied existing test structure using RFC test vectors and ones generated with python

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Contributor

@Wollac Wollac left a comment

Choose a reason for hiding this comment

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

Can the test vectors be moved into a different file? sha.rs has gotten so big, that it cannot be reviewed in GitHub. Maybe under tests similar to curl and blake?
Also, ideally the test vectors should be in a separate file than the test code. That way, the code can be checked and diff'ed without having to look at megabytes of test data.

@l1h3r l1h3r mentioned this pull request Dec 18, 2020
5 tasks
Copy link
Contributor

@Wollac Wollac left a comment

Choose a reason for hiding this comment

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

lgtm

@l1h3r l1h3r merged commit c251496 into dev Jan 7, 2021
@l1h3r l1h3r deleted the feat/sha-384 branch January 7, 2021 20:33
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.

2 participants