added crypto class and cypher suites #1722
Conversation
Removed 2.7
Lint + 2.7 back in
|
Wow, this is quite a lot of new crypto! @zachriggle or @heapcrash WDYT? |
|
Looks like lots of (cool!) new code, but I think it should have doctests before we merge it in. I don't see any documentation at all, let alone tests. For @sebmichel1983, you can add doctests via e.g. def add(a, b):
"""
Adds two numbers
>>> add(1,2)
3
"""
return (a+b)Edit: It would also be nice if all of these routines were exposed as top-level routines, e.g. |
pwnlib/crypto/__init__.py
Outdated
| from pwnlib.crypto.ciphers.vigenere import CipherVigenere | ||
| from pwnlib.crypto.ciphers.xor import CipherXor | ||
|
|
||
| class Crypto(): |
There was a problem hiding this comment.
You should be able to use the module hack seen elsewhere in Pwntools to make this "automagic"
There was a problem hiding this comment.
Im not sure what you mean with this, can you give me an example?
There was a problem hiding this comment.
Can you please also tell me, how to follow your suggestions about making routines top-level as you told here:
Edit: It would also be nice if all of these routines were exposed as top-level routines, e.g. crypto.rot13 rather than CryptoRot13. By convention, pretty much everything in Pwntools is lower-case.
I could rename the classes, but im not sure how to do this the way you want it to, that it fits.
pwnlib/crypto/ciphers/vigenere.py
Outdated
| if(key is None): | ||
| raise ValueError('No key given') | ||
|
|
||
| if not (type(key) is str): |
There was a problem hiding this comment.
We should probably handle bytes() everywhere that we handle str(), for Python3-related reasons. It's reasonable for a key to be a bytes object and not a str object.
There was a problem hiding this comment.
Also, use isinstance, not type. It's reasonable to work with subclasses of bytes or str. six.ensure_bytes is probably useful here.
There was a problem hiding this comment.
Can you give me a hint on how to change the code to what is wanted? Its my first pull request to a project and i am stumbling on some parts.
|
Hey guys, thanks for your comments. I will try to fix the code the next days and will also add more ciphers. Can someone please tell me, how i can extend the documentation with the new class which i want to implement here? Best regards, Sebastian |
There was a problem hiding this comment.
Great thing. There are some minor things I don't like the most.
But there is also the question about design choices:
- which cipher algorithms work with strings? i.e. accept text strings as input?
- which cipher algorithms accept byte strings as input?
- which cipher algorithms return byte strings?
- which cipher algorithms return text strings?
- is it possible to make the spectrum bigger? Like it works bytes to bytes, and text to text does not work yet, but it is just as useful and makes logical sense, so it can be useful to enable the version, too.
- example of a thing that does not make logical sense: decoding base64 to text, or encoding text with base64. Don't worry, your code does not do any of these yet.
- Have you thought about that? Did you encounter any obstacles? We will be ready to help, you will just need to ask and wait quite a while.
CHANGELOG.md
Outdated
|
|
||
| ## 4.5.0 (`dev`) | ||
| - [#1722][1722] Added Crypto class and cypher suites |
There was a problem hiding this comment.
Please try to keep these sorted. not change the blank lines formatting, and add a link as the other entries have below
|
To add documentation, look around |
|
@sebmichel1983 @procrash @lukas-mertens Are you still interested in pushing this PR forward? Do you need any more assistance with adding documentation and tests? I'd rather not let this effort go to waste! You can run the doctests in a Docker container using |
|
Closing this due to inactivity. You're welcome to resurrect this PR if anyone wants to work on it. In the meantime one can use https://github.com/tigertv/secretpy directly for the classic ciphers. |
Pwntools Pull Request
Thanks for contributing to Pwntools! Take a moment to look at
CONTRIBUTING.mdto make sure you're familiar with Pwntools development.Please provide a high-level explanation of what this pull request is for.
Testing
I created a simple Crypto() class to implement known and well used cyphers which are used often in CTFs.
Target Branch
Depending on what the PR is for, it needs to target a different branch.
You can always change the branch after you create the PR if it's against the wrong branch.
devdevstablestablebranchbetabetabranch, but notstabledevChangelog
After creating your Pull Request, please add and push a commit that updates the changelog for the appropriate branch.
You can look at the existing changelog for examples of how to do this.