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

T4406: Add public API endpoint to display information #4380

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

oniko94
Copy link
Contributor

@oniko94 oniko94 commented Mar 5, 2025

Change summary

Adds a public endpoint /info to the HTTP(S) API to display some basic information about the router, namely router's hostname, VyOS version and pre-login banner.

The endpoint accepts version and hostname query parameters, set to valid boolean values (1, 0, 'yes', 'no' etc. are acceptable as well), which dictate whether to include respective data into the response.

  • If values are set to False or equivalent, a correspondent field in the response object is set to an empty string.
  • If no query parameters are supplied, all three fields are populated with actual data by default.
  • The query parameters' type is validated. Extra parameters besides version and hostname are not allowed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

https://vyos.dev/T4406

Related PR(s)

How to test / Smoketest result

  • Build the debian package for vyos-1x
  • Install it to vyos-1x instance
  • Enable the HTTP API
  • Send a GET request to the API endpoint, for example curl --insecure --location --request GET 'https://<INSTANCE_ADDRESS>/info
  • Try different combinations of query parameters, for example ?version=0&hostname=1 or ?hostname=false or ?version=yes&hostname=0

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@oniko94 oniko94 requested a review from a team as a code owner March 5, 2025 17:55
Copy link

github-actions bot commented Mar 5, 2025

👍
No issues in PR Title / Commit Title

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

Other than the trivial question of the exclamation mark, I see nothing against this. Brute force attack operators typically blast exploits at everything without any checks, so exposing the version string to the public is not a big risk even in public networks.

Copy link
Contributor

@jestabro jestabro left a comment

Choose a reason for hiding this comment

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

@oniko94 why not keep this endpoint with the others in api/rest/routers.py ? I can think of reasons to do it the way that you have, just curious as to the reasoning of your decision ...

@oniko94
Copy link
Contributor Author

oniko94 commented Mar 6, 2025

@jestabro I followed mainly two reasons behind putting this endpoint in this file:

  1. 'src/services/api/rest/routers' contains the routes that all require authentication by an API key. Meanwhile, if I understood the ticket correctly, we would want this endpoint to be publicly available for anonymous users. It would require to declare additional FastAPI router instance in this case, which could potentially cause confusion which router does what.
  2. In my opinion it would also make more sense to have this endpoint active as soon as the HTTPS API service is activated, even if the user hasn't enabled the REST API specifically, or requires only the GraphQL HTTPS API. Thus, it seemed logical to me to keep it at the FastAPI app initialization level

@jestabro jestabro self-requested a review March 6, 2025 12:11
@jestabro
Copy link
Contributor

jestabro commented Mar 6, 2025

I fully agree; thanks.

@andamasov andamasov requested a review from Copilot March 6, 2025 12:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR introduces a new public API endpoint /info to provide basic router information, such as the hostname, version, and a pre-login banner. Key changes include:

  • Adding a new endpoint in the HTTP API server to handle /info requests.
  • Implementing query parameter validation via the new InfoQueryParams model.
  • Integrating file-read operations and error handling to return formatted service responses.

Reviewed Changes

File Description
src/services/vyos-http-api-server Adds the /info endpoint with query parameter handling and error logging.
src/services/api/rest/models.py Introduces the InfoQueryParams model to validate incoming query parameters.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

@oniko94 oniko94 force-pushed the feature/T4406-vyos-info-api-endpoint branch from d8acdcc to 9fe7d85 Compare March 6, 2025 12:47
Copy link

github-actions bot commented Mar 6, 2025

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests (no interfaces) 👍 passed
  • CLI Smoketests (interfaces only) 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed
  • TPM tests 👍 passed

@c-po c-po added the bp/circinus Create automatic backport for circinus label Mar 6, 2025
@c-po c-po merged commit 0ca7771 into vyos:current Mar 6, 2025
16 of 17 checks passed
@vyosbot vyosbot added mirror-initiated This PR initiated for mirror sync workflow mirror-completed and removed mirror-initiated This PR initiated for mirror sync workflow labels Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bp/circinus Create automatic backport for circinus current mirror-completed
Development

Successfully merging this pull request may close these issues.

5 participants