-
Notifications
You must be signed in to change notification settings - Fork 43
Regorus extension to invoke C# code #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (1)
- bindings/csharp/Regorus/Regorus.csproj: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kusha, thank you for taking on this super useful feature has has been in the back burner for a while.
/// </summary> | ||
/// <param name="payload">Deserialized JSON object containing the payload from Rego</param> | ||
/// <returns>Object that will be serialized to JSON and converted to a Rego value</returns> | ||
public delegate object RegoCallback(object payload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: An expectation on the Rust side is that an Engine is efficient to clone. It is much faster to add policies to an engine and clone it many times than creating many instances of the engine and adding policies to them individually.
Cloning an engine will also clone the extension. And a clone could be used from another thread. This means that the extension must be stateless or must be safe to be called from multiple threads.
bindings/csharp/Regorus/Regorus.cs
Outdated
@@ -202,7 +229,139 @@ public void SetGatherPrints(bool enable) | |||
return CheckAndDropResult(Regorus.Internal.API.regorus_engine_take_prints(E)); | |||
} | |||
|
|||
|
|||
// Generic callback handler that routes to the appropriate user-provided callback | |||
private static unsafe byte* CallbackHandler(byte* payloadPtr, void* contextPtr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could generate closures that forward to RegoCallback delegates and register the closures with Regorus. We might be able to avoid maintaining a dictionary for dispatching.
private static unsafe byte* CallbackHandler(byte* payloadPtr, void* contextPtr) | |
private static RegorusCallbackDelegate GenerateRegorusCallback(RegoCallback extension) | |
{ | |
return delegate(byte* payloadPtr, void* contextPtr | |
{ | |
... | |
var result = extension(payloadObject); | |
... | |
}; | |
} | |
private static unsafe byte* CallbackHandler(byte* payloadPtr, void* contextPtr) |
bindings/ffi/src/lib.rs
Outdated
@@ -15,6 +19,60 @@ pub enum RegorusStatus { | |||
RegorusStatusError, | |||
} | |||
|
|||
// Define callback function type for orchestration to Regorus communication | |||
pub type RegorusCallbackFn = extern "C" fn(payload: *const c_char, context: *mut c_void) -> *mut c_char; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The context
is a nice idea and can be leveraged for C and C++ extensions too.
bindings/ffi/src/lib.rs
Outdated
|
||
#[no_mangle] | ||
/// Construct a new Engine | ||
/// | ||
/// See https://docs.rs/regorus/latest/regorus/struct.Engine.html | ||
pub extern "C" fn regorus_engine_new() -> *mut RegorusEngine { | ||
let engine = ::regorus::Engine::new(); | ||
let mut engine = ::regorus::Engine::new(); | ||
let _ = engine.add_extension("invoke".to_string(), 2, Box::new(invoke_callback)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To enable writing function(args)
instead of invoke("function", args)
in the rego policy, we could change the behavior of the interpreter so that it looks at the table of foreign language extensions if no function exists for a given name.
On the other hand, invoke
does make it clear to the reader of the policy that a foreign language function is being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think invoke()
would be a cleaner option. Although we can check that it is possible to to trigger invoke from another extension, so that user can optionally use add_extension("function"...)
that will invoke("function"...)
@mkulke I think you will find this interesting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me.
Two comments:
-
It would be really great if there was a way to use the
invoke
builtin-extension to implement missing builtins. For example, the JWT functions and the crypto functions would ideally be implemented viainvoke
to route to regulation compliant libraries in the client language (e.g. C#). This doesn't have to be part of the PR. -
It would be great if you can document your usecase (leaving out confidential info) as part of the PR.
@@ -15,6 +16,30 @@ pub enum RegorusStatus { | |||
RegorusStatusError, | |||
} | |||
|
|||
// Store a callback function and its context | |||
#[no_mangle] | |||
pub extern "C" fn regorus_register_callback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub extern "C" fn regorus_register_callback( | |
pub extern "C" fn regorus_register_invoke_callback( |
|
||
// Remove a callback function | ||
#[no_mangle] | ||
pub extern "C" fn regorus_unregister_callback(name: *const c_char) -> RegorusStatus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub extern "C" fn regorus_unregister_callback(name: *const c_char) -> RegorusStatus { | |
pub extern "C" fn regorus_unregister_invoke_callback(name: *const c_char) -> RegorusStatus { |
var engine = new Regorus.Engine(); | ||
|
||
// Enable the invoke extension | ||
bool enableResult = engine.EnableInvoke(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To catch regressions, I'd prefer a test over a script.
See example code to invoke C# code during policy evaluation:
bindings/csharp/scripts/invoke.csx
.bindings/csharp/scripts
also contains compilation insturctions.Changes:
invoke("name", args...)
calls from policy