Review Comments #1
Conversation
| // Client identifier | ||
| char clientname[INET6_ADDRSTRLEN]; | ||
| /* | ||
| * what is the purpose of this "ssh-pairing" default value of |
There was a problem hiding this comment.
It's for the comment part of the authorized_keys line. Normally that's the hostname of the client system, but I wanted to avoid the potential issues of a reverse DNS lookup
There was a problem hiding this comment.
Obtaining the numerical IP address of the peer should practically never fail on a connected socket, aside from out of memory situations and alike. It might be better to exit with an error code if no numerical IP address can be obtained. Then no default name is required.
It could be worth a thought to add a command line option to pass the desired hostname for the authorized_keys file. This would make the file more descriptive than numerical IP addresses or a default name.
If clientname is also used to store hostnames then it might make more sense to use the NI_MAXHOST constant to dimension the clientname string. At least this would put less of a questionmark in my mind, since placing a numerical IP address within NI_MAXHOST bytes will always be possible, while placing a hostname in INET6_ADDRSTRLEN bytes won't.
There was a problem hiding this comment.
Obtaining the numerical IP address of the peer should practically never fail on a connected socket, aside from out of memory situations and alike. It might be better to exit with an error code if no numerical IP address can be obtained. Then no default name is required.
Can be done, but that terminates the connection which is a bit annoying. It seemed easier for me to just add a fallback.
It could be worth a thought to add a command line option to pass the desired hostname for the authorized_keys file. This would make the file more descriptive than numerical IP addresses or a default name.
Could be done, but IMO the caller could simply replace the whole "comment" column of the output in that case. Initially I targeted ssh-pairing for the initial enrollment for a server, which should require as little input as possible, but it might also be a good usecase for subsequent enrollments from other clients later.
If clientname is also used to store hostnames then it might make more sense to use the NI_MAXHOST constant to dimension the clientname string. At least this would put less of a questionmark in my mind, since placing a numerical IP address within NI_MAXHOST bytes will always be possible, while placing a hostname in INET6_ADDRSTRLEN bytes won't.
Ok, will do.
| * I don't really get why the INTERACTIVE auth method is offered in | ||
| * the first place. It looks like the only purpose is to send the | ||
| * "Received %d public keys" message to the connecting client. This | ||
| * should be documented better. |
There was a problem hiding this comment.
Almost. Without offering keyboard-interactive, the client shows a Permission denied message. It still works, but it's misleading. This is explained in the readme as well.
There was a problem hiding this comment.
I see. A short comment in the code about this wouldn't hurt.
Also, at least in my mind (as an expert only, maybe), the message Permission denied makes complete sense, since the login is not supposed to succeed at this stage.
There was a problem hiding this comment.
I see. A short comment in the code about this wouldn't hurt.
Can do and will also try to clarify the section in the README.
Also, at least in my mind (as an expert only, maybe), the message Permission denied makes complete sense, since the login is not supposed to succeed at this stage.
Correct. It's basically a trick to turn a simple ssh command which does a "login attempt" into something which looks more like pairing to the user. "Permission denied" is rather misleading and I'd assume that users treat this as an error. Ignoring errors during a security sensitive operation isn't something we should expect or ask for.
There was a problem hiding this comment.
But there still seems to be the problem what happens if the client does not accept keyboard-interactive, right? Then there will neither be the "improved error message" nor the verification string visible on the client side.
There was a problem hiding this comment.
Correct. In that case it's not distinguishable from the client whether the connection got accepted by ssh-pairing-server or a full sshd. In the README it's meanwhile documented that the message on the client is mandatory, so without keyboard-interactive this doesn't work (securely).
There was a problem hiding this comment.
The clean approach would be not to print out any keys until some level of trust and consistency is established.
Technically that's not established in the server component at all, that's only after the user confirms that in a later step. From my PoV, the server executable is just a component of a complete system and not by itself complete. The README mentions explicitly that the output must not be used directly.
There was a problem hiding this comment.
That's true. But at this stage you know that something is off, so why risk continue processing the data?
There was a problem hiding this comment.
It doesn't really process it though.
There was a problem hiding this comment.
All right, it offers it for processing by another component. But why?
There was a problem hiding this comment.
Less complexity in the server component. I don't see any downside to offer it. Validation happens only after the process exits anyway.
|
|
||
| ssh_message message; | ||
| while ((message = ssh_message_get(session))) { | ||
| /* I would move the processing done in the while loop into a |
There was a problem hiding this comment.
Then I'd either need global variables or have to pass multiple pointers...
There was a problem hiding this comment.
You could introduce a struct keeping the runtime program state this could generally improve readability.
| * any kind of informational output messages to the | ||
| * server side. | ||
| * | ||
| * Then you could also drop this INTERACTIVE auth path |
There was a problem hiding this comment.
Some kind of response that indicates a successful reception is IMO important, as it tells the client that it was the "chosen one" for the enrollment. I can remove the from %s@%s part though if you think that it's not useful or even dangerous.
There was a problem hiding this comment.
So you see this as part of the verification process. It could even work out ... the client verifies the server's host key, so at this point the messages returned from the server should be trusted. No man-in-the-middle or IP-spoofer should be able to get to this point.
In that case this is a vital part of the verification process and the user needs to be clearly instructed to look out for this message. It might even make sense to report back the fingerprints of the individual keys seen by the server.
The from %s@%s part can be safely dropped I guess it doesn't add much value for the legitimate client but could pose a small information leak for non-legit clients.
There was a problem hiding this comment.
So you see this as part of the verification process. It could even work out ... the client verifies the server's host key, so at this point the messages returned from the server should be trusted. No man-in-the-middle or IP-spoofer should be able to get to this point.
Yep!
In that case this is a vital part of the verification process and the user needs to be clearly instructed to look out for this message.
I'll try to make that more clear.
It might even make sense to report back the fingerprints of the individual keys seen by the server.
That's what I did initially (before 26751d9). It's why authorized_keys is stored as a string instead of printed immediately. I found that there's a maximum length of strings for keyboard interactive apparently so they were cut off and I decided to just keep it simple and print the number. Showing the truncated data like jeos-firstboot does would probably be possible but is no longer secure. Using the sha256 hashes like ssh-keygen -l might work, but I don't think for authorized_keys that's a format users are used to, while the ssh-rsa AAA... string is recognizable.
There was a problem hiding this comment.
Ah, so repurposing this channel for public key exchange is a bit hairy after all. A condensed message will have to suffice then I guess.
| * if these should ever change then we get an inconsistency. would it | ||
| * be possible to query these paths from libssh? | ||
| * | ||
| * furthermore, what if in sshd_config different settings are found? I |
There was a problem hiding this comment.
There's some support for sshd_config parsing like OpenSSH does, but there are differences. libssh needs explicit HostKey options in the config, while sshd uses the ones listed here if no HostKey appears in the config. I don't see any way to work around that. Worth a feature request upstream I suppose.
As a workaround it would be possible to pass a libssh compatible config file explicitly, either as argument to ssh-pairing-server or by using the library default location of /etc/libssh/libssh_server.config.
There was a problem hiding this comment.
That's a pitty. So the pairing-server is not a full drop-in replacement for sshd. And this can have undesirable side effects for non-default configurations.
There was a problem hiding this comment.
Yeah, but those are rare, especially when the system is still being set up and there was no opportunity for the user to change anything yet.
I've got to file the libssh issue upstream still, once it's fixed it'll "just work" (tm)
There was a problem hiding this comment.
I've got to file the libssh issue upstream still, once it's fixed it'll "just work" (tm)
2 participants
This PR# serves for providing some inline code review comments as per bsc#1218959.