From cbc5c4b51fcd0a990216fc31abe308a9e85fd9df Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Wed, 17 Jun 2020 12:23:46 -0500 Subject: [PATCH 1/1] tests: Modernize writekeyfile.c tests/auth/writekeyfile.c contains some code used to generate tests/auth/KeyFile, which is used to test code interpreting the old-style KeyFile format. This code currently has a few problems: - We don't check the results of afstest_mkdtemp, which could allow symlink attacks from other users on the system. - We duplicate some logic from afstest_BuildTestConfig, in order to build a temporary config dir. - writekeyfile isn't built or run by default (it only exists to generate KeyFile, so it's almost never run), so eventual bitrot is quite likely, and the existing code already generates warnings. To avoid this, change writekeyfile.c to use the existing afstest_BuildTestConfig to generate a local config dir. To ensure we avoid bitrot, build writekeyfile by default, and create a test to run it, to make sure it can generate a KeyFile as expected. Note that the KeyFile.short we test against is different than the KeyFile currently in the tree. The existing KeyFile was generated from an older OpenAFS release, which always generated 100-byte KeyFiles, even if we only have a few keys. The current codebase only writes out as much key data as needed, so the generated KeyFiles are shorter (but still understandable by older OpenAFS releases). Keep the old 100-byte KeyFile around, since that's what older OpenAFS would generate, and create a new KeyFile.short to test against, to make sure our code for generating KeyFiles doesn't change any further. Change-Id: Ibe9246c6dd808ed2b2225dd7be2b27bbdee072fd Reviewed-on: https://gerrit.openafs.org/14246 Reviewed-by: Cheyenne Wills Reviewed-by: Benjamin Kaduk Tested-by: BuildBot --- tests/TESTS | 1 + tests/auth/KeyFile.short | Bin 0 -> 40 bytes tests/auth/Makefile.in | 8 +++---- tests/auth/writekeyfile.c | 57 +++++++++++++++++----------------------------- tests/auth/writeoldkey-t | 47 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 40 deletions(-) create mode 100644 tests/auth/KeyFile.short create mode 100755 tests/auth/writeoldkey-t diff --git a/tests/TESTS b/tests/TESTS index 654b7af..72f1158 100644 --- a/tests/TESTS +++ b/tests/TESTS @@ -5,6 +5,7 @@ auth/keys auth/superuser auth/authcon auth/realms +auth/writeoldkey cmd/command opr/cache opr/dict diff --git a/tests/auth/KeyFile.short b/tests/auth/KeyFile.short new file mode 100644 index 0000000000000000000000000000000000000000..6e5801e0298ccfa9cb3e9a13aaae3a530fbca85b GIT binary patch literal 40 ncmZQzU|?ooU|?iqV&M=_aA*LsnOL9z$Yzlg`}b^Dx&B`OE^q~! literal 0 HcmV?d00001 diff --git a/tests/auth/Makefile.in b/tests/auth/Makefile.in index 25964d9..aa89f75 100644 --- a/tests/auth/Makefile.in +++ b/tests/auth/Makefile.in @@ -4,7 +4,7 @@ abs_top_builddir=@abs_top_builddir@ include @TOP_OBJDIR@/src/config/Makefile.config include @TOP_OBJDIR@/src/config/Makefile.pthread -TESTS = authcon-t superuser-t keys-t realms-t +TESTS = authcon-t superuser-t keys-t realms-t writekeyfile MODULE_CFLAGS=-I$(TOP_OBJDIR) -I$(srcdir)/../common/ @@ -33,8 +33,8 @@ keys-t: keys-t.o ../common/config.o ../common/network.o realms-t: realms-t.o ../common/config.o ../common/network.o $(LT_LDRULE_static) realms-t.o ../common/config.o ../common/network.o $(MODULE_LIBS) -writekeyfile: writekeyfile.o - $(LT_LDRULE_static) writekeyfile.o $(MODULE_LIBS) +writekeyfile: writekeyfile.o ../common/config.o + $(LT_LDRULE_static) writekeyfile.o ../common/config.o $(MODULE_LIBS) test.cs.c: test.xg $(RXGEN) -A -x -C -o $@ $(srcdir)/test.xg @@ -53,4 +53,4 @@ superuser-t.o: test.h clean: $(LT_CLEAN) rm -f *.o *.cs.c *.ss.c *.xdr.c test.h \ - writekeyfile $(TESTS) + $(TESTS) diff --git a/tests/auth/writekeyfile.c b/tests/auth/writekeyfile.c index a622ce0..4a07e61 100644 --- a/tests/auth/writekeyfile.c +++ b/tests/auth/writekeyfile.c @@ -8,40 +8,29 @@ #include #include #include +#include #include +#include "common.h" + int main(int argc, char **argv) { struct afsconf_dir *dir; - char buffer[1024]; + char *dirname; char *block; - char *dirEnd; - FILE *file; + char *keyfile = NULL; int in, out; size_t len; - int code; - - snprintf(buffer, sizeof(buffer), "%s/afs_XXXXXX", gettmpdir()); - afstest_mkdtemp(buffer); - dirEnd = buffer + strlen(buffer); - - /* Create a CellServDB file */ - strcpy(dirEnd, "/CellServDB"); - file = fopen(buffer, "w"); - fprintf(file, ">example.org # An example cell\n"); - fprintf(file, "127.0.0.1 #test.example.org\n"); - fclose(file); - /* Create a ThisCell file */ - strcpy(dirEnd, "/ThisCell"); - file = fopen(buffer, "w"); - fprintf(file, "example.org\n"); - fclose(file); + dirname = afstest_BuildTestConfig(); + if (dirname == NULL) { + fprintf(stderr, "Unable to create tmp config dir\n"); + exit(1); + } - *dirEnd='\0'; - dir = afsconf_Open(strdup(buffer)); + dir = afsconf_Open(dirname); if (dir == NULL) { fprintf(stderr, "Unable to open configuration directory\n"); exit(1); @@ -54,15 +43,18 @@ main(int argc, char **argv) afsconf_Close(dir); /* Copy out the resulting keyfile into our homedirectory */ - strcpy(dirEnd, "/KeyFile"); - in = open(buffer, O_RDONLY); + opr_Verify(asprintf(&keyfile, "%s/KeyFile", dirname) > 0); + in = open(keyfile, O_RDONLY); out = open("KeyFile", O_WRONLY | O_CREAT, 0644); block = malloc(1024); do { len = read(in, block, 1024); - if (len > 0) - write(out, block, len); + if (len > 0) { + if (write(out, block, len) != len) { + len = -1; + } + } } while (len > 0); if (len == -1) { @@ -73,14 +65,7 @@ main(int argc, char **argv) close(in); close(out); - strcpy(dirEnd, "/KeyFile"); - unlink(buffer); - strcpy(dirEnd, "/CellServDB"); - unlink(buffer); - strcpy(dirEnd, "/ThisCell"); - unlink(buffer); - strcpy(dirEnd, "/UserList"); - unlink(buffer); - *dirEnd='\0'; - rmdir(buffer); + afstest_UnlinkTestConfig(dirname); + + return 0; } diff --git a/tests/auth/writeoldkey-t b/tests/auth/writeoldkey-t new file mode 100755 index 0000000..1238dce --- /dev/null +++ b/tests/auth/writeoldkey-t @@ -0,0 +1,47 @@ +#!/usr/bin/env perl + +use strict; +use warnings; +use Test::More; +use File::Temp qw/tempdir/; +use FindBin qw($Bin); +use Cwd qw/abs_path/; +use File::Compare; +use Sys::Hostname; +use Socket; + +# Run tests/auth/writekeyfile, and check that the KeyFile that it generates +# matches what we expect. + +if (!defined(gethostbyname(hostname()))) { + # writekeyfile needs a hostname to generate a config dir + plan skip_all => 'Cannot resolve hostname'; +} +plan tests => 1; + +my $cmd; +if (defined($ENV{BUILD})) { + $cmd = $ENV{BUILD} . "/auth/writekeyfile"; +} else { + $cmd = $Bin . "/writekeyfile"; +} +$cmd = abs_path($cmd); + +my $keyfile; +if (defined($ENV{SOURCE})) { + $keyfile = $ENV{SOURCE} . "/auth/KeyFile.short"; +} else { + $keyfile = $Bin . "/KeyFile.short"; +} +$keyfile = abs_path($keyfile); + +my $dir = tempdir('afs_XXXXXX', CLEANUP => 1); + +chdir($dir) + or die("chdir $dir failed: $?"); + +system($cmd) == 0 + or die("$cmd failed: $?"); + +ok(compare("KeyFile", $keyfile) == 0, + "writekeyfile generates expected KeyFile"); -- 1.9.4