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

[uss_qualifier] netrid: DSS0020 - check DSS endpoints are encrypted #834

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Oct 31, 2024

This new scenario checks that port 80 of a DSS's base URL either:

  • refuses connections
  • answers with a redirect to the https base url

The scenario also checks that the https port is available to receive requests. Currently there is no check to enforce a minimal cipher strength.

Note: this scenario requires a DSS configured with an https:// base url, meaning that locally and in CI, it will be skipped.

Part of the effort on #754

@Shastick Shastick force-pushed the dss-0020-encrypted-connection branch 2 times, most recently from d701aba to b86c40b Compare October 31, 2024 16:03
@Shastick Shastick marked this pull request as ready for review October 31, 2024 16:14
@Shastick Shastick force-pushed the dss-0020-encrypted-connection branch 2 times, most recently from a199941 to 33d0ad1 Compare October 31, 2024 16:34
@Shastick Shastick force-pushed the dss-0020-encrypted-connection branch 2 times, most recently from 20f9e56 to 58d985c Compare November 13, 2024 12:26
self._dss.participant_id,
) as check:
try:
response = requests.get(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is valid to fail the requirement by calling a GET on the base URL. It is not specified in the OpenAPI so even if something is served as HTTP here, that would not be invalid.
Alternatively, attempt to do a valid query (e.g. search for some ISAs) by forcing HTTP and not following redirects: if that succeeds, we can for sure fail the requirement because an endpoint got served over HTTP.

Copy link
Contributor Author

@Shastick Shastick Nov 14, 2024

Choose a reason for hiding this comment

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

For background, the requirement states:

Communication between a USS and DSS instances shall (DSS0020) be encrypted using an industry-standard encryption mechanism with a minimum encryption strength of 128 bits.

Here I interpret that anything else than a redirection is a form of communication, and that if it happens over HTTP, then the requirement is failed. Hence a query to any endpoint is fair game.

I also agree that issuing a query that we would expect would succeed over https would make sense, and we can probably simply do both. Would it make sense to you if I add it separately? (and with doing two requests instead of one we already reduce the chances of running into a spurious timeout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should also be noted that DSS0020 has nothing to do with the OpenAPI Spec (it does not mention it).

Copy link
Contributor

@mickmis mickmis Nov 19, 2024

Choose a reason for hiding this comment

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

Hence a query to any endpoint is fair game.

My point was that the raw base URL is actually not an endpoint. It's a prefix to the endpoints.
Depending on how the infra around the DSS is deployed, the request might not even reach at all the DSS, e.g. if you have an ingress/RP that will forward calls only for specific paths. In that case it could be responding through HTTP and I'd see no failure of the requirement.

details=f"Was expecting a redirection to https://{parsed_url.hostname}/{parsed_url.path}, was {response.headers.get('Location')}",
)
except requests.RequestException:
# Connection was impossible: all is good
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this if the parent exception of all exceptions from requests. If so it does not appear valid to interpret that as passing the test: a network timeout or DNS issue due to unrelated reasons could be interpreted as a successful test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this might be a little bit too wide and I can narrow things down to cover time-outs and explicit connection refusals.

Regarding the specific points you noted, however:

  • a timeout may occur even if we narrow down to catching timeouts or refused connections
  • permanent DNS issues would cause the whole test suite to fail, so this check passing would not carry much weight?

I'm not sure we can reasonably cover the situation where we get a false positive due to a time-out?

We could attempt multiple connections, but we still won't cover situations where routing was consistently broken for a short while.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now specifically looking for errno.ECONNREFUSED and errno.ETIMEDOUT

self._dss.participant_id,
) as check:
try:
requests.get(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark as above: this is not specified in OpenAPI so we should not rely on it to succeed/fail tests.

Copy link
Contributor Author

@Shastick Shastick Nov 14, 2024

Choose a reason for hiding this comment

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

The way I see it, is that the OpenAPI spec defines an application protocol, and that here we are testing something about the transport layer.

At the level we are testing we don't really care about what happens at the application level: we're expecting the socket to open successfully and that's it.

Ie, requirement DSS0020 has nothing to do with the OpenAPI spec, and if something is broken with the HTTP protocol while the secure socket is fine, then a whole load of other requirements will fail?

@Shastick Shastick requested a review from mickmis November 14, 2024 18:03
@Shastick Shastick force-pushed the dss-0020-encrypted-connection branch 13 times, most recently from ef4f9cc to e6138bf Compare November 21, 2024 08:47
@mickmis mickmis force-pushed the dss-0020-encrypted-connection branch from e6138bf to 2311d59 Compare December 3, 2024 14:34
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

I've reworked the scenario to simplify it after the discussions regarding hitting an endpoint or not.
Since it cannot run in the CI, I tested it locally by hard-coding URLs http://github.com and https//github.com in the python scenario, and it behaves as expected.

@mickmis mickmis requested a review from barroco December 9, 2024 13:19
@mickmis mickmis merged commit 89c3b38 into interuss:main Dec 11, 2024
20 checks passed
@mickmis mickmis deleted the dss-0020-encrypted-connection branch December 11, 2024 11:29
github-actions bot added a commit that referenced this pull request Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants