Ali Caglayan (Mar 21 2022 at 13:40): Ali Caglayan (Mar 21 2022 at 13:40):Since I am adding the @coqbot bench command which will allow any user to bench, is security a concern? I don't know how much damage you can do in a bench script on the machines for instance? cc @Gaëtan Gilbert @Pierre-Marie Pédrot
Pierre-Marie Pédrot (Mar 21 2022 at 13:41):probably. Can't you check that the use who requested the bench is a member of some reasonable dev set? e.g. any maintainer team
Maxime Dénès (Mar 21 2022 at 13:45):Those are physical machines, so yes, giving access to anybody would be a big pb.
Maxime Dénès (Mar 21 2022 at 13:45):(if the setup is still the same as it used to be)
Ali Caglayan (Mar 21 2022 at 14:11):We can definitely restrict users in coqbot
Ali Caglayan (Mar 21 2022 at 14:11):However I feel like this is definitely not coqbots job to do
Ali Caglayan (Mar 21 2022 at 14:11):It has happened in the past that a check in coqbot has been implemented incorrectly
Ali Caglayan (Mar 21 2022 at 14:12):Currently coqbot is the user that will start the bench in the coqbot PR
Ali Caglayan (Mar 21 2022 at 14:13):I don't think it is possible for a users account to be used by coqbot to submit the bench request
Ali Caglayan (Mar 21 2022 at 14:14):Can we not run the bench scripts as a user with restricted permissions on the machines? That way we won't have sudo powers and whatnot.
Maxime Dénès (Mar 21 2022 at 14:15):Too risky, as they'd still share the same user on the machine. So a malicious user can plant something for the next one.
Maxime Dénès (Mar 21 2022 at 14:17):Altough, I haven't looked at the setup in a long time
Ali Caglayan (Mar 21 2022 at 14:17):What to do then? Do you agree that restricting from coqbots side seems like hiding the problem?
Maxime Dénès (Mar 21 2022 at 14:18):Not sure it is hiding. If we trust coqbot from the point of view of this infrastructure, then we need to restrict access to coqbot, right?
Ali Caglayan (Mar 21 2022 at 14:20):Here is a scenario that I am thinking about:
Ali Caglayan (Mar 21 2022 at 14:22):It seems to me that coqbot should not have permission to start the bench, otherwise we need to do security checks in coqbot
Théo Zimmermann (Mar 21 2022 at 14:23):There is no getting away that we need to do security checks in coqbot anyway...
Maxime Dénès (Mar 21 2022 at 14:23):Isn't there a review process for coqbot PRs?
Maxime Dénès (Mar 21 2022 at 14:23):Ok, so coqbot should not start the bench then :)
Théo Zimmermann (Mar 21 2022 at 14:23):Yes, there's a review process for coqbot PRs.
Théo Zimmermann (Mar 21 2022 at 14:24):And yes, there's a risk of vulnerabilities in the code of the bot leading to security issues with the Coq project infrastructure.
Maxime Dénès (Mar 21 2022 at 14:25):Isn't that also true for merges?
Ali Caglayan (Mar 21 2022 at 14:25):Merges can be undone, damage to benches cannot
Maxime Dénès (Mar 21 2022 at 14:25):Well, for undoing you need to notice something is off
Maxime Dénès (Mar 21 2022 at 14:26):My point is we should not be too scared. Having to sneak in a coqbot PR takes more effort than if we just open the infrastructure to arbitrary code execution.
Maxime Dénès (Mar 21 2022 at 14:27):As an attacker, I'd probably have more incentive sneaking in a malicious change in Coq than breaking the bench infrastructure
Ali Caglayan (Mar 21 2022 at 14:27):This leaves the question of my innocent looking PR letting coqbot start the bench
Gaëtan Gilbert (Mar 21 2022 at 14:27):are you saying random PRs get deployed without review and before merging?
Ali Caglayan (Mar 21 2022 at 14:28):No they are not deployed without review
Ali Caglayan (Mar 21 2022 at 14:28):However I did not think about the security risks of my PR until today
Ali Caglayan (Mar 21 2022 at 14:28):And we can basically run whatever we like on bench as a result
Ali Caglayan (Mar 21 2022 at 14:29):It is not currently deployed
Maxime Dénès (Mar 21 2022 at 14:29):It was the right time to ask, then :)
Maxime Dénès (Mar 21 2022 at 14:30):Btw, another thing we could consider is to put some virtualization layer in this infra
Ali Caglayan (Mar 21 2022 at 14:30):I can submit a PR to coq/coq that will brick the bench, if my PR was deployed coqbot would have run it
Maxime Dénès (Mar 21 2022 at 14:30):But that's some work
Ali Caglayan (Mar 21 2022 at 14:31):Virtualization for a bench might be counterproductive however.
Ali Caglayan (Mar 21 2022 at 14:31):Isn't performance something we want here?
Maxime Dénès (Mar 21 2022 at 14:31):Virtualization has very low performance penalty these days
Maxime Dénès (Mar 21 2022 at 14:31):What is more tricky for the bench is stability of execution time
Maxime Dénès (Mar 21 2022 at 14:32):That would need to be measured
Maxime Dénès (Mar 21 2022 at 14:32):That's what we couldn't get on regular CI infra
Ali Caglayan (Mar 21 2022 at 14:33):So is it not possible to set a linux user to only be able to create files and run existing programs?
Ali Caglayan (Mar 21 2022 at 14:35):Or maybe I am scaring myself too much. Perhaps the best solution is for coqbot to check who sent the request
Maxime Dénès (Mar 21 2022 at 14:35):The user is already non-privileged
Maxime Dénès (Mar 21 2022 at 14:35):I think it is safer for coqbot to just check the user is in a list, like PMP said
Ali Caglayan (Mar 21 2022 at 14:36):Fair enough I will try to do that then
Théo Zimmermann (Mar 21 2022 at 14:37):Regarding which list to use, my suggestion was to use the "@coq/contributors" team. It is a pretty large team, but we are liberal in how we add people to the GitLab organization anyway, so it matches OK with this process. Is that OK with everyone?
Maxime Dénès (Mar 21 2022 at 14:40):Sounds reasonable to me. If we have logs with the user who triggered a call to the bench, even better.
Ali Caglayan (Mar 21 2022 at 14:42):Technically we have logs. "Are they easily accessible" is another question.
Maxime Dénès (Mar 21 2022 at 14:52):Ok, but at least in case of a pb, we can go and see what happened
Paolo Giarrusso (Mar 22 2022 at 06:48):As long as coqbot jobs lack permission to tamper with the logs.
Last updated: Jun 05 2023 at 10:01 UTC