topdown: Add built-in HMAC functions #4100
Conversation
| @@ -0,0 +1,156 @@ | |||
| --- | |||
There was a problem hiding this comment.
I just realized there is more in-depth documentation for built-in:s at extensions/#custom-built-in-functions-in-go. Should probably link to there from here.
There was a problem hiding this comment.
That is a little different, though: it's about extending OPA, so you'd build your own binary with the extra built-in func included. This document describes how to contribute to OPA, so the new built-in function would become part of "core OPA" so to speak. There certainly is overlap, but it's a different twist.
There was a problem hiding this comment.
It is different for sure, but I think @johanfylling is right that it would be good to reference here.. at least in the sense that one step that's currently not mentioned is to first have some form of approval for the built-in. Without that—i.e. for someone who wants to integrate Kafka/Redis/DynamoDB/Whatever—the custom function is probably a better alternative.
There was a problem hiding this comment.
On a related note, this feels a bit like these are two changes: a docs addition and new built-in functions.
It would be nice if we untangled that a bit, we could massage it into two commits -- two PRs would be another option, but since it's here already, it's OK I think.
There was a problem hiding this comment.
Yea, I agree; I should have gone that route.
| | Built-in | Description | Wasm Support | | ||
| | ------- |-------------|---------------| | ||
| ... | ||
| | <span class="opa-keep-it-together">``output := repeat(string, count)``</span> | ``output`` is ``string`` repeated ``count``times | ✅ | |
There was a problem hiding this comment.
Haven't seen any wasm mention here, so I think this should say "SDK dependent"
There was a problem hiding this comment.
Changing to SDK-dependent. Would probably be good to also add info to this how-to about Wasm. I don't think I can currently bild and test for that on my M1 mac; so something to return to later, perhaps, either for me or someone else.
There was a problem hiding this comment.
It's cumbersome, but you could perhaps use a docker container...? Not sure how that's done on M1.
test/cases/testdata/cryptohmacsha256/test-cryptohmacsha256.yaml
Outdated
Show resolved
Hide resolved
|
LGTM modulo the comments that @srenatus and @anderseknert posted. |
|
This looks like it's ready to go to me. Let's deal with the Wasm changes separately. As soon as this is squashed, we can merge. |
Add crypto.hmac.* built-in functions for the MD5, SHA-1, SHA-256 and SHA-512 hashing algorithms. Add documentation for how to contribute new built-in functions. Fixes: open-policy-agent#1740 Signed-off-by: Johan Fylling <johan.dev@fylling.se>
johanfylling
force-pushed
the
feature/1740/hmac_builtin
branch
from
f4e7779
to
727dbb5
Compare
Add crypto.hmac.* built-in functions for the MD5, SHA-1, SHA-256 and SHA-512 hashing algorithms.
Add documentation for how to contribute new built-in functions.
Fixes: #1740
Signed-off-by: Johan Fylling johan.dev@fylling.se