A C++ library that sends and receives data using HTTPS or HTTP

Question

#include <iostream>
#include <openssl/x509.h>
#include <openssl/x509_vfy.h>
#include <sstream>
#include <sys/types.h>
#include <sys/socket.h>

#include <netdb.h>
#include <string.h>
#include <unistd.h>

#include <vector>
#include <string>

#include <openssl/ssl.h>
#include <openssl/err.h>
#include <openssl/bio.h>
#include <openssl/conf.h>

class Website
{
    int status, sock, ssl_sock;
    struct addrinfo hints;
    struct addrinfo *servinfo;
    SSL_CTX* ctx = NULL;
    BIO *web = NULL, *out = NULL;
    SSL *ssl = NULL;
    long res = 1;
 
    struct URL
    {
        std::string host;
        std::string port;
        std::string protocol;
    };
    URL url;
    public:
      Website(std::string url){
          parseUrl(url);
          if(Website::url.protocol == "http"){
            establishConn();
            std::cout << "Err\n";
          } else if(Website::url.protocol == "https"){
            initSSL();
            initCTX();
            /*
            if((web = BIO_new_ssl_connect(ctx)) == NULL) throw "Error in bio ssl";
            if(BIO_set_conn_hostname(web, Website::url.host.c_str()) != 1) throw "BIO hostname error"; 
            if(BIO_set_conn_port(web, Website::url.port.c_str()) != 1) throw "BIO port error";
            if(BIO_set_nbio(web, 1) != 1) throw "Error setting BIO to nonblocking";
            BIO_get_ssl(web, &ssl);
            if(ssl == NULL) throw "Error in ssl";*/
            const char* const PREFERED_CIPHERS = "HIGH:!aNULL:!kRSA:!PSK:!SRP:!MD5:!RC4";
            if(SSL_set_cipher_list(ssl, PREFERED_CIPHERS) != 1) throw "Cipher error";
            if(SSL_set_tlsext_host_name(ssl, Website::url.host.c_str()) != 1) throw "Hostname error";
            /*if(BIO_do_connect(web) == 0) throw "Error connecting";
            //if(BIO_do_handshake(web) == 0) throw "Error handshake";
            X509* cert = SSL_get_peer_certificate(ssl);
            if(cert) X509_free(cert);
            if(cert == NULL) {ERR_print_errors_fp(stderr); throw "Error with cert";}
            if(SSL_get_verify_result(ssl) != X509_V_OK) throw "Error verifying cert";*/
            
            establishConn();
            ssl_sock = SSL_get_fd(ssl);
            if(SSL_set_fd(ssl, sock) == 0) throw "Error setting fd";
            int SSL_status = SSL_connect(ssl);
            switch(SSL_get_error(ssl,SSL_status)){
                case SSL_ERROR_NONE:
                    //No error, do nothing
                    break;
                case SSL_ERROR_ZERO_RETURN:
                    throw "Peer has closed connection";
                    break;
                case SSL_ERROR_SSL:
                    ERR_print_errors_fp(stderr);
                    SSL_shutdown(ssl);

                    throw "Error in SSL library";
                    break;
                    
                default:
                    ERR_print_errors_fp(stderr);
                    throw "Unknown error";
                    break;

            }
            std::cout << "Ssl connection using " << SSL_get_cipher(ssl) << "\n";


          }
      }
     
      std::string get(std::string loc, int maxsize){
        std::string request = "GET "+ loc + "\r\n\r\n";
        char *recvBuf = new char[maxsize];
        memset(recvBuf, 0, strlen(recvBuf));
        Website::sendToSite(request);
        Website::recvFromSite(recvBuf, maxsize);
        std::string reply(recvBuf);
        return reply;
      }
    ~Website(){
      if(Website::url.protocol =="http"){
        close(sock);
        freeaddrinfo(servinfo);
      }else if(Website::url.protocol == "https"){
        SSL_free(ssl);
        SSL_CTX_free(ctx);
      }
     }

    private:
      void sendToSite(std::string request){
        if(Website::url.protocol == "http"){
            if (send(sock, request.c_str(), strlen(request.c_str()), 0)  == -1) throw "Error sending message";
        } else if(Website::url.protocol == "https"){
            int len = SSL_write(ssl, request.c_str(), strlen(request.c_str()));
            if(len < 0) throw "Error sending ssl packet"; 
        }
      }

      void recvFromSite(char buf[], int maxsize){
        if(Website::url.protocol == "http"){
            if (recv(sock, buf, maxsize, 0) == -1) throw "Error receving message";
        } else if(Website::url.protocol == "https"){
            int amountRead = 0;
           while(amountRead < maxsize){
                int readPart = SSL_read(ssl, buf, maxsize - amountRead);
                if(readPart < 0) throw "Error reccieving message";
                amountRead += readPart;
           
           } 
        }
      }
      //Setting up the SSL
      void initSSL(void){
        SSL_library_init();

      }
      void initCTX(){
        const SSL_METHOD* method = TLS_method();
        if((ctx = SSL_CTX_new(method)) == NULL) throw "Could not create CTX";
        SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL);
        SSL_CTX_set_verify_depth(ctx, 4);
        SSL_CTX_set_options(ctx, SSL_OP_ALL);
        if(SSL_CTX_set_default_verify_paths(ctx) == 0) throw "Couldn't se default verify paths";
        if((ssl = SSL_new(ctx)) == NULL) throw "Couldn't create SSL";

 
  
      }
      void establishConn(){
        memset(&hints, 0, sizeof hints);
        hints.ai_family = AF_UNSPEC;
        hints.ai_socktype = SOCK_STREAM;
        if((status = getaddrinfo(Website::url.host.c_str(), Website::url.port.c_str(), &hints, &servinfo)) != 0) throw "Something wrong with getaddrinfo";
        if((sock = socket(servinfo->ai_family, servinfo->ai_socktype, servinfo->ai_protocol)) == -1) throw "Something wrong with creating socket";
        if((connect(sock, servinfo->ai_addr, servinfo->ai_addrlen)) != 0) throw "Error in connecting to website";       
      }
//Filles struct Website::url  with host as first argument and path as second
  void parseUrl(std::string url){
    // Check wether url is http or https
    if(url.rfind("http://", 0) == 0){
        Website::url.port = "80";
        Website::url.host = url.substr(7);
        Website::url.protocol = "http";  
    } else if (url.rfind("https://", 0) == 0){
        Website::url.port = "443";
        Website::url.host = url.substr(8);
        Website::url.protocol = "https";
    } else {
        throw "Invalid url, must start with http:// or https://";
    }
  }
};/*
int verify_callback(int preverify, X509_STORE_CTX* x509_ctx){
  int depth = X509_STORE_CTX_get_error_depth(x509_ctx);
  int err = X509_STORE_CTX_get_error(x509_ctx);
        
  X509* cert = X509_STORE_CTX_get_current_cert(x509_ctx);
  X509_NAME* iname = cert ? X509_get_issuer_name(cert) : NULL;
  X509_NAME* sname = cert ? X509_get_subject_name(cert) : NULL;
        
  std::cout << "Issuer (cn)" << (char *)iname;
  std::cout << "Subject (cn)"<<(char *) sname;
  return preverify;
 }*/

This is a C++ library that sends data through the web using SSL or HTTP. I would like some help with making my code more readable and efficient

Answer 1

First impression

Well I spend some time and while still there many issues left lets get to the most urgent ones:

The most important rules in software development are divide&conquer, you have one monolith class that is doing every thing which makes things buggy, slow and hard to read. Minimise coupling while keeping a high cohesion, between the parts of you code. Doing this over member variables i.e. globals is bad, return values and parameters are key.

Bugs

When I tried to use your class, it was hard to find what is the supposed interface. Also I got an infinite loop when using TLS, since you are using SSL_read wrong: Return codes of 0 also indicates an error. See

<= 0 The read operation was not successful, because either the connection was closed, an error occurred or action must be taken by the calling process. Call SSL_get_error(3) with the return value ret to find out the reason.

Same bug for SSL_write, you should check for <= 0 not < 0.

Also dead commented code is clutter that should be erased, if you fear you need it later, then your source control system lite git is to the rescue. Furthermore don't comment the obvious, use doxygen syntax to just write a line what the class or function intends to do and its assumptions. To know how it's implemented, you have to read the code either way.

Proposal code

So I have taken the liberty to make a suggestion which tackles the first most urgent concerns:

website.hpp

#ifndef WEBSITE_HPP
#define WEBSITE_HPP

#include <iostream>

#include <sstream>
#include <memory>
#include <netdb.h>
#include <string.h>
#include <unistd.h>

#include <vector>
#include <string>

namespace myWeb {

class Website {
public:
    Website() = delete;
    Website(std::string url);
    std::string get(std::string loc, int maxsize);
    ~Website();

private:
    struct SSLHelper;
    std::unique_ptr<SSLHelper> sslHelper;

    int status, sock;
    struct addrinfo hints;
    struct addrinfo *servinfo;


    long res = 1;

    struct URL {
        std::string host;
        std::string port;
        std::string protocol;
    };
    URL url;
    void sendToSite(std::string request);
    void recvFromSite(char buf[], int maxsize);
    void establishConn();
    void parseUrl(std::string url);

};

}

#endif

website.cpp

#include <lib_name/lib_name.hpp>

#include <openssl/x509.h>
#include <openssl/x509_vfy.h>
#include <sys/types.h>
#include <sys/socket.h>

#include <openssl/ssl.h>
#include <openssl/err.h>
#include <openssl/bio.h>
#include <openssl/conf.h>

namespace myWeb {

struct Website::SSLHelper {
    SSL_CTX *ctx = nullptr;
    BIO *web = nullptr, *out = nullptr;
    SSL *ssl = nullptr;
    int ssl_sock{};

    SSLHelper(std::string hostName) {
        SSL_library_init();

        const SSL_METHOD *method = TLS_method();
        if ((ctx = SSL_CTX_new(method)) == NULL) throw "Could not create CTX";
        SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL);
        SSL_CTX_set_verify_depth(ctx, 4);
        SSL_CTX_set_options(ctx, SSL_OP_ALL);
        if (SSL_CTX_set_default_verify_paths(ctx) == 0) throw "Couldn't se default verify paths";
        if ((ssl = SSL_new(ctx)) == NULL) throw "Couldn't create SSL";
        const char *const PREFERED_CIPHERS = "HIGH:!aNULL:!kRSA:!PSK:!SRP:!MD5:!RC4";
        if (SSL_set_cipher_list(ssl, PREFERED_CIPHERS) != 1) throw "Cipher error";
        if (SSL_set_tlsext_host_name(ssl, hostName.c_str()) != 1) throw "Hostname error";
    }
    void upgrade_ssl(int sock) {
        ssl_sock = SSL_get_fd(ssl);
        if (SSL_set_fd(ssl, sock) == 0) throw "Error setting fd";
        int SSL_status = SSL_connect(ssl);
        switch (SSL_get_error(ssl, SSL_status)) {
            case SSL_ERROR_NONE:
//No error, do nothing
                break;
            case SSL_ERROR_ZERO_RETURN:
                throw "Peer has closed connection";
                break;
            case SSL_ERROR_SSL:
                ERR_print_errors_fp(stderr);
                SSL_shutdown(ssl);

                throw "Error in SSL library";
                break;

            default:
                ERR_print_errors_fp(stderr);
                throw "Unknown error";
                break;

        }
        std::cout << "Ssl connection using " << SSL_get_cipher(ssl) << "\n";
    }
    ~SSLHelper(){
        SSL_free(ssl);
        SSL_CTX_free(ctx);
    }
};

Website::Website(std::string url){
    parseUrl(url);
    if (Website::url.protocol == "http") {
        establishConn();
        std::cout << "Err\n";
    } else if (Website::url.protocol == "https") {
        this->sslHelper = std::make_unique<SSLHelper>(this->url.host);

        establishConn();
        sslHelper->upgrade_ssl(this->sock);
    }
}

std::string Website::get(std::string loc, int maxsize) {
    std::string request = "GET " + loc + "\r\n\r\n";
    char *recvBuf = new char[maxsize];
    memset(recvBuf, 0, strlen(recvBuf));
    Website::sendToSite(request);
    Website::recvFromSite(recvBuf, maxsize);
    std::string reply(recvBuf);
    return reply;
}

Website::~Website() {
    close(sock);
    freeaddrinfo(servinfo);
}

void Website::sendToSite(std::string request){
    if(Website::url.protocol == "http"){
        if (send(sock, request.c_str(), strlen(request.c_str()), 0)  == -1) throw "Error sending message";
    } else if(Website::url.protocol == "https"){
        int len = SSL_write(sslHelper->ssl, request.c_str(), strlen(request.c_str()));
        if(len < 0) throw "Error sending ssl packet";
    }
}

void Website::recvFromSite(char buf[], int maxsize){
    if(Website::url.protocol == "http"){
        if (recv(sock, buf, maxsize, 0) == -1) throw "Error receving message";
    } else if(Website::url.protocol == "https"){
        int amountRead = 0;
        while(amountRead < maxsize){
            int readPart = SSL_read(sslHelper->ssl, buf, maxsize - amountRead);
            if(readPart < 0) throw "Error reccieving message";
            amountRead += readPart;

        }
    }
}

void Website::establishConn(){
    memset(&hints, 0, sizeof hints);
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    if((status = getaddrinfo(Website::url.host.c_str(), Website::url.port.c_str(), &hints, &servinfo)) != 0) throw "Something wrong with getaddrinfo";
    if((sock = socket(servinfo->ai_family, servinfo->ai_socktype, servinfo->ai_protocol)) == -1) throw "Something wrong with creating socket";
    if((connect(sock, servinfo->ai_addr, servinfo->ai_addrlen)) != 0) throw "Error in connecting to website";
}
//Filles struct Website::url  with host as first argument and path as second
void Website::parseUrl(std::string url){
    // Check wether url is http or https
    if(url.rfind("http://", 0) == 0){
        Website::url.port = "80";
        Website::url.host = url.substr(7);
        Website::url.protocol = "http";
    } else if (url.rfind("https://", 0) == 0){
        Website::url.port = "443";
        Website::url.host = url.substr(8);
        Website::url.protocol = "https";
    } else {
        throw "Invalid url, must start with http:// or https://";
    }
}

}

What I have done first was to stream line the user facing header. Now the interface and internals of Website is much clearer, and even the SSL dependency is not viral to the user of your library. Also I refactored the SSL/TLS concerns in a separate class. It's not perfect but a start, that should make using the SSL simpler and clearer. Patterns that relate to this are the PIMPL pattern and RAII.

If you wonder how to get to the header/include, I suggest using CMake, how to use it is a bit beyond this scope, but I modified a project template of mine, you can find what I actually used here: https://github.com/Superlokkus/spielwiese/tree/code_review_the_masked_rebel . Please note that I was a bit lazy so lib_name should be replaced my your library's name.

The important things are that OpenSSL can be linked privately thanks to the .cpp via target_link_libraries(${PROJECT_NAME} PRIVATE OpenSSL::SSL)

Conclusion

In the end I assume you are a beginner, and so does you code look, but to reach out to get feedback and post you code to review, is something very positive and professional! Keep iterating, don't forget to commit&push, and this will get good in no time. Although for non educational purposes I would refer to boost asio or boost beast. The next steps would be: Correct OpenSSL usage and throw the SSL error. Then stuffing the non SSL code into another class, with both the SSL and non SSL class sharing a common interface. This can be done via virtual abstract functions for runtime or at compile time via CRTP, so that the Website class don't even care which one is used. Don't use C-style buffers, use std::vector<std::uint8_t> or std::array<std::uint8_t>, with iterators. Also double check your SSL code, my proposal is not checked for TLS/OpenSSL correctness.

Answer 2

I notice you have a lot of lines like
if(Website::url.protocol =="http")
that is, comparing the string against a literal. First of all, don't qualify the members with the current class. You are writing code for the body of a function in Website so you don't need to put Website:: everywhere!

Anyway, my real point is that this string comparison is slow, and repetitive. You keep checking the same thing. I suggest making an enumeration type for the supported protocols, and parsing it once.

All the differences though are not about the individual protocols, but just whether TLS is selected or not. Make a separate function that checks for that.

You actually have different code for http vs https in every member, including the destructor and which member data gets used (sock vs ssl etc.). So, these should really be different classes. Make a base class, and separate derived classes for each case, using virtual functions.

Don't use the C macro NULL. C++ has a keyword nullptr.

In C++, you don't need to put struct in front of structure type names; e.g. struct addrinfo hints; should just be addrinfo hints;. You probably copied socket sample code from C examples.

Answer 3

One of the beauties of C++ is RAII. Without it, you get many leaks unless you pay very close attention to a lot of details. I tried that old C-like method many times, but I can tell you that 99.9% of the time, you get it wrong. So always use RAII if you do not want to leak all over the place.

What happens in your code?

...
ssl = SSL_new(ctx);
...
switch(SSL_get_error(ssl,SSL_status))
{
case SSL_ERROR_ZERO_RETURN:
    throw "Peer has closed connection";
...

This is in the constructor. What you are saying is: let's allocate an SSL socket, then you see an error occurred and you throw. You just leaked that socket.

The solution is to place the socket in a smart pointer. On a throw, the socket will be closed. Not only that, use that smart pointer in your class definition and you don't even need a destructor. Everything will be deallocated automatically.

I have such an implementation in my eventdispatcher. The SSL socket is actually a pointer, so that one is easy. You can directly use a shared_ptr<SSL_>(). For the plain socket, though, you have to use the close() function to close it. That makes things a bit more complicated. I use my raii_generic_deleter.h for such resources (however, note that in my implementation I use the same BIO interface: when you don't add the SSL layer, then it works like a plain socket).

Now you can freely throw anywhere / anytime.

By the way, if you call a C++ function, the likelihood that it throws is rather high. So without RAII your code would have to take care of that too. Something like:

auto a = new SomeClass();  // this can throw std::bad_alloc

is not valid if you don't use RAII. Instead you need:

SomeClass * a = nullptr;
try
{
    a = new SomeClass()
}
catch(...)
{
    // do any clean up of anything allocated before `a`
    ...
    throw;
}

As I mentioned above. This is a lot of work. That's why RAII is your friend. The C++ compiler automatically handles all your resources cleanup.

---

As a side note, for your exceptions, at a minimum, use std::exception():

throw std::exception("message");

Better yet, get acquainted with the various error types; to get started, I suggest the following two:

throw std::logic_error("review your code, something went wrong");
throw std::runtime_error("the socket couldn't be allocated");

If you use those two, at least you convey a strong message. Others are useful too, but 90% of my code uses just these and I'm good most of the time. (Well, really I have my own exceptions deriving from those two and once in a while from std::range_error).