URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: opened
PR body: """ TL;DR - this is to implement functionality similar to both of `sshd_config:AllowUsers` and of `PAM's own rhost verification`.
This was asked in IRC and [mailing list](https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o...) (with little follow up in both). The reasoning behind implementation can be seen in linked mailing list thread.
Current PR provides basic functionality of comparing rhost (from pam) with values stored in LDAP. To enable this set `ldap_access_order = rhost` and `ldap_user_authorized_rhost = <ldap_field_name| default: rhost>` in sssd.conf.
It _currently*_ provides similar rule evaluation as currently it works for host based authentication.
TODO: - [ ] Finalize logic of using DNS/rDNS for rules validation (currently working on basic idea how it should work - any help here?) - [ ] Implement use of DNS/rDNS (with optional switch to enable/disable) - [ ] Documentation - [ ] Test coverage (didn't see test coverage for host auth, so is it needed?) """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-301707090
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-301707095
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: edited
Changed field: body Original value: """ TL;DR - this is to implement functionality similar to both of `sshd_config:AllowUsers` and of `PAM's own rhost verification`.
This was asked in IRC and [mailing list](https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o...) (with little follow up in both). The reasoning behind implementation can be seen in linked mailing list thread.
Current PR provides basic functionality of comparing rhost (from pam) with values stored in LDAP. To enable this set `ldap_access_order = rhost` and `ldap_user_authorized_rhost = <ldap_field_name| default: rhost>` in sssd.conf.
It _currently*_ provides similar rule evaluation as currently it works for host based authentication.
TODO: - [ ] Finalize logic of using DNS/rDNS for rules validation (currently working on basic idea how it should work - any help here?) - [ ] Implement use of DNS/rDNS (with optional switch to enable/disable) - [ ] Documentation - [ ] Test coverage (didn't see test coverage for host auth, so is it needed?) """
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
sumit-bose commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-301709091
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: edited
Changed field: body Original value: """ TL;DR - this is to implement functionality similar to both of `sshd_config:AllowUsers` and of `PAM's own rhost verification`.
This was asked in IRC and [mailing list](https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o...) (with little follow up in both). The reasoning behind implementation can be seen in linked mailing list thread.
Current PR provides basic functionality of comparing rhost (from pam) with values stored in LDAP. To enable this set `ldap_access_order = rhost` and `ldap_user_authorized_rhost = <ldap_field_name| default: rhost>` in sssd.conf.
It _currently*_ provides similar rule evaluation as currently it works for host based authentication.
TODO: - [ ] Finalize logic of using DNS/rDNS for rules validation (currently working on basic idea how it should work - any help here?) - [ ] Implement use of DNS/rDNS (with optional switch to enable/disable) - [ ] Documentation - [ ] Test coverage (didn't see test coverage for host auth, so is it needed?)
* It is entirely possible that logic might slightly change, but mostly I imagine it staying the same. """
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ Updated the code to fix broken build. With this change it would use authorized_host field for rhost verification.
Meanwhile I did not find how can I add extra LDAP field to be retrieved (with default value) that would be outside of RFC scopes? Adding them directly into RFC scope breaks tests and I do not feel like adding field to AD and other providers since don't think this feature is applicable there.
"""
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-312611042
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
lslebodn commented: """
Meanwhile I did not find how can I add extra LDAP field to be retrieved (with default value) that would be outside of RFC scopes? Adding them directly into RFC scope breaks tests and I do not feel like adding field to AD and other providers since don't think this feature is applicable there.
You can check following commit 3cf7fdfcaedb986f42a6640e26aa057007b64045. It added a new optiona and default value was set only for IPA provider.
"""
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-312612850
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ @lslebodn thanks for the pointer! Then the important question is it acceptable to add this new custom field that is outside of scope of RFCs to the RFC scoped blocks and to providers other than LDAP? If yes, then sure I would rather do that than share field with another verification mechanism. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-312614193
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
lslebodn commented: """ Probably, I still do not understand the problem. What do you mean by "to add new custom field"? Because most of attributes in `src/providers/ipa/ipa_opts.c` or in `src/providers/ad/ad_opts.c` are not part of standards rfc2307/rfc2307bis. Therefore they have default LDAP attribute set to NULL(`def_name` in `struct sdap_attr_map`) for these providers (arrays `rfc2307_group_map`, `rfc2307bis_group_map`)
If previous explanation does not help could you a little bit elaborate? """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-312617227
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ Ah, that's fine then. My understanding was that all those fields are part of corresponding RFCs. If most of them aren't, then I guess no problem to add one more field. I will update the code for this later using the commit referenced above.
Thanks! """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-312618661
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ Implemented simple rhost verification (without DNS resolution yet), tested works as expected and checks passed. Would be nice to have a review ;) """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-312708239
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ For the DNS/rDNS verification I am considering to implement following (bearing in mind [RFC 1912](https://tools.ietf.org/html/rfc1912)):
0. Documentation must **explicitly** state that use of DNS/rDNS is going to introduce delays and should be used with caution and recommend to set `UseDNS no` in `sshd_conf` to avoid problems with not matching rDNS. 1. If PAM provides IP address (IPv4 or IPv6) as rhost, then use it directly, if PAM provides hostname, resolve it to IP address (IPv4 and/or IPv6) using forward resolution (as per RFC 1912 recommendation for FCrDNS) and use this IP address directly. 2. Relevant LDAP records must be prepended with record type identifier in a manner `[!]identifier:record`. Allowed identifiers are `ip4|ip6|host`. For example record `host:host1.example.com` to allow access from host with DNS record host1.example.com and `!ipv4:192.0.0.1` to deny access from rhost with IPv4 address 192.0.0.1. This is to spare some time on figuring out wether record is valid IPv4/IPv6 or is it a hostname. 3. Additional configuration option `ldap_authorized_rhost_use_dns = <bool> (Default: False)`. This option would enable/disable use of DNS/rDNS in verification process. 1. If disabled whatever is received from LDAP record is matched as-is to whatever received from PAM as users rhost (without resolution mentioned in point 1 and ignoring identifier from point 2). 2. When enabled the following logic would be applied: 1. If LDAP record is IPv4 or IPv6 address, match against rhost (IPv4 or IPv6). 2. If LDAP record is a hostname, then perform forward resolution of that hostname to IP address (v4 and/or v6), then match resulting addresses against rhost. If both v4 and v6 IP addresses are available in rhost (after resolution in point 1), then each one must match (i.e. strict matching)
Please let me know if that is good, or any adjustments to this (e.g. throw away point 2 and attempt to check type of record inside SSSD)?
I will hold on with implementation until any feedback on these. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-315590636
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ Hi, can anyone, please provide feedback on these? I am hoping to have this reviewed/accepted soon since I have to use unreliable workarounds to achieve the same functionality... """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-316086309
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ How would I retrieve already parsed options from config file without calling `ldap_get_options`?
I searched through the code, but it does not seem to be stored anywhere (or I could not find it) and I don't think calling `ldap_get_options` is a good option in the function that will be repeated rather frequently...
What I want is from [here](https://github.com/SSSD/sssd/pull/275/commits/0f312f6b91406b8725677621570aa5...) to see whether certain parameter was set to "yes|no", however all close callee functions also have no access to this pre-parsed list.
I guess I am missing something since I would expect this to be possible. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-317354040
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ I have seen next release plans email which mentions test coverage (which I strongly agree with), if someone could point me to the implementation of tests that is considered good I will work on this ASAP.
Getting this into the sssd is currently on my high prio list as I am not willing to maintain my own build of sssd for a long term (and that is what I currently have do to use my patch on 1.5k+ managed hosts) """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-332076566
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
pbrezina commented: """ Hi Alexey, I am sorry that it took so long for us to reply. We are grateful for your contributions, but you picked a wrong time for it since we were quite busy with finishing our tasks. I will try to look into your patches this week.
It would be great if you could also work on unit and integration test for the new functionality. You can look at `src/tests/cmocka/test_simple_access.c`. I don't think we have any integration test for access functionality, but you can inspire with other tests at `src/tests/intg`. Thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-332164472
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ @pbrezina Thank you for the pointers, I see there is a generic `src/tests/cmoka/test_sdap_access.c`, which currently only has unit test (singular) for `nds_check_expired`, I will add rhosts functionality tests there, unless someone from maintainers team tells otherwise :) """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-332402061
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
pbrezina commented: """ I have no problem with putting it into sdap_access tests. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-332514433
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
pbrezina commented: """ Code-wise ack. I still need to test it, but I will do that probably in the second half of October since I'll be on PTO. Do you also plan on working on the hostname resolution? """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-332521646
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ I am thinking if can first merge what it is now and then add hostname resolution in the next release. I already have some work for resolution, but not nearly good enough yet.
Meanwhile I will try to finish unit tests ASAP.
On 27 Sep 2017, at 21:32, Pavel Březina notifications@github.com wrote:
Code-wise ack. I still need to test it, but I will do that probably in the second half of October since I'll be on PTO. Do you also plan on working on the hostname resolution?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.
"""
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-332531021
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ Added unit tests for the functionality (also squashed all commits to one). """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-333123408
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ Note that I've sent 2 more PRs that backport these changes to 1.14 and 1.13. Hoping that this all could make its way into 1.16. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337193148
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
jhrozek commented: """ @akamensky I've pushed some stylistic amendments to my github branch at https://github.com/jhrozek/sssd/tree/review Mostly it's about line length and checking all allocation results in tests against NULL. If you agree, can you squash the patches prefixed with "SQ" into yours and resubmit?
I'm also running a Coverity scan and if that finishes clean, I'll ack (otherwise I'll provide fixup patches because the Coverity instance is hosted internally at Red Hat).
And I have two more requests -- could you add an explicit statement to the manpage stating that the `rhost` field is set by the application and the admin must check what the application sends before enabling this test?
Finally, could you change the first line of the commit message to make it clearer what the PR is about? Just something like "LDAP: Add support for rhost access control would do".
Thank you very much for your patience and persistence. I'm really impressed how you tackled everything including unit tests. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337510450
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
jhrozek commented: """ OK, coverity report came back clean,so I don't have any more comments """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337516258
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ @jhrozek I've added the changes you requested (style patches + man + commit name). Will update backport PRs shortly. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337539562
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
akamensky commented: """ @jhrozek I've added the changes you requested (style patches + man + commit name). Will update backport PRs shortly. Thank you for those suggestions. Let me know if there is anything else I can improve in these changes. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337539562
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
jhrozek commented: """ I'm fine now and adding the Accepted label. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337829496
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
jhrozek commented: """ Actually, I'll add the label when the internal CI (which, unlike the PR CI tests on multiple OSs) finishes """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337829689
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
jhrozek commented: """ CI: http://vm-058-233.$ABC/logs/job/79/82/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337850776
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
pbrezina commented: """ Code-wise ack. I didn't test those patches myself, but I believe Jakub did so I will move on. """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337851772
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
lslebodn commented: """ master: * f34a8330c1615511795847b0a1454249d782db2a """
See the full comment at https://github.com/SSSD/sssd/pull/275#issuecomment-337919298
URL: https://github.com/SSSD/sssd/pull/275 Author: akamensky Title: #275: Implement access verification by rhost using ldap_access_order rhost option Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/275/head:pr275 git checkout pr275
URL: https://github.com/SSSD/sssd/pull/275 Title: #275: Implement access verification by rhost using ldap_access_order rhost option
Label: +Pushed
sssd-devel@lists.fedorahosted.org