aes(128|256)-gcm support #845
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Could someone review this please ? Maybe @mfazekas ? |
|
Maybe i'll need some explanations on integration tests cause i can't reproduce fails on my local setup. |
There are some instructions to getting integration tests running with vagrant: |
I'm aware about the instructions, and I've succeeded to run tests on my server. But actually, both integration and unit tests runs successfully. Or at least it looks like. If someone have some time to checkout my branch and to try to run tests, it would be helpful. In the top of that, if the CI could be more verbose, it would help me a lot. |
@Slokilla when I'm running your branch I get a lot of errors locally as well: CI seems to timeout on a test that's deadlocked that's why we don't see any logs. while master passes just fine |
|
@Slokilla can we work on this on the next couple weeks? |
flux-benj
reviewed
May 17, 2023
|
@Slokilla Is our assumption correct that this change would be supported for v6.0 and above? |
Slokilla
force-pushed
the
gcm
branch
2 times, most recently
from
bb6f8fc
to
60eba6b
Compare
Slokilla
force-pushed
the
gcm
branch
2 times, most recently
from
d3a5abb
to
98e153f
Compare
|
@flux-benj @mfazekas can you take a look ? |
mfazekas
approved these changes
May 21, 2023
flux-johnm
added a commit
to flux-johnm/net-ssh
that referenced
this pull request
May 21, 2023
A fork from net-ssh#845 Add support for AEAD-AES256-GCM Add support for AEAD-AES128-GCM Adding test for aead ciphers Add aes256-gcm cipher mode Fixing exising tests
flux-benj
reviewed
May 22, 2023
| end | ||
|
|
||
| def padding=(pad) | ||
| # DO NOTHING (always 0) |
There was a problem hiding this comment.
Minor nit: this comment could be nicer to readers and explain that the padding content is 0 and this does not refer to padding length.
| server.cipher.auth_data = payload_length | ||
| payload = server.cipher.update(@packet.to_s) + server.final_cipher | ||
| padding_length = payload[0].unpack('C').first.to_i | ||
| payload = payload[1..@packet_length - padding_length - 1] |
There was a problem hiding this comment.
Note: This range looks correct against the RFC but might pay to double check.
Slokilla
force-pushed
the
gcm
branch
2 times, most recently
from
1432501
to
18808a4
Compare
Ok, I've added it. It fails, and i don't understand why. Would you please take a look @mfazekas ? |
flux-benj
reviewed
May 22, 2023
| line | ||
| end | ||
| end | ||
| config_lines.push("Ciphers #{cipher}") |
There was a problem hiding this comment.
The CI failures suggest to me that the ssh server is failing to start which generates a pid, since the config file is derived from the tempfile which the config_lines are written to. I've not tested this but from an eyeball, looks to be due to writing the cipher as aes128-gcm where a valid cipher configuration for ssh_config is aes128-gcm@openssh.com
There was a problem hiding this comment.
Yep the log file shows:
Bad SSH2 cipher spec 'aes128-gcm'.There was a problem hiding this comment.
May I ask you where did you find this log line ?
There was a problem hiding this comment.
If you pass the debug: True flag to start_sshd_7_or_later then it'll print the log path of that's the sshd. I'll also add a PR to tail the log file if sshd fails to start:
20dd0f4
mfazekas
reviewed
May 23, 2023
mfazekas
reviewed
May 23, 2023
mfazekas
requested changes
May 23, 2023
There was a problem hiding this comment.
@Slokilla can you run your version against an actual ssh server?
I've tried sshd in osx. And it wasn't working for me at all, server disconnected before password, that's can be a HMAC verification issue. Same issue that shown by integration server:
$:.push('./lib')
require 'net/ssh'
require 'byebug'
puts Net::SSH::Version::CURRENT
Net::SSH.start("localhost", ENV.fetch("USER", nil), encryption: "aes256-gcm@openssh.com", verbose: :debug) do |ssh|
ssh.exec! 'echo Foo '
end7.1.0
D, [2023-05-23T21:00:57.448434 #42854] DEBUG -- net.ssh.transport.session[21c]: establishing connection to localhost:22
D, [2023-05-23T21:00:57.450681 #42854] DEBUG -- net.ssh.transport.session[21c]: connection established
I, [2023-05-23T21:00:57.450767 #42854] INFO -- net.ssh.transport.server_version[230]: negotiating protocol version
D, [2023-05-23T21:00:57.450791 #42854] DEBUG -- net.ssh.transport.server_version[230]: local is `SSH-2.0-Ruby/Net::SSH_7.1.0 x86_64-darwin20'
D, [2023-05-23T21:00:57.469551 #42854] DEBUG -- net.ssh.transport.server_version[230]: remote is `SSH-2.0-OpenSSH_8.6'
I, [2023-05-23T21:00:57.471082 #42854] INFO -- net.ssh.transport.algorithms[244]: sending KEXINIT
D, [2023-05-23T21:00:57.471250 #42854] DEBUG -- socket[258]: queueing packet nr 0 type 20 len 876
D, [2023-05-23T21:00:57.471325 #42854] DEBUG -- socket[258]: sent 880 bytes
D, [2023-05-23T21:00:57.483302 #42854] DEBUG -- socket[258]: read 1056 bytes
D, [2023-05-23T21:00:57.483413 #42854] DEBUG -- socket[258]: received packet nr 0 type 20 len 1052
I, [2023-05-23T21:00:57.483456 #42854] INFO -- net.ssh.transport.algorithms[244]: got KEXINIT from server
I, [2023-05-23T21:00:57.483529 #42854] INFO -- net.ssh.transport.algorithms[244]: negotiating algorithms
D, [2023-05-23T21:00:57.483607 #42854] DEBUG -- net.ssh.transport.algorithms[244]: negotiated:
* kex: curve25519-sha256
* host_key: ssh-ed25519
* encryption_server: aes256-gcm@openssh.com
* encryption_client: aes256-gcm@openssh.com
* hmac_client: hmac-sha2-512-etm@openssh.com
* hmac_server: hmac-sha2-512-etm@openssh.com
* compression_client: none
* compression_server: none
* language_client:
* language_server:
D, [2023-05-23T21:00:57.483629 #42854] DEBUG -- net.ssh.transport.algorithms[244]: exchanging keys
D, [2023-05-23T21:00:57.483990 #42854] DEBUG -- socket[258]: queueing packet nr 1 type 30 len 44
D, [2023-05-23T21:00:57.484057 #42854] DEBUG -- socket[258]: sent 48 bytes
D, [2023-05-23T21:00:57.489188 #42854] DEBUG -- socket[258]: read 208 bytes
D, [2023-05-23T21:00:57.489272 #42854] DEBUG -- socket[258]: received packet nr 1 type 31 len 188
D, [2023-05-23T21:00:57.489886 #42854] DEBUG -- socket[258]: queueing packet nr 2 type 21 len 20
D, [2023-05-23T21:00:57.489947 #42854] DEBUG -- socket[258]: sent 24 bytes
D, [2023-05-23T21:00:57.490016 #42854] DEBUG -- socket[258]: received packet nr 2 type 21 len 12
D, [2023-05-23T21:00:57.490324 #42854] DEBUG -- net.ssh.authentication.session[26c]: beginning authentication of `boga'
D, [2023-05-23T21:00:57.490382 #42854] DEBUG -- socket[258]: using encrypt-then-mac
D, [2023-05-23T21:00:57.490484 #42854] DEBUG -- socket[258]: queueing packet nr 3 type 5 len 32
D, [2023-05-23T21:00:57.490555 #42854] DEBUG -- socket[258]: sent 100 bytes
D, [2023-05-23T21:00:57.491934 #42854] DEBUG -- socket[258]: read 0 bytes
Traceback (most recent call last):
13: from ./tryout/test-gcm.rb:18:in `<main>'
12: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh.rb:259:in `start'
11: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/authentication/session.rb:61:in `authenticate'
10: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/authentication/session.rb:130:in `expect_message'
9: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/authentication/session.rb:101:in `next_message'
8: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/authentication/session.rb:101:in `loop'
7: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/authentication/session.rb:102:in `block in next_message'
6: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/transport/session.rb:175:in `next_message'
5: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/transport/session.rb:190:in `poll_message'
4: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/transport/session.rb:190:in `loop'
3: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/transport/session.rb:193:in `block in poll_message'
2: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/transport/packet_stream.rb:102:in `next_packet'
1: from /Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/transport/packet_stream.rb:102:in `loop'
/Users/boga/Work/OSS/NetSSH/net-ssh/lib/net/ssh/transport/packet_stream.rb:108:in `block in next_packet': connection closed by remote host (Net::SSH::Disconnect)
Raw ssh was working fine with the same server
ssh -c aes128-gcm@openssh.com localhost -vvvServer shows this error:
ssh_dispatch_run_fatal: Connection from ::1 port 64341: message authentication code incorrect [preauth]
Working on it, gonna ping you when ready. |
|
I have a docker project with a self compile openssh server via full debug options, if need : https://github.com/fwininger/docker_ssh_debug |
I believe this is a fallout from the changes spurred by #845 (comment) - removing the hmac ref to aes-xxx-gcm was right in the configuration but we still need to ensure the aead server/client is used instead of the etm (from combing the logs for five minutes, I may be wrong), just without the entries in the hmac section (IMO) |
Ok, it looks good : require 'net/ssh'
Net::SSH.start('127.0.0.1', 'slo', encryption: 'aes256-gcm@openssh.com') do |ssh|
puts ssh.exec!('echo toto')
endleads to : The error was that cipher.to_s did not return the name of the cipher. |
|
Looks like my last change fails on ruby 2.6 & 2.7, will fix it tommorow. |
|
The test were mocking |
Add support for AEAD-AES256-GCM Add support for AEAD-AES128-GCM Adding test for aead ciphers Add aes256-gcm cipher mode Fixing exising tests Implementing both 128 and 256 aead-aes-gcm Add support for AEAD-AES256-GCM Add support for AEAD-AES128-GCM Adding test for aead ciphers Add aes256-gcm cipher mode Fixing exising tests
|
@Slokilla it's still failing for me: D, [2023-06-10T08:05:23.893095 #4610] DEBUG -- socket[258]: using encrypt-then-mac
D, [2023-06-10T08:05:23.894860 #4610] DEBUG -- socket[258]: queueing packet nr 3 type 5 len 32
D, [2023-06-10T08:05:23.898647 #4610] DEBUG -- socket[258]: sent 100 bytes
D, [2023-06-10T08:05:23.927577 #4610] DEBUG -- socket[258]: read 0 bytes
/net-ssh/lib/net/ssh/transport/packet_stream.rb:108:in `block in next_packet': connection closed by remote host (Net::SSH::Disconnect)
tail /var/log/auth.log
...
Jun 10 08:05:23 ubuntu-jammy sshd[4611]: debug1: KEX done [preauth]
Jun 10 08:05:23 ubuntu-jammy sshd[4611]: ssh_dispatch_run_fatal: Connection from 127.0.0.1 port 55402: message authentication code incorrect [preauth]
...Same for aes256: |
mfazekas
reviewed
Jun 10, 2023
Comment on lines
+164
to
+177
| elsif client.hmac.aead | ||
| # Details of this implementation can be found in RFC 5647 | ||
| unencrypted_data = [padding_length, payload, padding].pack("CA*A*") | ||
|
|
||
| # Despite SSH spec, when using AEAD encryption, packet_length is not ciphered. It is transported as identity, | ||
| # and added in the auth_data. See RFC 5647 7.3 | ||
| client.cipher.auth_data = [packet_length].pack('N') | ||
| encrypted_data = client.update_cipher(unencrypted_data) | ||
| encrypted_data << client.final_cipher | ||
|
|
||
| # For SSH, GCM auth_tag acts as MAC | ||
| mac = client.auth_tag | ||
|
|
||
| message = [packet_length].pack("N") + encrypted_data + [mac].pack("A*") |
There was a problem hiding this comment.
I'm refactoring this part in chacha20-poly1035, and it looks like this:
if client.cipher.implicit_mac?
unencrypted_data = [padding_length, payload, padding].pack("CA*A*")
message = client.cipher.update_cipher_mac(unencrypted_data, client.sequence_number)I think this would work for this use case as well, what do you think?
net-ssh/lib/net/ssh/transport/packet_stream.rb
Lines 147 to 149 in d211cba
mfazekas
reviewed
Jun 10, 2023
| padding_length = @packet.read_byte | ||
|
|
||
| payload = @packet.read(@packet_length - padding_length - 1) | ||
| if server.hmac.aead |
There was a problem hiding this comment.
FWIW This is what the chachapoly version looks like:
net-ssh/lib/net/ssh/transport/packet_stream.rb
Lines 265 to 269 in 5bd3223
|
@Slokilla I've merged chacha20-poly1305 which creates a lot of conflicts. As noted #845 (comment) I'm still getting incorrect MAC from SSHD server when checking out the latest version of your branch. I've tried that on ab76e3c. If you can fix that I can merge the your changes with the ChaCha poly changes. |
|
Coming back to this, we have a strong desire to get this in and released so we can get off of our internal fork which adds this cipher. This MR looks to be disbanded so was wondering if this work is going to be picked up again or we should start a new MR considering the comments above. Thoughts? |
|
@flux-benj so my main issue with this request is that it was not working, when I've tried with openssh #845 (comment) . Probably something trivial, but debugging it and fixing is requires a bit of an effort. So if you have something working, pls open a draft PR, and we can go from there. You should be able to use the intergration test from this PR: |
|
@mfazekas / @flux-benj : this PR gives the direction to make the support of aes-gcm, however we don't have the time to complet this features. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've added the support for the GCM algorithmes, whoch actually implies the support of AEAD algorithms.
Used cipher name are :
Example call:
or
I think the more sensitive point is the implementation of the GCM iv/counter, I really need you to pay a special intention to it during review.
Close #834