PR feedback

interuss · Sep 14, 2023 · 508a6f6 · 508a6f6
1 parent 24e4b55
commit 508a6f6
Show file tree

Hide file tree

Showing 10 changed files with 91 additions and 43 deletions.
diff --git a/monitoring/monitorlib/fetch/__init__.py b/monitoring/monitorlib/fetch/__init__.py
@@ -269,19 +269,23 @@ def describe_query(
     query_type: Optional[QueryType] = None,
     server_id: Optional[str] = None,
 ) -> Query:
-    return Query(
+    query = Query(
         request=describe_request(resp.request, initiated_at),
         response=describe_response(resp),
-        server_id=server_id,
-        query_type=query_type,
     )
+    if query_type is not None:
+        query.query_type = query_type
+    if server_id is not None:
+        query.server_id = server_id
+    return query
 
 
 def query_and_describe(
     client: Optional[infrastructure.UTMClientSession],
     verb: str,
     url: str,
     query_type: Optional[QueryType] = None,
+    server_id: Optional[str] = None,
     **kwargs,
 ) -> Query:
     """Attempt to perform a query, and then describe the results of that attempt.
@@ -294,6 +298,7 @@ def query_and_describe(
         verb: HTTP verb to perform at the specified URL.
         url: URL to query.
         query_type: If specified, the known type of query that this is.
+        server_id: If specified, the participant identifier of the server being queried.
         **kwargs: Any keyword arguments that should be applied to the <session>.request method when invoking it.
 
     Returns:
@@ -305,7 +310,6 @@ def query_and_describe(
     else:
         utm_session = True
     req_kwargs = kwargs.copy()
-    req_kwargs.pop("server_id", None)
     if "timeout" not in req_kwargs:
         req_kwargs["timeout"] = TIMEOUTS
 
@@ -320,7 +324,7 @@ def query_and_describe(
                 client.request(verb, url, **req_kwargs),
                 t0,
                 query_type=query_type,
-                server_id=kwargs.get("server_id", None),
+                server_id=server_id,
             )
         except (requests.Timeout, urllib3.exceptions.ReadTimeoutError) as e:
             failure_message = f"query_and_describe attempt {attempt + 1} from PID {os.getpid()} to {verb} {url} failed with timeout {type(e).__name__}: {str(e)}"
@@ -350,7 +354,7 @@ def query_and_describe(
             elapsed_s=(t1 - t0).total_seconds(),
             reported=StringBasedDateTime(t1),
         ),
-        server_id=kwargs.get("server_id", None),
+        server_id=server_id,
     )
     if query_type is not None:
         result.query_type = query_type

diff --git a/monitoring/uss_qualifier/resources/astm/f3411/dss.py b/monitoring/uss_qualifier/resources/astm/f3411/dss.py
@@ -69,6 +69,7 @@ def __init__(
 
         if has_private_address is not None:
             self.has_private_address = has_private_address
+            self.local_debug = True
         if local_debug is not None:
             self.local_debug = local_debug
 

diff --git a/monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py b/monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
@@ -22,6 +22,8 @@
     NetRIDServiceProvider,
 )
 
+from loguru import logger
+
 
 class AggregateChecks(ReportEvaluationScenario):
     _rid_version: RIDVersion
@@ -73,6 +75,7 @@ def __init__(
             participant: list()
             for participant in self._participants_by_base_url.values()
         }
+
         for query in self._queries:
             for base_url, participant in self._participants_by_base_url.items():
                 if query.request.url.startswith(base_url):
@@ -141,7 +144,6 @@ def run(self):
         self.end_test_scenario()
 
     def _verify_https_everywhere(self):
-
         for participant_id, participant_queries in self._queries_by_participant.items():
             self._inspect_participant_queries(participant_id, participant_queries)
 
@@ -155,12 +157,9 @@ def _verify_https_everywhere(self):
             # TODO clean this up: this is an internal requirement and not a check,
             #  leaving as-is during development to make sure the test-suite runs but we know about unattributed queries
             #  ultimately this check could go into the constructor and blow things up early
-            self.record_note(
-                "unattributed_queries",
-                f"found {len(unattr_queries)} unattributed queries",
-            )
+
             with self.check(
-                "All interactions happen over https",
+                "No unattributed queries",
                 [],
             ) as check:
                 check.record_failed(
@@ -170,13 +169,12 @@ def _verify_https_everywhere(self):
     def _inspect_participant_queries(
         self, participant_id: str, participant_queries: List[fetch.Query]
     ):
-        found_cleartext_query = False
+        cleartext_queries = []
         for query in participant_queries:
             if query.request.url.startswith("http://"):
-                found_cleartext_query = True
                 if participant_id not in self._debug_mode_usses:
-                    self.record_note(
-                        "cleartext-query",
+                    cleartext_queries.append(query)
+                    logger.info(
                         f"query is not https: {participant_id} - {query.request.url}",
                     )
 
@@ -185,9 +183,17 @@ def _inspect_participant_queries(
                 "All interactions happen over https",
                 [participant_id],
             ) as check:
-                if found_cleartext_query:
+                if len(cleartext_queries) > 0:
+                    timestamps = [
+                        q.request.initiated_at.datetime for q in cleartext_queries
+                    ]
+                    urls = set([q.request.url for q in cleartext_queries])
                     check.record_failed(
-                        "found at least one cleartext http query", Severity.Medium
+                        summary=f"found {len(cleartext_queries)} cleartext http queries",
+                        details=f"unique cleartext urls: {urls}",
+                        severity=Severity.Medium,
+                        participants=[participant_id],
+                        query_timestamps=timestamps,
                     )
         else:
             self.record_note(
@@ -203,7 +209,6 @@ def _dp_display_data_details_times_step(self):
         for participant, all_queries in self._queries_by_participant.items():
             relevant_queries: List[fetch.Query] = list()
             for query in all_queries:
-
                 if (
                     query.status_code == 200
                     and query.has_field_with_value("query_type")
@@ -303,7 +308,6 @@ def _dp_display_data_times_step(self):
 
         pattern = re.compile(r"/display_data\?view=(-?\d+(.\d+)?,){3}-?\d+(.\d+)?")
         for participant, all_queries in self._queries_by_participant.items():
-
             # identify successful display_data queries
             relevant_queries: List[fetch.Query] = list()
             for query in all_queries:

diff --git a/monitoring/uss_qualifier/scenarios/astm/netrid/common/misbehavior.py b/monitoring/uss_qualifier/scenarios/astm/netrid/common/misbehavior.py
@@ -255,6 +255,7 @@ def _evaluate_and_test_authentication(
                 get_details=True,
                 rid_version=self._rid_version,
                 session=self._dss.client,
+                server_id=self._dss.participant_id,
             )
             # We fish out the queries that were used to grab the flights from the SP,
             # and attempt to re-query without credentials. This should fail.
@@ -285,6 +286,7 @@ def _evaluate_and_test_authentication(
                     url=fq.query.request.url,
                     json=fq.query.request.json,
                     data=fq.query.request.body,
+                    server_id=self._dss.participant_id,
                 )
                 logger.info(
                     f"Repeating query to {fq.query.request.url} without credentials"

diff --git a/monitoring/uss_qualifier/scenarios/astm/netrid/v19/aggregate_checks.md b/monitoring/uss_qualifier/scenarios/astm/netrid/v19/aggregate_checks.md
@@ -56,6 +56,24 @@ of the durations for the subsequent display data queries do not exceed the respe
 of the durations for the replies to requested flights in an area do not exceed the respective thresholds
 `NetSpDataResponseTime95thPercentile` (1 second) and `NetSpDataResponseTime99thPercentile` (3 seconds).
 
+## Verify https is in use test case
+
+### Verify https is in use test step
+
+Inspects all record queries for their usage of https. If services such as a service provider, observer or DSS are marked
+as being in "local debug" mode, they may serve requests over http without producing failed checks despite their lack of encryption.
+
+#### All interactions happen over https check
+
+If non-encrypted interactions such as plaintext queries over http are allowed, **[astm.f3411.v19.NET0220](../../../../requirements/astm/f3411/v19.md)** is not satisfied.
+
+#### No unattributed queries check
+
+All queries must have been attributed to a participant: an unattributed query means that one of the test cases has not
+properly recorded to which participant it was made.
+
+This is an internal requirement and does not necessarily imply that there is a problem with the participants under test.
+
 ## Mock USS interactions evaluation test case
 
 In this test case, the interactions with a mock_uss instance (if provided) are obtained and then examined to verify
@@ -71,14 +89,3 @@ If one of the Display Provider test participants was found to have sent a query
 area requested, then that participant will have violated **[astm.f3411.v19.NET0240](../../../../requirements/astm/f3411/v19.md)**.
 
 TODO: Implement this check
-
-## Verify https is in use test case
-
-### Verify https is in use test step
-
-Inspects all record queries for their usage of https. If resources such as a service provide, observer or DSS are marked
-as being in "local debug" mode, they may serve requests over https without breaking the test suite.
-
-#### All interactions happen over https check
-
-If non-encrypted interactions such as plaintext queries over http are allowed, **[astm.f3411.v19.NET0220](../../../../requirements/astm/f3411/v19.md)** is not satisfied.
diff --git a/monitoring/uss_qualifier/scenarios/astm/netrid/v22a/aggregate_checks.md b/monitoring/uss_qualifier/scenarios/astm/netrid/v22a/aggregate_checks.md
@@ -56,6 +56,24 @@ of the durations for the subsequent display data queries do not exceed the respe
 of the durations for the replies to requested flights in an area do not exceed the respective thresholds
 `NetSpDataResponseTime95thPercentile` (1 second) and `NetSpDataResponseTime99thPercentile` (3 seconds).
 
+## Verify https is in use test case
+
+### Verify https is in use test step
+
+Inspects all record queries for their usage of https. If services such as a service provider, observer or DSS are marked
+as being in "local debug" mode, they may serve requests over http without producing failed checks despite their lack of encryption.
+
+#### No unattributed queries check
+
+All queries must have been attributed to a participant: an unattributed query means that one of the test cases has not
+properly recorded to which participant it was made.
+
+This is an internal requirement and does not necessarily imply that there is a problem with the participants under test.
+
+#### All interactions happen over https check
+
+If non-encrypted interactions such as plaintext queries over http are allowed, **[astm.f3411.v19.NET0220](../../../../requirements/astm/f3411/v19.md)** is not satisfied.
+
 ## Mock USS interactions evaluation test case
 
 In this test case, the interactions with a mock_uss instance (if provided) are obtained and then examined to verify
@@ -71,14 +89,3 @@ If one of the Display Provider test participants was found to have sent a query
 area requested, then that participant will have violated **[astm.f3411.v22a.NET0240](../../../../requirements/astm/f3411/v22a.md)**.
 
 TODO: Implement this check
-
-## Verify https is in use test case
-
-### Verify https is in use test step
-
-Inspects all record queries for their usage of https. If resources such as a service provide, observer or DSS are marked
-as being in "local debug" mode, they may serve requests over https without breaking the test suite.
-
-#### All interactions happen over https check
-
-If non-encrypted interactions such as plaintext queries over http are allowed, **[astm.f3411.v19.NET0220](../../../../requirements/astm/f3411/v19.md)** is not satisfied.
diff --git a/monitoring/uss_qualifier/suites/astm/netrid/f3411_19.md b/monitoring/uss_qualifier/suites/astm/netrid/f3411_19.md
@@ -21,7 +21,7 @@
     <th>Checked in</th>
   </tr>
   <tr>
-    <td rowspan="43" style="vertical-align:top;"><a href="../../../requirements/astm/f3411/v19.md">astm<br>.f3411<br>.v19</a></td>
+    <td rowspan="44" style="vertical-align:top;"><a href="../../../requirements/astm/f3411/v19.md">astm<br>.f3411<br>.v19</a></td>
     <td><a href="../../../requirements/astm/f3411/v19.md">A2-6-1,1a</a></td>
     <td>Implemented</td>
     <td><a href="../../../scenarios/astm/netrid/v19/dss_interoperability.md">ASTM F3411-19 NetRID DSS interoperability</a></td>
@@ -166,6 +166,11 @@
     <td>Implemented</td>
     <td><a href="../../../scenarios/astm/netrid/v19/dss_interoperability.md">ASTM F3411-19 NetRID DSS interoperability</a></td>
   </tr>
+  <tr>
+    <td><a href="../../../requirements/astm/f3411/v19.md">NET0220</a></td>
+    <td>Implemented</td>
+    <td><a href="../../../scenarios/astm/netrid/v19/aggregate_checks.md">ASTM F3411-19 NetRID aggregate checks</a></td>
+  </tr>
   <tr>
     <td><a href="../../../requirements/astm/f3411/v19.md">NET0240</a></td>
     <td>Planned</td>

diff --git a/monitoring/uss_qualifier/suites/astm/netrid/f3411_22a.md b/monitoring/uss_qualifier/suites/astm/netrid/f3411_22a.md
@@ -20,6 +20,12 @@
     <th>Status</th>
     <th>Checked in</th>
   </tr>
+  <tr>
+    <td rowspan="1" style="vertical-align:top;"><a href="../../../requirements/astm/f3411/v19.md">astm<br>.f3411<br>.v19</a></td>
+    <td><a href="../../../requirements/astm/f3411/v19.md">NET0220</a></td>
+    <td>Implemented</td>
+    <td><a href="../../../scenarios/astm/netrid/v22a/aggregate_checks.md">ASTM F3411-22a NetRID aggregate checks</a></td>
+  </tr>
   <tr>
     <td rowspan="52" style="vertical-align:top;"><a href="../../../requirements/astm/f3411/v22a.md">astm<br>.f3411<br>.v22a</a></td>
     <td><a href="../../../requirements/astm/f3411/v22a.md">A2-6-1,1a</a></td>

diff --git a/monitoring/uss_qualifier/suites/uspace/network_identification.md b/monitoring/uss_qualifier/suites/uspace/network_identification.md
@@ -15,6 +15,12 @@
     <th>Status</th>
     <th>Checked in</th>
   </tr>
+  <tr>
+    <td rowspan="1" style="vertical-align:top;"><a href="../../requirements/astm/f3411/v19.md">astm<br>.f3411<br>.v19</a></td>
+    <td><a href="../../requirements/astm/f3411/v19.md">NET0220</a></td>
+    <td>Implemented</td>
+    <td><a href="../../scenarios/astm/netrid/v22a/aggregate_checks.md">ASTM F3411-22a NetRID aggregate checks</a></td>
+  </tr>
   <tr>
     <td rowspan="52" style="vertical-align:top;"><a href="../../requirements/astm/f3411/v22a.md">astm<br>.f3411<br>.v22a</a></td>
     <td><a href="../../requirements/astm/f3411/v22a.md">A2-6-1,1a</a></td>

diff --git a/monitoring/uss_qualifier/suites/uspace/required_services.md b/monitoring/uss_qualifier/suites/uspace/required_services.md
@@ -16,6 +16,12 @@
     <th>Status</th>
     <th>Checked in</th>
   </tr>
+  <tr>
+    <td rowspan="1" style="vertical-align:top;"><a href="../../requirements/astm/f3411/v19.md">astm<br>.f3411<br>.v19</a></td>
+    <td><a href="../../requirements/astm/f3411/v19.md">NET0220</a></td>
+    <td>Implemented</td>
+    <td><a href="../../scenarios/astm/netrid/v22a/aggregate_checks.md">ASTM F3411-22a NetRID aggregate checks</a></td>
+  </tr>
   <tr>
     <td rowspan="52" style="vertical-align:top;"><a href="../../requirements/astm/f3411/v22a.md">astm<br>.f3411<br>.v22a</a></td>
     <td><a href="../../requirements/astm/f3411/v22a.md">A2-6-1,1a</a></td>