Opened 3 years ago

Closed 3 years ago

#8108 enhancement closed fixed (fixed)

conch: support hmac-sha2-256 and hmac-sha2-512

Reported by: Colin Watson Owned by: Colin Watson
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: branches/conch-ssh-hmac-sha2-8108
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

RFC 6668 defines hmac-sha2-256 and hmac-sha2-512 MAC algorithms for SSH. conch can easily support both, and should, since hmac-sha1 is beginning to look a bit creaky.

I'll attach a patch shortly to add these.

Attachments (1)

conch-hmac-sha2.patch (4.9 KB) - added by Colin Watson 3 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by DefaultCC Plugin

Cc: z3p added

Changed 3 years ago by Colin Watson

Attachment: conch-hmac-sha2.patch added

comment:2 Changed 3 years ago by Colin Watson

Keywords: review added
Owner: Colin Watson deleted

This should do the job. The only bit that was at all non-trivial is that SHA-512 has a different block size, so I needed to remove hardcoding of 64.

The test method names aren't ideal. I would prefer test_hmac_sha2_512 etc. myself, but that doesn't seem to be permitted by Twisted's coding standards and my attempts to use underscores in test method names like that have failed review in the past. Suggestions welcome if you also don't like this!

comment:3 Changed 3 years ago by Adi Roiban

Author: adiroiban
Branch: branches/conch-ssh-hmac-sha2-8108

(In [46228]) Branching to conch-ssh-hmac-sha2-8108.

comment:4 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Colin Watson

Thanks!

Changes looks good.

I also don't like the test names... but yes, this is the convention. I will try to see if we can change this.

The current sha2-512 test is failing.... i think that this is due to a bad assertion.

I tried to fix the tests.

While looking a the code I found out that for none getMAC returns a simple tuple and the result no longer has the key attached. I think that this is a bug and should be followd up in a separate ticket.

The new tests contain a lot of duplication. I have tried to refactor this into a custom assertion call.

While working on this I have also update the client example to show the logs.

If you have time, please consider reviewing https://twistedmatrix.com/trac/ticket/7715 so that we can also have an up to date sample ssh server.

Please check my latest changes and make sure they are OK.

Changes sent to buildbot.

Thanks!

comment:5 Changed 3 years ago by Colin Watson

Owner: changed from Colin Watson to Adi Roiban

Thanks for fixing that up. I swear the tests passed for me before I submitted this! I must have made a mistake somewhere ...

Anyway, your test corrections look fine. I think it would be more readable if you passed blockSize rather than blockPadSize to assertGetMAC, though, and had assertGetMAC calculate the required padding with (blockSize - digestSize). Precomputing the padding makes it harder to do things like "git grep -w 128 twisted/conch" to look for places where block sizes are hardcoded.

comment:6 Changed 3 years ago by Adi Roiban

Owner: changed from Adi Roiban to Colin Watson

I did not wanted to pass the blockSize to avoid duplicating the same logic used in the production code.

Why would you want to look for hardcoded block sizes in tests?

I think that in test, those values should be hardcoded or decoupled from the production code so that the checks are somehow more reliable.

Again, many thanks for the patch!

re-assigning before merge

comment:7 Changed 3 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [46231]) Merge conch-ssh-hmac-sha2-8108: Add support in twisted.conch.ssh for hmac-sha2-256 and hmac-sha2-512.

Author: cjwatson Reviewer: adiroiban Fixes: #8108

Note: See TracTickets for help on using tickets.