gh-libtom/libtomcrypt
Watch
1
0
Fork
You've already forked libtomcrypt
 0
Code Issues 42 Pull requests 17 Releases 9 Wiki Activity

[WIP/RFC] Remove prng registry #515

Open
sjaeckel wants to merge 4 commits from remove-prng-registry into develop
pull from: remove-prng-registry
merge into: gh-libtom:develop
Conversation 7 Commits 4 Files changed 66 +348 -668
sjaeckel commented 2019-10-20 06:05:03 -05:00 (Migrated from github.com)

The first step into the direction of a registry-free LibTomCrypt

  • documentation is added or updated
  • tests are added or updated
The first step into the direction of a registry-free LibTomCrypt * [ ] documentation is added or updated * [x] tests are added or updated
buggywhip (Migrated from github.com) reviewed 2019-10-20 06:05:03 -05:00
karel-m (Migrated from github.com) reviewed 2019-10-20 06:05:03 -05:00
karel-m commented 2019-10-20 12:55:09 -05:00 (Migrated from github.com)

I do support this idea; however, this is going to break a lot of things. Basically nearly all SW using libtomcrypt.

IMO this PR should be labeled LTCv2.0 and the timing of merging into develop should be carefully considered.

I'll hopefully find some time for deeper review.

I do support this idea; however, this is going to break a lot of things. Basically nearly all SW using libtomcrypt. IMO this PR should be labeled `LTCv2.0` and the timing of merging into develop should be carefully considered. I'll hopefully find some time for deeper review.
sjaeckel commented 2019-10-21 04:51:55 -05:00 (Migrated from github.com)

I do support this idea; however, this is going to break a lot of things. Basically nearly all SW using libtomcrypt.

yep, one has to break stuff sometimes to be able to advance

IMO this PR should be labeled LTCv2.0 and the timing of merging into develop should be carefully considered.

yes it will be 2.0 and IMO the branch is called develop for a reason and contains the next version, no matter how hard it will break previous versions, so I think it should be merged as soon as it's ready.

I'll hopefully find some time for deeper review.

The most important part of this PR is the decision if desc can be put into state or whether we should do it somehow differently!?

The rest of the PR is going through the sources and adapting to the new API.

> I do support this idea; however, this is going to break a lot of things. Basically nearly all SW using libtomcrypt. yep, one has to break stuff sometimes to be able to advance > IMO this PR should be labeled `LTCv2.0` and the timing of merging into develop should be carefully considered. yes it will be 2.0 and IMO the branch is called develop for a reason and contains the next version, no matter how hard it will break previous versions, so I think it should be merged as soon as it's ready. > I'll hopefully find some time for deeper review. The most important part of this PR is the decision if `desc` can be put into `state` or whether we should do it somehow differently!? The rest of the PR is going through the sources and adapting to the new API.
levitte commented 2024-08-20 03:27:26 -05:00 (Migrated from github.com)

... registry-free LibTomCrypt

Oh, is that the line of thinking going forward?

I'm frankly sad to see this concept go, I very much liked the way that made LTC expandable on an ad-hoc basis, for example to try out a new cipher (say?) and have it easily hooked into the LTC modes, just to mention an example...

> ... registry-free LibTomCrypt Oh, is that the line of thinking going forward? I'm frankly sad to see this concept go, I very much liked the way that made LTC expandable on an ad-hoc basis, for example to try out a new cipher (say?) and have it easily hooked into the LTC modes, just to mention an example...
sjaeckel commented 2024-08-21 12:40:26 -05:00 (Migrated from github.com)

I'm frankly sad to see this concept go, I very much liked the way that made LTC expandable on an ad-hoc basis, for example to try out a new cipher (say?) and have it easily hooked into the LTC modes, just to mention an example...

Oh, there's no plan to remove the basic architecture of hooking stuff together. The only thing that I believe would be better is the removal of the static registries. If we take gcm as example I imagine the changes as follows:

diff --git a/src/encauth/gcm/gcm_init.c b/src/encauth/gcm/gcm_init.c
index 1822bdcc..2a3c7d80 100644
--- a/src/encauth/gcm/gcm_init.c
+++ b/src/encauth/gcm/gcm_init.c
@@ -19,3 +19,3 @@
  */
-int gcm_init(gcm_state *gcm, int cipher,
+int gcm_init(gcm_state *gcm, const struct ltc_cipher_descriptor *cipher,
              const unsigned char *key,  int keylen)

This would've basically prevented the race condition of SM4 we just saw.

Currently we're carrying a lot of times a descriptor/state+id in the API, whereas we could simply combine both into the state.

> I'm frankly sad to see this concept go, I very much liked the way that made LTC expandable on an ad-hoc basis, for example to try out a new cipher (say?) and have it easily hooked into the LTC modes, just to mention an example... Oh, there's no plan to remove the basic architecture of hooking stuff together. The only thing that I believe would be better is the removal of the static registries. If we take gcm as example I imagine the changes as follows: ```patch diff --git a/src/encauth/gcm/gcm_init.c b/src/encauth/gcm/gcm_init.c index 1822bdcc..2a3c7d80 100644 --- a/src/encauth/gcm/gcm_init.c +++ b/src/encauth/gcm/gcm_init.c @@ -19,3 +19,3 @@ */ -int gcm_init(gcm_state *gcm, int cipher, +int gcm_init(gcm_state *gcm, const struct ltc_cipher_descriptor *cipher, const unsigned char *key, int keylen) ``` This would've basically prevented the race condition of SM4 we just saw. Currently we're carrying a lot of times a descriptor/state+id in the API, whereas we could simply combine both into the state.
levitte commented 2024-08-22 02:32:20 -05:00 (Migrated from github.com)

It kind of makes sense, and I can agree with that... it does cut away the possibility for more complex systems, where there's a part that needs to be able to find implementations by name. However, it is questionable if that complexity level must be part of ltc itself, or if it is better left to be implemented by apps, or another library on top of ltc.

Thank you for clarifying!

It kind of makes sense, and I can agree with that... it does cut away the possibility for more complex systems, where there's a part that needs to be able to find implementations by name. However, it is questionable if that complexity level must be part of ltc itself, or if it is better left to be implemented by apps, or another library on top of ltc. Thank you for clarifying!
This pull request has changes conflicting with the target branch.
  • demos/small.c
  • demos/timing.c
  • makefile.mingw
  • makefile.msvc
  • makefile.unix
  • makefile_include.mk
  • sources.cmake
  • src/headers/tomcrypt_pk.h
  • src/headers/tomcrypt_pkcs.h
  • src/headers/tomcrypt_private.h
  • src/math/rand_prime.c
  • src/misc/crypt/crypt_register_prng.c
  • src/pk/dh/dh_generate_key.c
  • src/pk/dsa/dsa_generate_key.c
  • src/pk/dsa/dsa_generate_pqg.c
  • src/pk/dsa/dsa_sign_hash.c
  • src/pk/ecc/ecc_sign_hash.c
  • src/pk/pkcs1/pkcs_1_oaep_encode.c
  • src/pk/pkcs1/pkcs_1_pss_encode.c
  • src/pk/pkcs1/pkcs_1_v1_5_encode.c
  • src/pk/rsa/rsa_encrypt_key.c
  • src/pk/rsa/rsa_make_key.c
  • src/pk/rsa/rsa_sign_hash.c
  • tests/dh_test.c
  • tests/ecc_test.c
  • tests/pkcs_1_eme_test.c
  • tests/pkcs_1_emsa_test.c
  • tests/pkcs_1_oaep_test.c
  • tests/pkcs_1_pss_test.c
  • tests/pkcs_1_test.c
  • tests/prng_test.c
  • tests/rsa_test.c
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'remove-prng-registry:remove-prng-registry'
git switch 'remove-prng-registry'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'
git merge --no-ff 'remove-prng-registry'
git switch 'remove-prng-registry'
git rebase 'develop'
git switch 'develop'
git merge --ff-only 'remove-prng-registry'
git switch 'remove-prng-registry'
git rebase 'develop'
git switch 'develop'
git merge --no-ff 'remove-prng-registry'
git switch 'develop'
git merge --squash 'remove-prng-registry'
git switch 'develop'
git merge --ff-only 'remove-prng-registry'
git switch 'develop'
git merge 'remove-prng-registry'
git push origin 'develop'
Sign in to join this conversation.
Reviewers
No reviewers
buggywhip
karel-m
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
feature

   
help wanted

   
improve API

   
invalid

   
needs documentation

   
not-planned
This ticket is not planned to be implemented and/or fixed from the core team.

  
rtfm

   
stale

   
support

   
wishlist
No labels
bug
duplicate
enhancement
feature
help wanted
improve API
invalid
needs documentation
not-planned
rtfm
stale
support
wishlist
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-libtom/libtomcrypt!515
No description provided.