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

[WIP/RFC] Remove prng registry #515

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Conversation

sjaeckel
Copy link
Member

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

  • documentation is added or updated
  • tests are added or updated

@sjaeckel sjaeckel requested review from buggywhip and karel-m October 20, 2019 11:05
@karel-m
Copy link
Member

karel-m commented Oct 20, 2019

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
Copy link
Member Author

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.

@sjaeckel sjaeckel force-pushed the remove-prng-registry branch 2 times, most recently from 990d623 to 0050955 Compare October 21, 2019 10:09
@sjaeckel sjaeckel changed the title RFC: Remove prng registry [WIP/RFC] Remove prng registry Oct 29, 2019
@sjaeckel sjaeckel mentioned this pull request Aug 20, 2024
2 tasks
@levitte
Copy link
Collaborator

levitte commented Aug 20, 2024

... 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
Copy link
Member Author

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.

@sjaeckel sjaeckel force-pushed the remove-prng-registry branch from 0050955 to ce68cee Compare August 21, 2024 18:01
@levitte
Copy link
Collaborator

levitte commented Aug 22, 2024

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!

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.

3 participants