alibabacloud: don't use ReadAll in metadata response read #22479
Conversation
nebril
added
the
release-note/misc
| respBytes := make([]byte, 0, resp.ContentLength) | ||
| _, err = reader.Read(respBytes) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
| respBytes := make([]byte, 0, resp.ContentLength) | |
| _, err = reader.Read(respBytes) | |
| if err != nil { | |
| respBytes := make([]byte, 0, resp.ContentLength) | |
| n, err = reader.Read(respBytes) | |
| if err != nil { | |
| ... | |
| } | |
| if n == bufferSize && respBytes[n-1] != EOF { | |
| // Double buffersize and try again. If it still fails, log an error: "AlibabaCloud metadata buffer size too small in Please report to developers" | |
| } |
Is it worth checking if the last byte received is EOF, in the case the metadata payload size ever increases passed 1MB?
There was a problem hiding this comment.
Might as well make the buffer 2MB then. Do you think 1MB is not enough for alibaba node metadata?
There was a problem hiding this comment.
I just have no idea how much metadata could be returned. I'm interested to make Cilium more robust in case of failures where we've defined hard limits. I'd rather that we find a way to warn somehow when we've reached a limit, rather than just silently bomb out. I'm less concerned about the buffer size.
There was a problem hiding this comment.
Maybe what I'm saying here is that if we think 1MB is more than enough, then let's keep it at that. But let's also add the logic to detect if the metadata exceeded that buffer, and warn if so.
There was a problem hiding this comment.
@christarazi As far as I could tell, all instances of getMetadata are logging the error returned from this call. I will add logging here, but am concerned this will cause the same error to be logged twice. On the other hand I agree that having the buffer be to small is a good thing to get fixed as soon as possible.
christarazi
added
kind/enhancement
|
@nebril ping |
This change limits how big metadata response can be to avoid leaking memory by potential attackers doctoring very big HTTP responses. Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
nebril
force-pushed
the
pr/nebril-fix-read-all
branch
from
7ef8b03
to
ab4a3aa
Compare
| if _, err := reader.ReadByte(); err == nil { | ||
| log.Error("Buffer size too small. Please report to developers") | ||
| return "", io.ErrShortBuffer | ||
| } |
There was a problem hiding this comment.
Why are we checking for nil error? I don't think I'm understanding
There was a problem hiding this comment.
My initial implementation was wrong, I assumed that buffered reader would return error if the size of buffer would be exceeded which ended up not being the case.
ReadByte returns error if there is no new byte to read. err == nil means that there are more bytes to read, which means that the response is bigger than 1MB, so we log an error and return.
aanm
added
the
needs-backport/1.13
|
Superseded by #22602 |
|
@christarazi @nebril Maybe we should keep this one open because we would like to backport this to all branches. |
joestringer
removed
the
needs-backport/1.13
This change limits how big metadata response can be to avoid leaking memory by potential attackers doctoring very big HTTP responses.
Signed-off-by: Maciej Kwiek maciej@isovalent.com