feature-2924: Add an option to suppress server identification headers

[byarr]

remove-version-and-server-headers had recent pushes 29 minutes ago Add a hide_server_id_headers option that suppresses the surreal-version and server headers

Thank you for submitting this pull request! We really appreciate you spending the time to work on these changes.

What is the motivation?

Offering up information such as server running and the particular version can make it easier for bad actors to exploit known vulnerabilities.

What does this change do?

This adds a CLI option to not emit the server and surreal-version headers. By setting this flag, the server no longer emits this information.

What is your testing strategy?

Testing has been done manually. Starting the server without the flag set, curl and check the headers are present. Start the server again with the flag set - use curl to confirm the headers are not present.

Will look at integration tests now.

Is this related to any issues?

Closes #2924

Does this change need documentation?

Probably - not done yet.

If this pull request requires changes, updates, or improvements to the documentation, then add a corresponding issue on the docs.surrealdb.com repository, and link to it here.

• No documentation needed
• Feat(docusaurus): Adds styling, theming and menus docs.surrealdb.com#1

Have you read the Contributing Guidelines?

• I have read the Contributing Guidelines

[gguillemas]

Thank you for contributing this PR @byarr! To my knowledge, these headers are used by the SDKs in order to check compatibility with the backend. Have you done any testing regarding compatibility with the SDKs? Our original concern in #2924 was that naively implementing this option would lead to users enabling it while being unaware of the impact on their clients. I personally am happy to support an option to disable these headers, but I would like be aware of the impact of doing so before merging.

[byarr]

Have you done any testing regarding compatibility with the SDKs? Our original concern in #2924 was that naively implementing this option would lead to users enabling it while being unaware of the impact on their clients.

Not yet. Probably should have raised this a draft PR. I'll take a look a the SDK behaviour next.

[gguillemas]

Hi @byarr! Thanks a lot for contributing this change.

We have discussed about it internally and we have agreed that it is worth offering this option to users regardless of potential compatibility impacts, especially when implemented as an environment variable like you did in your PR. Ideally, we would like our SDKs to rely on active mechanisms of identifying the server and, in the mean time, we will document that disabling the server identification headers may affect compatibility when using SDKs or third-party software integrations. For the time being, this option will remain useful to users that are okay with the potential impact of removing headers to benefit privacy.

Regarding the specific changes that you made, they look good! I would probably change the name of the environment variable and option to be more generic so that it can include future headers that are used to identify the service. I would propose SURREAL_NO_IDENTIFICATION_HEADERS for the environment variable and no_identification_headers for the option.

If you are able, it would be nice to have an integration test in tests/http_integration.rs. Otherwise I can do it 👍

[gguillemas]

I have requested some small changes, but otherwise the change is great!