4 ms·

Thanks for doing the issue sleuthing. This is an excruciatingly bad look. You'd have thought with all the code-owner functionality that GL has, they would lock

Thanks for doing the issue sleuthing. This is an excruciatingly bad look. You'd have thought with all the code-owner functionality that GL has, they would lock down the `/lib/gitlab/auth/` files to require a security engineer to give additional signoff on top of a normal review. It looks like anyone at Gitlab can approve changes to the auth code (except LDAP): https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/CODEOWNERS#L58 https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/C... which is terrifying if true.

mdaniel 5y ago
> to require a security engineer to give additional signoff on top of a normal review Like this? > cc @gitlab-com/gl-security/appsec https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#note_756311666 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#... I do so desperately hope it doesn't come across as throwing shade, because hindsight-2020-etc, but I do also think there was some kind of weird process breakdown here because this change somehow slipped past a "4 eyes" and an appsec review phase
- deleted 5y ago
  [deleted]
nightpool 5y ago
This was already required in this case, you can see in the comments that an additional AppSec review was done and the appsec team signed off on the MR above and beyond the normal code review process.
- bloomburger 5y ago
  And two of those appsec reviewers are now out of office for the next two weeks…
  - orojackson 5y ago
    Heh, was this you [1]? Pretty much asking the same good questions you brought up in a different post. Additionally, I see that the Senior Director of Engineering, Tim Zallmann, has left a bunch of GitLab project repos about 14 hours ago as of this writing. He was one of the folks who tried pinging [3] Mr. Coutable (he's one of the reviewers that's currently OOO). The ping is likely regarding the discovery of the security vulnerability. [1] https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#note_898315619 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#... [2] https://gitlab.com/users/timzallmann/activity https://gitlab.com/users/timzallmann/activity [3] https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#note_893221370 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#...

mdaniel 5y ago

> to require a security engineer to give additional signoff on top of a normal review Like this? > cc @gitlab-com/gl-security/appsec https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#note_756311666 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#... I do so desperately hope it doesn't come across as throwing shade, because hindsight-2020-etc, but I do also think there was some kind of weird process breakdown here because this change somehow slipped past a "4 eyes" and an appsec review phase

deleted 5y ago
[deleted]

nightpool 5y ago

This was already required in this case, you can see in the comments that an additional AppSec review was done and the appsec team signed off on the MR above and beyond the normal code review process.

bloomburger 5y ago
And two of those appsec reviewers are now out of office for the next two weeks…
- orojackson 5y ago
  Heh, was this you [1]? Pretty much asking the same good questions you brought up in a different post. Additionally, I see that the Senior Director of Engineering, Tim Zallmann, has left a bunch of GitLab project repos about 14 hours ago as of this writing. He was one of the folks who tried pinging [3] Mr. Coutable (he's one of the reviewers that's currently OOO). The ping is likely regarding the discovery of the security vulnerability. [1] https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#note_898315619 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#... [2] https://gitlab.com/users/timzallmann/activity https://gitlab.com/users/timzallmann/activity [3] https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#note_893221370 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#...