Improve SAML authentication

In the initial implementation there were still some issues that needed
to be fixed.
One main improvement is that the http server thread does not run the
tunnel thread. Instead the http server is first shut down and then
the tunnel is started as usual.

src/config.c

@@ -45,6 +45,8 @@ const struct vpn_config invalid_cfg = {
 	.password = {'\0'},
 	.password_set = 0,
 	.cookie = NULL,
+	.saml_port = 0,
+	.saml_session_id = {'\0'},
 	.otp = {'\0'},
 	.otp_prompt = NULL,
 	.otp_delay = -1,
@@ -418,9 +420,7 @@ int load_config(struct vpn_config *cfg, const char *filename)
 			if (strncmp(cfg->user_cert, "pkcs11:", 7) == 0)
 				cfg->use_engine = 1;
 		} else if (strcmp(key, "saml-login") == 0) {
-			free(cfg->saml_port);
-			cfg->saml_port = atol(val);
-			continue;
+			cfg->saml_port = atoi(val);
 		} else if (strcmp(key, "user-key") == 0) {
 			free(cfg->user_key);
 			cfg->user_key = strdup(val);

src/config.h

@@ -81,6 +81,7 @@ struct x509_digest {
  * We believe we are on the safe side using this value.
  */
 #define MAX_DOMAIN_LENGTH 256
+#define MAX_SAML_SESSION_ID_LENGTH 1024
 
 struct vpn_config {
 	char			gateway_host[GATEWAY_HOST_SIZE + 1];
@@ -91,8 +92,8 @@ struct vpn_config {
 	int			password_set;
 	char			otp[OTP_SIZE + 1];
 	char			*cookie;
-	int			    saml_port;
-	char			saml_session_id[1024];
+	int			saml_port;
+	char			saml_session_id[MAX_SAML_SESSION_ID_LENGTH];
 	char			*otp_prompt;
 	unsigned int		otp_delay;
 	int			no_ftm_push;

src/http.c

@@ -622,7 +622,7 @@ static int try_otp_auth(struct tunnel *tunnel, const char *buffer,
 
 
 /*
- * Authenticates to gateway by sending username and password.
+ * Authenticates to gateway by sending a SAML session token.
  *
  * @return  1   in case of success
  *          < 0 in case of error
@@ -634,12 +634,15 @@ int saml_login(struct tunnel *tunnel)
 	int ret;
 	ssl_connect(tunnel);
 
-	char uri[1024];
-	snprintf(uri, sizeof(uri), "/remote/saml/auth_id?id=%s", tunnel->config->saml_session_id);
+	char uri_pattern[] =  "/remote/saml/auth_id?id=%s";
+	int required_size = snprintf(NULL, 0, uri_pattern, tunnel->config->saml_session_id) + 1;
+	char *uri = alloca(required_size);
+	snprintf(uri, required_size, uri_pattern, tunnel->config->saml_session_id);
+
 	char *response;
 	uint32_t response_size = 0;
 	ret = http_request(tunnel, "GET", uri, "", &response, &response_size);
-	if(ret != 1 || response_size <= 15) return ret;
+	if(ret != 1 || response_size <= 15) return -1;
 	if (memcmp(response, "HTTP/1.1 200 OK", 15) != 0){
 		log_error("SAML login failed: %s\n", response);
 		return ret;
@@ -648,11 +651,9 @@ int saml_login(struct tunnel *tunnel)
 	if (ret == ERR_HTTP_NO_COOKIE){
 		log_error("SAML login failed: no cookie\n");
 		return ret;
-	}	
-
-	// free(response);
-
+	}
 
+	free(response);
 
 	return ret;
 }

src/http.h

@@ -19,7 +19,6 @@
 #define OPENFORTIVPN_HTTP_H
 
 #include "tunnel.h"
-#include "config.h"
 
 #include <stdint.h>
 

src/http_server.c

@@ -1,64 +1,70 @@
-#include <sys/socket.h>
 #include <netinet/in.h>
 #include <string.h>
 #include <unistd.h>
+#include <ctype.h>
+#include <netinet/tcp.h>
+
 #include "config.h"
 #include "log.h"
-#include <pthread.h>
 #include "tunnel.h"
-#include <ctype.h>
-#include <signal.h>
-#include <netinet/tcp.h>
 
+// Convenience function to send a response with a user readable status message and the
+// request URL shown for debug purposes.
+// The response is shown in the user's browser after being redirected from the Fortinet Server.
+static void send_status_response(int socket, const char *userMessage) {
+    const char *reply = "HTTP/1.1 200 OK\r\n"
+            "Content-Type: text/plain\r\n"
+            "Connection: close\r\n\r\n"
+            "%s\r\n"; // User readable message
+
+    int requiredSize = snprintf(NULL, 0, reply, userMessage) + 1;
+    char *buffer = alloca(requiredSize);
+    snprintf(buffer, requiredSize, reply, userMessage);
 
+    ssize_t write_result = write(socket, buffer, strlen(buffer));
+    (void)write_result;
+}
 
 int process_request(int new_socket, char *id) {
-    log_info("Processing request\n");
+    log_info("Processing HTTP SAML request\n");
 
     int flag = 1;
     setsockopt(new_socket, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof(int));
 
-    const int buffer_size = 2048;
-    char *response[buffer_size];
-    memset(response,' ',buffer_size);
-    response[buffer_size - 1] = '\0';
-    char *reply = "HTTP/1.1 200 OK\r\n"
-            "Content-Type: text/plain\r\n"
-            "Connection: close\r\n\r\n"
-            "SAML Login...\r\n\0";
-    memcpy(response,reply,strlen(reply));
-
     // Read the request
-    ssize_t write_result = write(new_socket, response, buffer_size);
-    (void)write_result;
-
-    // dup2(new_socket, STDOUT_FILENO);
-    // dup2(new_socket, STDERR_FILENO);
-
-
-
     char request[1024];
     ssize_t read_result = read(new_socket, request, sizeof(request));
 
     // Check for '=id' in the response
-    if (read_result < 0 || strlen(request) < 10 || strncmp(request, "GET /?id=", 9) != 0) {
+    // If the recevied request from the server is larger than the buffer, the result will not be null-terminated.
+    // Causing strlen to behave wrong.
+    if (read_result < 0 || read_result == sizeof(request)) {
+        log_error("Bad request\n");
+        send_status_response(new_socket, "Invalid redirect response from Fortinet server. VPN could not be established.");
+        return -1;
+    }
+
+    if (strlen(request) < 10 || strncmp(request, "GET /?id=", 9) != 0) {
         log_error("Bad request\n");
+        send_status_response(new_socket, "Invalid redirect response from Fortinet server. VPN could not be established.");
         return -1;
     }
 
     // Extract the id
     const int id_start = 9;
-    char *id_end = memchr(&request[id_start], ' ', 1000);
+    char *id_end = memchr(&request[id_start], ' ', sizeof(request) - id_start);
 
     if (id_end == NULL) {
         log_error("Bad request format\n");
+        send_status_response(new_socket, "Invalid formatting of Fortinet server redirect response. VPN could not be established.");
         return -1;
     }
 
     int id_length = id_end - &request[id_start];
 
-    if(id_length == 0 || id_length > 1000) {
+    if(id_length == 0 || id_length >= MAX_SAML_SESSION_ID_LENGTH) {
         log_error("Bad request id\n");
+        send_status_response(new_socket, "Invalid SAML session id received from Fortinet server. VPN could not be established.");
         return -1;
     }
 
@@ -67,19 +73,24 @@ int process_request(int new_socket, char *id) {
     for (int i = 0; i < id_length; i++) {
         if (isalnum(id[i]) || id[i] == '-') continue;
         log_error("Invalid id format\n");
+        send_status_response(new_socket, "Invalid SAML session id received from Fortinet server. VPN could not be established.");
         return -1;
     }
-    // close(new_socket);
+
     log_info("Extracted id: %s\n", id);
+    send_status_response(new_socket,
+                         "SAML session id received from Fortinet server. VPN will be established..."
+                         "\r\nYou can close this browser tab now.");
     return 0;
 }
 
 /**
  * run a http server to listen for saml login requests
+ *
+ * @return 0 in case of success
+ *         < 0 in case of error
 */
-void* start_http_server(void *void_config) {
-    struct vpn_config *config = (struct vpn_config *)void_config;
-
+int start_http_server(struct vpn_config *config) {
     int server_fd, new_socket;
     struct sockaddr_in address;
     int opt = 1;
@@ -89,13 +100,14 @@ void* start_http_server(void *void_config) {
     // Creating socket file descriptor
     if ((server_fd = socket(AF_INET, SOCK_STREAM, 0)) == 0) {
         log_error("Failed to create socket\n");
-        return NULL;
+        return -1;
     }
 
     // Forcefully attaching socket to the port
     if (setsockopt(server_fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt))) {
+        close(server_fd);
         log_error("Failed to set socket options\n");
-        return NULL;
+        return -1;
     }
 
     address.sin_family = AF_INET;
@@ -104,52 +116,34 @@ void* start_http_server(void *void_config) {
 
     // Forcefully attaching socket to the port
     if (bind(server_fd, (struct sockaddr *)&address, sizeof(address)) < 0) {
+        close(server_fd);
         log_error("Failed to bind socket to port %d \n",saml_port);
-        return NULL;
+        return -1;
     }
 
     if (listen(server_fd, 3) < 0) {
+        close(server_fd);
         log_error("Failed to listen on socket\n");
-        return NULL;
+        return -1;
     }
     log_info("Listening for saml login on port: %d\n", saml_port);
-    int running = 1;
-
-    pthread_t vpn_thread = 0;
 
-
-    while(running) {
+    while(1) {
         if ((new_socket = accept(server_fd, (struct sockaddr *)&address, (socklen_t*)&addrlen)) < 0) {
             log_error("Failed to accept connection\n");
             continue;
         }
 
         int result = process_request(new_socket, config->saml_session_id);
+        close(new_socket);
         if(result != 0) {
             log_error("Failed to process request\n");
             continue;
         }
-
-
-        // Kill previous thread if it exists
-        if (vpn_thread != 0) {
-            log_error("Stop existing tunnel\n");
-            pthread_kill(vpn_thread,SIGTERM);
-            pthread_join(vpn_thread, NULL);
-        }
-
-        int thread_create_result = pthread_create(&vpn_thread, NULL, run_tunnel_wrapper, (void *)config);
-        if (thread_create_result != 0) {
-            log_error("Failed to create VPN thread\n");
-            continue;
-        }
-
-
-        // pthread_detach(vpn_thread);
-
-        result = 0;
+        break;
     }
 
-    return NULL;
+    close(server_fd);
+    return 0;
 }
 

src/http_server.h

@@ -1,3 +1,3 @@
 #include "config.h"
 
-void* start_http_server(void *void_config);
+int start_http_server(struct vpn_config *config);

src/main.c

@@ -228,7 +228,7 @@ int main(int argc, char *argv[])
 		.password_set = 0,
 		.cookie = NULL,
 		.saml_port = 0,
-		.saml_session_id = NULL,
+		.saml_session_id = {'\0'},
 		.otp = {'\0'},
 		.otp_prompt = NULL,
 		.otp_delay = 0,
@@ -290,7 +290,7 @@ int main(int argc, char *argv[])
 		{"password",             required_argument, NULL, 'p'},
 		{"cookie",               required_argument, NULL, 0},
 		{"cookie-on-stdin",      no_argument, NULL, 0},
-		{"saml-login",			 optional_argument, NULL, 0},
+		{"saml-login",           optional_argument, NULL, 0},
 		{"otp",                  required_argument, NULL, 'o'},
 		{"otp-prompt",           required_argument, NULL, 0},
 		{"otp-delay",            required_argument, NULL, 0},
@@ -615,7 +615,7 @@ int main(int argc, char *argv[])
 				if(optarg != NULL){
 					port =	strtol(optarg, NULL, 0);
 				}
-				if (port < 0 || port > UINT_MAX) {
+				if (port < 0 || port > 65535) {
 					log_warn("Invalid saml listen port: %s! Default port is 8020 \n", optarg);
 					break;
 				}
@@ -732,7 +732,7 @@ int main(int argc, char *argv[])
 			goto user_error;
 		}
 	// If username but no password given, interactively ask user
-	if (!cfg.password_set && cfg.username[0] != '\0' && !cfg.cookie) {
+	if (!cfg.password_set && cfg.username[0] != '\0' && !cfg.cookie && !cfg.saml_port) {
 		char hint[USERNAME_SIZE + 1 + REALM_SIZE + 1 + GATEWAY_HOST_SIZE + 10];
 
 		sprintf(hint, "%s_%s_%s_password",
@@ -757,16 +757,18 @@ int main(int argc, char *argv[])
 	}
 
 	if(cfg.saml_port != 0) {
-		pthread_t server_thread;
+		// Wait for the SAML token from the HTTP GET request
+		if (start_http_server(&cfg) != 0) {
+			log_error("Failed to retrieve SAML cookie from HTTP\n");
+			ret = EXIT_FAILURE;
+			goto exit;
+		}
 
-		if(pthread_create(&server_thread, NULL, start_http_server, &cfg) != 0){
-			log_error("Failed to create saml login server thread\n");
-			// ret = EXIT_FAILURE;
+		if (strlen(cfg.saml_session_id) == 0) {
+			log_error("Failed to receive SAML session id\n");
+			ret = EXIT_FAILURE;
 			goto exit;
 		}
-		// log_debug("Running http server on port %d\n", cfg.saml_port);
-		while(get_sig_received() == 0);
-		goto exit;
 	}
 
 	do {