Skip to content
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

FederatedStore Double Cache Collision Bug #2886

Closed
GCHQDev404 opened this issue Feb 13, 2023 · 3 comments · Fixed by #2902
Closed

FederatedStore Double Cache Collision Bug #2886

GCHQDev404 opened this issue Feb 13, 2023 · 3 comments · Fixed by #2902
Assignees
Labels
bug Confirmed or suspected bug federated-store Specific to/touches the federated-store module

Comments

@GCHQDev404
Copy link
Contributor

Describe the bug
Having FederatedStore Rest service with no propterty gaffer.cache.service.name.suffix specified and then a addGraph for a second FederatedStore with no suffix, results in a Cache collision. Both graphs are the same. (Also a suffix could have been provided and be duplicated)

In a Graph as Service situation multiple user may add additional federated graph without a suffix and all have cache collision. This could expose inner graphs with weak/no security believing it was protected by the enclosing sub-federated graph. (equally suffix could have been provided and duplicated)

Rest FederatedStore{

    FedStoreA{
    owningUser=userA
    graphId = "FedFoo"
    suffix = "foo"
    cache = "FederatedCachefoo"
    graphs{
        graphAWithNoSecurityMapStore{}
    }
    }

    FedStoreB{
    owningUser=userB
    graphId = "CheatingGraph"
    suffix = "foo"
    cache = "FederatedCachefoo"
    graphs{
        graphAWithNoSecurityMapStore{} <---- exposed/stolen/duplicated
    }
    }

}

To Reproduce
Steps to reproduce the behavior:

Create FederatedStore Rest service with no specified suffix gaffer.cache.service.name.suffix in properties.
AddGraph FederatedStore with no specified suffix gaffer.cache.service.name.suffix in properties.
Run a FedereatedOperation{GetAllGraphIds} which goes to the subgraph.
subgraph return the graphId of itself because the two FederatedStore are the same due to a cache name collision.
Additional context
This maybe expected behaviour because the stores have been misconfigured to use the same cache.
But the possibly exploit to expose the contents of another FederatedStore in the same RestService seems incorrect.

No infinite loops should occur.

Possible Workarounds

Optional AddGraph Hooks to mitigate against the use of not providing a suffix
FederatedStore to mitigate against cache name collision in the same JVM, which does not effect load balancing of FederatedStore.

Impact

No one this is an Alpha 4 issue.

@GCHQDev404 GCHQDev404 added bug Confirmed or suspected bug federated-store Specific to/touches the federated-store module labels Feb 13, 2023
@GCHQDev404
Copy link
Contributor Author

This may have all been fixed by the implementation of #2890

@GCHQDev404
Copy link
Contributor Author

Accidental collision will be removed.

But if policy is required to not allow deliberate collisions then a custom hook or similar should be implemented to enforce policy.

Documents required.

GCHQDev404 added a commit that referenced this issue Feb 27, 2023
GCHQDev404 added a commit that referenced this issue Mar 1, 2023
…Store-Double-cache-removing-accidental-collision
GCHQDev404 added a commit that referenced this issue Mar 1, 2023
…Store-Double-cache-removing-accidental-collision
GCHQDev404 added a commit that referenced this issue Mar 1, 2023
GCHQDev404 added a commit that referenced this issue Mar 1, 2023
…-collision' into gh-2903-all-caches-require-suffix-to-avoid-collisions-within-a-federatedstore-environment

# Conflicts:
#	store-implementation/proxy-store/src/main/java/uk/gov/gchq/gaffer/proxystore/ProxyStore.java
GCHQDev404 added a commit that referenced this issue Mar 1, 2023
GCHQDev404 added a commit that referenced this issue Mar 1, 2023
GCHQDev404 added a commit that referenced this issue Mar 1, 2023
…-collision' into gh-2906-federatedstore-removegraph-must-clear-all-associated-caches

# Conflicts:
#	store-implementation/proxy-store/src/main/java/uk/gov/gchq/gaffer/proxystore/ProxyStore.java
GCHQDev404 added a commit that referenced this issue Mar 1, 2023
…-collision' into gh-2903-all-caches-require-suffix-to-avoid-collisions-within-a-federatedstore-environment
GCHQDev404 added a commit that referenced this issue Mar 2, 2023
GCHQDev404 added a commit that referenced this issue Mar 2, 2023
…2902)

* gh-2886 FederatedStore remove double caching accidental collision.

* gh-2886 FederatedStore remove double caching accidental collision.

* gh-2887 Test fixed to user correct cache instance due to suffix/name change.

* gh-2886 PR change

---------

Co-authored-by: GCHQDev404 <GCHQDev404@users.noreply.github.com>
@t92549
Copy link
Contributor

t92549 commented Mar 2, 2023

Closed by #2902

@t92549 t92549 closed this as completed Mar 2, 2023
GCHQDev404 added a commit that referenced this issue Mar 3, 2023
…tedstore environment (#2907)

* gh-2886 FederatedStore remove double caching accidental collision.

* gh-2886 FederatedStore remove double caching accidental collision.

* gh-2887 Test fixed to user correct cache instance due to suffix/name change.

* gh-2903 removing 1.12 cache tests and graphStorage zero argument constructor.

* gh-2903 All Cache requires suffix

* gh-2903 Test changes for Cache requiring suffix

* gh-2903 Cherry Picked, improvements.

* gh-2903 test fix

* gh-2903 merge conflicts

* gh-2903 Cache changes for Score Resolvers.

* gh-2903 spotless

* gh-2903 PR change
GCHQDev404 added a commit that referenced this issue Mar 3, 2023
GCHQDev404 added a commit that referenced this issue Mar 3, 2023
GCHQDev404 added a commit that referenced this issue Mar 6, 2023
…2912)

* gh-2886 FederatedStore remove double caching accidental collision.

* gh-2887 Test fixed to user correct cache instance due to suffix/name change.

* gh-2903 All Cache requires suffix

* gh-2903 Test changes for Cache requiring suffix

* gh-2903 Cherry Picked, improvements.

* gh-2903 test fix

* gh-2903 tidy.

* gh-2886 PR change

* gh-2886 review

* gh-2903 PR changes.

* gh-2903 PR changes.

* gh-2906 RemoveGraph to optionally delete Caches.

* gh-2906 spotless.

* gh-2909 duplication

* gh-2906 PR changes.

---------

Co-authored-by: GCHQDev404 <GCHQDev404@users.noreply.github.com>
GCHQDev404 added a commit that referenced this issue Mar 6, 2023
* gh-2886 FederatedStore remove double caching accidental collision.

* gh-2886 FederatedStore remove double caching accidental collision.

* gh-2887 Test fixed to user correct cache instance due to suffix/name change.

* gh-2903 removing 1.12 cache tests and graphStorage zero argument constructor.

* gh-2903 All Cache requires suffix

* gh-2903 Test changes for Cache requiring suffix

* gh-2903 spotless

* gh-2903 Cherry Picked, improvements.

* gh-2903 test fix

* gh-2903 test fix

* gh-2886 PR change

* gh-2903 tidy.

* gh-2886 review

* gh-2886 PR request

* gh-2886 PR request

* gh-2903 merge conflicts

* gh-2903 PR changes.

* gh-2903 PR changes.

* gh-2903 PR changes.

* gh-2903 PR changes.

* gh-2903 Cache changes for Score Resolvers.

* gh-2903 Cache changes for Score Resolvers.

* gh-2903 spotless

* gh-2909 AddGraph with Handlers.

* gh-2909 merge

* gh-2903 spotless

* gh-2909 improved getting operation declarations json

* gh-2909 duplication

* gh-2909 PR changes.

---------

Co-authored-by: GCHQDev404 <GCHQDev404@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed or suspected bug federated-store Specific to/touches the federated-store module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants