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

Users could impersonate the user who created the event. #3123

Open
kumada626 opened this issue May 16, 2024 · 8 comments
Open

Users could impersonate the user who created the event. #3123

kumada626 opened this issue May 16, 2024 · 8 comments

Comments

@kumada626
Copy link
Contributor

What happened:
When a user sends a request to the API to create an event, the creator's property can be set to an arbitrary value, so it is possible to set a value for a user other than oneself.
https://cd.screwdriver.cd/pipelines/9550/events/801009

What you expected to happen:
Users are possible to see who is really the user who created the event.

How to reproduce it:

  • Call POST /v4/event endpoint with creator property.
  • Any value set in creator is displayed in the UI's started by.
@kumada626
Copy link
Contributor Author

kumada626 commented May 16, 2024

In addressing this issue, one straightforward solution might be to prevent overwriting the creator field. However, this approach is not feasible as it would obscure the identification of periodic builds currently executed by setting the creator to "Screwdriver scheduler".

Solution

Users allow involve making only the username portion of the creator field immutable, while allowing the display name to be modified. Additionally, we could enhance the UI such that hovering over the display name reveals the username. This change would ensure that it's clear which user created the event.

show_username

Case: Periodic build

There are two ways to handle periodic jobs in this solution.

  1. Allow overwrite by sd:scheduler
    In this case, only if the displayname is sd:scheduler, allow it to be overwritten with that value.
    In this case, it is possible for the user to hide execution by his own API call under the guise of a scheduled build.

  2. Treat as any other case.
    In this case, displayname shows pipeline admin's name.
    However, there are some places where the process is split by the username being sd:scheduler, so if this part is changed, it is necessary to change the method of determining that the build is a periodic build in other places.

I would prefer to do it 1 better because the impact of the change is small.
If there are no objections to this approach, I propose we proceed with this method to resolve the issue.

@kumada626
Copy link
Contributor Author

@VonnyJap I commented on the actions taken to address this issue and the concerns raised. Any comments, including whether this correction should be made, would be appreciated.

@VonnyJap
Copy link
Member

@kumada626 - are you proposing to add a new field display name? Would you be able to provide some API examples for current vs future (the proposed solution)? My preferred approach is not to add any new additional field.
It will be better if the creator field is checked against an allowed values at the backend, i.e. only allow creator to be the pipeline admin, Screwdriver Scheduler, or External Trigger. What do you think? I have not checked though, but I believe we have data in the db to know who are the pipeline admins.

@kumada626
Copy link
Contributor Author

@VonnyJap No new field is needed for this proposal. Because creator already has name and username, we use name field as display name.
Your proposal that checks the creator field values at the backend seems good, but even then, I think it is possible for a user to pose as Screwdriver Scheduler like my proposal 1. Because scheduler build's creator is set in the queue-service process and queue-service call POST /v4/event endpoint using Pipeline admin's token.

@VonnyJap
Copy link
Member

VonnyJap commented May 29, 2024

Lets proceed with your recommendation here then. @kumada626

@tkyi
Copy link
Member

tkyi commented Jul 10, 2024

Do we need to update logic in event meta to match?
https://github.com/screwdriver-cd/launcher/blob/master/launch.go#L547

@kumada626
Copy link
Contributor Author

@tkyi I think no need to update logic in event meta.
This change does not change the name of the property that contains the name of the creator of the event.
Since this change only prohibits changing the creator to an arbitrary value, I do not think it will affect the logic in that location.

@tkyi
Copy link
Member

tkyi commented Aug 5, 2024

Hm we had a user that noticed meta get event.creator now returns committer username vs creator/started by username and are missing that creator info when using meta get

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants