epi-project / policy-reasoner

Implements the famous policy reasoner, known as `checker` in Brane terminology. Builds on top of reasoners like eFLINT and meant to be queried by Brane.
1 stars 1 forks source link

Active policy #39

Open DanielVoogsgerd opened 1 month ago

DanielVoogsgerd commented 1 month ago

Right now, a request is rejected by default if no active policy is found. However, some reasoners might not have configurable policies. A slightly pedantic variant of this is of course the no_op reasoner, which will not care what policy is set. And will just accept (or reject) any request that comes in.

I have no problem with rejecting without a policy, but this is in my opinion a clear responsibility of the ReasonerConnector and not the policy_reasoner framework itself, which currently handles it. Lets change policy to Option policy so that the ReasonerConnector can decide on how it wishes to handle this.

Lut99 commented 1 month ago

Hmm. I know that we did it this way because policy is sensitive and we should try to avoid accidental "allows" on things that should not be allowed at all costs.

I am a little hesitant with your suggestion as it feels like a "development"-requirement leaking into production. I.e., the no_op server is of course used for testing only, and in any implementation with an interesting reasoner it is crucial it can only allow things once configured properly. Or at the very least, we'll need a signoff that policy engineer has chosen a policy suitable for the current work.

I see that, of course, similar behaviour is added when we push it to the backend instead of the framework. But I don't like that the APIs have to be changed too; it seems like it is a hard requirement to provide a policy in any case where a configuration is needed.

I think I'm leaning towards being pedantic (:P) and insisting that the framework enforces the property that there must always be an active policy. However, you can maybe still get an easy setup by also providing a NoOpPolicyDataAccess connector (i.e., replacement for SQLite). That one just always has exactly one policy (an empty one) which is active by default and no-ops when you add one or change the active policy. Then you also get rid of the policy store machinery for reasoners that don't need it.

Thoughts?

DanielVoogsgerd commented 1 month ago

Fair enough. I see where you're coming from. I'm personally not a fan of anything but the reasoner making a verdict, but I suppose that is just a matter of opinion. I must admit I do not see any implementation of a reasoner that would be used in production that would make anything but a deny verdict when no policy is present. Maybe only if the policy could only be interpreted in a single way, but fair enough.

For arguments sake, having policy as on option is quite safe in rust as well as you would have to do something with the option in order to get the contents of your policy if you use it. Which elegantly allows you to just drop it if you don't care. 😛

Yeah, for the no_op connector injecting a mocked policy storage would indeed work, I think, I hope 😅

Lut99 commented 1 month ago

OK, so this is actually a really interesting point :)

I think that when we designed it, we assumed that all reasoners had a configuration that is sensitive but non-static and which needs to be given as input to all queries (as it happens in eFLINT). As a result, the framework itself is responsible for the link between configuration and policy. So in that sense, having no configuration seems weird; it's like disabling the main thing it contributes.

But you're right in that our assumption may actually be incorrect. Some reasoners have no configuration (e.g., no-op), but the configuration of others may be non-static or non-sensitive and, as a result, external. So maybe the policy-reasoner should stick to the (much simpler) reasoning (deliberation API), and we introduce a third layer that sits between the deliberation and the reasoning as a "battery-included" way of adding a policy-store to the story. I.e., we move the store to be a wrapper around arbitrary reasoners. Maybe the fact that we had two APIs to begin with was code smell all along ;)

The software engineer in me says that this is a concept split and we should completely revamp the project in order to incorporate it and don't sneak in parts of it until we do. But then again, that's also very scientific programmer of me, because it's not considering getting practical stuff out the door in reasonable times :slightly_smiling_face:

Alright, let me do it like this. I'll indulge myself a little (sorry) and open #42 to structure the discussion. In the meantime, I'll merge this to get you going for now and then we might unroll the changes later when we do the big one.

Lut99 commented 1 month ago

Oh the PR is still in draft xD OK, then I won't merge yet!

DanielVoogsgerd commented 1 month ago

Oh the PR is still in draft xD OK, then I won't merge yet!

Yeah, the implementation is a little haphazard right now, as like you mentioned, it flies a bit in the face of the entire structure of the project. I basically started at the top and just followed the type system to see how the project would look like if policies were optional, but this led to some oddities.

This is most clear in src/sqlite.rs Here, the PolicyDataError::NotFound is doing too much work. It is used in cases like get_version(id) where it reflects a policy version that is requested but does not exist, but it is also used for get_active() in case no policy is active, which are quite different of course.

Right now, I am still using the NotFound error, but some of that can and must probably be offloaded to the added Option type, like here. However, I am slightly hesitant to invest much time into it until we have figured out how we actually want to structure it.

Lut99 commented 1 month ago

Agree. Especially if it already (kinda) works, better discuss it first.

That said, how blocking is this? We can merge anything kinda working for now to get you going?

DanielVoogsgerd commented 1 month ago

It isn't blocking at all, right now I just reference this branch as my source for the policy reasoner, and I am fine. The next part of Brane is not working for some other reason I have yet to debug (see email).

Lut99 commented 1 month ago

Oh shoot I forgot about the email :# I'll look at it right away!

And OK cool, good to know.