Skip to content
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

Implement access verification by rhost using ldap_access_order rhost option #275

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@akamensky
Copy link
Contributor

commented May 16, 2017

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

@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2017

Can one of the admins verify this patch?

1 similar comment
@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2017

Can one of the admins verify this patch?

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

ok to test

@akamensky akamensky force-pushed the akamensky:master branch from 196ba33 to 13577af Jul 3, 2017

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

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.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2017

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 3cf7fdf.
It added a new optiona and default value was set only for IPA provider.

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

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

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2017

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?

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

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!

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

Implemented simple rhost verification (without DNS resolution yet), tested works as expected and checks passed. Would be nice to have a review ;)

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2017

For the DNS/rDNS verification I am considering to implement following (bearing in mind RFC 1912):

  1. 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.
  2. 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.
  3. 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.
  4. 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.

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

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

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2017

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

@akamensky akamensky force-pushed the akamensky:master branch from a850cbb to fe128e7 Sep 6, 2017

@akamensky akamensky force-pushed the akamensky:master branch from fe128e7 to dfad3dd Sep 22, 2017

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2017

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)

@pbrezina

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

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.

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2017

@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 :)

@pbrezina

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

I have no problem with putting it into sdap_access tests.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

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?

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2017

@akamensky akamensky force-pushed the akamensky:master branch 2 times, most recently from 73571ec to a208783 Sep 29, 2017

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

Added unit tests for the functionality (also squashed all commits to one).

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2017

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.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

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

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

OK, coverity report came back clean,so I don't have any more comments

@akamensky akamensky force-pushed the akamensky:master branch from a208783 to 38930e0 Oct 18, 2017

@akamensky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

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

LDAP: Add support for rhost access control
This patch implements verification of pam_rhost against
rules stored in LDAP entry of a user.

@akamensky akamensky force-pushed the akamensky:master branch from 38930e0 to eb2dec9 Oct 18, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

I'm fine now and adding the Accepted label.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

Actually, I'll add the label when the internal CI (which, unlike the PR CI tests on multiple OSs) finishes

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

@jhrozek jhrozek added the Accepted label Oct 19, 2017

@pbrezina

This comment has been minimized.

Copy link
Member

commented Oct 19, 2017

Code-wise ack. I didn't test those patches myself, but I believe Jakub did so I will move on.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

master:

@lslebodn lslebodn closed this Oct 19, 2017

@lslebodn lslebodn added the Pushed label Oct 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.