Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I am implementing RSA algorithm in C++ and here's my design(code structure).

keygen.h

namespace rsa{
    class keygen{
        public:
        //Constructor
        keygen(size);
        //Generate keys
        void generate();
        //Getters
        char* gete(){ return xyz; }
        .. 
        ..
        ..

        private:
        //initializes bignums
        void initall();
        //Private Member variables goes here
   }
}

prime.h

namespace rsa{
    //First 100 primes
    unsigned int primes[]={2,3,5,7,11.....541};

    //MillarRabin Primality
    bool isPrime(mpz_t, unsigned short);
    //Get Random Prime
    void getPrime(mpz_t, mpz_t)
}

endec.h

namespace rsa{
    //Encryption
    const char* encryption(const char* text, const char* n, const char* e);
    //Decryption
    const char* decryption(const char* cipher, const char* n, const char* d);
}

Is this a good design? How can I make it better? I want to improve the structure or the overall design, that's why dint post any code. Things like naming standards, using classes wherever applicable, standard function signature and similar is what I'm looking for.

share|improve this question
I think you did not provide enough information - just a few lines of code. It's hard to say something interesting about this besides trivial things like "it is better to use Keygen instead of keygen for a class name". – Michael Apr 23 '12 at 16:31
I am pretty sure that is not how RSA public key encryption works. You need to do more research on it but I do not expect to see you passing a password to the encrypt() decrypt() functions. And what is with passing char* pointers around use std::string to represent a password or create a Key type to represent a key etc. – Loki Astari Apr 23 '12 at 17:10
@Michael- I want to improve the design, that's why dint post any code. Added more details in question. – vidit Apr 23 '12 at 17:32
1  
@LokiAstari- Can you please tell me why you think so? Why passing passwords(keys here) as arguments is a bad idea? I can use strings, will change that. – vidit Apr 23 '12 at 17:39
1  
@LokiAstari- I know that and I never mentioned password anywhere. N and E are required for encryption, and N and D are required for decryption, that's what I'm passing as arguments. – vidit Apr 23 '12 at 18:38
show 1 more comment

1 Answer

up vote 3 down vote accepted

Just a couple of short comments as a starting point.

First of all, you don't need separate encrypt and decrypt functions -- the algorithm is identical for both. You just pass different parameters for them.

Second, I wouldn't use char const * for the input or output of the encrypt/decrypt function. It might be barely allowable for the plaintext (since you could restrict that to not contain zero bytes) but is definitely not acceptable as the encrypted data (since you can't stop it from containing zero bytes). Since, however, the same function can do either encryption or decryption, you don't want to use char const * for input or output. Personally, I'd probably have it take iterators for the input and output, so you can work with string, vector<char>, vector<wchar_t>, etc.

Finally, I don't think I'd put implementation details like the first 100 primes directly in the rsa namespace -- I'd have rsa contain a detail namespace to contain details like that. I'm not sure I'd have keygen either -- I think I'd make key a class, probably with the default ctor deleted, and a static member function to create a key, and overloading operator>> and operator<< to serialize keys in a standard format (e.g., X.509, PKCS #8). About all that would be visible to the outside world would be key and crypt (or whatever you decide to call the combined encrypt/decrypt funct(ion|or).

Edit: the key class and signatures would be something like this:

class key { 
    key() = delete; // `=delete;` is C++11 only; without that just declare private
public:
    static key create_key();
};
share|improve this answer
That was the type of answer I was looking for, thank you Sir. But why passing iterators is better than passing strings directly? and can you please give little more detail about using static member function(signature) and overloading operators? – vidit Apr 24 '12 at 11:15
Iterators let you change outside data types more easily, and they're easier to string together with other algorithms, data structures, etc. See edited answer about function signatures. – Jerry Coffin Apr 24 '12 at 13:27
And how is the user supposed to get those keys(n,e and d)? Using operators >> and <<? – vidit Apr 24 '12 at 15:31
@vidit: Yes, you'd generate a new key with key::generate_key(), and use operator<< to write it to a stream and operator>> to read it from a stream. I haven't bothered to show those, but they follow pretty much the usual pattern: std::ostream &operator<<(std::ostream &, key const &); and std::istream &operator>>(std::istream &, key &); – Jerry Coffin Apr 24 '12 at 15:33
Thanks again Sir. One last question, is this the right question for CodeReview? – vidit Apr 24 '12 at 15:36
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.