[Asterisk-code-review] STIR/SHAKEN: Switch to base64 URL encoding. (asterisk[16])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15848 )

Change subject: STIR/SHAKEN: Switch to base64 URL encoding.
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/c/asterisk/+/15848/2/include/asterisk/utils.h 
File include/asterisk/utils.h:

https://gerrit.asterisk.org/c/asterisk/+/15848/2/include/asterisk/utils.h@291 
PS2, Line 291: int ast_base64url_encode_full(char *dst, const unsigned char *src, int srclen, int max, int linebreaks);
Docs for this and how is it different from _encode?


https://gerrit.asterisk.org/c/asterisk/+/15848/2/include/asterisk/utils.h@314 
PS2, Line 314: char *ast_base64url_decode_string(const char *src);
So the _decode and _encode variants are for binary data and the _string variants are for when you have or expect a string, yes?  Might want to state that.


https://gerrit.asterisk.org/c/asterisk/+/15848/2/main/utils.c 
File main/utils.c:

https://gerrit.asterisk.org/c/asterisk/+/15848/2/main/utils.c@73 
PS2, Line 73: static char base64[64];
            : static char base64url[64];
            : static char b2a[256];
            : static char b2a_url[256];
Shouldn't these be in thread local storage?  Otherwise 2 threads could be trying to access it at the same time.  Even with TLS, you should document the concurrency restrictions in the header file.  
How are the initialized?  If there's data in these left over from a previous encode/decode, what will happen?  Do you need a base64_init function?  Maybe it's be better to have the caller pass in a work buffer.

Actually looking below...  Do these even need to be here at all?  They're small enough that they could be declared in the functions themselves.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15848
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Icf094a2a54e87db91d6b12244c9f5ba4fc2e0b8c
Gerrit-Change-Number: 15848
Gerrit-PatchSet: 2
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Mon, 03 May 2021 12:36:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210503/4d3b7c6d/attachment.html>