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

Audit trail: implement auditable event logging for sensitive actions #5863

Closed
5 tasks done
brainwane opened this issue May 16, 2019 · 10 comments · Fixed by #6339
Closed
5 tasks done

Audit trail: implement auditable event logging for sensitive actions #5863

brainwane opened this issue May 16, 2019 · 10 comments · Fixed by #6339
Labels
admin Features needed for the Admin UI (people running the site) CSS/SCSS requires change to CSS/SCSS files documentation feature request HTML requires change to HTML files javascript requires change to JavaScript files UX/UI design, user experience, user interface

Comments

@brainwane
Copy link
Contributor

brainwane commented May 16, 2019

Warehouse is adding an advanced audit trail of user actions beyond the current (existing) journal. This will, for instance, allow publishers to track all actions taken by third party services on their behalf.

  • Add auditing for user actions in PyPI
  • Add auditing for project actions in PyPI
  • Implement a User view for User auditing, allowing publishers to track all actions
    taken by third party services on their behalf
  • Implement a Project view for Project auditing for project maintainers to audit
    actions similarly
  • Implement an Admin view for PyPI.org administrators to audit actions similarly

So:

  • Each user will be able to view a log of sensitive actions performed that are relevant to their user account.
  • Each user who maintains at least one project on PyPI will be able to view a log of sensitive actions
    (performed by ANY user) relevant to projects they act in the Owner Role on.
  • And PyPI administrators will be able to view the full audit log.

We'll be working on this in 2019. The Packaging Working Group, seeking donations and further grants to fund more work, got some new funding from the Open Technology Fund, and the audit log is part of the current grant-funded project.

@brainwane
Copy link
Contributor Author

Noting here that sensitive actions worth including in the event log would include renaming a project, per #1919.

@trishankatdatadog
Copy link
Contributor

Ok, @woodruffw gave me a clue, and asked me to look at all the metrics calls, like so:

$ git grep -C1 self._metrics
warehouse/accounts/services.py-        )
warehouse/accounts/services.py:        self._metrics = metrics
warehouse/accounts/services.py-
--
warehouse/accounts/services.py-
warehouse/accounts/services.py:        self._metrics.increment("warehouse.authentication.start", tags=tags)
warehouse/accounts/services.py-
--
warehouse/accounts/services.py-            logger.warning("Global failed login threshold reached.")
warehouse/accounts/services.py:            self._metrics.increment(
warehouse/accounts/services.py-                "warehouse.authentication.ratelimited",
--
warehouse/accounts/services.py-            if not self.ratelimiters["user"].test(user.id):
warehouse/accounts/services.py:                self._metrics.increment(
warehouse/accounts/services.py-                    "warehouse.authentication.ratelimited",
--
warehouse/accounts/services.py-
warehouse/accounts/services.py:                self._metrics.increment("warehouse.authentication.ok", tags=tags)
warehouse/accounts/services.py-
--
warehouse/accounts/services.py-            else:
warehouse/accounts/services.py:                self._metrics.increment(
warehouse/accounts/services.py-                    "warehouse.authentication.failure",
--
warehouse/accounts/services.py-        else:
warehouse/accounts/services.py:            self._metrics.increment(
warehouse/accounts/services.py-                "warehouse.authentication.failure", tags=tags + ["failure_reason:user"]
--
warehouse/accounts/services.py-        tags = tags if tags is not None else []
warehouse/accounts/services.py:        self._metrics.increment("warehouse.authentication.two_factor.start", tags=tags)
warehouse/accounts/services.py-
--
warehouse/accounts/services.py-            logger.warning("Global failed login threshold reached.")
warehouse/accounts/services.py:            self._metrics.increment(
warehouse/accounts/services.py-                "warehouse.authentication.two_factor.ratelimited",
--
warehouse/accounts/services.py-        if not self.ratelimiters["user"].test(user_id):
warehouse/accounts/services.py:            self._metrics.increment(
warehouse/accounts/services.py-                "warehouse.authentication.two_factor.ratelimited",
--
warehouse/accounts/services.py-        if totp_secret is None:
warehouse/accounts/services.py:            self._metrics.increment(
warehouse/accounts/services.py-                "warehouse.authentication.two_factor.failure",
--
warehouse/accounts/services.py-        if valid:
warehouse/accounts/services.py:            self._metrics.increment("warehouse.authentication.two_factor.ok", tags=tags)
warehouse/accounts/services.py-        else:
warehouse/accounts/services.py:            self._metrics.increment(
warehouse/accounts/services.py-                "warehouse.authentication.two_factor.failure",
--
warehouse/accounts/services.py-        self._api_base = api_base
warehouse/accounts/services.py:        self._metrics = metrics
warehouse/accounts/services.py-        self._help_url = help_url
--
warehouse/accounts/services.py-    def _metrics_increment(self, *args, **kwargs):
warehouse/accounts/services.py:        self._metrics.increment(*args, **kwargs)
warehouse/accounts/services.py-
--
warehouse/accounts/services.py-
warehouse/accounts/services.py:        self._metrics_increment("warehouse.compromised_password_check.start", tags=tags)
warehouse/accounts/services.py-
--
warehouse/accounts/services.py-            logger.warning("Error contacting HaveIBeenPwned: %r", exc)
warehouse/accounts/services.py:            self._metrics_increment(
warehouse/accounts/services.py-                "warehouse.compromised_password_check.error", tags=tags
--
warehouse/accounts/services.py-            if hashed_password[5:] == possible.lower():
warehouse/accounts/services.py:                self._metrics_increment(
warehouse/accounts/services.py-                    "warehouse.compromised_password_check.compromised", tags=tags
--
warehouse/accounts/services.py-        # If we made it to this point, then the password is safe.
warehouse/accounts/services.py:        self._metrics_increment("warehouse.compromised_password_check.ok", tags=tags)
warehouse/accounts/services.py-        return False
--
warehouse/rate_limiting/__init__.py-                logging.warning("Error computing rate limits: %r", exc)
warehouse/rate_limiting/__init__.py:                self._metrics.increment(
warehouse/rate_limiting/__init__.py-                    "warehouse.ratelimiter.error", tags=[f"call:{fn.__name__}"]
--
warehouse/rate_limiting/__init__.py-        self._identifiers = identifiers
warehouse/rate_limiting/__init__.py:        self._metrics = metrics
warehouse/rate_limiting/__init__.py-

From the metrics available so far, I will say that:

  1. Maintainers should not see compromised password checks for Owners and other Maintainers, only themselves.
  2. Maintainers should not see rate limit messages for Owners and other Maintainers, only themselves.
  3. Maintainers should not see MFA authentication messages for Owners and other Maintainers, only themselves.

@trishankatdatadog
Copy link
Contributor

I'm not saying the following is necessarily correct, but they should provide a way to reason about this. Hope this helps!

image

image

@brainwane
Copy link
Contributor Author

@eirnym
Copy link

eirnym commented Jun 9, 2019

I want to propose also flagging projects as spam/malware by the community. Self-regulation is good thing and some rules we can take from stack overflow community. This will greatly help the administrators

@brainwane
Copy link
Contributor Author

@eirnym Thanks for the suggestion -- we're tracking that feature request as #3896.

@brainwane
Copy link
Contributor Author

This work is part of the milestone of work we're doing for the Open Tech Fund-supported security work. Right now, as I understand it, @woodruffw and @nlhkabu are working on the API key work, but after that, they'll be working on this issue.

@brainwane
Copy link
Contributor Author

User exarkun in IRC just pointed out that the public might want to know who uploaded a particular release, and that info perhaps should be in the ownership history/audit log for a project.

@brainwane
Copy link
Contributor Author

Warehouse maintainers and @woodruffw discussed design and scope for this feature in a meeting today.

Design: We chose a basic approach for the auditing API: a new API, called in tandem with metrics calls in places where both are needed/desired.

Storage: We have some research to do into how long we will retain user-related events (#3532), but we aim to retain project-related events permanently. We discussed a more flexible storage medium than Postgres (maybe a document store?) and started thinking about what kinds of data we'll need to store.

Scope: To avoid running over our funds for this project, we're limiting the scope for what we'll accomplish right now. We'll focus on:

  • implementing the necessary API calls internally
  • implementing storage
  • implementing a handful of auditable events
  • creating a few views for those, and exposing what makes sense today (e.g.: recent logins, project change events, and 2FA events)
    • we may want to show most project-related events to all users, but hide some info about each event from users who aren't maintainers or owners, e.g. IP. @nlhkabu is taking note of this for designing templates.

Right now we want to ensure that we can store and retrieve events. We can add more auditable events down the line.

Timing: This is the first thing we're working on in August, and we are aiming to get it merged and deployed in August.

@woodruffw woodruffw mentioned this issue Aug 1, 2019
11 tasks
@woodruffw
Copy link
Member

Began work on this in #6339.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admin Features needed for the Admin UI (people running the site) CSS/SCSS requires change to CSS/SCSS files documentation feature request HTML requires change to HTML files javascript requires change to JavaScript files UX/UI design, user experience, user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants