-
Notifications
You must be signed in to change notification settings - Fork 7
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
Initial implementation #3
Conversation
Did a pass over the cryptographic parts of this PR at least |
Thanks for reviewing! To say if this is mergeable atm, hmm no. Basic client-side supports are supposedly fine, but server hosting is clearly devastating, so I think this is not in a really good shape. Until the integration tests to make private keys and certs for different TLS schemes are made with automated GH Actions to check for them, I don't want to give this a pass, sadly I'm pretty preoccupated with other things in life, I can only offer limited supports. |
I meant that to say I took a look over the cryptographic parts of the PR |
Hmm this is my result:
|
Ah, why do I forgot to read again... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I see no major reason not to merge this
As originally discussed in this issue: #3 (comment) The code was originally developed as Apache 2.0+MIT+ISC, though this repo included only a MIT license in #3. To my understanding there are three relevant contributors: myself, @stevefan1999-personal, and @ctz.
As originally discussed in this issue: #3 (comment) The code was originally developed as Apache 2.0+MIT+ISC, though this repo included only a MIT license in #3. To my understanding there are three relevant contributors: myself, @stevefan1999-personal, and @ctz.
I think I made it straight in the Zulip chat that I should try and see if this could be merged.
As I previously said in the chat, I used a lot of Rust type gymnastics which may look clever on the surface but not only it is very mentally challenging (it took me a few nights to sort them out actually), and worse, it induces a lot of cognitive loads for future maintenances, so what if we want to scale it out to support other TLS compatible algorithms and crypto suites, that would be pretty hard because of all those generic trait bounds and type information which took a lot of time to put the puzzle together.
But at the same time, as a competent programmer, you should not repeat yourself (at least DRY is a very important principle to me), and this is a pretty conflicting philosophy for me. This is the very reason I'm reluctant to make a PR in the first place.
But then I have an eureka moment: why don't I just rewrite the egregious generics parts with macros? Annnnnnnd boom! I decided to take some time to do it, and it definitely looked cleaner and easier to understand now, with the added benefit of easier to debug, much better documentations thanks to concrete implementations and having concise information for tracing in the future.
Meanwhile only the AES-GCM part for AEAD has not yet finished with the macro transition. I have also added some integration tests using various example sites in BadSSL as an example, but it would be better if we can host those test sites locally, particularly for air-gapped environments where they may even control the test environments. I have also added some generic websites as test vectors, but we are yet to have crypto suites control to have a more precise test against a specific crypto suites (say for example, TLS1.3 predominately uses ChaCha20-Poly1305 and so AES-GCM is rarely used and less coverage is reported in the integration tests).
Still, I would continue regarding this code as highly experimental, and potentially bug-ridden. In particular, the ECC256 BadSSL test is still failing while others looked...fine. My old repo will be archived and cease to update, and the efforts are all scoped under my fork now.
Fixes #2 and #1