Re: [PATCH v2 11/31] qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'

[Eric Blake]

On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the tls-* objects.
> 
> The 'loaded' property doesn't seem to make sense as an external
> interface: It is automatically set to true in ucc->complete, and
> explicitly setting it to true earlier just means that additional options
> will be silently ignored.
> 
> In other words, the 'loaded' property is useless. Mark it as deprecated
> in the schema from the start.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/crypto.json | 98 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/qom.json    | 12 +++++-
>  2 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/crypto.json b/qapi/crypto.json
> index 0fef3de66d..7116ae9a46 100644
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -442,3 +442,101 @@
>  { 'struct': 'SecretKeyringProperties',
>    'base': 'SecretCommonProperties',
>    'data': { 'serial': 'int32' } }
> +
> +##
> +# @TlsCredsProperties:
> +#
> +# Properties for objects of classes derived from tls-creds.
> +#
> +# @verify-peer: if true the peer credentials will be verified once the
> +#               handshake is completed.  This is a no-op for anonymous
> +#               credentials. (default: true)
> +#
> +# @dir: the path of the directory that contains the credential files
> +#
> +# @endpoint: whether the QEMU network backend that uses the credentials will 
> be
> +#            acting as a client or as a server (default: client)
> +#
> +# @priority: a gnutls priority string as described at
> +#            https://gnutls.org/manual/html_node/Priority-Strings.html
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'TlsCredsProperties',
> +  'data': { '*verify-peer': 'bool',
> +            '*dir': 'str',
> +            '*endpoint': 'QCryptoTLSCredsEndpoint',
> +            '*priority': 'str' } }

Matches crypto/tlscreds.c:qcrypto_tls_creds_class_init().

> +
> +##
> +# @TlsCredsAnonProperties:
> +#
> +# Properties for tls-creds-anon objects.
> +#
> +# @loaded: if true, the credentials are loaded immediately when applying this
> +#          option and will ignore options that are processed later. Don't 
> use;
> +#          only provided for compatibility. (default: false)
> +#
> +# Features:
> +# @deprecated: Member @loaded is deprecated.  Setting true doesn't make 
> sense,
> +#              and false is already the default.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'TlsCredsAnonProperties',
> +  'base': 'TlsCredsProperties',
> +  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] } } }

Since we documented that 'verify-peer' is a no-op for this struct, is it
worth altering our type hierarchy to make it explicit, as in:

TlsCredsCommonProperties - dir, endpoint, priority
TlsCredsProperties - TlsCredsCommonProperties + verify-peer
TlsCredsAnonProperties - TlsCredsCommonProperties + loaded
TlsCredsPskProperties - TlsCredsProperties + loaded, username

But even if not, this matches
crypto/tlscredsanon.c:qcrypto_tls_creds_anon_class_init().

> +
> +##
> +# @TlsCredsPskProperties:
> +#
> +# Properties for tls-creds-psk objects.
> +#
> +# @loaded: if true, the credentials are loaded immediately when applying this
> +#          option and will ignore options that are processed later. Don't 
> use;
> +#          only provided for compatibility. (default: false)
> +#
> +# @username: the username which will be sent to the server.  For clients 
> only.
> +#            If absent, "qemu" is sent and the property will read back as an
> +#            empty string.
> +#
> +# Features:
> +# @deprecated: Member @loaded is deprecated.  Setting true doesn't make 
> sense,
> +#              and false is already the default.
> +#
> +# Since: 3.0
> +##
> +{ 'struct': 'TlsCredsPskProperties',
> +  'base': 'TlsCredsProperties',
> +  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
> +            '*username': 'str' } }

This matches crypto/tlscredspsk.c:qcrypto_tls_creds_psk_class_init().

Do we want to use QAPI type inheritance to declare a union where
'endpoint' is the union discriminator, and 'username' is only present
for 'endpoint':'client'?  (Hmm, we'd have to improve the QAPI code
generator to allow a flat union as the branch of yet another flat union...)

> +
> +##
> +# @TlsCredsX509Properties:
> +#
> +# Properties for tls-creds-x509 objects.
> +#
> +# @loaded: if true, the credentials are loaded immediately when applying this
> +#          option and will ignore options that are processed later. Don't 
> use;
> +#          only provided for compatibility. (default: false)
> +#
> +# @sanity-check: if true, perform some sanity checks before using the
> +#                credentials (default: true)
> +#
> +# @passwordid: For the server-key.pem and client-key.pem files which contain
> +#              sensitive private keys, it is possible to use an encrypted
> +#              version by providing the @passwordid parameter.  This provides
> +#              the ID of a previously created secret object containing the
> +#              password for decryption.
> +#
> +# Features:
> +# @deprecated: Member @loaded is deprecated.  Setting true doesn't make 
> sense,
> +#              and false is already the default.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'TlsCredsX509Properties',
> +  'base': 'TlsCredsProperties',
> +  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
> +            '*sanity-check': 'bool',
> +            '*passwordid': 'str' } }

This matches crypto/tlscredsx509.c:qcrypto_tls_creds_x509_class_init().

> diff --git a/qapi/qom.json b/qapi/qom.json
> index 2668ad8369..f22b7aa99b 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -452,7 +452,11 @@
>      'rng-random',
>      'secret',
>      'secret_keyring',
> -    'throttle-group'
> +    'throttle-group',
> +    'tls-creds-anon',
> +    'tls-creds-psk',
> +    'tls-creds-x509',
> +    'tls-cipher-suites'

Matches crypto/tls-cipher-suites.c:qcrypto_tls_cipher_suites_class_init().

>    ] }
>  
>  ##
> @@ -488,7 +492,11 @@
>        'rng-random':                 'RngRandomProperties',
>        'secret':                     'SecretProperties',
>        'secret_keyring':             'SecretKeyringProperties',
> -      'throttle-group':             'ThrottleGroupProperties'
> +      'throttle-group':             'ThrottleGroupProperties',
> +      'tls-creds-anon':             'TlsCredsAnonProperties',
> +      'tls-creds-psk':              'TlsCredsPskProperties',
> +      'tls-creds-x509':             'TlsCredsX509Properties',
> +      'tls-cipher-suites':          'TlsCredsProperties'

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org