ENT-14104: Added GETPATCH protocol command for serving leech2 patches#6163
ENT-14104: Added GETPATCH protocol command for serving leech2 patches#6163larsewi wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
larsewi
force-pushed
the
leech2-patch-stream
branch
2 times, most recently
from
6ae1b0d to
7023be6
Compare
|
@cf-bottom Jenkins please :) |
larsewi
requested review from
SimonThalvorsen,
craigcomstock,
olehermanse and
victormlg
larsewi
force-pushed
the
leech2-patch-stream
branch
from
7023be6 to
55294e4
Compare
|
Alright, I triggered a build: Jenkins: https://ci.cfengine.com/job/pr-pipeline/13939/ Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-13939/ |
The hub sends 'GETPATCH <hash>' over the established TLS connection, and the server replies with a leech2 patch using a new patch stream API, which reuses the wire format from the file stream protocol. The patch is created by an enterprise hook; CFEngine Community refuses the request. This introduces protocol version 5 - "leech2". Ticket: ENT-14104 Changelog: Title Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
larsewi
force-pushed
the
leech2-patch-stream
branch
from
55294e4 to
4054ba7
Compare
olehermanse
left a comment
olehermanse
left a comment
There was a problem hiding this comment.
Looks good overall. Some comments / suggestions below.
| int ret = sscanf(recvbuffer, "GETPATCH %63s", last_hash); | ||
| if (ret != 1) | ||
| { | ||
| goto protocol_error; | ||
| } |
There was a problem hiding this comment.
Could check that last_hash only contains the allowed characters here. I.e. since hex hashes can only be a-f + 0-9, we could reject it if it contains other characters like /.;'\ etc.
| while (!eof) | ||
| { |
There was a problem hiding this comment.
We should probably set some limit here and handle the case where client is trying to send patches "forever"?
| /* Refuse using the stream protocol, as opposed to | ||
| * RefuseAccess(), because the client expects stream protocol | ||
| * messages after sending the GETPATCH request. */ | ||
| PatchStreamRefuse(ConnectionInfoSSL(conn->conn_info)); |
There was a problem hiding this comment.
Here I think you should handle the return value of PatchStremRefuse, or comment on why it is not necessary.
| /* A failed patch stream is not fatal to the connection. The | ||
| * enterprise hook does all relevant Log()ing. */ | ||
| ServeLeech2Patch(conn, last_hash); |
There was a problem hiding this comment.
AI review:
The comment says a failed stream "is not fatal to the connection," which is fine for the application-level failure, but if the refusal message itself fails to send the TLS connection may be in a broken state and the busy-loop will keep trying to serve it. Existing handlers (e.g., COOKIE → ReturnCookies) at least break to the protocol-error path on failure.
| return false; | ||
| } | ||
|
|
||
| if (msg_len > 0) |
There was a problem hiding this comment.
When do we receive a 0 length message?
| * @param data Is set to the received patch buffer (caller takes ownership | ||
| * and must free it with free()) |
There was a problem hiding this comment.
Highlight the fact that it may be set to NULL and must be checked for NULL before access?
| Log(LOG_LEVEL_VERBOSE, "%14s %7s %.7s...", | ||
| "Received:", "GETPATCH", last_hash); |
There was a problem hiding this comment.
This won't be formatted correctly / as expected. %7s is meant to pad anything below 7 characters up to 7, so all of them are aligned - however GETPATCH is 8 characters, so all those log messages will be misaligned. We should probably change this and all the other log message format strings to %8s.
4 participants
The hub sends 'GETPATCH ' over the established TLS connection, and the server replies with a leech2 patch using a new patch stream API, which reuses the wire format from the file stream protocol. The patch is created by an enterprise hook; CFEngine Community refuses the request. This introduces protocol version 5 - "leech2".
Ticket: ENT-14104
Together with: