| Message ID | 20240529141945.41669-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CI: Misc improvements | expand |
On Wed, May 29, 2024 at 03:19:43PM +0100, Andrew Cooper wrote: > This restriction doesn't provide any security because anyone with suitable > permissions on the HW runners can bypass it with this local patch. > > Requiring branches to be protected hampers usability of transient testing > branches (specifically, can't delete branches except via the Gitlab UI). > > Drop the requirement. > > Fixes: 746774cd1786 ("automation: introduce a dom0less test run on Xilinx hardware") > Fixes: 0ab316e7e15f ("automation: add a smoke and suspend test on an Alder Lake system") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Runners used to be set to run only on protected branches. I think it isn't the case anymore from what I see, but it needs checking (I don't see specific settings in all the projects). If it were still the case, removing variable check would result in jobs forever pending. Other than that, I'm okay with this change, since the hw runners are added only to select projects. You can interpret this as Acked-by, if you verify if indeed runners are not limited to protected branches only. I will need to adjust setting of my project, to set "QUBES_JOBS" only to some branches - I used to use branch protection rules as a proxy to selecting on which branch to run hw tests... > --- > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Michal Orzel <michal.orzel@amd.com> > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > Fixes because this wants backporting, but it also needs acks from both Marek > and Stefano as the owners of the hardware in question. > --- > automation/gitlab-ci/test.yaml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml > index ad249fa0a5d9..efd3ad46f08e 100644 > --- a/automation/gitlab-ci/test.yaml > +++ b/automation/gitlab-ci/test.yaml > @@ -92,7 +92,7 @@ > when: always > only: > variables: > - - $XILINX_JOBS == "true" && $CI_COMMIT_REF_PROTECTED == "true" > + - $XILINX_JOBS == "true" > tags: > - xilinx > > @@ -112,7 +112,7 @@ > when: always > only: > variables: > - - $QUBES_JOBS == "true" && $CI_COMMIT_REF_PROTECTED == "true" > + - $QUBES_JOBS == "true" > tags: > - qubes-hw2 > > -- > 2.30.2 >
On Thu, 30 May 2024, Marek Marczykowski-Górecki wrote: > On Wed, May 29, 2024 at 03:19:43PM +0100, Andrew Cooper wrote: > > This restriction doesn't provide any security because anyone with suitable > > permissions on the HW runners can bypass it with this local patch. > > > > Requiring branches to be protected hampers usability of transient testing > > branches (specifically, can't delete branches except via the Gitlab UI). > > > > Drop the requirement. > > > > Fixes: 746774cd1786 ("automation: introduce a dom0less test run on Xilinx hardware") > > Fixes: 0ab316e7e15f ("automation: add a smoke and suspend test on an Alder Lake system") > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Runners used to be set to run only on protected branches. I think it > isn't the case anymore from what I see, but it needs checking (I don't > see specific settings in all the projects). If it were still the case, > removing variable check would result in jobs forever pending. Andrew, thank you so much for pointing this out. I think the idea was that we can specify the individual users with access to protected branches. We cannot add restrictions for unprotected branches. So if we set the gitlab runner to only run protected jobs, then the $CI_COMMIT_REF_PROTECTED check makes sense. Not for security, but to prevent the jobs from getting stuck waiting for a runner that will never arrive. However, like Marek said, now the gitlab runners don't have the "Protected" check set, so it is all useless :-( I would prefer to set "Protected" in the gitlab runners settings so that it becomes easier to specify users that can and cannot trigger the jobs. Then, we'll need the $CI_COMMIT_REF_PROTECTED check, not for security, but to avoid pipelines getting stuck for unprotected branches. It is really difficult to restrict users from triggering jobs in other way because they are all automatically added to all subprojects. Would you guys be OK if I set "Protected" in the Xilinx and Qubes gitlab runners as soon as possible?
On Thu, May 30, 2024 at 05:43:12PM -0700, Stefano Stabellini wrote: > On Thu, 30 May 2024, Marek Marczykowski-Górecki wrote: > > On Wed, May 29, 2024 at 03:19:43PM +0100, Andrew Cooper wrote: > > > This restriction doesn't provide any security because anyone with suitable > > > permissions on the HW runners can bypass it with this local patch. > > > > > > Requiring branches to be protected hampers usability of transient testing > > > branches (specifically, can't delete branches except via the Gitlab UI). > > > > > > Drop the requirement. > > > > > > Fixes: 746774cd1786 ("automation: introduce a dom0less test run on Xilinx hardware") > > > Fixes: 0ab316e7e15f ("automation: add a smoke and suspend test on an Alder Lake system") > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > > > Runners used to be set to run only on protected branches. I think it > > isn't the case anymore from what I see, but it needs checking (I don't > > see specific settings in all the projects). If it were still the case, > > removing variable check would result in jobs forever pending. > > Andrew, thank you so much for pointing this out. > > I think the idea was that we can specify the individual users with > access to protected branches. We cannot add restrictions for unprotected > branches. So if we set the gitlab runner to only run protected jobs, > then the $CI_COMMIT_REF_PROTECTED check makes sense. Not for security, > but to prevent the jobs from getting stuck waiting for a runner that > will never arrive. > > However, like Marek said, now the gitlab runners don't have the > "Protected" check set, so it is all useless :-( > > I would prefer to set "Protected" in the gitlab runners settings so that > it becomes easier to specify users that can and cannot trigger the jobs. Owners of subprojects can control branch protection rules, so this feature doesn't help with limiting access to runners added to the whole group. Qubes runners are not group runners, they are project runners added only to select projects. I don't remember why exactly runners got "protected" disabled, but AFAIR there was some issue with that setting. > Then, we'll need the $CI_COMMIT_REF_PROTECTED check, not for security, > but to avoid pipelines getting stuck for unprotected branches. > > It is really difficult to restrict users from triggering jobs in other > way because they are all automatically added to all subprojects. > > > Would you guys be OK if I set "Protected" in the Xilinx and Qubes gitlab > runners as soon as possible?
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml index ad249fa0a5d9..efd3ad46f08e 100644 --- a/automation/gitlab-ci/test.yaml +++ b/automation/gitlab-ci/test.yaml @@ -92,7 +92,7 @@ when: always only: variables: - - $XILINX_JOBS == "true" && $CI_COMMIT_REF_PROTECTED == "true" + - $XILINX_JOBS == "true" tags: - xilinx @@ -112,7 +112,7 @@ when: always only: variables: - - $QUBES_JOBS == "true" && $CI_COMMIT_REF_PROTECTED == "true" + - $QUBES_JOBS == "true" tags: - qubes-hw2
This restriction doesn't provide any security because anyone with suitable permissions on the HW runners can bypass it with this local patch. Requiring branches to be protected hampers usability of transient testing branches (specifically, can't delete branches except via the Gitlab UI). Drop the requirement. Fixes: 746774cd1786 ("automation: introduce a dom0less test run on Xilinx hardware") Fixes: 0ab316e7e15f ("automation: add a smoke and suspend test on an Alder Lake system") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Michal Orzel <michal.orzel@amd.com> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> Fixes because this wants backporting, but it also needs acks from both Marek and Stefano as the owners of the hardware in question. --- automation/gitlab-ci/test.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)