Re: [Qemu-devel] [PATCH v2] docs: update information for TLS certificate management

[Kashyap Chamarthy <kchamart@redhat.com>]

On Thu, Jan 25, 2018 at 05:09:30PM +0000, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>

[...]

> +@node vnc_setup_sasl
> +
> +@subsection Configuring SASL mechanisms
> +
> +The following documentation assumes use of the Cyrus SASL implementation on a
> +Linux host, but the principals should apply to any other SASL implementation

s/principals/principles/

[I can imagine how the typo could've come about -- as you talk about
Kerberos principal(s) further below :-)]

> +or host. When SASL is enabled, the mechanism configuration will be loaded from
> +system default SASL service config /etc/sasl2/qemu.conf. If running QEMU as an
> +unprivileged user, an environment variable SASL_CONF_PATH can be used to make
> +behaviour suddenly changedit search alternate locations for the service config.

The last part above reads odd, maybe you accidentally removed some
words?

[...]

> +This says to use the 'GSSAPI' mechanism with the Kerberos v5 protocol, with
> +the server principal stored in /etc/qemu/krb5.tab. For this to work the
> +administrator of your KDC must generate a Kerberos principal for the server,
> +with a name of 'qemu/somehost.example.com@@EXAMPLE.COM' replacing
> +'somehost.example.com' with the fully qualified host name of the machine
> +running QEMU, and 'EXAMPLE.COM' with the Kerberos Realm.
> +
> +When using TLS, if username+password authentication is desired, then a
> +reasonable configuration is
> +
> +@example
> +mech_list: scram-sha-1
> +sasldb_path: /etc/qemu/passwd.db
> +@end example
> +
> +The saslpasswd2 program can be used to populate the passwd.db file with
> +accounts.

Maybe: "saslpasswd2" --> "@code{saslpasswd2}"

(Also helps with consistency, as I see you've used the @code{} attribute
further below.)

> +
> +Other SASL configurations will be left as an exercise for the reader. Note that
> +all mechanisms except GSSAPI, should be combined with use of TLS to ensure a

Spurious comma above.

> +secure data channel.

[...]

> +The GNUTLS package includes a command called @code{certtool} which can

Super nit: s/GNUTLS/GnuTLS/

[...]

> +@node tls_creds_setup
> +@subsection TLS x509 credential configuration
>  
> -@node vnc_setup_sasl
> +QEMU has a standard mechanism for loading x509 credentials that will be
> +used for network services and clients. It requires specifying the
> +@code{tls-creds-x509} class name to the @code{-object} command line
> +argument for the system emulators. This also works for the helper tools
> +like @code{qemu-nbd} and @code{qemu-img}, but is named @code{--object}.

The '-object' for QEMU command-line, and the double-dashed '--object'
for external tools looks a tiny bit odd.  But not this patch's problem.

(I just noticed Eric remarked on this too.)

  
> -@subsection Configuring SASL mechanisms
> +When specifying the object, the @code{dir} parameters specifies which
> +directory contains the credential files. This directory is expected to
> +contain files with the names mentioned previously, @code{ca-cert.pem},
> +@code{server-key.pem}, @code{server-cert.pem}, @code{client-key.pem}
> +and @code{client-cert.pem} as appropriate. It is also possible to
> +include a set of pre-generated diffie-hellman parameters in a file

Would be nice capitalize proper nouns: s/diffie-hellman/Diffie–Hellman
(DH) --- 'DH' in brackets because you use that acronym two lines below,
although it should be obvious to those setting up TLS.

> +@code{dh-params.pem}, which can be created using the
> +@code{certtool --generate-dh-params} command. If omitted, QEMU will
> +dynamically generate DH parameters when loading the credentials.

[...]

  
> -The saslpasswd2 program can be used to populate the passwd.db file with
> -accounts.
> +Network services which support TLS will all have a @code{tls-creds}
> +parameter which expects the ID of the tls credentials object. 

s/tls/TLS

[...]

Whether you fix these minor nits or not, your write-up is a strict
improvement, so:

    Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com>

-- 
/kashyap