| Message ID | 20181116155325.22428-6-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc fixes to NBD | expand |
On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: > Add helpers to common.tls for creating TLS certificates for a CA, > server and client. MUCH appreciated! We NEED this coverage, easily automated. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ > 1 file changed, 139 insertions(+) > create mode 100644 tests/qemu-iotests/common.tls > > diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls > new file mode 100644 I was a bit surprised that this wasn't 100755, but this matches the fact that none of the other common.* are executable. And after thinking more, it makes sense - they aren't standalone scripts, but designed to be sourced, and 'source' doesn't care about execute bits. > +tls_dir="${TEST_DIR}/tls" > + > +function tls_x509_cleanup() > +{ > + rm -f ${tls_dir}/*.pem > + rm -f ${tls_dir}/*/*.pem > + rmdir ${tls_dir}/* > + rmdir ${tls_dir} Why not just: rm -rf $tls_dir Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, then all uses of ${tls_dir} need to be in "". > +} > + > + > +function tls_x509_init() > +{ > + mkdir "${tls_dir}" And this just highlights the quoting inconsistency. Should this use mkdir -p? > + > +function tls_x509_create_root_ca() > +{ > + name=$1 > + > + test -z "$name" && name=ca-cert Could also be shortened as: name=${1:-ca-cert} > + > + cat > ${tls_dir}/ca.info <<EOF > +cn = Cthulu Dark Lord Enterprises $name s/Cthulu/Cthulhu/ - after all, we don't want him coming after us just because we botched the spelling of his name :) > +ca > +cert_signing_key > +EOF > + > + certtool --generate-self-signed \ > + --load-privkey ${tls_dir}/key.pem \ > + --template ${tls_dir}/ca.info \ > + --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1 More missing "" > + > + rm -f ${tls_dir}/ca.info > +} > + > + > +function tls_x509_create_server() > +{ > + caname=$1 > + name=$2 > + > + mkdir ${tls_dir}/$name > + cat > ${tls_dir}/cert.info <<EOF > +organization = Cthulu Dark Lord Enterprises $name Matched spelling > +function tls_x509_create_client() > +{ > + caname=$1 > + name=$2 > + > + mkdir ${tls_dir}/$name > + cat > ${tls_dir}/cert.info <<EOF > +country = South Pacific > +locality = R'lyeh > +organization = Cthulu Dark Lord Enterprises $name And again Needs several touch-ups, but the idea itself is sound.
On Fri, Nov 16, 2018 at 10:39:03AM -0600, Eric Blake wrote: > On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: > > Add helpers to common.tls for creating TLS certificates for a CA, > > server and client. > > MUCH appreciated! We NEED this coverage, easily automated. > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 139 insertions(+) > > create mode 100644 tests/qemu-iotests/common.tls > > > > diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls > > new file mode 100644 > > I was a bit surprised that this wasn't 100755, but this matches the fact > that none of the other common.* are executable. And after thinking more, it > makes sense - they aren't standalone scripts, but designed to be sourced, > and 'source' doesn't care about execute bits. > > > +tls_dir="${TEST_DIR}/tls" > > + > > +function tls_x509_cleanup() > > +{ > > + rm -f ${tls_dir}/*.pem > > + rm -f ${tls_dir}/*/*.pem > > + rmdir ${tls_dir}/* > > + rmdir ${tls_dir} > > Why not just: > rm -rf $tls_dir Yeah, I guess we could do that for simplicity > Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, > then all uses of ${tls_dir} need to be in "". Hmm, yes. > > +} > > + > > + > > +function tls_x509_init() > > +{ > > + mkdir "${tls_dir}" > > And this just highlights the quoting inconsistency. Should this use mkdir > -p? I assume $TEST_DIR would already exist, so wouldn't need -p. > > + > > +function tls_x509_create_root_ca() > > +{ > > + name=$1 > > + > > + test -z "$name" && name=ca-cert > > Could also be shortened as: > > name=${1:-ca-cert} ok > > + > > + cat > ${tls_dir}/ca.info <<EOF > > +cn = Cthulu Dark Lord Enterprises $name > > s/Cthulu/Cthulhu/ - after all, we don't want him coming after us just > because we botched the spelling of his name :) > > > +ca > > +cert_signing_key > > +EOF > > + > > + certtool --generate-self-signed \ > > + --load-privkey ${tls_dir}/key.pem \ > > + --template ${tls_dir}/ca.info \ > > + --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1 > > More missing "" > > > + > > + rm -f ${tls_dir}/ca.info > > +} > > + > > + > > +function tls_x509_create_server() > > +{ > > + caname=$1 > > + name=$2 > > + > > + mkdir ${tls_dir}/$name > > + cat > ${tls_dir}/cert.info <<EOF > > +organization = Cthulu Dark Lord Enterprises $name > > Matched spelling > > > +function tls_x509_create_client() > > +{ > > + caname=$1 > > + name=$2 > > + > > + mkdir ${tls_dir}/$name > > + cat > ${tls_dir}/cert.info <<EOF > > +country = South Pacific > > +locality = R'lyeh > > +organization = Cthulu Dark Lord Enterprises $name > > And again > > Needs several touch-ups, but the idea itself is sound. Yes will fix Regards, Daniel
On 19.11.18 11:27, Daniel P. Berrangé wrote: > On Fri, Nov 16, 2018 at 10:39:03AM -0600, Eric Blake wrote: >> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote: >>> Add helpers to common.tls for creating TLS certificates for a CA, >>> server and client. >> >> MUCH appreciated! We NEED this coverage, easily automated. >> >>> >>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >>> --- >>> tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 139 insertions(+) >>> create mode 100644 tests/qemu-iotests/common.tls >>> >>> diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls >>> new file mode 100644 >> >> I was a bit surprised that this wasn't 100755, but this matches the fact >> that none of the other common.* are executable. And after thinking more, it >> makes sense - they aren't standalone scripts, but designed to be sourced, >> and 'source' doesn't care about execute bits. >> >>> +tls_dir="${TEST_DIR}/tls" >>> + >>> +function tls_x509_cleanup() >>> +{ >>> + rm -f ${tls_dir}/*.pem >>> + rm -f ${tls_dir}/*/*.pem >>> + rmdir ${tls_dir}/* >>> + rmdir ${tls_dir} >> >> Why not just: >> rm -rf $tls_dir > > Yeah, I guess we could do that for simplicity > >> Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, >> then all uses of ${tls_dir} need to be in "". > > Hmm, yes. Which by the way is a very good reason *not* to blindly use "rm -r". So far we only seem to have one instance of "rm -r" in the iotests (and that is on three files, so I don't even know why that has -r), and I'm glad about that. I prefer for scripts to only delete what they have created and not blindly delete something. Wildcards are already kind of pushing it. Maybe the user has created the tls dir beforehand, then I'd prefer for the iotests not to just delete it and everything in it. But the worst of course would be if we get escaping wrong somewhere (as demonstrated here) and suddenly we delete a completely unrelated directory by accident. Anyone remember Steam's 'rm -rf "$STEAMROOT"/*'? Everyone knows they have to be careful with deleting things, but most of the time it is a bother if you're in an interactive shell and know your directory structure and all the arguments you're passing perfectly well. But a script doesn't know either, and "it's a bother" is not really an argument if you have to write the code just once. Max
On 11/19/18 5:04 AM, Max Reitz wrote: >>>> +tls_dir="${TEST_DIR}/tls" >>>> + >>>> +function tls_x509_cleanup() >>>> +{ >>>> + rm -f ${tls_dir}/*.pem >>>> + rm -f ${tls_dir}/*/*.pem >>>> + rmdir ${tls_dir}/* >>>> + rmdir ${tls_dir} >>> >>> Why not just: >>> rm -rf $tls_dir >> >> Yeah, I guess we could do that for simplicity >> >>> Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, >>> then all uses of ${tls_dir} need to be in "". >> >> Hmm, yes. > > Which by the way is a very good reason *not* to blindly use "rm -r". If we ever revive Jeff Cody's patches to let each test run with its own temporary directory, then we don't need this function at all. With that in place, you either run the testsuite in debug mode (all temporary files preserved) or in normal mode (./check itself does rm -rf on the temporary directory). > > So far we only seem to have one instance of "rm -r" in the iotests (and > that is on three files, so I don't even know why that has -r), and I'm > glad about that. But until we have the global implementation of per-test temporary directories with cleanup relegated to the testsuite driver, I'm fine following your preference of explicit deletion of specific files rather than recursive deletion of the tree, even if it is more lines of code.
On Mon, Nov 19, 2018 at 08:27:56AM -0600, Eric Blake wrote: > On 11/19/18 5:04 AM, Max Reitz wrote: > > > > > > +tls_dir="${TEST_DIR}/tls" > > > > > + > > > > > +function tls_x509_cleanup() > > > > > +{ > > > > > + rm -f ${tls_dir}/*.pem > > > > > + rm -f ${tls_dir}/*/*.pem > > > > > + rmdir ${tls_dir}/* > > > > > + rmdir ${tls_dir} > > > > > > > > Why not just: > > > > rm -rf $tls_dir > > > > > > Yeah, I guess we could do that for simplicity > > > > > > > Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces, > > > > then all uses of ${tls_dir} need to be in "". > > > > > > Hmm, yes. > > > > Which by the way is a very good reason *not* to blindly use "rm -r". > > If we ever revive Jeff Cody's patches to let each test run with its own > temporary directory, then we don't need this function at all. With that in > place, you either run the testsuite in debug mode (all temporary files > preserved) or in normal mode (./check itself does rm -rf on the temporary > directory). That would be very nice - i'm often just commenting out the rm lines to preserve temp files. Regards, Daniel
diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls new file mode 100644 index 0000000000..6178ca5764 --- /dev/null +++ b/tests/qemu-iotests/common.tls @@ -0,0 +1,139 @@ +#!/bin/bash +# +# Helpers for TLS related config +# +# Copyright (C) 2018 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +tls_dir="${TEST_DIR}/tls" + +function tls_x509_cleanup() +{ + rm -f ${tls_dir}/*.pem + rm -f ${tls_dir}/*/*.pem + rmdir ${tls_dir}/* + rmdir ${tls_dir} +} + + +function tls_x509_init() +{ + mkdir "${tls_dir}" + + # use a fixed key so we don't waste system entropy on + # each test run + cat > ${tls_dir}/key.pem <<EOF +-----BEGIN PRIVATE KEY----- +MIICdQIBADANBgkqhkiG9w0BAQEFAASCAl8wggJbAgEAAoGBALVcr +BL40Tm6yq88FBhJNw1aaoCjmtg0l4dWQZ/e9Fimx4ARxFpT+ji4FE +Cgl9s/SGqC+1nvlkm9ViSo0j7MKDbnDB+VRHDvMAzQhA2X7e8M0n9 +rPolUY2lIVC83q0BBaOBkCj2RSmT2xTEbbC2xLukSrg2WP/ihVOxc +kXRuyFtzAgMBAAECgYB7slBexDwXrtItAMIH6m/U+LUpNe0Xx48OL +IOn4a4whNgO/o84uIwygUK27ZGFZT0kAGAk8CdF9hA6ArcbQ62s1H +myxrUbF9/mrLsQw1NEqpuUk9Ay2Tx5U/wPx35S3W/X2AvR/ZpTnCn +2q/7ym9fyiSoj86drD7BTvmKXlOnOwQJBAPOFMp4mMa9NGpGuEssO +m3Uwbp6lhcP0cA9MK+iOmeANpoKWfBdk5O34VbmeXnGYWEkrnX+9J +bM4wVhnnBWtgBMCQQC+qAEmvwcfhauERKYznMVUVksyeuhxhCe7EK +mPh+U2+g0WwdKvGDgO0PPt1gq0ILEjspMDeMHVdTwkaVBo/uMhAkA +Z5SsZyCP2aTOPFDypXRdI4eqRcjaEPOUBq27r3uYb/jeboVb2weLa +L1MmVuHiIHoa5clswPdWVI2y0em2IGoDAkBPSp/v9VKJEZabk9Frd +a+7u4fanrM9QrEjY3KhduslSilXZZSxrWjjAJPyPiqFb3M8XXA26W +nz1KYGnqYKhLcBAkB7dt57n9xfrhDpuyVEv+Uv1D3VVAhZlsaZ5Pp +dcrhrkJn2sa/+O8OKvdrPSeeu/N5WwYhJf61+CPoenMp7IFci +-----END PRIVATE KEY----- +EOF +} + + +function tls_x509_create_root_ca() +{ + name=$1 + + test -z "$name" && name=ca-cert + + cat > ${tls_dir}/ca.info <<EOF +cn = Cthulu Dark Lord Enterprises $name +ca +cert_signing_key +EOF + + certtool --generate-self-signed \ + --load-privkey ${tls_dir}/key.pem \ + --template ${tls_dir}/ca.info \ + --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1 + + rm -f ${tls_dir}/ca.info +} + + +function tls_x509_create_server() +{ + caname=$1 + name=$2 + + mkdir ${tls_dir}/$name + cat > ${tls_dir}/cert.info <<EOF +organization = Cthulu Dark Lord Enterprises $name +cn = localhost +dns_name = localhost +dns_name = localhost.localdomain +ip_address = 127.0.0.1 +ip_address = ::1 +tls_www_server +encryption_key +signing_key +EOF + + certtool --generate-certificate \ + --load-ca-privkey ${tls_dir}/key.pem \ + --load-ca-certificate ${tls_dir}/$caname-cert.pem \ + --load-privkey ${tls_dir}/key.pem \ + --template ${tls_dir}/cert.info \ + --outfile ${tls_dir}/$name/server-cert.pem 2>&1 | head -1 + ln -s ${tls_dir}/$caname-cert.pem ${tls_dir}/$name/ca-cert.pem + ln -s ${tls_dir}/key.pem ${tls_dir}/$name/server-key.pem + + rm -f ${tls_dir}/cert.info +} + + +function tls_x509_create_client() +{ + caname=$1 + name=$2 + + mkdir ${tls_dir}/$name + cat > ${tls_dir}/cert.info <<EOF +country = South Pacific +locality = R'lyeh +organization = Cthulu Dark Lord Enterprises $name +cn = localhost +tls_www_client +encryption_key +signing_key +EOF + + certtool --generate-certificate \ + --load-ca-privkey ${tls_dir}/key.pem \ + --load-ca-certificate ${tls_dir}/$caname-cert.pem \ + --load-privkey ${tls_dir}/key.pem \ + --template ${tls_dir}/cert.info \ + --outfile ${tls_dir}/$name/client-cert.pem 2>&1 | head -1 + ln -s ${tls_dir}/$caname-cert.pem ${tls_dir}/$name/ca-cert.pem + ln -s ${tls_dir}/key.pem ${tls_dir}/$name/client-key.pem + + rm -f ${tls_dir}/cert.info +}
Add helpers to common.tls for creating TLS certificates for a CA, server and client. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 tests/qemu-iotests/common.tls