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

[Kashyap Chamarthy]

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

[...]

> address@hidden vnc_setup_sasl
> +
> address@hidden 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
> +
> address@hidden
> +mech_list: scram-sha-1
> +sasldb_path: /etc/qemu/passwd.db
> address@hidden 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/

[...]

> address@hidden tls_creds_setup
> address@hidden TLS x509 credential configuration
>  
> address@hidden 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
> address@hidden 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.)

  
> address@hidden 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},
> address@hidden, @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.

> address@hidden, which can be created using the
> address@hidden --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 <address@hidden>

-- 
/kashyap