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

Changes to clean up bash interactions and clarify some parts #87

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

toddobryan
Copy link

@toddobryan toddobryan commented Nov 25, 2024

that confused me. See #86

@s-makin s-makin added the review: technical Review technical accuracy, completeness, up-to-dateness label Nov 25, 2024
Copy link
Collaborator

@s-makin s-makin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to submit this PR, this is a much needed change. Just a couple of small nits, and then I'll be happy to merge :)

```

NOTE: Originally included `dn: cn=admin,dc=example,dc=com` but this is not output by the command.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line can be deleted - if cn=admin isn't included in the output then the original output was incorrect (no need to keep it in a note)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the cn=admin,dc=example,dc=com entry is actually in the config database. I'm not sure if that's a change or if it's always been like that.

Copy link
Contributor

@panlinux panlinux Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this has changed between ubuntu releases. In focal and earlier, the dn: cn=admin,dc=example,dc=com entry would be created, but starting with jammy, it's not, and only exists in the cn=config suffix. I didn't check interim releases, just the LTSs.

Specifically, this is when it changed:

openldap (2.4.51+dfsg-1) unstable; urgency=medium
...
  * Remove the redundant cn=admin,<suffix> entry from the default DIT for new
    installs. For new installs going forward, the root credentials will be
    stored in olcRootDN/olcRootPW only. (Closes: #821331)
...
 -- Ryan Tandy <[email protected]>  Sun, 23 Aug 2020 11:09:57 -0700

The linked bug is http://bugs.debian.org/821331

I suggest to default the document to the behavior in the latest LTS (Noble as of now), and add a remark that in focal and earlier an additional cn=admin,dc=example,dc=com entry would have also been created.


```console
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind changing all the console code blocks to text please? This will avoid italics in the rendered docs (which are harder to read)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

Copy link
Contributor

@cpaelzer cpaelzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @toddobryan loving your contribution to make this more useful.
I'm reviewing this together with @slyon right now, he might have more in a bit.
But I've found some small element that could make it better to be consistent in the new way you are going for (I hope I didn't overlook a reason for this to be intentional).

I've left in-line comments at each - but essentially I'm asking to also resolve the last three remaining leading $ in front of the commands as well - so that all are in the same style.

```

Finally, run the `ldapmodify` command:

```bash
$ sudo ldapmodify -Q -Y EXTERNAL -H ldapi:/// -f changerootpw.ldif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While fixing the commands to be able to be simple single lines for copy and paste - could we also fix the three remaining ones with leading $? This is one of them.

@@ -196,7 +220,9 @@ We can check that the information has been correctly added with the `ldapsearch`

```bash
$ ldapsearch -x -LLL -b dc=example,dc=com '(uid=john)' cn gidNumber
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While fixing the commands to be able to be simple single lines for copy and paste - could we also fix the three remaining ones with leading $? This is one of them.

@@ -208,18 +234,29 @@ Here we used an LDAP "filter": `(uid=john)`. LDAP filters are very flexible and
(&(objectClass=posixGroup)(memberUid=john))
```

NOTE: Replacing `(uid=john)` with this doesn't actually return anything. A filter that the user could try would be pretty helpful.

That is a logical "AND" between two attributes. Filters are very important in LDAP and mastering their syntax is extremely helpful. They are used for simple queries like this, but can also select what content is to be replicated to a secondary server, or even in complex ACLs. The full specification is defined in [RFC 4515](http://www.rfc-editor.org/rfc/rfc4515.txt).

Notice we set the `userPassword` field for the "john" entry to the cryptic value `{CRYPT}x`. This essentially is an invalid password, because no hashing will produce just `x`. It's a common pattern when adding a user entry without a default password. To change the password to something valid, you can now use `ldappasswd`:

```bash
$ ldappasswd -x -D cn=admin,dc=example,dc=com -W -S uid=john,ou=people,dc=example,dc=com
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While fixing the commands to be able to be simple single lines for copy and paste - could we also fix the three remaining ones with leading $? This is one of them.

@@ -208,18 +234,29 @@ Here we used an LDAP "filter": `(uid=john)`. LDAP filters are very flexible and
(&(objectClass=posixGroup)(memberUid=john))
```

NOTE: Replacing `(uid=john)` with this doesn't actually return anything. A filter that the user could try would be pretty helpful.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the &(objectClass=posixGroup)(memberUid=john)) doesn't seem to produce any results. I was unable to find another working filter, without any LDAP experience on my end.

@toddobryan Do you have a working example, by chance, or maybe @sergiodj or @panlinux can help with that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toddobryan please be aware that #103 seems to outline how to fix this.
So you could integrate that into your PR making it not only improve things in general, but also fix this issue with the search returning nothing.

Copy link
Collaborator

@s-makin s-makin Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toddobryan This part was solved in #108 so you will need to rebase your changes to incorporate this fix :)

@s-makin
Copy link
Collaborator

s-makin commented Jan 21, 2025

Hi @toddobryan, I just wanted to check in and see how things are going :) we're really keen to land these changes since they are a much-needed improvement to the existing docs. To that end, please let me know if there's any help we can provide in incorporating the parallel changes that were merged recently (in PR #108).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review: technical Review technical accuracy, completeness, up-to-dateness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants