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.

Sign up for GitHub

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

Jump to bottom

Fix #451: Allow overriding length of decimalized signatures #452

Merged
merged 5 commits into from
Jan 24, 2023
Merged

Fix #451: Allow overriding length of decimalized signatures #452

merged 5 commits into from
Jan 24, 2023

Conversation

petrdvorak
Copy link
Member

No description provided.

@petrdvorak petrdvorak requested a review from hvge January 21, 2023 13:06
@romanstrobl romanstrobl self-requested a review January 23, 2023 10:52
romanstrobl
romanstrobl requested changes Jan 23, 2023
View reviewed changes
Copy link
Member

@romanstrobl romanstrobl left a comment

Choose a reason for hiding this comment

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

This is a nice improvement, however I do not like the "magical" behaviour. E.g. when signature length is set to 2, the actual signature length is 4. We should reject invalid setting in my opinion, because this can lead to misleading implementations.

powerauth-docs/test-vectors/signatures-offline.json Outdated Show resolved Hide resolved
...to/src/main/java/io/getlime/security/powerauth/crypto/lib/config/SignatureConfiguration.java Outdated Show resolved Hide resolved
hvge
hvge requested changes Jan 23, 2023
View reviewed changes
Copy link
Member

@hvge hvge left a comment

Choose a reason for hiding this comment

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

I found mostly the same issues as @romanstrobl

...ypto/src/test/java/io/getlime/security/powerauth/crypto/lib/util/GenerateVectorDataTest.java Outdated Show resolved Hide resolved
powerauth-docs/test-vectors/signatures-offline.json Outdated Show resolved Hide resolved
...to/src/main/java/io/getlime/security/powerauth/crypto/lib/config/SignatureConfiguration.java Outdated Show resolved Hide resolved
@petrdvorak
Copy link
Member Author

I will have a look.

hvge
hvge approved these changes Jan 24, 2023
View reviewed changes
Copy link
Member

@hvge hvge left a comment

Choose a reason for hiding this comment

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

Looks OK now. The last thing that's missing is a short note in the documentation about this option.

romanstrobl
romanstrobl approved these changes Jan 24, 2023
View reviewed changes
Copy link
Member

@romanstrobl romanstrobl left a comment

Choose a reason for hiding this comment

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

Nice solution for the configuration, thanks!

@petrdvorak petrdvorak merged commit 6d81bf1 into develop Jan 24, 2023
@petrdvorak petrdvorak deleted the issues/451-decimal-len branch January 24, 2023 17:39
jandusil pushed a commit that referenced this pull request Dec 6, 2023
@petrdvorak @jandusil
Fix #451: Allow overriding length of decimalized signatures (#452)
faa9a34 
* Fix #451: Allow overriding length of decimalized singatures

* Change code based on code review

* Fix test vector

* Fix JavaDoc

* Add invalid format to exception message

(cherry picked from commit 6d81bf1)
jandusil added a commit that referenced this pull request Dec 7, 2023
@jandusil
Fix #451: Allow overriding length of decimalized signatures (#452) (#553
0a1cdca 
)

* Fix #451: Allow overriding length of decimalized singatures
- Change code based on code review
- Fix test vector
- Fix JavaDoc
- Add invalid format to exception message
(cherry picked from commit 6d81bf1)
Co-authored-by: Petr Dvořák <petr@wultra.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@banterCZ banterCZ banterCZ left review comments

@hvge hvge hvge approved these changes

@romanstrobl romanstrobl romanstrobl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@petrdvorak @banterCZ @hvge @romanstrobl