diff mbox series

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

Message ID 20210224135255.253837-12-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show
Series qapi/qom: QAPIfy --object and object-add | expand

Commit Message

Kevin Wolf Feb. 24, 2021, 1:52 p.m. UTC
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(-)

Comments

Eric Blake Feb. 26, 2021, 7:33 p.m. UTC | #1
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>
Kevin Wolf March 2, 2021, 6:27 p.m. UTC | #2
Am 26.02.2021 um 20:33 hat Eric Blake geschrieben:
> 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().

We can't turn a no-op into an error without a deprecation period.

> > +
> > +##
> > +# @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...)

Probably not now then.

It also has the same problem as above, but I guess you could use the
deprecation period to build the required QAPI infrastructure. :-)

Kevin
diff mbox series

Patch

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' } }
+
+##
+# @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'] } } }
+
+##
+# @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' } }
+
+##
+# @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' } }
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'
   ] }
 
 ##
@@ -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'
   } }
 
 ##