[DISCUSS] Add a SBuf-based Base64 encoder/decoder - #2452
Conversation
yadij
added
the
M-ignored-by-merge-bots
|
The only issue I see is that Squid base64 encoding is only used on buffers. So we would end up adding a stream just for the purpose of using the encoder. |
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
I had already flagged this API direction as problematic before this PR was posted. Will find time to detail that claim.
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
IMO, the serious problems identified in this incomplete review are enough to warrant switching to a different approach to base64-encoding. There are other, mostly secondary problems in this PR that this review does not flag. Most will probably disappear on the adjusted path.
I can suggest starting with an API like this:
namespace AnyP {
/// A base64-encoded version of the given input.
SBuf Base64Encode(const SBuf &);
} // namespace AnyP
kinkie
changed the title
kinkie
added
the
S-waiting-for-reviewer
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
this might be in a state where it's discussable. It has the encoder, decoder, and some users. What do you think?
This is shaping nicely IMO, thank you! In addition to specific change requests, please finalize PR title/description and go through the changes to fix const, AAA, and odd formatting that misses spaces around = and between function call arguments.
| #include "sbuf/forward.h" | ||
|
|
||
| /// Thrown by Base64Decode() when the input is not valid base64. | ||
| class DecodeException : public TextException |
There was a problem hiding this comment.
Please do not introduce a custom exception type to report decoding errors, especially when that type is effectively unused outside of unit tests. Instead, return std::optional<SBuf>, especially since the only non-test caller immediately handles decoding errors (i.e. without propagating them a lot further up the stack to some distant high-level code).
This is the second time we are discussing a recent attempt to throw from a decoding function. The first one resulted in a AnyP::Uri::Decode()-related bug fixed in commit ad365ed. I am not claiming there is a stable pattern here, but we should be mindful of that first failure. Let's drop exceptions from this code, at least as a starting point.
| SBuf toEncode(username); | ||
| toEncode.append(request->peer_login + 1); | ||
| SBuf encoded = Base64Encode(toEncode); | ||
| httpHeaderPutStrf(hdr_out, header, "Basic %.*s", (int)encoded.length(), encoded.rawContent()); |
There was a problem hiding this comment.
Please upgrade this code to use SQUIDSBUFPH and friends now that it is printing an SBuf. Same for other similar cases.
rousskov
removed
the
S-waiting-for-reviewer
| char *dst, | ||
| uint8_t src); | ||
|
|
||
| /* Returns the number of output characters. DST should point to an |
There was a problem hiding this comment.
Given how many cases were missed by recent commit 74b1276 and followup commit 19a590c, I think this PR should rename base64_encode_update() declared below to base64_encode_updateXXX(). We clearly cannot rely on PR authors finding remaining relevant cases; we need the compiler to flag them for us. Hopefully, after this PR, there will not be many such cases left.
There was a problem hiding this comment.
this PR should rename base64_encode_update() declared below to base64_encode_updateXXX()
No. base64_encode_update() is part of the libnettle API, of which this file is an extract for systems that do not carry it.
There was a problem hiding this comment.
No. base64_encode_update() is part of the libnettle API, of which this file is an extract for systems that do not carry it.
Given how many cases were missed by recent commit 74b1276 and followup commit 19a590c, I think a better reaction would look more like this:
Sounds good, and I will also add a trivial wrapper with the same base64_encode_updateXXX() name to keep libnettle-based builds happy.
3 participants
Stemming from the converstaions in PR #2447 which focuses on
addressing a problem as narrowly as possible, develop a longer-term solution,
based around SBuf