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

mgr/dashboard: Audit REST API calls #24475

Merged
merged 1 commit into from Nov 5, 2018

Conversation

Projects
None yet
9 participants
@votdev
Contributor

votdev commented Oct 8, 2018

Fixes: https://tracker.ceph.com/issues/36193

Enable API auditing with ceph dashboard set-audit-api-enabled true (default is false). If you do not want to log the request payload, then disable it via set-audit-api-log-payload false (default is true).

Example output:

2018-10-08 10:25:21.850994 mgr.x [INF] [DASHBOARD] from='https://[::1]:44410' path='/api/auth' method='POST' user='None' params='{"username": "admin", "password": "***", "stay_signed_in": false}'

Signed-off-by: Volker Theile vtheile@suse.com

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

@votdev votdev force-pushed the votdev:feature_36193 branch from c19c35c to 5d4b2c2 Oct 9, 2018

@votdev

This comment has been minimized.

Contributor

votdev commented Oct 10, 2018

@sebastian-philipp All comments have been addressed.

@votdev votdev force-pushed the votdev:feature_36193 branch from 5d4b2c2 to 6be77ae Oct 10, 2018

@votdev votdev requested review from sebastian-philipp and removed request for jcsp Oct 10, 2018

@votdev votdev changed the title from [DNM] mgr/dashboard: Audit REST API calls to mgr/dashboard: Audit REST API calls Oct 10, 2018

@votdev votdev force-pushed the votdev:feature_36193 branch 2 times, most recently from a144db8 to ff9b8cb Oct 10, 2018

@jcsp

This comment has been minimized.

Contributor

jcsp commented Oct 10, 2018

I think this probably needs something to eliminate passwords from the logged data, right? Assuming that user creation etc comes through normal POST paths

@s0nea

This comment has been minimized.

Contributor

s0nea commented Oct 12, 2018

jenkins retest this please

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Oct 12, 2018

Shouldn't these set-audit-api-enabled and set-audit-api-log-payload be added to the documentation (dashboard.rst)?

@votdev votdev force-pushed the votdev:feature_36193 branch 2 times, most recently from 0809ed6 to 653c765 Oct 22, 2018

@votdev votdev force-pushed the votdev:feature_36193 branch from 653c765 to 11ace16 Oct 23, 2018

@s0nea

This comment has been minimized.

Contributor

s0nea commented Oct 23, 2018

I guess you're going to reorganize/reword your commits later?

@votdev votdev force-pushed the votdev:feature_36193 branch from 11ace16 to 391f048 Oct 23, 2018

@votdev votdev changed the title from mgr/dashboard: Audit REST API calls to [WIP] mgr/dashboard: Audit REST API calls Oct 23, 2018

@votdev votdev force-pushed the votdev:feature_36193 branch from 391f048 to dc5871a Oct 24, 2018

--------
The REST API is capable of logging PUT, POST and DELETE requests to the Ceph
audit log. This feature is disabled by default, but can be enabled easily::

This comment has been minimized.

@LenzGr

LenzGr Oct 29, 2018

Contributor

Small suggestion:

Suggested change Beta
audit log. This feature is disabled by default, but can be enabled easily::
audit log. This feature is disabled by default, but can be enabled with the
following command::

This comment has been minimized.

@votdev

votdev Oct 29, 2018

Contributor

Done

* user - The name of the user, otherwise 'None'
The logging of the request payload (the arguments and their values) can be enabled
or disabled if required::

This comment has been minimized.

@LenzGr

LenzGr Oct 29, 2018

Contributor

What's the default when enabling API logging? It may make sense to mention that it's logging the payload by default (if I read the code correctly).

This comment has been minimized.

@votdev

votdev Oct 29, 2018

Contributor

Done

@votdev votdev force-pushed the votdev:feature_36193 branch from d422118 to fe1ec37 Oct 29, 2018

@votdev votdev force-pushed the votdev:feature_36193 branch from fe1ec37 to 6e5d250 Oct 30, 2018

@p-na p-na removed the needs-rebase label Oct 31, 2018

@p-na

I like the addition and it works well, except for the password length being logged.

# Hide sensitive data like passwords, ... Extend list if necessary.
for param_name in ['password', 'passwd']:
if param_name in params:
params[param_name] = '*' * len(params[param_name])

This comment has been minimized.

@p-na

p-na Oct 31, 2018

Contributor

I'm slightly concerned about the security of replacing each character of a password with an asterisk. IMO it would be better if the length of a password wasn't logged. Can you please adapt it?

This comment has been minimized.

@votdev

votdev Oct 31, 2018

Contributor

Done

@p-na

This comment has been minimized.

Contributor

p-na commented Oct 31, 2018

Nit: The example output in the commit isn't quite correct. It shows the password in plain text.

@votdev votdev force-pushed the votdev:feature_36193 branch from 6e5d250 to b13f4a9 Oct 31, 2018

@votdev

This comment has been minimized.

Contributor

votdev commented Oct 31, 2018

Nit: The example output in the commit isn't quite correct. It shows the password in plain text.

Fixed

@p-na

p-na approved these changes Oct 31, 2018

@votdev votdev force-pushed the votdev:feature_36193 branch from b13f4a9 to d7a95a9 Oct 31, 2018

@votdev votdev added the DNM label Nov 2, 2018

@votdev votdev changed the title from mgr/dashboard: Audit REST API calls to [DNM] mgr/dashboard: Audit REST API calls Nov 2, 2018

@ceph ceph deleted a comment from LenzGr Nov 2, 2018

@votdev votdev force-pushed the votdev:feature_36193 branch 2 times, most recently from bf6e3c8 to 596df0e Nov 2, 2018

@votdev votdev removed the DNM label Nov 5, 2018

@votdev votdev changed the title from [DNM] mgr/dashboard: Audit REST API calls to mgr/dashboard: Audit REST API calls Nov 5, 2018

mgr/dashboard: Audit REST API calls
Fixes: https://tracker.ceph.com/issues/36193

Enable API auditing with 'ceph dashboard set-audit-api-enabled true' (default is false). If you do not want to log the request payload, then disable it via 'set-audit-api-log-payload false' (default is true).

Example output:
2018-10-08 10:25:21.850994 mgr.x [INF] [DASHBOARD] from='https://[::1]:44410' path='/api/auth' method='POST' user='None' params='{"username": "admin", "password": "***", "stay_signed_in": false}'

Signed-off-by: Volker Theile <vtheile@suse.com>

@votdev votdev force-pushed the votdev:feature_36193 branch from 596df0e to 22e07ff Nov 5, 2018

@LenzGr LenzGr merged commit de159f6 into ceph:master Nov 5, 2018

5 of 6 checks passed

ceph dashboard tests ceph dashboard tests failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@votdev votdev deleted the votdev:feature_36193 branch Nov 5, 2018

votdev added a commit to votdev/ceph that referenced this pull request Nov 7, 2018

mgr/dashboard: backend api tests: tasks.mgr.dashboard.test_osd.OsdTes…
…t failures

- Fix bug in Dashboard QA unit test framework. Don't set the application type header manually, this is done by the requests library if required.
- Enhance QA unit test helper: Print the response of the API request if it fails. This should help to identify the problem more easily.
- Fix bug in the OSD controller. A parameter needs to be converted to integer.
- Take care that the params of the request object are not modified.

The issue was introduced by PR ceph#24475. The CherryPy json_in plugin disclosed the errorneous unit test helper implementation.

Fixes: https://tracker.ceph.com/issues/36708

Signed-off-by: Volker Theile <vtheile@suse.com>

jmolmo added a commit to jmolmo/ceph that referenced this pull request Nov 28, 2018

mgr/dashboard: backend api tests: tasks.mgr.dashboard.test_osd.OsdTes…
…t failures

- Fix bug in Dashboard QA unit test framework. Don't set the application type header manually, this is done by the requests library if required.
- Enhance QA unit test helper: Print the response of the API request if it fails. This should help to identify the problem more easily.
- Fix bug in the OSD controller. A parameter needs to be converted to integer.
- Take care that the params of the request object are not modified.

The issue was introduced by PR ceph#24475. The CherryPy json_in plugin disclosed the errorneous unit test helper implementation.

Fixes: https://tracker.ceph.com/issues/36708

Signed-off-by: Volker Theile <vtheile@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment