[Qemu-devel] [PATCH] char: don't silently skip tn3270 protocol init when TLS is enabled

[Qemu-devel] [PATCH] char: don't silently skip tn3270 protocol init when TLS is enabled

[Daniel P. Berrange]

Even if common tn3270 implementations do not support TLS, it is trivial to
have them proxied over a proxy like stunnel which adds TLS at the sockets
layer. We should thus not silently skip tn3270 protocol initialization
when TLS is enabled.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 chardev/char-socket.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 53eda8ef00..6013972f72 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -623,8 +623,7 @@ static void tcp_chr_tls_handshake(QIOTask *task,
     if (qio_task_propagate_error(task, NULL)) {
         tcp_chr_disconnect(chr);
     } else {
-        /* tn3270 does not support TLS yet */
-        if (s->do_telnetopt && !s->is_tn3270) {
+        if (s->do_telnetopt) {
             tcp_chr_telnet_init(chr);
         } else {
             tcp_chr_connect(chr);
-- 
2.14.3

Re: [Qemu-devel] [PATCH] char: don't silently skip tn3270 protocol init when TLS is enabled

[Cornelia Huck]

On Thu, 21 Dec 2017 10:54:33 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> Even if common tn3270 implementations do not support TLS, it is trivial to
> have them proxied over a proxy like stunnel which adds TLS at the sockets
> layer. We should thus not silently skip tn3270 protocol initialization
> when TLS is enabled.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 53eda8ef00..6013972f72 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -623,8 +623,7 @@ static void tcp_chr_tls_handshake(QIOTask *task,
>      if (qio_task_propagate_error(task, NULL)) {
>          tcp_chr_disconnect(chr);
>      } else {
> -        /* tn3270 does not support TLS yet */
> -        if (s->do_telnetopt && !s->is_tn3270) {
> +        if (s->do_telnetopt) {
>              tcp_chr_telnet_init(chr);
>          } else {
>              tcp_chr_connect(chr);

With this patch applied, attaching an additional 3270 tty still works
for me as before.

I have no idea how to try this out with TLS, though. Would you be able
to run a sanity check? (The instructions in
https://wiki.qemu.org/Features/3270 work fine with a tcg s390x guest.)

I don't really expect problems, but I'm not too familiar with the QEMU
telnet code.

Re: [Qemu-devel] [PATCH] char: don't silently skip tn3270 protocol init when TLS is enabled

[Daniel P. Berrange]

On Thu, Dec 21, 2017 at 04:52:35PM +0100, Cornelia Huck wrote:
> On Thu, 21 Dec 2017 10:54:33 +0000
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > Even if common tn3270 implementations do not support TLS, it is trivial to
> > have them proxied over a proxy like stunnel which adds TLS at the sockets
> > layer. We should thus not silently skip tn3270 protocol initialization
> > when TLS is enabled.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  chardev/char-socket.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 53eda8ef00..6013972f72 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -623,8 +623,7 @@ static void tcp_chr_tls_handshake(QIOTask *task,
> >      if (qio_task_propagate_error(task, NULL)) {
> >          tcp_chr_disconnect(chr);
> >      } else {
> > -        /* tn3270 does not support TLS yet */
> > -        if (s->do_telnetopt && !s->is_tn3270) {
> > +        if (s->do_telnetopt) {
> >              tcp_chr_telnet_init(chr);
> >          } else {
> >              tcp_chr_connect(chr);
> 
> With this patch applied, attaching an additional 3270 tty still works
> for me as before.
> 
> I have no idea how to try this out with TLS, though. Would you be able
> to run a sanity check? (The instructions in
> https://wiki.qemu.org/Features/3270 work fine with a tcg s390x guest.)

NB, the x3270 emulator that's described in that page would not do TLS
in the manner than it's provided by QEMU. x3270 wants to start in plain
text mode, and then use STARTTLS command to negotiate use of TLS afterwards.

The QEMU chardev code doesn't work that way - it immediately activates
TLS when started, with no optional negotiation.

We could potentially add support for STARTTLS telent/tn3270 negotiation,
but that would be a separate mode from the TLS support we currently have
in QEMU chardevs.

I'm guessing the original author who added this check I'm removing was
mixing these two modes of operating, and hence mistakenly added this
check to stop 3270 init.

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 :|

Re: [Qemu-devel] [PATCH] char: don't silently skip tn3270 protocol init when TLS is enabled

[Cornelia Huck]

On Thu, 21 Dec 2017 16:19:04 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Thu, Dec 21, 2017 at 04:52:35PM +0100, Cornelia Huck wrote:
> > On Thu, 21 Dec 2017 10:54:33 +0000
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> >   
> > > Even if common tn3270 implementations do not support TLS, it is trivial to
> > > have them proxied over a proxy like stunnel which adds TLS at the sockets
> > > layer. We should thus not silently skip tn3270 protocol initialization
> > > when TLS is enabled.
> > > 
> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > ---
> > >  chardev/char-socket.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 53eda8ef00..6013972f72 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -623,8 +623,7 @@ static void tcp_chr_tls_handshake(QIOTask *task,
> > >      if (qio_task_propagate_error(task, NULL)) {
> > >          tcp_chr_disconnect(chr);
> > >      } else {
> > > -        /* tn3270 does not support TLS yet */
> > > -        if (s->do_telnetopt && !s->is_tn3270) {
> > > +        if (s->do_telnetopt) {
> > >              tcp_chr_telnet_init(chr);
> > >          } else {
> > >              tcp_chr_connect(chr);  
> > 
> > With this patch applied, attaching an additional 3270 tty still works
> > for me as before.
> > 
> > I have no idea how to try this out with TLS, though. Would you be able
> > to run a sanity check? (The instructions in
> > https://wiki.qemu.org/Features/3270 work fine with a tcg s390x guest.)  
> 
> NB, the x3270 emulator that's described in that page would not do TLS
> in the manner than it's provided by QEMU. x3270 wants to start in plain
> text mode, and then use STARTTLS command to negotiate use of TLS afterwards.
> 
> The QEMU chardev code doesn't work that way - it immediately activates
> TLS when started, with no optional negotiation.

Ah, OK. IIRC, z/VM usually uses STARTTLS.

> 
> We could potentially add support for STARTTLS telent/tn3270 negotiation,
> but that would be a separate mode from the TLS support we currently have
> in QEMU chardevs.
> 
> I'm guessing the original author who added this check I'm removing was
> mixing these two modes of operating, and hence mistakenly added this
> check to stop 3270 init.

Maybe add a sentence regarding STARTTLS vs. QEMU TLS support in the
patch description?

In any case, this does not seem to do any harm, so feel free to add my

Acked-by: Cornelia Huck <cohuck@redhat.com>