undefined | Better HN

0 pointstheptip4y ago0 comments

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/C... which is terrifying if true.

0 comments

mdaniel4y 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#...

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

1 more reply

nightpool4y 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.

bloomburger4y ago

And two of those appsec reviewers are now out of office for the next two weeks…

orojackson4y 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#...

[2] https://gitlab.com/users/timzallmann/activity

[3] https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#...

j / k navigate · click thread line to collapse