-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SAML2 group mapping #11273
base: dev
Are you sure you want to change the base?
SAML2 group mapping #11273
Conversation
DryRun Security SummaryThe GitHub Pull Request summarizes several key changes to the DefectDojo application, including SAML2 configuration, deduplication and hashcode generation, additional authentication methods, security-related settings, and Celery, logging, and file upload settings, all aimed at enhancing the security and usability of the application. Expand for full summarySummary: The code changes in this GitHub Pull Request cover several areas of the DefectDojo application, with a focus on enhancing the security and usability of the application. The key changes include:
Overall, these changes demonstrate a thoughtful approach to application security, with a focus on improving the flexibility, integrity, and security of the DefectDojo application. As an application security engineer, I would recommend thoroughly reviewing the changes to ensure that they do not introduce any unintended security vulnerabilities, and that the new security-related settings are properly configured and maintained. Files Changed:
Code AnalysisWe ran Riskiness🟢 Risk threshold not exceeded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change. I was considering similar functionality in the past, and I'm happy that somebody has done it now.
A couple of comments:
- Thanks for the nice explanation in the description of PR, can you put it in the documentation as well? https://documentation.defectdojo.com/integrations/social-authentication/
- If you have an implementation that can filter-out the right list of groups, why not create them? I'm not saying it has to be default, but opt-in.
- Other social provider's groups are markered (Remote, Azure). Can you add ”SAML” as a social provider?
django-DefectDojo/dojo/models.py
Line 257 in 090b2c7
social_provider = models.CharField(max_length=10, choices=SOCIAL_CHOICES, blank=True, null=True, help_text=_("Group imported from a social provider."), verbose_name=_("Social Authentication Provider"))
Thank you
return user | ||
|
||
# list of all existing "SAML2-mapped" groups | ||
all_saml_groups = {group.name: group for group in Dojo_Group.objects.all() if self.group_re.match(group.name)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all_saml_groups = {group.name: group for group in Dojo_Group.objects.all() if self.group_re.match(group.name)} | |
all_saml_groups = {group.name: group for group in Dojo_Group.objects.filter(social_provider=Dojo_Group.SAML) if self.group_re.match(group.name)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this makes sense thinking of the AzureAD pipeline yet it "forces" groups to have been created by this (instead of added manually to match IdP groups), right?
@@ -177,6 +177,11 @@ | |||
"Lastname": "last_name", | |||
}), | |||
DD_SAML2_ALLOW_UNKNOWN_ATTRIBUTE=(bool, False), | |||
# similar to DD_SOCIAL_AUTH_AZUREAD_TENANT_OAUTH2_GROUPS_FILTER, regular expression for which SAML2 groups to map to Dojo groups (with same name) | |||
# Groups need to already exist in Dojo. And if value is not set, no group processing is done | |||
DD_SAML2_GROUPS_FILTER=(str, ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call it _READERS
as it adds a Reader role? In the future
DD_SAML2_GROUPS_FILTER=(str, ""), | |
DD_SAML2_GROUPS_FILTER_READERS=(str, ""), |
Or even more future-proof (if new roles would be created): dict with mapping to roles
DD_SAML2_GROUPS_FILTER=(str, ""), | |
DD_SAML2_GROUPS_FILTER=(dict, {"readers": ""}), |
More info, about how dict works with environ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first sight, I agree this would make sense, but does it really? When would anyone not be just reader in a group where the membership is expected to be managed by IdP?
Noone should be able to add/remove people from the group in Dojo, they should do it in IdP, otherwise that person will eventually be removed from the group (upon login)
Maybe I'm missing some use case in my mind, please let me know if so. Otherwise this would just increase complexity of the configuration without any added benefit
Thank you @kiblik Quick feedback on number 2: my initial implementation was to create the groups on demand (not optional) but there's the save signal on groups that uses user for that request and fails (with NoneType exception as it is unexpected) because there's no user (at that time yet as not done authenticating). I decided to remove creation as that also allows more granular control on the groups. If I make it optional too, it would allow both things, but what's the right fix on the owner? Should I modify the signal too to allow a group without owner? |
Is it the owner of a group? I'm not able to find this attribute. Can you send the link to which signal was failing, please? |
I believe it was this one https://github.com/fopina/django-DefectDojo/blob/268583954fb74635fc708c49d8826e1534cd8f5d/dojo/group/utils.py#L37 Easy to fix in several ways, not sure which one is preferred |
It is hard to say about preference but impersonate came to my mind first. |
I meant less technically but more business oriented: which user should own the group? Should anyone? This was the reason I didn’t try to solve it. |
Co-authored-by: kiblik <[email protected]>
@kiblik my bad, AzureAD pipeline does create the groups but one of the required settings skips that owner user in the signal. I guess I can simply do the same then, I'll update the PR next week 👍 |
c851936
to
d400356
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Description
While SAML2 authentication works well as it is, it's not very useful for authorization.
This PR introduces the ability to map SAML2 IdP groups (or some other list attribute) to Dojo groups.
And its configuration is similar to Azure AD group matching:
Testing
Regular Web Application
SAML2 web-app
http://localhost:8080/saml/acs/
as callback url and clickEnable
As there are no groups in Auth0 and roles are not shared via SAML assertions, we can push them (roles) into a custom assertion (based on this and this links