Re: [PATCH v4 07/33] io: tls: Allow terminating the TLS session gracefully with EOF

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Feb 04, 2025 at 05:02:23PM +0100, Maciej S. Szmigiero wrote:
> On 4.02.2025 16:15, Daniel P. Berrangé wrote:
> > On Thu, Jan 30, 2025 at 11:08:28AM +0100, Maciej S. Szmigiero wrote:
> > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> > > 
> > > Currently, hitting EOF on receive without sender terminating the TLS
> > > session properly causes the TLS channel to return an error (unless
> > > the channel was already shut down for read).
> > > 
> > > Add an optional setting whether we instead just return EOF in that
> > > case.
> > > 
> > > This possibility will be soon used by the migration multifd code.
> > > 
> > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > > ---
> > >   include/io/channel-tls.h | 11 +++++++++++
> > >   io/channel-tls.c         |  6 ++++++
> > >   2 files changed, 17 insertions(+)
> > > 
> > > diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
> > > index 26c67f17e2d3..8552c0d0266e 100644
> > > --- a/include/io/channel-tls.h
> > > +++ b/include/io/channel-tls.h
> > > @@ -49,6 +49,7 @@ struct QIOChannelTLS {
> > >       QCryptoTLSSession *session;
> > >       QIOChannelShutdown shutdown;
> > >       guint hs_ioc_tag;
> > > +    bool premature_eof_okay;
> > >   };
> > >   /**
> > > @@ -143,4 +144,14 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc,
> > >   QCryptoTLSSession *
> > >   qio_channel_tls_get_session(QIOChannelTLS *ioc);
> > > +/**
> > > + * qio_channel_tls_set_premature_eof_okay:
> > > + * @ioc: the TLS channel object
> > > + *
> > > + * Sets whether receiving an EOF without terminating the TLS session properly
> > > + * by used the other side is considered okay or an error (the
> > > + * default behaviour).
> > > + */
> > > +void qio_channel_tls_set_premature_eof_okay(QIOChannelTLS *ioc, bool enabled);
> > > +
> > >   #endif /* QIO_CHANNEL_TLS_H */
> > > diff --git a/io/channel-tls.c b/io/channel-tls.c
> > > index aab630e5ae32..1079d6d10de1 100644
> > > --- a/io/channel-tls.c
> > > +++ b/io/channel-tls.c
> > > @@ -147,6 +147,11 @@ qio_channel_tls_new_client(QIOChannel *master,
> > >       return NULL;
> > >   }
> > > +void qio_channel_tls_set_premature_eof_okay(QIOChannelTLS *ioc, bool enabled)
> > > +{
> > > +    ioc->premature_eof_okay = enabled;
> > > +}
> > > +
> > >   struct QIOChannelTLSData {
> > >       QIOTask *task;
> > >       GMainContext *context;
> > > @@ -279,6 +284,7 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> > >               tioc->session,
> > >               iov[i].iov_base,
> > >               iov[i].iov_len,
> > > +            tioc->premature_eof_okay ||
> > >               qatomic_load_acquire(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ,
> > >               errp);
> > >           if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
> > 
> > IMHO a better way to do this is by defining an new flag for use with
> > the qio_channel_readv_full() method. That makes the ignoring of
> > premature shutdown a contextually scoped behaviour rather than a
> > global behaviour.
> 
> Something named like QIO_CHANNEL_READ_FLAG_TLS_EARLY_EOF_OKAY?

Since the flags are defined at the non-TLS layer in the API, I would
pick  "QIO_CHANNEL_READ_RELAXED_EOF", as it could conceptually make
sense to other layered channel protocols beyond TLS, even if we only
ever implement it for TLS.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|