#12184 Add support in conch.ssh to ETM HMACs. #12186
Conversation
CodSpeed Performance ReportMerging #12186 will not alter performanceComparing Summary
|
src/twisted/conch/ssh/transport.py
Outdated
| @@ -214,7 +216,7 @@ def _getMAC( | |||
| result.key = key | |||
| return result | |||
|
|
|||
| def encrypt(self, blocks): | |||
| def _encrypt(self, blocks): | |||
There was a problem hiding this comment.
SSHCiphers is kind of a public API, so for the final merge we might need to revert this change.
But I think that this API should be private
src/twisted/conch/ssh/transport.py
Outdated
| @@ -257,6 +259,49 @@ def makeMAC(self, seqid, data): | |||
| data = struct.pack(">L", seqid) + data | |||
| return hmac.HMAC(self.outMAC.key, data, self.outMAC[0]).digest() | |||
|
|
|||
| def createPacket(self, payload: bytes, outgoingPacketSequence: int) -> bytes: | |||
There was a problem hiding this comment.
I moved this in ciphers as I feel that it can help reduce the API for SSHCiphers.
But I am ok to move it back to SSHTransportBase
This is used for sending a packet, and in this way, you don't need to expose the isETM info the the transport.
But for receiving data the code is still in SSHTransportBase
src/twisted/conch/ssh/transport.py
Outdated
| + payload | ||
| + randbytes.secureRandom(lenPad) | ||
|
|
||
| wirePacket = self.currentEncryptions.createPacket( |
There was a problem hiding this comment.
This might be moved out of SSHCiphers and have a private helper in SSHTransportBase
src/twisted/conch/ssh/transport.py
Outdated
| # Not enough data for a block | ||
| return | ||
|
|
||
| try: | ||
| if self.currentEncryptions.inMACType.endswith(b"-etm@openssh.com"): |
There was a problem hiding this comment.
Maybe we can have something like:
| if self.currentEncryptions.inMACType.endswith(b"-etm@openssh.com"): | |
| if self.currentEncryptions.isETM(): |
| return self.inMACType.endswith(b"-etm@openssh.com") | ||
|
|
||
|
|
||
| class _InvalidPacket(Exception): |
There was a problem hiding this comment.
This exception is used to share more code between ETM and MTE code.
| """ | ||
| @return True if outgoing MAC is ETM. | ||
| """ | ||
| return self.outMACType.endswith(b"-etm@openssh.com") |
There was a problem hiding this comment.
| @@ -279,6 +281,24 @@ def verify(self, seqid, data, mac): | |||
| outer = hmac.HMAC(self.inMAC.key, data, self.inMAC[0]).digest() | |||
| return hmac.compare_digest(mac, outer) | |||
|
|
|||
| def isOutMACETM(self) -> bool: | |||
| """ | |||
| @return True if outgoing MAC is ETM. | |||
There was a problem hiding this comment.
| @return True if outgoing MAC is ETM. | |
| @return: True if outgoing MAC is Encrypt-Then-MAC, otherwise False. |
|
|
||
| def isInMACETM(self) -> bool: | ||
| """ | ||
| @return True if incoming MAC is ETM. |
There was a problem hiding this comment.
| @return True if incoming MAC is ETM. | |
| @return: True if incoming MAC is Encrypt-Then-MAC, otherwise False |
|
|
||
| class _InvalidPacket(Exception): | ||
| """ | ||
| Used internally when an invalid packet was received. |
There was a problem hiding this comment.
| Used internally when an invalid packet was received. | |
| An invalid packet was received. |
| # 4 for the packet length and 1 for the padding length | ||
| totalSize = 5 + len(payload) | ||
| lenPad = bs - (totalSize % bs) | ||
| lenPad = bs - (payloadSize % bs) | ||
| if lenPad < 4: | ||
| lenPad = lenPad + bs |
There was a problem hiding this comment.
| lenPad = lenPad + bs | |
| lenPad += bs |
| """ | ||
| @return True if outgoing MAC is ETM. | ||
| """ | ||
| return self.outMACType.endswith(b"-etm@openssh.com") |
There was a problem hiding this comment.
Is outMACType intended to be a mutable attribute? Perhaps this could be computed in __init__?
There was a problem hiding this comment.
True. Not at __init__ , but at setKeys(). I will do that. Thanks
| + payload | ||
| + randbytes.secureRandom(lenPad) | ||
|
|
||
| # Create the unencrypted packet. |
There was a problem hiding this comment.
Shouldn't this packet construction live in the currentEncryptions attribute? The only reference here to self other than self.currentEncryptions is to self.outgoingPacketSequence, so maybe the thing to do here is return self.currentEncryptions.makePacket(payload, self.outgoingPacketSequence) ?
(This is definitely a big improvement to at least get stuff into a functional shape, but pushing it a little further would put the responsibility in the right place.)
There was a problem hiding this comment.
Shouldn't this packet construction live in the currentEncryptions attribute?
Good question. This was my first refactoring.
And this is why I wanted an initial review for this draft PR.
Take a look at the first commit from the PR and let me know if you think that
return self.currentEncryptions.makePacket(payload, self.outgoingPacketSequence) looks better
My initial refactoring was to move everything into SSHCiphers... but then, this means that SSHCiphers, beside actual cipher stuff it also need to know thinkgs about SSH transport and binary packet format.
This is why in the second version I moved the code to transport.
In the end, I think that is best to keep this createPacket code into SSHTransport and not into SSHCipher
| if self.currentEncryptions.isInMACETM(): | ||
| payload = self._getPacketETM() | ||
| else: | ||
| payload = self._getPacketMTE() |
There was a problem hiding this comment.
I think it would be a lot easier to read if these ...MTE / ...ETM methods were encapsulated within their own object, so there were a protocol for the operation, then a MTE implementation and an ETM implementation, then a call to self._packetGetter.getPacket() (or something) rather than switching to one of two mostly-similar methods.
There was a problem hiding this comment.
If you are going to make this a conditional though, there should still only be one assignment:
| if self.currentEncryptions.isInMACETM(): | |
| payload = self._getPacketETM() | |
| else: | |
| payload = self._getPacketMTE() | |
| payload = self._getPacketETM() if self.currentEncryptions.isInMACETM() else self._getPacketMTE() |
There was a problem hiding this comment.
Thanks.
I will see what we can do... in the future, we will also have an AES-GCM implementation.
so inline if / else is not ok for the long term.
I went with these simple methods inside the SSHTranport as I don't feel that is that much code to move into an object.
| except _InvalidPacket as error: | ||
| self.sendDisconnect( | ||
| DISCONNECT_PROTOCOL_ERROR, | ||
| error.args[0].encode("ascii"), | ||
| ) | ||
| return |
There was a problem hiding this comment.
If we are always discarding the traceback from this new exception, it might be better expressed as an addition to the union; less stack-walking that way, more explicit that it's a possibility. i.e. payload: bytes | None | Literal[Invalid.packet] and then if packet is Invalid.packet: ?
There was a problem hiding this comment.
We don't keep the traceback, but we extract the error message.
I think that what is missing here, is self._log.failure(error.args[0]).
This should make it similar to the other error exception handling.
I prefer to use the exception here as I would like to have the error message, and I feel that raising an exceptoin is the "standard" way of signaling an invalid packet while also having an error details.
From my experience, the whole log calls from the SSH transport are not well designed.
For my production code, I ignore them and I have various custom hooks to add logs.
Improving the logs would be a different tasks for conch.ssh
| try: | ||
| payload = self.incomingCompression.decompress(payload) | ||
| except Exception: | ||
| self._log.failure("Error decompressing payload") |
There was a problem hiding this comment.
love to see the new logging system used as intended :)
There was a problem hiding this comment.
As far as I know, this code is already in trunk.
I don't have much exprience with Twisted log system.
I remember that at some point I tried to document the new logging system, how to use and how to write test for it, but it wasn't a succesfull PR.
|
Many thanks @glyph for the review. It's very useful to get a second opinion. I think that the major comment here is See my inline answer. I kept
So my preference is the current refactoring, with the code hosted in SSHTransportBase. Should we consider the methods of the They are public, but I fell that they should be private and end users should not mess with that code. |
There was a problem hiding this comment.
Tests are failing and I see plenty of feedback that hasn't been responded to or addressed, so I don't think that this ought to be in the review queue.
If you have a specific question for me feel free to @ me and just ask (I'm not sure I see one that needs an answer any more?), I'll do my best to reply; no need to request additional reviews when the code is still in more of a draft state.
Scope and purpose
Fixes #12184
It adds for now
hmac-sha2-256-etm@openssh.comandhmac-sha2-512-etm@openssh.comI don't think that we need
hmac-sha1-etm@openssh.comorhmac-md5-etm@openssh.com.They can be added later.
How to test
ssh server
You can use OpenSSH ssh client to connect using only ETM HMAC