[WIP] Do not truncate existing files when saving keys

dst_key_tofile() and dst__privstruct_writefile() open their destination
files using fopen() with mode "w", which causes the opened file to be
truncated.  These functions are used by certain DNSSEC utilities which
work on existing files, e.g. dnssec-settime.  If such a utility is used
to modify a key file just as named tries to read it, the latter may
consider the file to be empty and abort or postpone the operation in
progress.  To prevent this, make the aforementioned functions write the
key to a temporary file first (in the same directory as the destination
file) and then move that temporary file to its final location, making it
impossible for the key file to become empty at any point.

This change means that DNSSEC utilities working on existing key files
now require the directory containing the modified key files to be
writable.

TODO:

  - CHANGES note
  - perhaps needs a release note?
  - perhaps needs an ARM update?

bin/tests/system/dnssec/clean.sh

@@ -11,6 +11,11 @@
 
 set -e
 
+if [ -d ./settime-keys ]; then
+	chmod 755 ./settime-keys/test*
+	rm -rf ./settime-keys
+fi
+
 rm -f ./*/K* ./*/keyset-* ./*/dsset-* ./*/dlvset-* ./*/signedkey-* ./*/*.signed
 rm -f ./*/example.bk
 rm -f ./*/named.conf
@@ -95,6 +100,7 @@ rm -f ./ns7/split-rrsig.db ./ns7/split-rrsig.db.unsplit
 rm -f ./nsupdate.out*
 rm -f ./python.out.*
 rm -f ./rndc.out.*
+rm -f ./settime.out.*
 rm -f ./signer/*.db
 rm -f ./signer/*.signed.post*
 rm -f ./signer/*.signed.pre*

bin/tests/system/dnssec/tests.sh

@@ -3192,6 +3192,32 @@ n=$((n+1))
 test "$ret" -eq 0 || echo_i "failed"
 status=$((status+ret))
 
+echo_i "checking that dnssec-settime does not work with a non-writable key directory ($n)"
+ret=0
+keydir="settime-keys/test$n"
+mkdir -p "$keydir" || ret=1
+key=$($KEYGEN -K "$keydir" -q -a "$DEFAULT_ALGORITHM" -b "$DEFAULT_BITS" -P 19990101000000 .) || ret=1
+chmod 555 "$keydir" || ret=1
+$SETTIME -K "$keydir" -P 20000101000000 "$key" > settime.out.test$n 2>&1 && ret=1
+grep "20000101000000" "$keydir/$key.key" > /dev/null 2>&1 && ret=1
+grep "20000101000000" "$keydir/$key.private" > /dev/null 2>&1 && ret=1
+n=$((n+1))
+if [ "$ret" -ne 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+echo_i "checking that dnssec-settime works with a writable key directory ($n)"
+ret=0
+keydir="settime-keys/test$n"
+mkdir -p "$keydir" || ret=1
+key=$($KEYGEN -K "$keydir" -q -a "$DEFAULT_ALGORITHM" -b "$DEFAULT_BITS" -P 19990101000000 .) || ret=1
+chmod 755 "$keydir" || ret=1
+$SETTIME -K "$keydir" -P 20000101000000 "$key" > settime.out.test$n 2>&1 || ret=1
+grep "20000101000000" "$keydir/$key.key" > /dev/null 2>&1 || ret=1
+grep "20000101000000" "$keydir/$key.private" > /dev/null 2>&1 || ret=1
+n=$((n+1))
+if [ "$ret" -ne 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
 echo_i "check that CDS records are signed using KSK by dnssec-signzone ($n)"
 ret=0
 dig_with_opts +noall +answer @10.53.0.2 cds cds.secure > dig.out.test$n

lib/dns/dst_api.c

@@ -1540,6 +1540,7 @@ write_public_key(const dst_key_t *key, int type, const char *directory) {
 	isc_buffer_t keyb, textb, fileb, classb;
 	isc_region_t r;
 	char filename[NAME_MAX];
+	char tempname[NAME_MAX + 14];	/* see isc_file_mktemplate() docs */
 	unsigned char key_array[DST_KEY_MAXSIZE];
 	char text_array[DST_KEY_MAXTEXTSIZE];
 	char class_array[10];
@@ -1579,15 +1580,21 @@ write_public_key(const dst_key_t *key, int type, const char *directory) {
 	/*
 	 * Create public key file.
 	 */
-	if ((fp = fopen(filename, "w")) == NULL)
+	ret = isc_file_mktemplate(filename, tempname, sizeof(tempname));
+	if (ret != ISC_R_SUCCESS) {
+		return (ret);
+	}
+
+	if ((fp = fopen(tempname, "w")) == NULL) {
 		return (DST_R_WRITEERROR);
+	}
 
 	if (issymmetric(key)) {
 		access = 0;
 		isc_fsaccess_add(ISC_FSACCESS_OWNER,
 				 ISC_FSACCESS_READ | ISC_FSACCESS_WRITE,
 				 &access);
-		(void)isc_fsaccess_set(filename, access);
+		(void)isc_fsaccess_set(tempname, access);
 	}
 
 	/* Write key information in comments */
@@ -1643,6 +1650,12 @@ write_public_key(const dst_key_t *key, int type, const char *directory) {
 		ret = DST_R_WRITEERROR;
 	fclose(fp);
 
+	if (ret != ISC_R_SUCCESS) {
+		(void)isc_file_remove(tempname);
+	} else {
+		ret = isc_file_rename(tempname, filename);
+	}
+
 	return (ret);
 }
 

lib/dns/dst_parse.c

@@ -596,6 +596,7 @@ dst__privstruct_writefile(const dst_key_t *key, const dst_private_t *priv,
 	FILE *fp;
 	isc_result_t result;
 	char filename[NAME_MAX];
+	char tempname[NAME_MAX + 14];	/* see isc_file_mktemplate() docs */
 	char buffer[MAXFIELDSIZE * 2];
 	isc_fsaccess_t access;
 	isc_stdtime_t when;
@@ -639,14 +640,20 @@ dst__privstruct_writefile(const dst_key_t *key, const dst_private_t *priv,
 			      filename, (unsigned int)mode);
 	}
 
-	if ((fp = fopen(filename, "w")) == NULL)
+	result = isc_file_mktemplate(filename, tempname, sizeof(tempname));
+	if (result != ISC_R_SUCCESS) {
+		return (result);
+	}
+
+	if ((fp = fopen(tempname, "w")) == NULL) {
 		return (DST_R_WRITEERROR);
+	}
 
 	access = 0;
 	isc_fsaccess_add(ISC_FSACCESS_OWNER,
 			 ISC_FSACCESS_READ | ISC_FSACCESS_WRITE,
 			 &access);
-	(void)isc_fsaccess_set(filename, access);
+	(void)isc_fsaccess_set(tempname, access);
 
 	dst_key_getprivateformat(key, &major, &minor);
 	if (major == 0 && minor == 0) {
@@ -762,6 +769,13 @@ dst__privstruct_writefile(const dst_key_t *key, const dst_private_t *priv,
 	fflush(fp);
 	result = ferror(fp) ? DST_R_WRITEERROR : ISC_R_SUCCESS;
 	fclose(fp);
+
+	if (result != ISC_R_SUCCESS) {
+		(void)isc_file_remove(tempname);
+	} else {
+		result = isc_file_rename(tempname, filename);
+	}
+
 	return (result);
 }