Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

Allow to pass pre-generated safe prime numbers to Credential Definition creation #143

Conversation

wulfraem
Copy link

@wulfraem wulfraem commented Sep 2, 2020

To allow speeding up credential definition creation we wanted to create the safe prime numbers beforehand with ursa::helpers::generate_safe_prime and hold onto them until the actual credential definition is created.

In our use case we have to create multiple credential definitions at a time and having a bunch of safe primes ready for this improves performance of this a lot. :)

- add call pattern to allow creating credential primary keys with existing prime numbers
- allows to decouple prime number generation from actual credential definition creation for performance optimization
- add test for creating cred def with existing primes

Signed-off-by: wulfraem <wulfraem@users.noreply.github.com>
Copy link
Contributor

@mikelodder7 mikelodder7 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. LGTM

Copy link
Contributor

@brentzundel brentzundel left a comment

Choose a reason for hiding this comment

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

This PR concerns me deeply. It goes against the stated ideals of simple usability, and allows any numbers to be submitted as "primes," which could be a massive footgun.
We need to discuss this much more before merging this.

@@ -780,7 +822,33 @@ impl Issuer {
CredentialPrimaryPublicKeyMetadata,
)> {
trace!(
"Issuer::_new_credential_primary_keys: >>> credential_schema: {:?}",
"Issuer::_new_credential_primary_keys_with_primes: >>> credential_schema: {:?}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Issuer::_new_credential_primary_keys_with_primes: >>> credential_schema: {:?}",
"Issuer::_new_credential_primary_keys: >>> credential_schema: {:?}",

@brentzundel
Copy link
Contributor

@wulfraem
We discussed this on the Ursa call today, and would like to make it possible for this to happen. We are grateful for contributions and apologize for not responding more fully until now.

Here are the key concerns around your proposed changes:

  1. Lack of input validation for the prime values
  2. A reduction in clarity for the API
  3. The library is currently undergoing significant refactoring.

In greater detail:

  1. At a minimum, the input values (the two safe primes) would need to be checked for:
    1. primality - Since there is no way to guarantee that input values were generated using ursa::helpers::generate_safe_prime, there should be primality checking of the input values. We anticipate this may duplicate some of the prime-checking that has already occurred for valid primes, but it would also add protection for unwary users.
    2. valid range - The call to generate_safe_primes currently expects a size. There would need to be a check to insure that the input values don't exceed this range.
  2. We are concerned that the changed API surface may be confusing to users. Though your added comments are helpful, we feel there could be more done to guide users to the proper understanding of which function best suits their needs.
  3. The ursa library is currently experiencing a large refactoring effort. This is more informational for you than anything else. Because of this refactoring, changes you propose may meet with greater than usual need to make the PR up-do-date with the master branch and deal with conflicts.

Please respond with any questions you may have. We also welcome you to join our biweekly call to discuss this PR with the group directly. You should be able to find meeting time and connection information on the hyperledger calendar

Base automatically changed from master to main February 3, 2021 23:50
@Alexis-Falquier
Copy link

Alexis-Falquier commented Jun 16, 2021

@brentzundel are there any updates to this PR? Any plans to merge it soon?

@brentzundel
Copy link
Contributor

@Alexis-Falquier I have not heard a response from the OP about our concerns. Until those are addressed, I do not support merging this PR.

@brentzundel
Copy link
Contributor

This PR has been dormant for over a year, with no response from @wulfraem
Closing

@brentzundel brentzundel closed this Aug 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants