Pull ACLs into their own struct

Rakefile

@@ -82,13 +82,22 @@ file check("client") =>
   build/parse.o
   build/client.o
   build/serve.o 
+  build/acl.o
   build/ioutil.o 
   build/util.o} do |t|
   gcc_link t.name, t.prerequisites + [LIBCHECK]
 end
 
+file check("acl") =>
+%w{tests/check_acl.c
+  build/parse.o
+  build/acl.o
+  build/util.o} do |t| 
+  gcc_link t.name, t.prerequisites + [LIBCHECK]
+end
 
-(TEST_MODULES-["client"]).each do |m|
+
+(TEST_MODULES- %w{acl client}).each do |m|
   deps = ["tests/check_#{m}.c", "build/ioutil.o", "build/util.o"]
   maybe_obj_name = "build/#{m}.o"
 

src/acl.c

@@ -0,0 +1,99 @@
+#include <stdlib.h>
+
+#include "util.h"
+#include "parse.h"
+
+#include "acl.h"
+
+
+struct acl * acl_create( int len, char ** lines, int default_deny )
+{
+	struct acl * acl;
+
+	acl = (struct acl *)xmalloc( sizeof( struct acl ) );
+	acl->len = parse_acl( &acl->entries, len, lines );
+	acl->default_deny = default_deny;
+	return acl;
+}
+
+
+static int testmasks[9] = { 0,128,192,224,240,248,252,254,255 };
+
+/** Test whether AF_INET or AF_INET6 sockaddr is included in the given access
+  * control list, returning 1 if it is, and 0 if not.
+  */
+static int is_included_in_acl(int list_length, struct ip_and_mask (*list)[], union mysockaddr* test)
+{
+	NULLCHECK( test );
+
+	int i;
+	
+	for (i=0; i < list_length; i++) {
+		struct ip_and_mask *entry = &(*list)[i];
+		int testbits;
+		unsigned char *raw_address1, *raw_address2;
+		
+		debug("checking acl entry %d (%d/%d)", i, test->generic.sa_family, entry->ip.family);
+		
+		if (test->generic.sa_family != entry->ip.family) {
+			continue;
+		}
+		
+		if (test->generic.sa_family == AF_INET) {
+			debug("it's an AF_INET");
+			raw_address1 = (unsigned char*) &test->v4.sin_addr;
+			raw_address2 = (unsigned char*) &entry->ip.v4.sin_addr;
+		}
+		else if (test->generic.sa_family == AF_INET6) {
+			debug("it's an AF_INET6");
+			raw_address1 = (unsigned char*) &test->v6.sin6_addr;
+			raw_address2 = (unsigned char*) &entry->ip.v6.sin6_addr;
+		}
+		
+		debug("testbits=%d", entry->mask);
+		
+		for (testbits = entry->mask; testbits > 0; testbits -= 8) {
+			debug("testbits=%d, c1=%02x, c2=%02x", testbits, raw_address1[0], raw_address2[0]);
+			if (testbits >= 8) {
+				if (raw_address1[0] != raw_address2[0])
+					goto no_match;
+			}
+			else {
+				if ((raw_address1[0] & testmasks[testbits%8]) !=
+				    (raw_address2[0] & testmasks[testbits%8]) )
+				    	goto no_match;
+			}
+			
+			raw_address1++;
+			raw_address2++;
+		}
+		
+		return 1;
+		
+		no_match: ;
+		debug("no match");
+	}
+	
+	return 0;
+}
+
+int acl_includes( struct acl * acl, union mysockaddr * addr )
+{
+	NULLCHECK( acl );
+
+	if ( 0 == acl->len ) {
+		return !( acl->default_deny );
+	} 
+	else {
+		return is_included_in_acl( acl->len, acl->entries, addr );
+	}
+}
+
+
+void acl_destroy( struct acl * acl )
+{
+	free( acl->entries );
+	acl->len = 0;
+	acl->entries = NULL;
+	free( acl );
+}

src/acl.h

@@ -0,0 +1,34 @@
+#ifndef ACL_H
+#define ACL_H
+
+#include "parse.h"
+
+struct acl {
+	int len;
+	int default_deny;
+	struct ip_and_mask (*entries)[];
+};
+
+/** Allocate a new acl structure, parsing the given lines to sockaddr
+ * structures in the process.  After allocation, acl->len might not
+ * equal len.  In that case, there was an error in parsing and acl->len
+ * will be the index of the failed entry in lines.
+ *
+ * default_deny controls the behaviour of an empty list: if true, all
+ * requests will be denied.  If true, all requests will be accepted.
+ */
+struct acl * acl_create( int len, char **lines, int default_deny );
+
+
+/** Check to see whether an address is allowed by an acl.
+ * See acl_create for how the default_deny setting affects this.
+ */
+int acl_includes( struct acl *, union mysockaddr *);
+
+
+/** Free the acl structure and the internal acl entries table.
+ */
+void acl_destroy( struct acl * );
+
+
+#endif

src/control.c

@@ -32,6 +32,7 @@
 #include "readwrite.h"
 #include "bitset.h"
 #include "self_pipe.h"
+#include "acl.h"
 
 #include <stdlib.h>
 #include <string.h>
@@ -259,22 +260,21 @@ int control_mirror(struct control_params* client, int linesc, char** lines)
 /** Command parser to alter access control list from socket input */
 int control_acl(struct control_params* client, int linesc, char** lines)
 {
-	int parsedc;
+	NULLCHECK( client );
-	struct ip_and_mask (*acl)[], (*old_acl)[];
 
-	parsedc = parse_acl(&acl, linesc, lines);
+	struct acl * old_acl = client->serve->acl;
-	if (parsedc != linesc) {
+	struct acl * new_acl = acl_create( linesc, lines, old_acl ? old_acl->default_deny : 0 );
+	
+	if (new_acl->len != linesc) {
 		write(client->socket, "1: bad spec: ", 13);
-		write(client->socket, lines[parsedc], 
+		write(client->socket, lines[new_acl->len], 
-		  strlen(lines[parsedc]));
+		  strlen(lines[new_acl->len]));
 		write(client->socket, "\n", 1);
-		free(acl);
+		acl_destroy( new_acl );
 	}
 	else {
-		old_acl = client->serve->acl;
+		client->serve->acl = new_acl;
-		client->serve->acl = acl;
+		acl_destroy( old_acl );
-		client->serve->acl_entries = linesc;
-		free(old_acl);
 		write_socket("0: updated");
 	}
 	

src/flexnbd.c

@@ -19,6 +19,7 @@
  *  elsewhere in the program.
  */
 
+
 #include "serve.h"
 #include "util.h"
 
@@ -33,7 +34,9 @@
 #include <signal.h>
 
 #include <getopt.h>
+
 #include "options.h"
+#include "acl.h"
 
 
 void exit_err( char *msg )
@@ -50,11 +53,8 @@ void params_serve(
 	char *s_ctrl_sock,
 	int default_deny,
 	int acl_entries,
-	char** s_acl_entries /* first may actually be path to control socket */
+	char** s_acl_entries )
-)
 {
-	int parsed;
-
 	out->tcp_backlog = 10; /* does this need to be settable? */
 
 	if (s_ip_address == NULL)
@@ -72,14 +72,9 @@ void params_serve(
 	 * we pass NULL. */
 	out->control_socket_name = s_ctrl_sock;
 
-	/* If this is true then an empty ACL means "nobody is allowed to connect",
+	out->acl = acl_create( acl_entries, s_acl_entries, default_deny );
-	 * rather than "anybody is allowed to connect" */
+	if (out->acl && out->acl->len != acl_entries)
-	out->default_deny = default_deny;
+		SERVER_ERROR("Bad ACL entry '%s'", s_acl_entries[out->acl->len]);
-
-	out->acl_entries = acl_entries;
-	parsed = parse_acl(&out->acl, acl_entries, s_acl_entries);
-	if (parsed != acl_entries)
-		SERVER_ERROR("Bad ACL entry '%s'", s_acl_entries[parsed]);
 
 	out->bind_to.v4.sin_port = atoi(s_port);
 	if (out->bind_to.v4.sin_port < 0 || out->bind_to.v4.sin_port > 65535)

src/serve.c

@@ -65,65 +65,6 @@ void server_unlock_io( struct server* serve )
 	);
 }
 
-static int testmasks[9] = { 0,128,192,224,240,248,252,254,255 };
-
-/** Test whether AF_INET or AF_INET6 sockaddr is included in the given access
-  * control list, returning 1 if it is, and 0 if not.
-  */
-int is_included_in_acl(int list_length, struct ip_and_mask (*list)[], union mysockaddr* test)
-{
-	NULLCHECK( test );
-
-	int i;
-	
-	for (i=0; i < list_length; i++) {
-		struct ip_and_mask *entry = &(*list)[i];
-		int testbits;
-		unsigned char *raw_address1, *raw_address2;
-		
-		debug("checking acl entry %d (%d/%d)", i, test->generic.sa_family, entry->ip.family);
-		
-		if (test->generic.sa_family != entry->ip.family)
-			continue;
-		
-		if (test->generic.sa_family == AF_INET) {
-			debug("it's an AF_INET");
-			raw_address1 = (unsigned char*) &test->v4.sin_addr;
-			raw_address2 = (unsigned char*) &entry->ip.v4.sin_addr;
-		}
-		else if (test->generic.sa_family == AF_INET6) {
-			debug("it's an AF_INET6");
-			raw_address1 = (unsigned char*) &test->v6.sin6_addr;
-			raw_address2 = (unsigned char*) &entry->ip.v6.sin6_addr;
-		}
-		
-		debug("testbits=%d", entry->mask);
-		
-		for (testbits = entry->mask; testbits > 0; testbits -= 8) {
-			debug("testbits=%d, c1=%02x, c2=%02x", testbits, raw_address1[0], raw_address2[0]);
-			if (testbits >= 8) {
-				if (raw_address1[0] != raw_address2[0])
-					goto no_match;
-			}
-			else {
-				if ((raw_address1[0] & testmasks[testbits%8]) !=
-				    (raw_address2[0] & testmasks[testbits%8]) )
-				    	goto no_match;
-			}
-			
-			raw_address1++;
-			raw_address2++;
-		}
-		
-		return 1;
-		
-		no_match: ;
-		debug("no match");
-	}
-	
-	return 0;
-}
-
 /** Prepares a listening socket for the NBD server, binding etc. */
 void serve_open_server_socket(struct server* params)
 {
@@ -259,13 +200,10 @@ int server_acl_accepts( struct server *params, union mysockaddr * client_address
 	NULLCHECK( client_address );
 
 	if (params->acl) {
-		if (is_included_in_acl(params->acl_entries, params->acl, client_address))
+		return acl_includes( params->acl, client_address );
-			return 1;
-	} else {
-		if (!params->default_deny)
-			return 1;
 	}
-	return 0;
+
+	return 1;
 }
 
 
@@ -405,11 +343,11 @@ void serve_accept_loop(struct server* params)
 		client_fd = accept(activity_fd, &client_address.generic, &socklen);
 		
 		if (activity_fd == params->server_fd) {
-			debug("Accepted nbd client");
+			debug("Accepted nbd client socket");
 			accept_nbd_client(params, client_fd, &client_address);
 		}
 		if (activity_fd == params->control_fd) {
-			debug("Accepted control client"); 
+			debug("Accepted control client socket"); 
 			accept_control_connection(params, client_fd, &client_address);
 		}
 			

src/serve.h

@@ -8,6 +8,7 @@
 #endif
 
 #include "parse.h"
+#include "acl.h"
 
 #include <sys/types.h>
 
@@ -46,12 +47,8 @@ struct client_tbl_entry {
 struct server {
 	/** address/port to bind to */
 	union mysockaddr     bind_to;
-	/** does an empty ACL mean "deny all"? */
+	/** access control list */
-	int                  default_deny;
+	struct acl *         acl;
-	/** number of entries in current access control list*/
-	int                  acl_entries;
-	/** pointer to access control list entries*/
-	struct ip_and_mask   (*acl)[0];
 	/** (static) file name to serve */
 	char*                filename;
 	/** file name of INCOMPLETE flag */

tests/check_acl.c

@@ -0,0 +1,122 @@
+#include <check.h>
+#include <stdio.h>
+
+#include "acl.h"
+
+START_TEST( test_null_acl )
+{
+	struct acl *acl = acl_create( 0,NULL, 0 );
+
+	fail_if( NULL == acl, "No acl alloced." );
+	fail_unless( 0 == acl->len, "Incorrect length" );
+}
+END_TEST
+
+
+START_TEST( test_parses_single_line )
+{
+	char *lines[] = {"127.0.0.1"};
+	struct acl * acl = acl_create( 1, lines, 0 );
+
+	fail_unless( 1 == acl->len, "Incorrect length." );
+	fail_if( NULL == acl->entries, "No entries present." );
+}
+END_TEST
+
+
+START_TEST( test_destroy_doesnt_crash )
+{
+	char *lines[] = {"127.0.0.1"};
+	struct acl * acl = acl_create( 1, lines, 0 );
+
+	acl_destroy( acl );
+}
+END_TEST
+
+
+START_TEST( test_includes_single_address )
+{
+	char *lines[] = {"127.0.0.1"};
+	struct acl * acl = acl_create( 1, lines, 0 );
+	union mysockaddr x;
+	
+	parse_ip_to_sockaddr( &x.generic, "127.0.0.1" );
+
+	fail_unless( acl_includes( acl, &x ), "Included address wasn't covered" );
+}
+END_TEST
+
+
+START_TEST( test_doesnt_include_other_address )
+{
+	char *lines[] = {"127.0.0.1"};
+	struct acl * acl  = acl_create( 1, lines, 0 );
+	union mysockaddr x;
+
+	parse_ip_to_sockaddr( &x.generic, "127.0.0.2" );
+	fail_if( acl_includes( acl, &x ), "Excluded address was covered." );
+}
+END_TEST
+
+
+START_TEST( test_default_deny_rejects )
+{
+	struct acl * acl = acl_create( 0, NULL, 1 );
+	union mysockaddr x;
+
+	parse_ip_to_sockaddr( &x.generic, "127.0.0.1" );
+
+	fail_if( acl_includes( acl, &x ), "Default deny accepted." );
+}
+END_TEST
+
+
+START_TEST( test_default_accept_rejects )
+{
+	struct acl * acl = acl_create( 0, NULL, 0 );
+	union mysockaddr x;
+
+	parse_ip_to_sockaddr( &x.generic, "127.0.0.1" );
+
+	fail_unless( acl_includes( acl, &x ), "Default accept rejected." );
+}
+END_TEST
+
+
+Suite* acl_suite()
+{
+	Suite *s = suite_create("acl");
+	TCase *tc_create = tcase_create("create");
+	TCase *tc_includes = tcase_create("includes");
+	TCase *tc_destroy = tcase_create("destroy");
+	
+	tcase_add_test(tc_create, test_null_acl);
+	tcase_add_test(tc_create, test_parses_single_line);
+
+	tcase_add_test(tc_includes, test_includes_single_address);
+	tcase_add_test(tc_includes, test_doesnt_include_other_address);
+	tcase_add_test(tc_includes, test_default_deny_rejects);
+	tcase_add_test(tc_includes, test_default_accept_rejects);
+
+	tcase_add_test(tc_destroy, test_destroy_doesnt_crash);
+
+	suite_add_tcase(s, tc_create);
+	suite_add_tcase(s, tc_includes);
+	suite_add_tcase(s, tc_destroy);
+
+	
+	return s;
+}
+
+int main(void)
+{
+	set_debug(1);
+	int number_failed;
+	Suite *s = acl_suite();
+	SRunner *sr = srunner_create(s);
+	srunner_run_all(sr, CK_NORMAL);
+	number_failed = srunner_ntests_failed(sr);
+	srunner_free(sr);
+	return (number_failed == 0) ? 0 : 1;
+}
+