-
Notifications
You must be signed in to change notification settings - Fork 65
Description
There are 2 or 3 types of c-binding bugs I found related to access control and identity passing between the c wrapper step.
Bugs:
- Memory leaks: There are a lot of instances where the c type pointers are not freeing the identity pointer properly like it is for other properties.
For example incbindings/wrapper.goconsider:
func (w *CWrapper) AddDACPolicy(
ctx context.Context,
policy string,
) (client.AddPolicyResult, error) {
cIdentity := identityFromContext(ctx)
cPolicy := C.CString(policy)
defer C.free(unsafe.Pointer(cPolicy))
...Note: the cIdentity does not have a defer C.free call (I am assuming that is a memory bug, and not purposeful @ChrisBQu correct me if I am wrong please). There are many other instances like this.
- No identity being passed: The second bug is that the identity is not being passed at all. This means for all NAC instances where this happens, we would see
"not authorized to perform operation"error even when access is given. Kind of the same bug as the one highlighted in this PR comment: feat: Allow syncing of collection versions over P2P #4088 (comment)
Solution should be similar to this: https://www.loom.com/share/25979d65369a41d4b85f3a0f50e11b33?sid=8b566e4a-e944-4e98-b0c1-1a2f73987c36
Some NAC tests would have a todo marked this ticket where we need to add state.CClientType in the SupportedClientTypes list([]state.ClientType)
As of writing, this bug is at least in 3 places (likely way more places). VerifySignature and GetIdentityNode commands + the newly added commands all have this issue.
-
Also fix for: feat: Allow syncing of collection versions over P2P #4088 (comment)
-
Also fix for p2p nac cbinding tests introduced in: feat: Gate p2p ops with NAC #4111