Hi everyone, just published my first package in zig, would love to get everyone’s feedback on how to make it better/faster and more idiomatic. I’m open to any criticism.
p.s. I don’t have any plans to make it rfc compliant, this is more of a language learning project.
I have a few comments (I dont do crypto, so no clue there):
Builderbuild? Consider a more helpful name._sign with a 1-to-1 public function pub fn sign ?I hope that helps!
Could you please explain why you have that feeling? (cuz I felt like I followed some ideas from the std lib)
So that I can test token expiration, otherwise the library users would have to provide the current unix time, and I didn’t like that approach. Do you have any idea how I can make that better?
Does it make sense to allocate one big buffer and use like parts of it?
Nice work! Zig is a lot of fun, and it gives you so much control and opens your mind to how computers function in general.
So that I can test token expiration, otherwise the library users would have to provide the current unix time, and I didn’t like that approach.
I noticed exp is passed as an i64, so I feel there would be more parity for iat to also be an i64? Zig relies on documentation to communicate the exact intent behind those values, so if you explain clearly that iat is a Unix timestamp, I think people will get it.
Does it make sense to allocate one big buffer and use like parts of it?
You probably can find a way to do this in one big buffer, but let’s try something a little simpler before jumping that far (if you decide you need to).
Your _sign() and verify() functions have a lot of allocPrint()'s and gpa.alloc()'s in them as they’re composing the web token/base64 elements and stitching everything together. If you’re looking for a good starting place for fewer allocations, limit yourself to use a single Io.Writer.Allocating for the output. Aside from that, the only other allocation should be for composing the data with reusable scratch space that you’ll ideally overwrite several times:
var output: Io.Writer.Allocating = try .initCapacity(gpa, some_reasonable_estimation);
defer output.deinit(); // <-- don't worry, just return an owned slice at the end
var json_scratch_stream: Io.Writer.Allocating = try .initCapacity(gpa, some_reasonable_estimation);
defer json_scratch_stream.deinit();
// compose header JSON with the scratch stream
// ...
// write base 64 of header JSON directly to the output stream WITHOUT allocating
// (taken from Io.Writer's implementation of the b64 specifier)
var chunker = std.mem.window(u8, json_scratch_stream.written(), 3, 3);
var temp: [5]u8 = undefined;
while (chunker.next()) |chunk| {
// I'm assuming your Base64Encoder is initialized at this point
try output.writer.writeAll(encoder.encode(&temp, chunk));
}
try output.writer.writeByte('.'); // trailing dot
// reset the scratch stream for the next portion
json_scratch_stream.clearRetainingCapacity();
// ... the rest
// obviously omitting the payload and HMAC part, but just providing a general idea here
return try output.toOwnedSlice();
I personally love initCapacity() when you can estimate your data’s size relative to the parameters passed in. There’s little penalty to over-allocating, and if your calculations are correct, each stream’s underlying byte array is allocated/freed exactly once, which is a happy place to be.
This single output stream + scratch space pattern is really nice with dynamic data.
This might be minor to you (and I don’t really use something like this, so disregard this if you want) but I don’t see a reason why you force a generic here. So why does the algorithm need to be comptime here?
pub fn Builder(comptime alg: Algorithm) type
I would just pass it to either init or sign. Also since init just passes things through we usually do this to initialize it:
const Builder = Builder{
.private_key = my_private_key,
.public_key = my_public_key,
.iss = my_iss,
.algorithm = .sha256, // or default
}
Or just have it all as a single function because structs or classes which just have one function/method should just be a function instead (my take).
Also I don’t quite like the naming:
const Algorithm = enum { HS256, HS384, HS512 };
You need one more char to name it like the following. But this is obvious as (nearly) everyone knows what a sha256 or so is.
const Algorithm = enum { sha256, sha384, sha512 };
Sorry, I only had a quick glance at your code and did not manage to write very helpful comments. I tell myself that at least it sparked the more helpful replies by MiahDrao97 and pzittlau 
Anyway, regarding your questions:
The name Builder, the init “constructor” (see pzittlau’s comment), and the -in my opinion unnecessary- hiding of _sign gave me that impression.
I simply did not see that the function is in fact not a 1-to-1 wrapper… sorry again. But I can imagine a library user would also want to test token expiration or provide a value for iat in their code/tests. It makes sense to provide sign as convenience function, but I would also provide _sign itself.
Looking at the std library there are relatively few private functions - at least in the corners I looked at so far. From what I have seen they are mostly for de-duplicating code or little helper functions.
Depending on the situation, it does indeed make sense. But you do not necessarily have to go for one big buffer. MiahDrao97 explains some options much better than I could. The code gave me the impression that you may not be aware that many small memory allocations can be slower than fewer larger ones. (This comment was related to your question how to get it faster. ) Apart from the optimization aspect, having fewer points in which the code can fail is also good.
I want to use all zig’s cool features and also from my practice the algorithm never changes at runtime so why not make it comptime for future optimizations.
I did that, no more structs just exporting functions signHmac and verifyHmac.
These names come from JWT, I want to stick to what their website and rfc defines.
Thanks for your feedback I really appreciate it.
I got rid of that, not the package just exports 2 functions.
I am no longer doing that, I am just exposing signHmac and I let the user decide that iat and the rest of the claims.