Bug 1218959

Thank you for bringing this up. We will have a look.

I wonder whether it would be feasible to get the regular sshd log the required information. If not by default then maybe after sending it a custom signal. That way a separate pairing server would not be required.

https://github.com/openssh/openssh-portable/blob/0d96b1506b2f4757fefa5d1f884d49e96a6fd4c3/auth.c#L283

Raises debug level for password auth. With verbose level sshd also logs things like this:
Failed publickey for root from xxx port 44942 ssh2: RSA SHA256:hGfJuapGs0eil4aBt4ytwsFQ0hynUU5OpyAXeXzgGKU

# echo 'LogVerbose auth.c:auth_log():*' > /etc/ssh/sshd_config.d/verbose.conf

(In reply to Ludwig Nussel from comment #2)
> I wonder whether it would be feasible to get the regular sshd log the
> required information. If not by default then maybe after sending it a custom
> signal. That way a separate pairing server would not be required.

IMO that's a crude hack, log files are not usable API. I did look into that as well though for the initial PoC.

(In reply to Ludwig Nussel from comment #3)
> https://github.com/openssh/openssh-portable/blob/
> 0d96b1506b2f4757fefa5d1f884d49e96a6fd4c3/auth.c#L283
> 
> Raises debug level for password auth. With verbose level sshd also logs
> things like this:
> Failed publickey for root from xxx port 44942 ssh2: RSA
> SHA256:hGfJuapGs0eil4aBt4ytwsFQ0hynUU5OpyAXeXzgGKU
> 
> # echo 'LogVerbose auth.c:auth_log():*' > /etc/ssh/sshd_config.d/verbose.conf

That hash can be used for identifying keys, but not for adding them to authorized_keys. That needs the full pubkey in base64 ssh format.

The point is that sshd has the information, maybe we can find a way to get the data out of it without stopping sshd and using a separate service. That way pairing could be done at any point in time in production too. I don't think parsing well formed log messages are a bad approach for this purpose. Too bad sshd doesn't use the journal api. Anyway, maybe needs an option to sshd to log the full pubkey in format_method_key().

(In reply to Ludwig Nussel from comment #5)
> The point is that sshd has the information, maybe we can find a way to get
> the data out of it without stopping sshd and using a separate service.

Without special handling on the server side it wouldn't be possible to make this work securely. Currently the server handles a single connection only and the client gets a confirmation message. With sshd, it wouldn't be possible to tell from the client whether this particular connection was part of the pairing or some other connection in between got lucky. This is important though to ensure that the public keys to be imported are from the user's client.

> That
> way pairing could be done at any point in time in production too.

Technically it's possible to use a separate port, but then the known_hosts entry will be for that port and the actual connection will then start with

"Unknown key ... known as host:otherport. Accept?"

> I don't
> think parsing well formed log messages are a bad approach for this purpose.
> Too bad sshd doesn't use the journal api. Anyway, maybe needs an option to
> sshd to log the full pubkey in format_method_key().

Needing a modified sshd needs to be avoided. I very much doubt that such a change would get upstream. This method works everywhere with recent enough libssh.

I also believe that parsing logfiles is not a proper way to implement
production software - not in general and especially not when something as
sensitive as SSH is involved.

The ssh-pairing-server consists only of some 100 lines of C code so this
shouldn't be a big task. Looking at the complete integration is another step
though.

I will look into it.

So I added some review comments:

- via this "review PR#" for the ssh-pairing repo:
  https://github.com/Vogtinator/ssh-pairing/pull/1/commits/eb093e681041b21681ee9dcc36a3dacc88c7daf6
- in the draft PR# for jeos-firstboot:
  https://github.com/openSUSE/jeos-firstboot/pull/112#discussion_r1464690318

Apart from the specific code related comments I have one overarching design
concern:

What is happening here is that the to-be-authorized keys are transferred over
the same channel that needs to be authenticated with these authorized keys. So
security design wise this is a bit worrying without even looking at the
implementation details. On the other hand we know that the "second factor" for
online banking on smartphones is processed on the very same smartphone by many
banking apps these days, because it happens in supposed isolation. So there is
some change in what people deem appropriate in such scenarios these days.

Apart from this theoretical view point this fact that the keys are transferred
over the to-be-authenticated channel has some practical implications as well.
The client that offers its public keys to the server is not authenticated in
any way, so practically just anybody can try to connect to the server, and the
pairing-tool will list its public keys.

Thus to prevent any imposter client's keys to be added to the authorized keys
an additional step is needed in the workflow, where the fingerprint (ideally
visual host keys, random art image) of every added public key is displayed and
verified by the user on the server side. This needs to be made explicit and
the security implications / the importance of this step needs to be pointed
out to the end user.

Meanwhile all of the relevant code is in the ssh-pairing repo, the jeos-firstboot
PR only contains a tiny wrapper around the ssh-pairing tool.

(In reply to Matthias Gerstner from comment #9)
> So I added some review comments:
> 
> - via this "review PR#" for the ssh-pairing repo:
>  
> https://github.com/Vogtinator/ssh-pairing/pull/1/commits/
> eb093e681041b21681ee9dcc36a3dacc88c7daf6
> - in the draft PR# for jeos-firstboot:
>   https://github.com/openSUSE/jeos-firstboot/pull/112#discussion_r1464690318
> 
> Apart from the specific code related comments I have one overarching design
> concern:
> 
> What is happening here is that the to-be-authorized keys are transferred over
> the same channel that needs to be authenticated with these authorized keys.
> So
> security design wise this is a bit worrying without even looking at the
> implementation details.

The channel is supposed to be authenticated through host key validation, which
defends against MITM. Both the server and the client are supposed to be trusted.

> On the other hand we know that the "second factor"
> for
> online banking on smartphones is processed on the very same smartphone by
> many
> banking apps these days, because it happens in supposed isolation. So there
> is
> some change in what people deem appropriate in such scenarios these days.

Probably because convenience > security in many cases and they're told that
it's secure. It's effectively just 1.5 factors though, the phone + its authentication.

> Apart from this theoretical view point this fact that the keys are
> transferred
> over the to-be-authenticated channel has some practical implications as well.
> The client that offers its public keys to the server is not authenticated in
> any way, so practically just anybody can try to connect to the server, and
> the
> pairing-tool will list its public keys.
> 
> Thus to prevent any imposter client's keys to be added to the authorized keys
> an additional step is needed in the workflow, where the fingerprint (ideally
> visual host keys, random art image) of every added public key is displayed
> and
> verified by the user on the server side. This needs to be made explicit and
> the security implications / the importance of this step needs to be pointed
> out to the end user.

There's some discussion about that here: https://github.com/Vogtinator/ssh-pairing/pull/1#discussion_r1464816226
For completeness I'll mention it here as well:

ssh-pairing-server only accepts a single connection. If a potentially malicious
client connects before the actual client, the actual client will not be able
to connect to ssh-pairing-server anymore. This is visible by the lack of a
confirmation message on the client. If the message appears, it was definitely
sent by the correct host (validated through the host key).

The key import confirmation+selection dialog should make this clear meanwhile.

(In reply to fvogt@suse.com from comment #10)
> (In reply to Matthias Gerstner from comment #9)
> > What is happening here is that the to-be-authorized keys are transferred over
> > the same channel that needs to be authenticated with these authorized keys.
> > So
> > security design wise this is a bit worrying without even looking at the
> > implementation details.
> 
> The channel is supposed to be authenticated through host key validation, which
> defends against MITM. Both the server and the client are supposed to be trusted.

Yes the key exchange defends against MITM and the server host key verification
allows the client to verify that it is talking to the correct server. But not
the other way around. The server cannot tell at this stage if the client is to
be trusted.

> > Thus to prevent any imposter client's keys to be added to the authorized keys
> > an additional step is needed in the workflow, where the fingerprint (ideally
> > visual host keys, random art image) of every added public key is displayed
> > and
> > verified by the user on the server side. This needs to be made explicit and
> > the security implications / the importance of this step needs to be pointed
> > out to the end user.
> 
> There's some discussion about that here: https://github.com/Vogtinator/ssh-pairing/pull/1#discussion_r1464816226
> For completeness I'll mention it here as well:
> 
> ssh-pairing-server only accepts a single connection. If a potentially malicious
> client connects before the actual client, the actual client will not be able
> to connect to ssh-pairing-server anymore. This is visible by the lack of a
> confirmation message on the client. If the message appears, it was definitely
> sent by the correct host (validated through the host key).
> 
> The key import confirmation+selection dialog should make this clear meanwhile.

Yes, by now I better understand your design, that you are using the
keyboard-interactive auth reply as a verification channel back to the client.

The point that only one connection is accepted is certainly helpful but still
a bit shaky IMHO. You are relying on whatever `ssh_bind_accept(bind, session)`
from libssh is doing. From the apidoc all they say is this:

    Accept an incoming ssh connection and initialize the session.

There are no guarantees wrt to the requirements ssh-pairing-daemon has for
this public key exchange scheme. For example it could be possible that
multiple clients start a connection and receive the server's host key, and
`ssh_bind_accept()` only returns after a client accepted the host key and
progresses to the next protocol stages. This is not currently the case. But
libssh is a library, a kind of framework even, with some abstractions. And
these abstractions aren't necessarily tailored to what you are trying to do
here. The behaviour might change silently in the future.

The point is that there can be situations in-between, a rather large of
variables in this scheme, here are just a few items that come to my mind:

- another party accidentally of purposefully might connect to the
  ssh-pairing-daemon. The proper client will likely notice this, but it
  depends on a human factor. An unknown number of SSH public keys are now
  recorded on the server system. The user must now be able to properly
  identify this and recover from this situation.
- the legitimate client might not offer keyboard-interactive authentication
  for some reason, so the verification channel might be missing, which doesn't
  cause the workflow to abort currently. So this ends in a half-verified
  state that is undesirable.
- there are a number of interactive steps of high importance:
  - verification of the server's host key.
  - verification of the proper reply via the keyboard-interactive prompt.
  - ideally verification of the key fingerprint upon selection on the server
    side TUI.

In practice individual steps can easily break e.g. the end user accidentally
fails to accept the server's host key for some reason. Now the
ssh-pairing-daemon is gone. What happens in this case (on TUI level?). The
more goes wrong and the more the end user has to repeat, the more likely it
becomes (IMHO) that the end user gets careless. So you should go through all
possible error scenarios and check the outcome. Ideally the server side should
never record any state in error situations. Recovering from error situations
(starting over) should be simple and clean for the end user.

So in summary my main point is that you should eliminate as many of these
variables as you can, by rejecting any kinds of uncertainty, rather drop
everything and start over rather than keeping intermediate or not properly
verified state around. And then concenrate on one robust workflow. For
example, don't rely on this "only one connection is accepted" as a core
security feature. A small and robust workflow with no exceptions, like this:

- client connects to the pairing daemon, **verifies** the server's host key.
- client looks out for the proper verification message during
  keyboard-interactive stage, if it doesn't appear: start over.
- server looks out for the keyboard-interactive stage, if it isn't offered:
  abort, don't record any keys.
- end user browses through the offered list of keys, ideally also verifies
  their fingerprints again (could be seen as redundant, but provides another
  fool-proof layer of verification).

(In reply to Matthias Gerstner from comment #11)
> (In reply to fvogt@suse.com from comment #10)
> > (In reply to Matthias Gerstner from comment #9)
> > ssh-pairing-server only accepts a single connection. If a potentially malicious
> > client connects before the actual client, the actual client will not be able
> > to connect to ssh-pairing-server anymore. This is visible by the lack of a
> > confirmation message on the client. If the message appears, it was definitely
> > sent by the correct host (validated through the host key).
> > 
> > The key import confirmation+selection dialog should make this clear meanwhile.
> 
> Yes, by now I better understand your design, that you are using the
> keyboard-interactive auth reply as a verification channel back to the client.
> 
> The point that only one connection is accepted is certainly helpful but still
> a bit shaky IMHO. You are relying on whatever `ssh_bind_accept(bind,
> session)`
> from libssh is doing. From the apidoc all they say is this:
> 
>     Accept an incoming ssh connection and initialize the session.
> 
> There are no guarantees wrt to the requirements ssh-pairing-daemon has for
> this public key exchange scheme. For example it could be possible that
> multiple clients start a connection and receive the server's host key, and
> `ssh_bind_accept()` only returns after a client accepted the host key and
> progresses to the next protocol stages. This is not currently the case. But
> libssh is a library, a kind of framework even, with some abstractions. And
> these abstractions aren't necessarily tailored to what you are trying to do
> here. The behaviour might change silently in the future.

I highly doubt that. ssh_bind_accept accept()s a single connection and key
exchange is performed later by ssh_handle_key_exchange.

Additionally, only one client can ever reach the point of the loop where
public keys are read. Everything else results in failure.

If you still think this can for some reason happen, ssh_bind_accept_fd could
be used. That takes the FD from accept, i.e. a specific client connection.
IMO that's equivalent to ssh_bind_accept though, as documented.

> The point is that there can be situations in-between, a rather large of
> variables in this scheme, here are just a few items that come to my mind:
> 
> - another party accidentally of purposefully might connect to the
>   ssh-pairing-daemon. The proper client will likely notice this, but it
>   depends on a human factor. An unknown number of SSH public keys are now
>   recorded on the server system. The user must now be able to properly
>   identify this and recover from this situation.

Correct, it's impossible to eliminate the human factor.

> - the legitimate client might not offer keyboard-interactive authentication
>   for some reason, so the verification channel might be missing, which
> doesn't
>   cause the workflow to abort currently. So this ends in a half-verified
>   state that is undesirable.
> - there are a number of interactive steps of high importance:
>   - verification of the server's host key.
>   - verification of the proper reply via the keyboard-interactive prompt.
>   - ideally verification of the key fingerprint upon selection on the server
>     side TUI.
> 
> In practice individual steps can easily break e.g. the end user accidentally
> fails to accept the server's host key for some reason. Now the
> ssh-pairing-daemon is gone. What happens in this case (on TUI level?). The
> more goes wrong and the more the end user has to repeat, the more likely it
> becomes (IMHO) that the end user gets careless.

IMO it's rather the other way around. There is no timeout and the process is
as simple as it can be currently, so not much can go wrong. If something goes
wrong, the simplicity makes it very likely that no step gets skipped.

> So you should go through all
> possible error scenarios and check the outcome. Ideally the server side
> should
> never record any state in error situations. Recovering from error situations
> (starting over) should be simple and clean for the end user.

AFAICT this is already the case.

> So in summary my main point is that you should eliminate as many of these
> variables as you can, by rejecting any kinds of uncertainty, rather drop
> everything and start over rather than keeping intermediate or not properly
> verified state around. And then concenrate on one robust workflow. For
> example, don't rely on this "only one connection is accepted" as a core
> security feature. A small and robust workflow with no exceptions, like this:
> 
> - client connects to the pairing daemon, **verifies** the server's host key.
> - client looks out for the proper verification message during
>   keyboard-interactive stage, if it doesn't appear: start over.

The design is based on not requiring any special software on the client, so
reading the output is unavoidable. If a user can't do that then everything
is lost anyways.

> - server looks out for the keyboard-interactive stage, if it isn't offered:
>   abort, don't record any keys.

Ok.

> - end user browses through the offered list of keys, ideally also verifies
>   their fingerprints again (could be seen as redundant, but provides another
>   fool-proof layer of verification).

(In reply to fvogt@suse.com from comment #12)
> > There are no guarantees wrt to the requirements ssh-pairing-daemon has for
> > this public key exchange scheme. For example it could be possible that
> > multiple clients start a connection and receive the server's host key, and
> > `ssh_bind_accept()` only returns after a client accepted the host key and
> > progresses to the next protocol stages. This is not currently the case. But
> > libssh is a library, a kind of framework even, with some abstractions. And
> > these abstractions aren't necessarily tailored to what you are trying to do
> > here. The behaviour might change silently in the future.
> 
> I highly doubt that. ssh_bind_accept accept()s a single connection and key
> exchange is performed later by ssh_handle_key_exchange.
> 
> Additionally, only one client can ever reach the point of the loop where
> public keys are read. Everything else results in failure.
> 
> If you still think this can for some reason happen, ssh_bind_accept_fd could
> be used. That takes the FD from accept, i.e. a specific client connection.
> IMO that's equivalent to ssh_bind_accept though, as documented.

As I said it currently works as expected. The point is, there are no
guarantees in the library that this property is fulfilled in the future.
Therefore I wouldn't rely on this "only one connection is accepted" as a
security property.

For `ssh_bind_accept_fd` the apidoc also leaves room for quite anything:

    Accept an incoming ssh connection on the given file descriptor and initialize the session.

It is a bit like with undefined behaviour in C/C++, even if a compiler
generates suitable code at the moment, it could change anytime and it could
also turn of the refrigerator instead.

> > In practice individual steps can easily break e.g. the end user accidentally
> > fails to accept the server's host key for some reason. Now the
> > ssh-pairing-daemon is gone. What happens in this case (on TUI level?). The
> > more goes wrong and the more the end user has to repeat, the more likely it
> > becomes (IMHO) that the end user gets careless.
>
> IMO it's rather the other way around. There is no timeout and the process is
> as simple as it can be currently, so not much can go wrong. If something goes
> wrong, the simplicity makes it very likely that no step gets skipped.

I guess your vision of the average end user is too positive :-D

> > So you should go through all
> > possible error scenarios and check the outcome. Ideally the server side
> > should
> > never record any state in error situations. Recovering from error situations
> > (starting over) should be simple and clean for the end user.
> 
> AFAICT this is already the case.

That is good. I will have a final look at the adjusted code when all is ready.

(In reply to Matthias Gerstner from comment #13)
> (In reply to fvogt@suse.com from comment #12)
> > > There are no guarantees wrt to the requirements ssh-pairing-daemon has for
> > > this public key exchange scheme. For example it could be possible that
> > > multiple clients start a connection and receive the server's host key, and
> > > `ssh_bind_accept()` only returns after a client accepted the host key and
> > > progresses to the next protocol stages. This is not currently the case. But
> > > libssh is a library, a kind of framework even, with some abstractions. And
> > > these abstractions aren't necessarily tailored to what you are trying to do
> > > here. The behaviour might change silently in the future.
> > 
> > I highly doubt that. ssh_bind_accept accept()s a single connection and key
> > exchange is performed later by ssh_handle_key_exchange.
> > 
> > Additionally, only one client can ever reach the point of the loop where
> > public keys are read. Everything else results in failure.
> > 
> > If you still think this can for some reason happen, ssh_bind_accept_fd could
> > be used. That takes the FD from accept, i.e. a specific client connection.
> > IMO that's equivalent to ssh_bind_accept though, as documented.
> 
> As I said it currently works as expected. The point is, there are no
> guarantees in the library that this property is fulfilled in the future.
> Therefore I wouldn't rely on this "only one connection is accepted" as a
> security property.
> 
> For `ssh_bind_accept_fd` the apidoc also leaves room for quite anything:
> 
>     Accept an incoming ssh connection on the given file descriptor and
> initialize the session.

I don't see how the wording leaves no room for other behaviour.
Considering that the caller has to pass the FD to a specific client, I don't
see how the lib could change it in any case.

> It is a bit like with undefined behaviour in C/C++, even if a compiler
> generates suitable code at the moment, it could change anytime and it could
> also turn of the refrigerator instead.

I don't think that comparision fits at all. When undefined behaviour happens is
clearly defined by the standard. In this case I'm using a function exactly as
documented. What you're arguing is that the server relies on implementation details
in libssh.

In any case, nothing of this really matters as now the confirmation message is
viewed as a critical component. That can happen only for a single client.

> > > In practice individual steps can easily break e.g. the end user accidentally
> > > fails to accept the server's host key for some reason. Now the
> > > ssh-pairing-daemon is gone. What happens in this case (on TUI level?). The
> > > more goes wrong and the more the end user has to repeat, the more likely it
> > > becomes (IMHO) that the end user gets careless.
> >
> > IMO it's rather the other way around. There is no timeout and the process is
> > as simple as it can be currently, so not much can go wrong. If something goes
> > wrong, the simplicity makes it very likely that no step gets skipped.
> 
> I guess your vision of the average end user is too positive :-D

I wouldn't put it that way. IME, making a process as simple as possible means that
there's less motivation to work around it. Every additional step and complication
(edge or special cases) can lead the user to skip ahead and cause careless behaviour.

> > > So you should go through all
> > > possible error scenarios and check the outcome. Ideally the server side
> > > should
> > > never record any state in error situations. Recovering from error situations
> > > (starting over) should be simple and clean for the end user.
> > 
> > AFAICT this is already the case.
> 
> That is good. I will have a final look at the adjusted code when all is
> ready.

I think this might be the case meanwhile. Not sure whether I missed something though.

(In reply to fvogt@suse.com from comment #14)
> > For `ssh_bind_accept_fd` the apidoc also leaves room for quite anything:
> > 
> >     Accept an incoming ssh connection on the given file descriptor and
> > initialize the session.
> 
> I don't see how the wording leaves no room for other behaviour.
> Considering that the caller has to pass the FD to a specific client, I don't
> see how the lib could change it in any case.

So what do you believe does "initialize the session" mean then?

It's not specified (at least not in this spot) so it can be quite anything. In
my mind this leaves room to perform initial communication with the client in
some form before the connection is passed to the caller. And if this could
happen then it could also happen that this initial communication fails for
some reason or is rejected and the library might decide to try with the next
connecting client instead.

> I don't think that comparision fits at all. When undefined behaviour happens is
> clearly defined by the standard. In this case I'm using a function exactly as
> documented. What you're arguing is that the server relies on implementation details
> in libssh.

Yes exactly, the security property "only one connection is handled" depends on
implementation details in libssh that are not specified in the apidoc. And
undefined behaviour in C happens when a program is relying on anything that is
not explicitly guaranteed. So I wouldn't say that's such a bad comparison.

> I wouldn't put it that way. IME, making a process as simple as possible means that
> there's less motivation to work around it. Every additional step and complication
> (edge or special cases) can lead the user to skip ahead and cause careless behaviour.

I don't want to say that your implementation is lacking in this area but you'd
be surprised what some end users do. There is, of course, only so much we can
do about it.

In this light I find the process still complex, it is in the nature of what
you are trying to achieve though. The end user needs to carefully follow
instructions and it often won't have a clear idea as of why individual
steps are important. In this context the wildest things can happen, even if
just some small error occurs.

> I think this might be the case meanwhile. Not sure whether I missed something though.

All right I will look once more into the current state of both components.

(In reply to Matthias Gerstner from comment #15)
> (In reply to fvogt@suse.com from comment #14)
> > > For `ssh_bind_accept_fd` the apidoc also leaves room for quite anything:
> > > 
> > >     Accept an incoming ssh connection on the given file descriptor and
> > > initialize the session.
> > 
> > I don't see how the wording leaves no room for other behaviour.
> > Considering that the caller has to pass the FD to a specific client, I don't
> > see how the lib could change it in any case.
> 
> So what do you believe does "initialize the session" mean then?

The ssh_session struct.

> It's not specified (at least not in this spot) so it can be quite anything.
> In
> my mind this leaves room to perform initial communication with the client in
> some form before the connection is passed to the caller. And if this could
> happen then it could also happen that this initial communication fails for
> some reason or is rejected and the library might decide to try with the next
> connecting client instead.
> 
> > I don't think that comparision fits at all. When undefined behaviour happens is
> > clearly defined by the standard. In this case I'm using a function exactly as
> > documented. What you're arguing is that the server relies on implementation details
> > in libssh.
> 
> Yes exactly, the security property "only one connection is handled" depends
> on
> implementation details in libssh that are not specified in the apidoc. And
> undefined behaviour in C happens when a program is relying on anything that
> is
> not explicitly guaranteed. So I wouldn't say that's such a bad comparison.

I think you have a misunderstanding of undefined behaviour in the C sense:
The C (and C++) standards explicitly say when undefined behaviour happens.
There are also "unspecified behaviour" and "implementation-defined behaviour".
https://en.cppreference.com/w/cpp/language/ub

> > I wouldn't put it that way. IME, making a process as simple as possible means that
> > there's less motivation to work around it. Every additional step and complication
> > (edge or special cases) can lead the user to skip ahead and cause careless behaviour.
> 
> I don't want to say that your implementation is lacking in this area but
> you'd
> be surprised what some end users do. There is, of course, only so much we can
> do about it.
> 
> In this light I find the process still complex, it is in the nature of what
> you are trying to achieve though. The end user needs to carefully follow
> instructions and it often won't have a clear idea as of why individual
> steps are important. In this context the wildest things can happen, even if
> just some small error occurs.

Yes. My strategy is to make the "happy path" as simple as possible so that every
deviation from the expected behavior gets noticed by the user somehow and
disables their "autopilot", i.e. avoids fatigue resulting in the user just always
accepting "Yes I don't care" because it happens regularly. As seen with MFA push
notifications...

For that reason I'm also not sure about showing both randomart and sha256 host keys
on the connection screen, as it can be a bit much, but both of them are useful...

> > I think this might be the case meanwhile. Not sure whether I missed something though.
> 
> All right I will look once more into the current state of both components.

(In reply to fvogt@suse.com from comment #16)
> > > I don't see how the wording leaves no room for other behaviour.
> > > Considering that the caller has to pass the FD to a specific client, I don't
> > > see how the lib could change it in any case.
> > 
> > So what do you believe does "initialize the session" mean then?
> 
> The ssh_session struct.

I just looked in more detail in the libssh implementation. Currently they act
as expected i.e. if anything goes wrong they just return an error condition to
the caller. The thing is that security issues mostly don't result from
expected behaviour but from unexpected behaviour. It is fully imaginable that
the library adds some glue in the future e.g. they stumbled over a bug in
their logic and want to fix that transparently in the future by doing
something different.

Overall this aspect "only one connection is accepted" is too thin for my taste
and I simply won't add it to my considerations for the security design of this
helper.

Further reasons for this are:

- it is strange and unusual: I cannot think of any other system that uses the
  amount of possible connections to a network service as a core security feature.
- another imaginable scenario how this might break: currently the
  ssh-pairing-server still only listens on IPv4. The proper sshd could still
  be running or running again and bind on IPv6. On the client side, if a
  hostname is used, IPv6 might be preferred, and the wrong daemon is reached,
  it would even present the proper host key fingerprint, most likely.
- there exist other ways how e.g. ports can be shared by multiple programs at
  once. These ways usually require opt-in by all processes though.

All of this might sound far fetched for you but as I say security issues
mostly don't result from the way things are expected to be.

We do not need to agree on this. I simply won't count it into my observations.
And what I would like of you is that you look at the workflow as well without
relying on it as a core security. It will be healthy for the overall
resilience of the scheme.

> > Yes exactly, the security property "only one connection is handled" depends
> > on
> > implementation details in libssh that are not specified in the apidoc. And
> > undefined behaviour in C happens when a program is relying on anything that
> > is
> > not explicitly guaranteed. So I wouldn't say that's such a bad comparison.
> 
> I think you have a misunderstanding of undefined behaviour in the C sense:
> The C (and C++) standards explicitly say when undefined behaviour happens.
> There are also "unspecified behaviour" and "implementation-defined behaviour".
> https://en.cppreference.com/w/cpp/language/ub

That's true, undefined behaviour is not unspecified behaviour in the C
context. But this distinction is not that important for what I wanted to point
out in the ssh-pairing-server context.

> For that reason I'm also not sure about showing both randomart and sha256 host keys
> on the connection screen, as it can be a bit much, but both of them are useful...

In my eyes the randomart feature makes the key comparison much more pleasant
as you can use your brains pattern recognition instead of going over a long
series of boring characters.

Limiting the process to randomart would mean that on the client side the
corresponding option has to be configured or passed, if this fails then the
happy path is broken again.

I don't see a big issue in presenting both in the TUI, though. Or maybe you
can design the dialog in a way that e.g. the fingerprints are only accessible
via a sub-dialog reachable via a button or hotkey or something along these
lines.

I hope I will be able to have a look at your current code tomorrow.

(In reply to Matthias Gerstner from comment #17)
> (In reply to fvogt@suse.com from comment #16)
> > > > I don't see how the wording leaves no room for other behaviour.
> > > > Considering that the caller has to pass the FD to a specific client, I don't
> > > > see how the lib could change it in any case.
> > > 
> > > So what do you believe does "initialize the session" mean then?
> > 
> > The ssh_session struct.
> 
> I just looked in more detail in the libssh implementation. Currently they act
> as expected i.e. if anything goes wrong they just return an error condition
> to
> the caller. The thing is that security issues mostly don't result from
> expected behaviour but from unexpected behaviour. It is fully imaginable that
> the library adds some glue in the future e.g. they stumbled over a bug in
> their logic and want to fix that transparently in the future by doing
> something different.

My argument is that no matter what the library does (excluding unfathomably
crazyness like replacing the program), only one client connection can reach
the relevant code in ssh-pairing-server itself.

> Overall this aspect "only one connection is accepted" is too thin for my
> taste
> and I simply won't add it to my considerations for the security design of
> this
> helper.
> 
> Further reasons for this are:
> 
> - it is strange and unusual: I cannot think of any other system that uses the
>   amount of possible connections to a network service as a core security
> feature.

I don't think atomicity is that rare. It's like O_CREAT|O_EXCL on lock files.

> - another imaginable scenario how this might break: currently the
>   ssh-pairing-server still only listens on IPv4. The proper sshd could still
>   be running or running again and bind on IPv6. On the client side, if a
>   hostname is used, IPv6 might be preferred, and the wrong daemon is reached,
>   it would even present the proper host key fingerprint, most likely.

You don't even have to imagine such a complex scenario with IPv6. The sshd is
restarted immediately after ssh-pairing-server exits. If some (possibly
malicious) client connects before the legitimate client, the legitimate client
might already connect to sshd again.

In such cases the legimiate client will not see the success message from the
server.

> - there exist other ways how e.g. ports can be shared by multiple programs at
>   once. These ways usually require opt-in by all processes though.
> 
> All of this might sound far fetched for you but as I say security issues
> mostly don't result from the way things are expected to be.
> 
> We do not need to agree on this. I simply won't count it into my
> observations.
> And what I would like of you is that you look at the workflow as well without
> relying on it as a core security. It will be healthy for the overall
> resilience of the scheme.
> 
> > > Yes exactly, the security property "only one connection is handled" depends
> > > on
> > > implementation details in libssh that are not specified in the apidoc. And
> > > undefined behaviour in C happens when a program is relying on anything that
> > > is
> > > not explicitly guaranteed. So I wouldn't say that's such a bad comparison.
> > 
> > I think you have a misunderstanding of undefined behaviour in the C sense:
> > The C (and C++) standards explicitly say when undefined behaviour happens.
> > There are also "unspecified behaviour" and "implementation-defined behaviour".
> > https://en.cppreference.com/w/cpp/language/ub
> 
> That's true, undefined behaviour is not unspecified behaviour in the C
> context. But this distinction is not that important for what I wanted to
> point
> out in the ssh-pairing-server context.
> 
> > For that reason I'm also not sure about showing both randomart and sha256 host keys
> > on the connection screen, as it can be a bit much, but both of them are useful...
> 
> In my eyes the randomart feature makes the key comparison much more pleasant
> as you can use your brains pattern recognition instead of going over a long
> series of boring characters.
> 
> Limiting the process to randomart would mean that on the client side the
> corresponding option has to be configured or passed, if this fails then the
> happy path is broken again.
> 
> I don't see a big issue in presenting both in the TUI, though. Or maybe you
> can design the dialog in a way that e.g. the fingerprints are only accessible
> via a sub-dialog reachable via a button or hotkey or something along these
> lines.

Maybe it's just me being too used to the previous 2 line dialog *shrug*

> I hope I will be able to have a look at your current code tomorrow.

(In reply to fvogt@suse.com from comment #18)
> My argument is that no matter what the library does (excluding unfathomably
> crazyness like replacing the program), only one client connection can reach
> the relevant code in ssh-pairing-server itself.

And my argument is that the library can accept and close a connection for
whatever reasons until this "one and only" connection is returned. So we're in
agreement finally :-)

> > - it is strange and unusual: I cannot think of any other system that uses the
> >   amount of possible connections to a network service as a core security
> > feature.
> 
> I don't think atomicity is that rare. It's like O_CREAT|O_EXCL on lock files.

This isn't going into a fruitful direction. I guess you well know what I mean
and how this example is just awkward. This "only one connection is handled" is
far away from any reliable atomicity and that is why we're having this
discussion.

> In such cases the legimiate client will not see the success message from the
> server.

And that is why this "only one connection is handled" aspect is not part of
the core security of this scheme. It is a hardening. And that is all that I'm
trying to express here.

The success message always needs to be verified. If this is done then the
scheme is reliable regardless how many connections are accepted.

------------------------------------------------------------------------------

All right I had another look into the current code from here:

    - https://github.com/Vogtinator/ssh-pairing
    - https://github.com/Vogtinator/jeos-firstboot.git [ssh_enroll branch]

Comments for ssh-pairing:

    // Client identifier
    char clientname[NI_MAXHOST] = "ssh-pairing";

I would change this comment into something clearer like "Client identifier
that will be visible in the authorized_keys output for the received keys".

The important custom message "Received %d public keys" could be a bit clearer
as to where it comes from. A prefix of "ssh-pairing-server: <...>" might help
here.

There is still no enforcement of the keyboard-interactive phase. As you
stated above the reception and verification of the message sent back to the
client during this phase is vital to the security of this scheme. I suggest to
enforce this phase (I don't believe SSH client configurations that deny
keyboard/interactive are a common case to consider). As discussed elsewhere
already if no successful pass can be made then the program should not generate
any output at all to prevent public keys of undefined trust to spread.

The README in the repo is rather large but somehow I'm missing a concise
description of the security considerations. I would add a section like
this:

```
    ## Security Considerations
    
    Just anybody can attempt to contact the ssh-pairing-server presenting
    arbitrary public keys. Thus this key transmission scheme requires interactive
    verification of the process at various points:
    
    - the client needs to verify the SSH server's host key (as it always should)
      so it knows it is talking to the correct system and no MITM is around.
    - the client needs to check the ssh-pairing-server's custom response. In
      the context of the keyboard-interactive authentication phase the
      ssh-pairing-server responds with a special text "Received %d public
      keys". This signifies that the client's public keys actually have been
      processed.
    - as a hardening ssh-pairing-server only accepts a single SSH connection
      for processing public keys, so the workflow should succeed exactly once
      and no other connections should be processed. If it doesn't succeed for
      whatever reason then the process needs to be started over.
    - for maximum trust verification ideally the fingerprints of the to-be-added
      public keys on the server side are also interactively compared against the
      fingerprints on the client system.
```

On the jeos-firstboot side I'm missing a prompt to verify the custom
response on the client side. I can imagine a more wizard like step-by-step
guidance through the process with multiple screens:

1) connect to one of the following addresses via SSH and wait for the SSH host
   key verification prompt. [enter] to continue.

2) verify the host key against this data:

   <fingerprints>
   <randomart>

   [prev step] [next step (I verified!)]

3) verify that the message "%d public keys processed" appeared.

   [prev step] [next step (I verified!)]

4) select the public keys to import

   [ ] first
   [ ] second

   > and here maybe don't just use a simple selection list but open another
   > sub-dialog for each key, displaying its fingerprint and randomart image,
   > suggesting to the user to compare the fingerprint once more against the
   > one on the client side.

(In reply to Matthias Gerstner from comment #19)
> (In reply to fvogt@suse.com from comment #18)
> All right I had another look into the current code from here:
> 
>     - https://github.com/Vogtinator/ssh-pairing
>     - https://github.com/Vogtinator/jeos-firstboot.git [ssh_enroll branch]
> 
> Comments for ssh-pairing:
> 
>     // Client identifier
>     char clientname[NI_MAXHOST] = "ssh-pairing";
> 
> I would change this comment into something clearer like "Client identifier
> that will be visible in the authorized_keys output for the received keys".

Ok.

> The important custom message "Received %d public keys" could be a bit clearer
> as to where it comes from. A prefix of "ssh-pairing-server: <...>" might help
> here.

Ok.

> There is still no enforcement of the keyboard-interactive phase. As you
> stated above the reception and verification of the message sent back to the
> client during this phase is vital to the security of this scheme. I suggest
> to
> enforce this phase (I don't believe SSH client configurations that deny
> keyboard/interactive are a common case to consider). As discussed elsewhere
> already if no successful pass can be made then the program should not
> generate
> any output at all to prevent public keys of undefined trust to spread.

There is, implemented by https://github.com/Vogtinator/ssh-pairing/commit/09adb55e1a7513a334aacdd3f1c0d1a81a1c3250.
I just verified it again locally. If I use

ssh -o "KbdInteractiveAuthentication no" localhost

The client fails with

fvogt@localhost: Permission denied (publickey,keyboard-interactive).

and on the server a dialog appears with the message

> ssh-pairing-server exited with 1:
> Session did not complete until
> keyboard-interactive.

and exits.

> The README in the repo is rather large but somehow I'm missing a concise
> description of the security considerations. I would add a section like
> this:

Ok, I'll have a look.

> ```
>     ## Security Considerations
>     
>     Just anybody can attempt to contact the ssh-pairing-server presenting
>     arbitrary public keys. Thus this key transmission scheme requires
> interactive
>     verification of the process at various points:
>     
>     - the client needs to verify the SSH server's host key (as it always
> should)
>       so it knows it is talking to the correct system and no MITM is around.
>     - the client needs to check the ssh-pairing-server's custom response. In
>       the context of the keyboard-interactive authentication phase the
>       ssh-pairing-server responds with a special text "Received %d public
>       keys". This signifies that the client's public keys actually have been
>       processed.
>     - as a hardening ssh-pairing-server only accepts a single SSH connection
>       for processing public keys, so the workflow should succeed exactly once
>       and no other connections should be processed. If it doesn't succeed for
>       whatever reason then the process needs to be started over.
>     - for maximum trust verification ideally the fingerprints of the
> to-be-added
>       public keys on the server side are also interactively compared against
> the
>       fingerprints on the client system.
> ```
> 
> On the jeos-firstboot side I'm missing a prompt to verify the custom
> response on the client side.

There is, in the dialog to select public keys.

> I can imagine a more wizard like step-by-step
> guidance through the process with multiple screens:
> 
> 1) connect to one of the following addresses via SSH and wait for the SSH
> host
>    key verification prompt. [enter] to continue.

Currently 1+2 are combined and on completion moves to the next step
automatically.

> 2) verify the host key against this data:
> 
>    <fingerprints>
>    <randomart>
> 
>    [prev step] [next step (I verified!)]
> 
> 3) verify that the message "%d public keys processed" appeared.
> 
>    [prev step] [next step (I verified!)]

Currently 3+4 are combined.

> 4) select the public keys to import
> 
>    [ ] first
>    [ ] second
> 
>    > and here maybe don't just use a simple selection list but open another
>    > sub-dialog for each key, displaying its fingerprint and randomart image,
>    > suggesting to the user to compare the fingerprint once more against the
>    > one on the client side.

IMO that's too much on the complex side, though most users might only have a
single key.

(In reply to fvogt@suse.com from comment #20)
> > I would change this comment into something clearer like "Client identifier
> > that will be visible in the authorized_keys output for the received keys".
> 
> Ok.
> 
> > The important custom message "Received %d public keys" could be a bit clearer
> > as to where it comes from. A prefix of "ssh-pairing-server: <...>" might help
> > here.
> 
> Ok.

Will you ping me once there is something new to look at?

> > There is still no enforcement of the keyboard-interactive phase. As you
> > stated above the reception and verification of the message sent back to the
> > client during this phase is vital to the security of this scheme. I suggest
> > to
> > enforce this phase (I don't believe SSH client configurations that deny
> > keyboard/interactive are a common case to consider). As discussed elsewhere
> > already if no successful pass can be made then the program should not
> > generate
> > any output at all to prevent public keys of undefined trust to spread.
> 
> There is, implemented by https://github.com/Vogtinator/ssh-pairing/commit/09adb55e1a7513a334aacdd3f1c0d1a81a1c3250.
> I just verified it again locally. If I use
> 
> ssh -o "KbdInteractiveAuthentication no" localhost
> 
> The client fails with
> 
> fvogt@localhost: Permission denied (publickey,keyboard-interactive).
> 
> and on the server a dialog appears with the message
> 
> > ssh-pairing-server exited with 1:
> > Session did not complete until
> > keyboard-interactive.
> 
> and exits.

Ah okay I missed that. But it still generates the output for public keys of
undefined state. For hardening purpose I would not do that.

> > On the jeos-firstboot side I'm missing a prompt to verify the custom
> > response on the client side.
> 
> There is, in the dialog to select public keys.

Can you please point me to the source code location?

> > I can imagine a more wizard like step-by-step
> > guidance through the process with multiple screens:
> > 
> > 1) connect to one of the following addresses via SSH and wait for the SSH
> > host
> >    key verification prompt. [enter] to continue.
> 
> Currently 1+2 are combined and on completion moves to the next step
> automatically.

Is there an image that I can use for testing or is there a simple way to run
the jeos firstboot isolated to check the user experience?

> > 4) select the public keys to import
> > 
> >    [ ] first
> >    [ ] second
> > 
> >    > and here maybe don't just use a simple selection list but open another
> >    > sub-dialog for each key, displaying its fingerprint and randomart image,
> >    > suggesting to the user to compare the fingerprint once more against the
> >    > one on the client side.
> 
> IMO that's too much on the complex side, though most users might only have a
> single key.

If there is only one key then you could display a simpler dialog, of course.
But you have to deal with the possibility of multiple keys and this situation
should not be neglected.

(In reply to Matthias Gerstner from comment #21)
> (In reply to fvogt@suse.com from comment #20)
> > > I would change this comment into something clearer like "Client identifier
> > > that will be visible in the authorized_keys output for the received keys".
> > 
> > Ok.
> > 
> > > The important custom message "Received %d public keys" could be a bit clearer
> > > as to where it comes from. A prefix of "ssh-pairing-server: <...>" might help
> > > here.
> > 
> > Ok.
> 
> Will you ping me once there is something new to look at?

Ping! Did that last week already: https://github.com/Vogtinator/ssh-pairing/commit/24937265360cc325d9bd3c8aa0321fbe6d2f8bda

> > > There is still no enforcement of the keyboard-interactive phase. As you
> > > stated above the reception and verification of the message sent back to the
> > > client during this phase is vital to the security of this scheme. I suggest
> > > to
> > > enforce this phase (I don't believe SSH client configurations that deny
> > > keyboard/interactive are a common case to consider). As discussed elsewhere
> > > already if no successful pass can be made then the program should not
> > > generate
> > > any output at all to prevent public keys of undefined trust to spread.
> > 
> > There is, implemented by https://github.com/Vogtinator/ssh-pairing/commit/09adb55e1a7513a334aacdd3f1c0d1a81a1c3250.
> > I just verified it again locally. If I use
> > 
> > ssh -o "KbdInteractiveAuthentication no" localhost
> > 
> > The client fails with
> > 
> > fvogt@localhost: Permission denied (publickey,keyboard-interactive).
> > 
> > and on the server a dialog appears with the message
> > 
> > > ssh-pairing-server exited with 1:
> > > Session did not complete until
> > > keyboard-interactive.
> > 
> > and exits.
> 
> Ah okay I missed that. But it still generates the output for public keys of
> undefined state. For hardening purpose I would not do that.

IMO that's the responsibility of the integration of ssh-pairing-server into
whatever UI is put on top, in this case the bundled ssh-pairing script.
This is also documented as such in the readme.
That way it's also possible to support a slightly different workflow which
does not rely on KbdInteractive, without having to change ssh-pairing-server
itself. That way it can be used similar to ssh-keyscan.

> > > On the jeos-firstboot side I'm missing a prompt to verify the custom
> > > response on the client side.
> > 
> > There is, in the dialog to select public keys.
> 
> Can you please point me to the source code location?

https://github.com/Vogtinator/ssh-pairing/blob/b7fb8ca3d396bbe078ff6f545feff1f2b5453dfa/ssh-pairing#L41C17-L41C109

> > > I can imagine a more wizard like step-by-step
> > > guidance through the process with multiple screens:
> > > 
> > > 1) connect to one of the following addresses via SSH and wait for the SSH
> > > host
> > >    key verification prompt. [enter] to continue.
> > 
> > Currently 1+2 are combined and on completion moves to the next step
> > automatically.
> 
> Is there an image that I can use for testing or is there a simple way to run
> the jeos firstboot isolated to check the user experience?

You can just run ssh-pairing after meson install.

> > > 4) select the public keys to import
> > > 
> > >    [ ] first
> > >    [ ] second
> > > 
> > >    > and here maybe don't just use a simple selection list but open another
> > >    > sub-dialog for each key, displaying its fingerprint and randomart image,
> > >    > suggesting to the user to compare the fingerprint once more against the
> > >    > one on the client side.
> > 
> > IMO that's too much on the complex side, though most users might only have a
> > single key.
> 
> If there is only one key then you could display a simpler dialog, of course.
> But you have to deal with the possibility of multiple keys and this situation
> should not be neglected.

Yes, having a noticably different display for one vs. multiple keys is by itself
more complex already.

(In reply to fvogt@suse.com from comment #22)
> > Will you ping me once there is something new to look at?
> 
> Ping! Did that last week already: https://github.com/Vogtinator/ssh-pairing/commit/24937265360cc325d9bd3c8aa0321fbe6d2f8bda

Okay the changes look good.

> IMO that's the responsibility of the integration of ssh-pairing-server into
> whatever UI is put on top, in this case the bundled ssh-pairing script.

As I said before: Why risk that a mistake is done in any of N integrations on
top when you can prevent unverified data exiting the base component? The base
component _knows_ that verification was not possible in this state but leaves
further decisions to an unknown component on top?

> That way it's also possible to support a slightly different workflow which
> does not rely on KbdInteractive, without having to change ssh-pairing-server
> itself. That way it can be used similar to ssh-keyscan.

If this is really desired then the behaviour can be changed via a command line
parameter passed to the tool. No need to weaken the security stance of the
main purpose of the utility.

> > Can you please point me to the source code location?
> 
> https://github.com/Vogtinator/ssh-pairing/blob/b7fb8ca3d396bbe078ff6f545feff1f2b5453dfa/ssh-pairing#L41C17-L41C109

Thanks.

> > > > I can imagine a more wizard like step-by-step
> > > > guidance through the process with multiple screens:
> > > > 
> > > > 1) connect to one of the following addresses via SSH and wait for the SSH
> > > > host
> > > >    key verification prompt. [enter] to continue.
> > > 
> > > Currently 1+2 are combined and on completion moves to the next step
> > > automatically.
> > 
> > Is there an image that I can use for testing or is there a simple way to run
> > the jeos firstboot isolated to check the user experience?
> 
> You can just run ssh-pairing after meson install.

Sorry I meant not the ssh-pairing tool but the Jeos firstboot module. I want
to test the various screen in practice.

> > If there is only one key then you could display a simpler dialog, of course.
> > But you have to deal with the possibility of multiple keys and this situation
> > should not be neglected.
> 
> Yes, having a noticably different display for one vs. multiple keys is by itself
> more complex already.

You seem to have a strange notion of managing complexity. Reducing it by
neglecting relevant use cases is not the way it is successfully managed. You
could say: the tool simply doesn't support more than one key at all. Then
there is no fuzzy area left.

But if you want to support it then you need to take full responsibility for a
high quality outcome of all situations. If that means adding complexity,
then it has to be this way. There is only degree of freedom left in the way
the complexity is added then.

(In reply to Matthias Gerstner from comment #23)
> (In reply to fvogt@suse.com from comment #22)
> > > Will you ping me once there is something new to look at?
> > 
> > Ping! Did that last week already: https://github.com/Vogtinator/ssh-pairing/commit/24937265360cc325d9bd3c8aa0321fbe6d2f8bda
> 
> Okay the changes look good.
> 
> > IMO that's the responsibility of the integration of ssh-pairing-server into
> > whatever UI is put on top, in this case the bundled ssh-pairing script.
> 
> As I said before: Why risk that a mistake is done in any of N integrations on
> top when you can prevent unverified data exiting the base component? The base
> component _knows_ that verification was not possible in this state but leaves
> further decisions to an unknown component on top?

If the component on top displays the received keys and asks for confirmation,
that's fine though.

> > That way it's also possible to support a slightly different workflow which
> > does not rely on KbdInteractive, without having to change ssh-pairing-server
> > itself. That way it can be used similar to ssh-keyscan.
> 
> If this is really desired then the behaviour can be changed via a command
> line
> parameter passed to the tool. No need to weaken the security stance of the
> main purpose of the utility.

*shrug*. So back to asprintf?

> > > Can you please point me to the source code location?
> > 
> > https://github.com/Vogtinator/ssh-pairing/blob/b7fb8ca3d396bbe078ff6f545feff1f2b5453dfa/ssh-pairing#L41C17-L41C109
> 
> Thanks.
> 
> > > > > I can imagine a more wizard like step-by-step
> > > > > guidance through the process with multiple screens:
> > > > > 
> > > > > 1) connect to one of the following addresses via SSH and wait for the SSH
> > > > > host
> > > > >    key verification prompt. [enter] to continue.
> > > > 
> > > > Currently 1+2 are combined and on completion moves to the next step
> > > > automatically.
> > > 
> > > Is there an image that I can use for testing or is there a simple way to run
> > > the jeos firstboot isolated to check the user experience?
> > 
> > You can just run ssh-pairing after meson install.
> 
> Sorry I meant not the ssh-pairing tool but the Jeos firstboot module. I want
> to test the various screen in practice.

The jeos-firstboot module really just calls ssh-pairing:
https://github.com/openSUSE/jeos-firstboot/pull/116/files#diff-a4e5d4eb0367513646c0bd535d5c4ee397d996082adcee2b9c63a213287ca838R14

If you want to boot an image, you can use
https://build.opensuse.org/package/show/home:favogt:combustion/kiwi-templates-Minimal
or
https://build.opensuse.org/package/show/home:favogt:combustion/openSUSE-MicroOS-build

> > > If there is only one key then you could display a simpler dialog, of course.
> > > But you have to deal with the possibility of multiple keys and this situation
> > > should not be neglected.
> > 
> > Yes, having a noticably different display for one vs. multiple keys is by itself
> > more complex already.
> 
> You seem to have a strange notion of managing complexity. Reducing it by
> neglecting relevant use cases is not the way it is successfully managed. You
> could say: the tool simply doesn't support more than one key at all. Then
> there is no fuzzy area left.

I think you misunderstood: If a dialog is fine for multiple keys, it should also
be fine for a single key, to avoid having to special case that.

> But if you want to support it then you need to take full responsibility for a
> high quality outcome of all situations. If that means adding complexity,
> then it has to be this way. There is only degree of freedom left in the way
> the complexity is added then.

(In reply to fvogt@suse.com from comment #24)
> > As I said before: Why risk that a mistake is done in any of N integrations on
> > top when you can prevent unverified data exiting the base component? The base
> > component _knows_ that verification was not possible in this state but leaves
> > further decisions to an unknown component on top?
> 
> If the component on top displays the received keys and asks for confirmation,
> that's fine though.

This seems to be going in circles and doesn't answer the question.

> > > That way it's also possible to support a slightly different workflow which
> > > does not rely on KbdInteractive, without having to change ssh-pairing-server
> > > itself. That way it can be used similar to ssh-keyscan.
> > 
> > If this is really desired then the behaviour can be changed via a command
> > line
> > parameter passed to the tool. No need to weaken the security stance of the
> > main purpose of the utility.
> 
> *shrug*. So back to asprintf?

That is an implementation detail that isn't important for the security
properties and design.

I would rather keep an array of strings instead though. Or switch this whole
program to C++ and use a std::vector to make it less painful.

> The jeos-firstboot module really just calls ssh-pairing:
> https://github.com/openSUSE/jeos-firstboot/pull/116/files#diff-a4e5d4eb0367513646c0bd535d5c4ee397d996082adcee2b9c63a213287ca838R14

I mean the part where the actual TUI dialogs on top of ssh-pairing are
displayed.

> If you want to boot an image, you can use
> https://build.opensuse.org/package/show/home:favogt:combustion/kiwi-templates-Minimal
> or
> https://build.opensuse.org/package/show/home:favogt:combustion/openSUSE-MicroOS-build

I'll have a look into the images but currently I'm swamped with other tasks.

> > You seem to have a strange notion of managing complexity. Reducing it by
> > neglecting relevant use cases is not the way it is successfully managed. You
> > could say: the tool simply doesn't support more than one key at all. Then
> > there is no fuzzy area left.
> 
> I think you misunderstood: If a dialog is fine for multiple keys, it should also
> be fine for a single key, to avoid having to special case that.

It seems I didn't manage to get my point across.

But let me first look into the practical look & feel before we continue on
this path.

When I'm done with that I will ask a colleague to be the final judge of the
current state of the tool's design.

I just looked into the integration of this in jeos-firstboot in the image you provided me at https://download.opensuse.org/repositories/home:/favogt:/combustion/images/openSUSE-Tumbleweed-Minimal-VM.x86_64-1.0.0-kvm-and-xen-Build51.3.qcow2.

Following are a couple of aspects that are still relevant IMHO:

1) the tool does not offer to retry?
------------------------------------

if anything goes wrong then the error message is displayed and I'm kicked out
the login prompt.

since it's not unlikely that something goes wrong the tool should offer to
restart the process from the beginning.

2) typical error scenario: client side complains about changed host key
-----------------------------------------------------------------------

when setting up new hosts it's not uncommon that the IP address it uses is
already stored in the clients ~/.ssh/known_hosts for some other host that used
the IP before.

in that case the client refuses to connect and does also not transfer its
public keys to the server. on the tool side one will only see:

    connection reset by peer

So that is one case where the user first has to clean up its known_hosts
database and retry. Which emphasizes the need for a retry opportunity.

3) tool should explain how to get the fingerprint of a public key in the client side
------------------------------------------------------------------------------------

In the final screen where the collected public keys are presented it would be
helpful to present the command to type on the client side to display the
fingerprints of ones own public keys for comparison like

    ssh-keygen -lvf ~/.ssh/identity.pub

4) the list of keys to import contains little information
---------------------------------------------------------

Once public keys have been collected a plain list of public keys is presented
like

    [ ] ssh-rsa AAAAAB3Nz...mq7rfc27mBHDv7nX0+4T
    [ ] ssh-rsa [...]
    [ ] ssh-rsa [...]

This is really little information and the fingerprint isn't even complete.
Going for a sub-screen for each key where the randomart is also displayed
would improve the experience a lot I believe.

5) in summary: more wizard like approach seems advisable
--------------------------------------------------------

As I outlined earlier a more wizard like approach seems advisable. Especially
for the step "verify that the message 'ssh-pairing: Received 3 public keys'"
appeared. This should be an explicit confirmation screen, otherwise users will
easily overread this or "don't care".

From my side I'm done with this topic. It's a good idea that needs some more
honing in various spots IMHO. I've asked my colleagues in the team to also
chime in so that we have a second opinion, but I don't know when anybody will
get around to have a look.

I pushed some more changes to ssh-pairing as well as the jeos-firstboot:

The key import dialog is split, at first there's a dialog to confirm the message on the client. After that, there are individual dialogs for each key which show the full output of "ssh-keygen -l -v -f" including randomart.

The jeos-firstboot module now offers to try again if no keys were imported.

Packages in home:favogt:combustion should already reflect those.

With those changes the main remaining issues should be addressed and IMO it can be included in Tumbleweed images in this state.

For the others there is some progress as well:

Upstream libssh meanwhile implemented one missing part for sshd_config reading and I prepared https://github.com/Vogtinator/ssh-pairing/pull/2.

libssh doesn't appear to support listening on IPv4 and IPv6 simultaneously (yet), I filed https://gitlab.com/libssh/libssh-mirror/-/issues/250 for that.

(In reply to fvogt@suse.com from comment #28)
> I pushed some more changes to ssh-pairing as well as the jeos-firstboot:
> 
> The key import dialog is split, at first there's a dialog to confirm the message on the client. After that, there are individual dialogs for each key which show the full output of "ssh-keygen -l -v -f" including randomart.
> 
> The jeos-firstboot module now offers to try again if no keys were imported.

This sounds good.

> With those changes the main remaining issues should be addressed and IMO it can be included in Tumbleweed images in this state.

There is no action required on our side so you can just go ahead whenever you
deem it ready. I'm closing this AUDIT bug meanwhile. If there is any need for
discussion you can still post comments reach or reach us on Slack.

I will not dive into practical testing of the current state of the workflow
again, due to time limitations. I suggest to let one or two people test the
current state, that have not involved with this yet. Ask them how well the
workflow works for them and if anything could still be improved.

> For the others there is some progress as well:
> 
> Upstream libssh meanwhile implemented one missing part for sshd_config reading and I prepared https://github.com/Vogtinator/ssh-pairing/pull/2.

It is unfortunate, that libssh's support for the configuration file is so
rudimentary. Reimplementing the configuration file lookup etc. and still
ending up with different behaviour is bad.

It's also unexpected that this "pseudo ssh" service does not honor the sshd
configuration at all. But likely the smaller pain to live with.

> libssh doesn't appear to support listening on IPv4 and IPv6 simultaneously (yet), I filed https://gitlab.com/libssh/libssh-mirror/-/issues/250 for that.

I commented in the issue about possible approaches to get around this. I don't
know how important IPv6 will be in the context of this. It is likely hard to
tell, so it would be better to cover it.