-
Notifications
You must be signed in to change notification settings - Fork 121
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
Add ability to deny service proxy values for places #505
Conversation
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. |
There was a problem hiding this 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
Personally, I like /**
* 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(); |
There was a problem hiding this 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.
There was a problem hiding this 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.
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 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();
+ } |
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
I then added the following test to the @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");
} |
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work. 🚀
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.