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

Add ability to deny service proxy values for places #505

Merged
merged 12 commits into from
Aug 24, 2023

Conversation

aL118
Copy link
Contributor

@aL118 aL118 commented Jun 23, 2023

Closes https://github.com/NationalSecurityAgency/burrito-grande/issues/2269
Added blacklist functionality to ServiceProviderPlace. Rejects forms that are blacklisted from places with a wildcard proxy.

@jpdahlke jpdahlke added this to the v8.0.0 milestone Jun 24, 2023
@jpdahlke
Copy link
Collaborator

I marked this for 8.x because, as a goal for the 8 series, we would like to move away from black/white list terms. However, I don't have a direct replacement suggestion to give you at this moment. Give us a bit to discuss and we can move in that direction. This is new code but there is ton of old code subject to change as well.

@jpdahlke jpdahlke added the discussion Extra discussion with the team is required label Jun 24, 2023
Copy link
Collaborator

@fbruton fbruton left a comment

Choose a reason for hiding this comment

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

Rename blackList to either denyList or blockList

@drivenflywheel drivenflywheel self-requested a review June 26, 2023 15:35
@drivenflywheel
Copy link
Collaborator

Rename blackList to either denyList or blockList

Personally, I like disallowList better than blockList or denyList, but any of those should work:

    /**
     * Returns whether form is [blacklisted]
     */
    boolean isDenied(String s);
    boolean isBlocked(String s);
    boolean isDisallowed(String s);

    /**
     * Add to [blacklist]
     */
    void addToDenyList(String s);
    void addToBlockList(String s);
    void disallow(String s);

    /**
     * Clear everything in [blacklist]
     */
    void clearDenyList();
    void clearBlocklist();
    void clearDisallowList();

Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

The overall approach looks good and the code is very clean, but this currently lacks a way to populate the disallowList from config file entries, and that's a key requirement for this feature.

src/main/java/emissary/place/ServiceProviderPlace.java Outdated Show resolved Hide resolved
@jpdahlke jpdahlke removed the discussion Extra discussion with the team is required label Jul 20, 2023
@jpdahlke jpdahlke modified the milestones: v8.0.0, v8.0.0-M2 Jul 20, 2023
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Need to revisit the approach, because it's not yet giving us the desired behavior.

src/main/java/emissary/core/MobileAgent.java Outdated Show resolved Hide resolved
@drivenflywheel
Copy link
Collaborator

I'm still not sure this is working the way we need. I think, as I'd previously suspected, that we need to do the isDenied check further upstream. Some additional local testing makes me think that we need that check performed not in MobileAgent but in DirectoryPlace.nextKeys, specifically here:

    protected List<DirectoryEntry> nextKeys(final String dataID, final IBaseDataObject payload, @Nullable final DirectoryEntry lastPlace,
            final DirectoryEntryMap entries) {
        // Find the entry list for the type being requested
        final DirectoryEntryList currentList = getWildcardedEntryList(dataID, entries);

        // Nothing for the dataID or any wildcarded versions, we are done
        if ((currentList == null) || currentList.isEmpty()) {
            logger.debug("nextKey - nothing found here for {}", dataID);
            return Collections.emptyList();
        }
+
+      // remove denied entries
+      currentList.removeIf(de -> de.getLocalPlace() != null && de.getLocalPlace().isDenied(payload.currentForm()));
+
+      if (currentList.isEmpty()) {
+            logger.debug("nextKeys - no non-DENIED entries found here for {}", dataID);
+          return Collections.emptyList();
+      }

@drivenflywheel
Copy link
Collaborator

Functionally this now seems to be working exactly as intended, but I think we need the test to more directly map to the real wireup scenario. Here's what I did to verify:

I added the following to a new src/test/resources/emissary/place/sample/DelayPlace.cfg file:

PLACE_NAME = "DelayPlace"
SERVICE_NAME = "DELAY"
SERVICE_TYPE = "ANALYZE"
SERVICE_DESCRIPTION = "delay stuff"
SERVICE_COST = 50
SERVICE_QUALITY = 50

# Add Wildcard entry
SERVICE_PROXY = "*"
SERVICE_PROXY_DENY = "MYFORM"

DELAY_TIME_MILLIS = 200

I then added the following test to the RoutingAlgorithmTest class:

    @Test
    void testWildCardProxyHonorsDenyList() throws IOException {
        loadAllTestEntries();

        this.payload.pushCurrentForm("MYFORM");

        // create our place and add it to the directory.  This place proxies for "*" but explicitly denies "MYFORM"
        DelayPlace deniedWildcardPlace = new DelayPlace(new ResourceReader().getConfigDataName(DelayPlace.class).replace("/main/", "/test/"));
        this.dir.addTestEntry(deniedWildcardPlace.getDirectoryEntry());

        // Add another entry that proxies for "MYFORM".
        // Doesn't need an actual place, but does need a higher expense than deniedWildcardPlace
        DirectoryEntry expected = new DirectoryEntry("MYFORM.s4.ANALYZE.http://example.com:8001/A$9999");
        this.dir.addTestEntry(expected);

        final DirectoryEntry result = this.agent.getNextKeyAccess(this.dir, this.payload);
        assertEquals(expected,  result, "After a denied entry, should get next matching entry for the same stage");
    }

Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Need a "wildcard proxy + denied entry" test in RoutingAlgorithmTest, but that should be the last of the new code. Sorry if I'd steered this in the wrong direction - there's been a lot of excellent work demonstrated in this PR

drivenflywheel
drivenflywheel previously approved these changes Aug 3, 2023
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Excellent work. 🚀

@jpdahlke jpdahlke changed the title 2269 - Allow blacklist of Service Proxy's for places Add a way to deny service proxy values for places Aug 3, 2023
@jpdahlke jpdahlke changed the title Add a way to deny service proxy values for places Add ability to deny service proxy values for places Aug 3, 2023
@cfkoehler cfkoehler self-requested a review August 12, 2023 11:12
@cfkoehler cfkoehler modified the milestones: v8.0.0-M2, v8.0.0-M3 Aug 14, 2023
src/test/java/emissary/core/MobileAgentTest.java Outdated Show resolved Hide resolved
src/test/java/emissary/core/MobileAgentTest.java Outdated Show resolved Hide resolved
@jpdahlke jpdahlke added the feature A new feature that does not exist today label Aug 24, 2023
@jpdahlke jpdahlke merged commit 953b2cf into NationalSecurityAgency:master Aug 24, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature that does not exist today
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants