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

BED-4907: Edge type shortcuts #895

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

BED-4907: Edge type shortcuts #895

wants to merge 6 commits into from

Conversation

brandonshearin
Copy link
Contributor

@brandonshearin brandonshearin commented Oct 3, 2024

Description

Describe your changes in detail

i added two new edges to our schema in cue, ALL_AD_ATTACKS and ALL_AZ_ATTACKS.

this change tweaks the logic of the cypher emitter when it is converting SQL AST->cypher string. when emitting for a relationship pattern in formatRelationshipPattern, if the provided AST node has the graph.Kind of "ALL_AD_ATTACKS" or "ALL_AZ_ATTACKS", then that graph.Kind is replaced with the full expansion of pathfinding edges defined in cue. the pre built queries now utilize these two edges.

for example, when a user provides the query:

match p = ()-[:ALL_AD_ATTACKS]->() return p

it gets translated to the following cypher:

match p = ()-[:Owns|GenericAll|GenericWrite|WriteOwner|WriteDacl|MemberOf|ForceChangePassword|AllExtendedRights|AddMember|HasSession|Contains|GPLink|AllowedToDelegate|TrustedBy|AllowedToAct|AdminTo|CanPSRemote|CanRDP|ExecuteDCOM|HasSIDHistory|AddSelf|DCSync|ReadLAPSPassword|ReadGMSAPassword|DumpSMSAPassword|SQLAdmin|AddAllowedToAct|WriteSPN|AddKeyCredentialLink|SyncLAPSPassword|WriteAccountRestrictions|WriteGPLink|GoldenCert|ADCSESC1|ADCSESC3|ADCSESC4|ADCSESC5|ADCSESC6a|ADCSESC6b|ADCSESC7|ADCSESC9a|ADCSESC9b|ADCSESC10a|ADCSESC10b|ADCSESC13|DCFor|SyncedToEntraUser]->() return p

Motivation and Context

This PR addresses: BED-4907

Why is this change required? What problem does it solve?
we want to have edge type shortcuts for common cypher statements that are currently very laborious to craft.

How Has This Been Tested?

Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.

unit tests have been added to poke different ways that the emitter expands the new edge kinds

Screenshots (optional):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Database Migrations

Checklist:

@elikmiller
Copy link
Collaborator

I realize this PR is still a draft but I have a concern with the proposed solution here.

If, for example, a user ran the following cypher via the UI what is the expected result?

CREATE (a:User {name: 'Alice'})-[:ALL_AZ_ATTACKS]->(b:User {name: 'Bob'})

And what happens if they wanted to query this relationship later?

MATCH p=()-[:ALL_AZ_ATTACKS]->() RETURN p

@brandonshearin brandonshearin changed the title draft: BED-4907: Edge type shortcuts BED-4907: Edge type shortcuts Oct 4, 2024
@brandonshearin brandonshearin changed the title BED-4907: Edge type shortcuts wip: BED-4907: Edge type shortcuts Oct 4, 2024
@brandonshearin brandonshearin changed the title wip: BED-4907: Edge type shortcuts BED-4907: Edge type shortcuts Oct 4, 2024
@brandonshearin brandonshearin added work in progress This pull request is a work in progress and should not be merged enhancement New feature or request api A pull request containing changes affecting the API code. and removed work in progress This pull request is a work in progress and should not be merged labels Oct 7, 2024
@elikmiller
Copy link
Collaborator

Here is another scenario which I think is worth considering. What if a user wants to find all non-attack path relationships in the graph? A query like MATCH p=()-[r]->() WHERE NOT r:ALL_AD_ATTACKS RETURN p LIMIT 500 seems correct however running this query will happily return you many relationships which are indeed attack paths.

Here is an example using our test dataset.

Screenshot 2024-10-08 at 9 36 34 AM

My suggestion is to re-evaluate the current implementation (query transformation) and instead consider adding relationship properties to the graph to indicate whether or not an edge represents a valid attack path.

Copy link
Collaborator

@elikmiller elikmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my previous comments!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A pull request containing changes affecting the API code. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants