Hi Moemen,
On Fri, Apr 02, 2021 at 01:38:59AM +0200, Moemen MHEDHBI wrote:
> I have came across the same use-case as Jonathan so I gave it a try and
> implemented the converters for base64url variant.
>
> - Regarding the converters name, I have just prefixed "u" and used
> ubase64/ub64dec. Let me know if the names are not appropriate or if you
> rather prefer to add an argument to existing converters.
Thanks for this. I know it's not your fault but the initial names
"base64" and "b64dec" are quite messy because the former doesn't
indicate the direction. Thus I'd rather not replicate this error,
and at least make sure that "enc" or "dec" is explicit in both
keywords. Maybe "ub64enc" and "ub64dec" or ubase64enc/ubase64dec
are better (though shorter forms are often preferred for converters).
> - RFC1421 mention in base64.c is deprecated so I replaced it with
> RFC4648 to which base64/b64dec converters seem to still apply.
OK! Please do mention this in your commit message as well, after
all you made the effort of justifying it here, but the commit
message is the place for justifying a change :-)
> - not sure if samples list in sample.c should be formatted/aligned after
> the converters added in this patch. They seemed to be already not
> completely aligned. Anyway I have done the aligning on a separate patch
> so you can squash it or drop it at your convenience.
It's often the problem with such lists. Nobody wants to realign
everything when they're the first to break alignment, and at some point
too much is too much and some cleanup is needed. Thanks for doign this
one!
> Testing Example:
> haproxy.cfg:
> http-request return content-type text/plain lf-string
> %[req.hdr(Authorization),word(2,.),ub64dec]
>
> client:
> TOKEN =
> eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyIjoiZm9vIiwia2V5IjoiY2hhZTZBaFhhaTZlIn0.5VsVj7mdxVvo1wP5c0dVHnr-S_khnIdFkThqvwukmdg
> $ curl -H "Authorization: Bearer ${TOKEN}" 127.0.0.1:8080
> {"user":"foo","key":"chae6AhXai6e"}
You can put that in the commit message, it's extremely useful when
bissecting or even for those who would like to backport it.
I'm having some comments below about style issues however.
> --- a/src/base64.c
> +++ b/src/base64.c
> @@ -138,6 +138,58 @@ int base64dec(const char *in, size_t ilen, char *out,
> size_t olen) {
> return convlen;
> }
>
> +/* url variant of a2base64 */
> +int a2base64url(char *in, int ilen, char *out, int olen){
^
Opening brace on the next line please.
> + int convlen;
^ empty line after variables declarations
> + convlen = a2base64(in,ilen,out,olen);
^ ^ ^
Spaces after commas in function arguments
> + while (out[convlen-1]=='='){
^ ^ ^
spaces around operators
> + convlen--;
> + out[convlen]='\0';
^^
> + }
> + for(int i=0;i<convlen;i++){
^
no declaration inside the loop please.
Same general comments for the rest of the function overall.
> +/* url variant of base64dec */
> +int base64urldec(const char *in, size_t ilen, char *out, size_t olen) {
(...)
> + switch (ilen%4){
^
space before opening brace
> + case 0:
> + break;
> + case 2:
> + conv[ilen]= '=';
> + conv[ilen+1]= '=';
> + conv[ilen+2]= '\0';
> + ilen +=2;
> + break;
Please keep your indentation consistent.
> + case 3:
> + conv[ilen]= '=';
> + conv[ilen+1]= '\0';
> + ilen +=1;
> + break;
> + default:
> + return -1;
> + }
> + return base64dec(conv,ilen,out,olen);
^^
indentation issue here as well
Thanks!
Willy
