Group Maintainers are able to toggle the package and registries settings using this missing permission check.
Steps to reproduce
Login as Group owner and go to https://gitlab.com/groups/<GroupName>/-/settings/packages_and_registries. You should be able to toggle all the settings from here.
Now login as maintainer in a group and navigate to same url https://gitlab.com/groups/<GroupName>/-/settings/packages_and_registries but you wont be able to access it.
Now run the below Graphql to toggle Settings for Maven packages duplicates.
That report was just about dependency proxy settings but this also involves graphql for enabling Settings for Maven packages which maintainer can toggle.
Also, that documentation could be wrong too as https://gitlab.com/groups/<GroupName>/-/settings/packages_and_registries is not accessible for group maintainers so not sure if that documentation needs any correction. That report was closed by your team member without taking anyone's opinion and directly based on what is written in documentation . As i did not have proper evidence that time and also it was written in documentation, i did not follow up further. However, this involves multiple graphql queries as you see in report and all should work for group owner only as per UI.
I would suggest taking a view of Gitlab team as https://gitlab.com/groups/<GroupName>/-/settings/packages_and_registries is not accessible for group maintainers yet they can toggle all the settings from there.
Thank you for your submission! We were able to validate your report, and have submitted it to the appropriate remediation team for review. They will let us know the final ruling on this report, and when/if a fix will be implemented. Please note that the status and severity are subject to change.
Note that this appears to follow our authorization schema, however this particular feature flag is not included in the UI for some reason. Navigating to https://gitlab.com/groups/<GroupName>/-/settings/packages_and_registries as a maintainer ends up with a 404.
@m_gill - I want to acknowledge my bad judgment trying to react quickly here and not being aware of the scope and ownership of the Authentication and Authorization group. I will need @rchanila, @10io, or @sabrams confirmation here if this falls or not into ~"group::package".
We recently updated the graphql/api permissions to be maintainer from developer to conform with the frontend. I will have to look more closely, but it sounds like this particular page in the UI requires owner permissions.
@michelletorres no worries - we have a low level of shame too! I did think it was ~"group::package" though. "Permissions" or "settings" is too general to give 1 team. Thanks for confirming @sabrams!
it sounds like this particular page in the UI requires owner permissions.
Access to the settings tab, & therefore Packages & Registry settings on the left side is available only for group owners (they have :admin_group set on their group policy) and the same check is made if the user tries to visit the URL directly. So maintainers don't have access to this UI.
Raise the permissions of the APIs to owner, essentially repeating the changes made in #350682 (closed).
Lower the permissions of the UI to maintainer.
Both are small changes, but (1.) is a breaking change. We waited for %15.0 to raise the permissions to maintainer, so I expect we would have to wait until %16.0 to raise them to owner. (2.) is not a breaking change, but we need to decide if these settings are appropriate for maintainers to change. Based on the fact that we already decided maintainer made sense in the previous issues, I'd push for (2.) unless it strays greatly from what other similar settings.
I think we should go with option (2). The fix to this security issue is to ensure the UI and the API will behave in the same way to the same level of permissions.
Raising the permission is another good option, but I do not see it as a boring solution. Now, I still think it will be valuable to do a quick analysis if we really need to raise to owner or if keeping maintainer is enough. As that will be a breaking change, that gives us enough time for that validation.
2 sounds straightforward except that none of the group settings are visible for a maintainer.
If we look at the function definition only owners (:admin_grouppolicy is applicable only for role owner) are shown the link to group settings on the left sidebar, Is my understanding correct @sabrams ?
Assuming the above right, if we do go ahead with implementing option 2, the consequence of the would be that "Settings" tab with a single entry for "Packages & Registries" would start appearing for maintainers. Just wanted to call-out this UX change @michelletorres
I think I see where things got mixed up. We were dealing with both project and group level settings when we updated the permissions, but each page is viewable by different roles:
The project-level settings page is viewable by maintainers/owners of the project.
The group-level settings page is viewable by owners of the group.
The bad news is that we won't be able to lower the settings of the UI as the entire settings tab is only showed to Owners. So, we'd break all of the other settings.
I think that we'll need to update the API, which we'll need a breaking change for. Perhaps we can do it outside of the normal 16.0 cycle.
@vdesousa@michelletorres given that we're not resolving this until %16.0, what do we think of adding something to the documentation and making this issue public?
If we publicly acknowledge the discrepancy between the UI & the API then Group Owners / administrators have an opportunity to understand any risk it might pose them. The alternative is that it remains obscure and hidden for the next 10 months.
With the new SLOs, we will be out of the remediation timelines for this if we fix it in %16.0
@vdesousa - Should we create a new issue for the docs update, or should we update this one to keep it as the vulnerability that will be fixed by the doc changes and create a new one for the breaking change?
With the new SLOs, we will be out of the remediation timelines for this if we fix it in %16.0
Should we create a new issue for the docs update, or should we update this one to keep it as the vulnerability that will be fixed by the doc changes and create a new one for the breaking change?
@michelletorres I don't have a big preference but it makes the most sense to me to do the latter - update the docs through this issue and create a new one for the breaking change.
Just to clarify @michelletorres, to resolve this issue we are only making documentation changes to inform users of the permissions discrepancy and that we will be limiting to Owners in 16.0?
!92009 (closed) didn't update the documentation as expected. Why? Because the changes of that MR where on a file that is generated from code. That means that upon the next generation, the changes were undone.
To permanently have the documentation changed, we need to do that in the code. I'm not sure that there is a way to put a note there but as last resort solution, we could always update the mutation description field.
Given that this is a securitydocumentation, I gather that we should still follow the security workflow (changes for master + 3 most recent versions). Is that accurate?
Given that this is a securitydocumentation, I gather that we should still follow the security workflow (changes for master + 3 most recent versions). Is that accurate?
I have no idea , but from your comment, I'm assuming yes because we will end up updating the mutation description field?
@10io I can comment on !92009 (closed) When that landed in my inbox for review, I was still rather new to GraphQL and didn't double check that the changes were made in the Ruby source before I reviewed the changes. In an offline discussion at the time, Tim and I realised the mistake and the MR was closed with the intent that a new MR would be opened with the changes added in the source. I just poked about in my history and grepped the source, and it looks like the followup didn't happen.
I have no idea , but from your comment, I'm assuming yes because we will end up updating the mutation description field?
@michelletorres Yes, that's my assumption too. If it is the description field or something else, we will need to update the code so that the generated documentation looks good.
Good point @claytoncornell ! I missed that the MR was closed and not merged.
Well, at least, it gives us the changes that we want to do here.
Anyway, we will ping you in the related MR for this security issue.
@kmorrison1@nmalcolm Could one of you confirm that this change still needs to go through security workflow (1 MR for master + 3 MRs for backports) given that this issue is labeled with security? Thanks!
I think AppSec should make the decision, but wanted to share my perspective on this particular scenario:
In my mind, we are essentially adding a deprecation warning: Ability to access these settings in GraphQL will be restricted to Owners in 16.0. Deprecations don't have any specific guidelines about how soon the warning needs to be communicated before the major release. We could consider the GraphQL deprecation guideline that says parts of the schema should be available for at least 6 releases after initial deprecation, but we are dealing with authorization, not the schema itself, so I don't think that would apply. Either way, we should have about 6 more releases until we get to 16.0.
@10io@michelletorres I caught up on the discussion above and I would approve skipping the security release process here. We're acknowledging the discrepancy between UI/API and basically doing a "it's not a bug it's a feature" so let's not add work with the backports. I would also support making this issue public given that we're going to document instead of fixing it immediately.
The MR updating the GraphQL doc is now in production. I am closing this issue. #370471 (closed) is the followup where we will change the permission in %16.0. I am closing this issue.
Based on this thread I believe everyone is in favor of making this issue public now. @nmalcolm I just wanted to confirm this is ok before I go ahead and do that.