-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
add validation to prevent filters on dn lookups #9134
Conversation
[testonlyextended][extended:ldap_groups] |
[test] |
hoisted by your own petard? |
I blame the guy who wrote a set of tests that were wrong :). |
@stevekuznetsov other than the test, anything? |
} | ||
return validationResults | ||
} | ||
|
||
if _, err := ldap.CompileFilter(query.Filter); err != nil { |
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.
This implicitly disallows not specifying a filter, so how is the validation not failing for those missing filters?
This allows queries without filters, which complicates the config substantially... let's see the Docs PR to determine how onerous it is to explain this. |
It only allows them when the caller explicitly says, "this exact one (not category) will only be used to lookup dns". I don't think it affects docs at all. Even if it does, do you see us not doing this? |
I understand it's correct, but clearly there is a documentation gap -- we should definitely doc that a filter is not expected and will not be used if the UID attribute for a query is the DN. |
Evaluated for origin testonlyextended up to 9351adc |
Evaluated for origin test up to 9351adc |
continuous-integration/openshift-jenkins/testonlyextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/206/) (Extended Tests: ldap_groups) |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4422/) (Image: devenv-rhel7_4311) |
Evaluated for origin merge up to 9351adc |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4422/) |
Delete filter due to openshift/origin#9134
Bug https://bugzilla.redhat.com/show_bug.cgi?id=1339325
Tightens validation for ldap sync config. Filters on dn lookups don't work.
@stevekuznetsov can you tag this for the right extended test?