NEBULA-4397: Issue with enforcing SafeListing for both internal and external clients#7509
NEBULA-4397: Issue with enforcing SafeListing for both internal and external clients#7509SimonSapin merged 3 commits intodevfrom
Conversation
|
@DaleSeo, please consider creating a changeset entry in |
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: c91cd57040ffa1bb06eb0b00 |
glasser
left a comment
glasser
left a comment
There was a problem hiding this comment.
I'm a bit confused about how the tests are passing without apollo-router/src/services/layers/persisted_queries/snapshots/apollo_router__services__layers__persisted_queries__tests__pq_layer_freeform_graphql_with_safelist_log_unknown_true@logs.snap being updated — I had to when I was playing with this...
| // Note: it's kind of inconsistent that if we require | ||
| // IDs and skip_enforcement is set, we don't call | ||
| // log_unknown_operation on freeform GraphQL, but if we | ||
| // *don't* reuqire IDs and skip_enforcement is set, we |
There was a problem hiding this comment.
| // *don't* reuqire IDs and skip_enforcement is set, we | |
| // *don't* require IDs and skip_enforcement is set, we |
oops, my typo
@glasser I also thought it was strange that nothing broke in CI. When I ran |
DaleSeo
changed the title
| level: INFO | ||
| message: Loaded 2 persisted queries. | ||
| - fields: | ||
| enforcement_skipped: false |
There was a problem hiding this comment.
It's a problem that this test passes when the snapshot doesn't match. This indicates something is wrong with the assert_snapshot_subscriber implementation. This needs to be fixed - if it isn't done as part of this PR, then the router team should investigate separately.
There was a problem hiding this comment.
@pubmodmatt Good callout! I’ve submitted a ticket with the router team to investigate it separately.
|
@Mergifyio backport 1.x |
✅ Backports have been createdDetails
|
|
Note: This PR never had a changeset written for it, which is a miss! |
6 participants
A customer reported an issue while using a setup with both internal and external traffic where:
apollo_persisted_queries::safelist::skip_enforcementin context to bypass safelisting for internal operationslog_unknownto audit which operations would be blocked once fully enforcedlog_unknownfeature still logs these operations as "unknown operations"This is problematic because it makes it difficult to distinguish between:
This PR makes changes so that when logging unknown operations, we will include information about whether enforcement was skipped. This will enable the customer to filter their logs and distinguish between truly problematic external operations (
where enforcement_skippedis false) and internal operations that are intentionally allowed to bypass safelisting (where enforcement_skippedis true).Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩