Support ETM (Encrypt-then-MAC) variants for HMAC

[scott-xu]

Should fix #875 and fix #1050 and fix #837 and fix #1092

References:

1. https://github.com/openssh/openssh-portable/blob/cbbdf868bce431a59e2fa36ca244d5739429408d/PROTOCOL
2. https://github.com/openssh/openssh-portable/blob/cbbdf868bce431a59e2fa36ca244d5739429408d/packet.c

Written by hand without testing by now.

[scott-xu]

Would you mind giving a try for this PR? Note: it is written without testing at this moment. Feel free to improve the PR if you want. @PashCracken @SkymaX86 @robert-scheck @alef75 @Jedrzej94 @Ph43n0m @AliceTJ

[Rob-Hague]

Nice. Testing should be straightforward:

diff --git a/test/Renci.SshNet.IntegrationTests/HmacTests.cs b/test/Renci.SshNet.IntegrationTests/HmacTests.cs
index 993e5ec98..e5057248c 100644
--- a/test/Renci.SshNet.IntegrationTests/HmacTests.cs
+++ b/test/Renci.SshNet.IntegrationTests/HmacTests.cs
@@ -58,6 +58,24 @@ public void HmacSha2_512()
             DoTest(MessageAuthenticationCodeAlgorithm.HmacSha2_512);
         }

+        [TestMethod]
+        public void HmacSha1Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacSha1Etm);
+        }
+
+        [TestMethod]
+        public void HmacSha2_256_Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacSha2_256_Etm);
+        }
+
+        [TestMethod]
+        public void HmacSha2_512_Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacSha2_512_Etm);
+        }
+
         private void DoTest(MessageAuthenticationCodeAlgorithm macAlgorithm)
         {
             _remoteSshdConfig.ClearMessageAuthenticationCodeAlgorithms()

[scott-xu]

Do we want to support HmacSha1Etm? IMHO, HmacSha2_256_Etm and HmacSha2_512_Etm is sufficient (and secure). @Rob-Hague

[Rob-Hague]

I think we might as well, while we still have hmac-sha1

[Rob-Hague]

Looks good to me otherwise

[WojciechNagorski]

If it weren't a breaking change, I would approve it. I need some time to think it over and check it out.

@Rob-Hague any idea?

[Rob-Hague]

Yes I think we should go with the bool property on HashInfo and change IKeyExchange.CreateServerHash() from HashAlgorithm CreateServerHash() to HashAlgorithm CreateServerHash(out bool isEncryptThenMac). I don't see the problem with that.

[scott-xu]

Sorry I don't understand why you against HMAC so much.

The HashAlgorithm CreateServerHash(out bool isEncryptThenMac) is a break change anyway.

Shouldn't we focus on the implementation details of ETM? That's the main purpose of this PR.

[scott-xu]

I know everyone else's code always smells, but enough is enough - this is a collaboration, not a competition.

-- @zybexXL

[Rob-Hague]

I think we've made it pretty clear that it's an unnecessary breaking change

IKeyExchange takes a break either way. That's fine: it's in the internals of the library and less likely to affect anyone.

Breaking HashInfo does not need to happen. It is much closer to the common API that people use.

You said you had already gone in this direction in an earlier iteration. I've not seen any reason to stray from that.

Shouldn't we focus on the implementation details of ETM? That's the main purpose of this PR.

I've checked the details. They look good.

[scott-xu]

I've already explained why I abandoned the first iteration. I don't think HashInfo is widely used externally just like you believe IKeyExchange is not widely used externally.

[scott-xu]

To get the ball roll, someone has to compromise. Let me update the PR.

[scott-xu]

Thanks for making the change. It looks great to me. Another big pain point of the library bites the dust 🙂

Perhaps you could update the description to close a bunch of issues

The description has had the format to close the 2 related issues. Are there more?

[Rob-Hague]

Only one more as it turned out, I thought there were a lot more. I've updated the description

[WojciechNagorski]

@scott-xu thanks!

[scott-xu]

Cheers!