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.

Sign up for GitHub

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

Jump to bottom

Fixes #35527 - Include the remote IP in status #9423

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fixes #35527 - Include the remote IP in status #9423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/controllers/api/v2/home_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
module Api
module V2
class HomeController < V2::BaseController
include Foreman::Controller::SmartProxyAuth

before_action :require_admin, :only => [:index]
layout false

Expand All @@ -11,9 +13,11 @@ def index
Apipie.reload_documentation if Apipie.configuration.reload_controllers?
end

add_smart_proxy_filters :status
api :GET, "/status/", N_("Show status")

def status
@remote_ip = request.remote_ip if detected_proxy
Copy link
Member

Choose a reason for hiding this comment

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

why we'd add that only in case the proxy auth is used? I'd say it's reasonable to always tell which IP asked for the status

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial reasoning was that this was a way to see if authentication worked. Perhaps that's something we can rework and make it into a proper object with a link to the Smart Proxies API object page (something like /api/smart_proxies/:id IIRC).

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to go ahead with this as part of this PR? Or do you want to get it in as is?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

I further considered this. I'd say that it's a potential information leak if we return it for all users. If a setup is misconfigured, it could leak data an attacker could use.

For now I'd prefer to get this in as it is. We can later add the Smart Proxy details. If we have this in Foreman 3.5 we can use it in scripts to verify things so I'd also ask about a cherry pick into 3.5.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for late response here, I wouldn't have problem merging as is (right after testing), if you wish to still get it to 3.5, ping me directly pls.

I was curious, what information an attacker gets if we add remote ip? Who's IP would it be?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for late response here, I wouldn't have problem merging as is (right after testing), if you wish to still get it to 3.5, ping me directly pls.

As discussed offline: let's postpone this to later and discuss the implications further.

I was curious, what information an attacker gets if we add remote ip? Who's IP would it be?

If there's a reverse proxy in between that may be untrusted you could leak details. Defense in depth suggests you should leak as little information as possible to give attackers minimal information.

Copy link
Contributor

Choose a reason for hiding this comment

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

I further considered this. I'd say that it's a potential information leak if we return it for all users. If a setup is misconfigured, it could leak data an attacker could use.

For now I'd prefer to get this in as it is. We can later add the Smart Proxy details. If we have this in Foreman 3.5 we can use it in scripts to verify things so I'd also ask about a cherry pick into 3.5.

If an attacker has access to this data, it would be possible to run REX jobs; access the DB; see the logs and audits. I don't think this should block us from adding such a helpful information.

end
end
end
Expand Down
1 change: 1 addition & 0 deletions app/views/api/v2/home/status.json.rabl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ node(:result) { "ok" }
node(:status) { 200 }
node(:version) { SETTINGS[:version].full }
node(:api_version) { 2 }
node(:remote_ip) { @remote_ip } if @remote_ip