3:24 a.m.
On 07/25/2012 07:56 PM, Nick Guay wrote:
I mostly agree with Jakub's changes. My additions are noted
inline.
On 25/07/12 11:25, Jakub Hrozek wrote:
> On Wed, Jul 25, 2012 at 12:56:37PM +0200, Pavel Březina wrote:
>> On 07/25/2012 11:30 AM, Pavel Březina wrote:
>>> https://fedorahosted.org/sssd/ticket/1418
>>>
>>> Does it make sense? Would you add anything?
>> Self nack. I didn't update po4a.cfg to mark it for translation.
> The manpage is mostly good and informative. I would like it to be
> reviewed by an English native speaker (that would be Nick this week,
> probably) before submitting it upstream.
>
> Have you tested if the IPA example works? I know the sudoers compat tree
> is not readable anonymously, but I haven't tested if the host
> credentials work.
>
> What about just stating that in general you need to add the sudo service
> and specify the search base with ldap_sudo_search_base and including the
> examples at the bottom in the EXAMPLES section.
>
> See the grammar comments inline.
>
>> + <refsect1 id='sudo'>
>> + <title>Configuring sudo to cooperate with SSSD</title>
>> + <para>
>> + To enable SSSD as a source for sudo rules, add
>> + <emphasis>sss</emphasis> to
<emphasis>sudoers</emphasis>
>> entry in
> "..add sss to *the* sudoers entry.."
>
>> + <citerefentry>
>> + <refentrytitle>nsswitch.conf</refentrytitle>
>> + <manvolnum>5</manvolnum>
>> + </citerefentry>.
>> + </para>
>> + <para>
>> + For example, to configure sudo to first lookup rules in
>> the standard
>> + <citerefentry>
>> + <refentrytitle>sudoers</refentrytitle>
>> + <manvolnum>5</manvolnum>
>> + </citerefentry> file (which should contain rules that
>> apply to
>> + local users) and then in SSSD, the nsswitch.conf file
>> should contain
>> + the following line:
>> + </para>
>> + <para>
>> +<programlisting>
>> +sudoers: files sss
>> +</programlisting>
>> + </para>
>> + <para>
>> + More information about sudoers nsswitch.conf format as
>> well as
> What about "More information about configuring the sudoers search order
> from the nsswitch.conf file"
>
>> + information about an LDAP schema that is used to store
>> sudo rules in
> ..the LDAP schema..
>
>> + the directory can be found in
>> + <citerefentry>
>> + <refentrytitle>sudoers.ldap</refentrytitle>
>> + <manvolnum>5</manvolnum>
>> + </citerefentry>.
My take: "Please refer to sudoers.ldap(5) for information about the
sudoers source order in nsswitch.conf and an example LDAP schema for
storing sudo rules."
Jakub's version sounds better to me.
>> + </para>
>> + </refsect1>
>> +
>> + <refsect1 id='sssd'>
>> + <title>Configuring SSSD to fetch sudo rules</title>
>> + <para>
>> + The following example shows how to configure SSSD to
>> download sudo
>> + rules from an LDAP server.
>> + </para>
>> + <para>
>> +<programlisting>
>> +[sssd]
>> +config_file_version = 2
>> +services = nss, pam, sudo
>> +domains = EXAMPLE
>> +
>> +[domain/EXAMPLE]
>> +id_provider = ldap
>> +sudo_provider = ldap
>> +ldap_uri = ldap://example.com
>> +ldap_sudo_search_base = ou=sudoers,dc=example,dc=com
>> +</programlisting>
>> + </para>
>> + <para>
>> + Below given is shown how to configure SSSD to download sudo
>> + rules from an IPA server. Because SSSD does not have native
> This sentence doesn't sound right to me. What about "The following
> example illustrates setting up the SSSD to download sudo rules from an
> IPA server" ?
Agreed, but remove "the" from "the SSSD" or just change it to a
similar
format like the previous "The following example...".
>> + support of IPA provider for sudo yet, it is necessary to
>> use LDAP
> ...the LDAP provider..
>
>> + provider and set appropriate connection parameters.
"Because SSSD does not have..." -> "It is necessary to use the LDAP
sudo
provider, because SSSD does not have a native IPA sudo provider."
>> + </para>
>> + <para>
>> +<programlisting>
>> +[sssd]
>> +config_file_version = 2
>> +services = nss, pam, sudo
>> +domains = EXAMPLE
>> +
>> +[domain/EXAMPLE]
>> +id_provider = ipa
>> +ipa_domain = example.com
>> +ipa_server = ipa.example.com
>> +ldap_tls_cacert = /etc/ipa/ca.crt
>> +
>> +sudo_provider = ldap
>> +ldap_uri = ldap://ipa.example.com
>> +ldap_sudo_search_base = ou=sudoers,dc=example,dc=com
>> +ldap_sasl_mech = GSSAPI
>> +ldap_sasl_authid = host/hostname.example.com
>> +ldap_sasl_realm = EXAMPLE.COM
>> +krb5_server = ipa.example.com
>> +</programlisting>
>> + </para>
>> + </refsect1>
>> +
>> + <refsect1 id='cache'>
>> + <title>How does SSSD cache sudo rules</title>
> What about simply "The SUDO rule caching mechanism" ?
>
>> + <para>
>> + The biggest challenge, when developing sudo support in
>> SSSD, was to
>> + ensure that running sudo with SSSD as the data source
>> gives the
> ...provides the same user experience...
>
>> + same user experience and is as fast as sudo but keeps
>> providing
> Hey, shouldn't the sssd be faster than sudo? :-)
Faster than sudo+ldap yes. But I doubt it is faster than sudo+files. :)
>
>> + the most current set of rules as possible. To accomplish
>> these
>> + points, SSSD use three kind of updates. We call them
>> full refresh,
> ..satisfy these requirements.. ?
>
>> + smart refresh and rules refresh.
"To satisfy these requirments, SSSD uses three kinds of updates. They
are referred to as full refresh, smart refresh and rules refresh."
>> + </para>
>> + <para>
>> + The <emphasis>smart refresh</emphasis> periodically
>> downloads rules
>> + that are new or were modified after the last update. Its
>> primary
>> + goal is to keep the database growing by fetching only small
>> + increments that does not overload bandwith.
> "do" instead of "does". Overload doesn't sound right either,
maybe
> consume? Or generate network traffic? Generate high LDAP server load?
>
>> + </para>
>> + <para>
>> + The <emphasis>full refresh</emphasis> simply deletes
>> everything
> ...deletes all sudo rules stored in cache..
>
> We don't want to scare the admins by telling them we delete the cached
> credentials :-)
>
>> + that is in the cache and replaces it with all rules that
>> are stored
>> + on the server. This is used to keep the cache consistent
>> by removing
>> + every rule which was deleted from the server. Hovewer,
>> it may
> ..full refresh instead of "it" and "this" ?
>
>> + produce a lot of traffic (depending on how many rules
>> exist) and
>> + it should be run only few times a day.
> Only occasionally depending on the size of the environment?
>
>> + </para>
>> + <para>
>> + At least one of those two refresh types has to be
>> enabled to be run
>> + periodically.
> Sorry, I don't understand this sentence.
Maybe "Either smart refresh or full refresh must be enabled for the
cache to be periodically updated."
I decided to remove this sentence completely. Disabling any periodical
update is not recommended anyway.
>
>> + </para>
>> + <para>
>> + The <emphasis>rules refresh</emphasis> ensures that we
>> do not give
> grant sounds better to me than give
>
>> + the user more permission that defined. It is triggered
>> each time the
>> + user runs sudo. It will find all rules that apply to
>> this user,
"that defined" -> "than defined"
> The repeating "It" doesn't sound right, can you use "rules
refresh" or
> something like "this refresh type" ?
>
>> + check their expiration time and redownload them if
>> expired. In the
>> + case that any of these rule is missing on the server,
>> the SSSD will
> ..any of these rules are.. ?
>
>> + do an out of band full refresh because it is very
>> probable that more
>> + rules (that apply to other users) were deleted as well.
"beacuse it is very probable..." -> "because more rules (...) may have
been deleted."
>> + </para>
>> + <para>
>> + If enabled, SSSD will store only rules that can be
>> applied to this
>> + machine. This means rules that contain one of the
>> following in
> one of the following values
>
>> + <emphasis>sudoHost</emphasis> attribute:
> Have you considered using docbook's <itemizedlist> for the list of
> possible values?
>
>> + </para>
>> + <para>
>> + * keyword ALL
>> + </para>
>> + <para>
>> + * regullar expression
> I think that regular is spelled with a single l.
>
>> + </para>
>> + <para>
>> + * netgroup (in the form "+netgroup")
>> + </para>
>> + <para>
>> + * hostname or fully qualified domain name of this machine
>> + </para>
>> + <para>
>> + * one of the IP addresses of this machine
>> + </para>
>> + <para>
>> + * one of the IP addresses of the network
>> + (in the form "address/mask")
>> + </para>
>> + <para>
>> + There are many configuration options that can be used to
>> adjust
>> + the behaviour. Take a look at "ldap_sudo_*" in
> Please refer to instead of take a look?
>
>> + <citerefentry>
>> + <refentrytitle>sssd-ldap</refentrytitle>
>> + <manvolnum>5</manvolnum>
>> + </citerefentry> and "sudo_*" in
>> + <citerefentry>
>> + <refentrytitle>sssd.conf</refentrytitle>
>> + <manvolnum>5</manvolnum>
>> + </citerefentry>.
>> + </para>
>> + </refsect1>
>> +
>> + <xi:include xmlns:xi="http://www.w3.org/2001/XInclude"
>> href="include/seealso.xml" />
>> +
>> +</refentry>
>> +</reference>
Thanks for the review. New patch is attached.