Skip to content

Conversation

@domiborges
Copy link
Contributor

Hi! @ikerexxe suggested I could use the opportunity of #1238 to review and update these man pages. I did an initial review so we can start a discussion.

I'm marking the PR as a draft as it depends on #1238

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Mar 23, 2025

I have applied your first two patches into my PR. I've signed them on your behalf (with some minor tweaks too). Thanks!

The rest still need some review.

Copy link
Collaborator

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

Some minor comments inline

@domiborges
Copy link
Contributor Author

Hi! Sorry for the pause. There were some high priority items I had to address.
In this latest update, I hopefully addressed the existing comments and suggestions.

@domiborges domiborges changed the title WIP:Update man pages for chage, shadow, passwd Update man pages for chage, shadow, passwd May 27, 2025
Copy link
Collaborator

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

It looks good on my end, but we'll need to fix the git history before merging. Thank you for this work Dominika!

By the way, CI error is unrelated.

@hallyn
Copy link
Member

hallyn commented May 30, 2025

@ikerexxe are you going to fix the git history?

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented May 31, 2025

I would like to review myself too. Please don't merge for now.

Comment on lines -100 to +103
<command>passwd</command> will reject any password which is not
suitably complex. Care must be taken not to include the system
default erase or kill characters.
<command>passwd</command> rejects passwords that do not meet
the complexity requirements.
Do not include the system default erase or kill characters.
Copy link
Collaborator

@alejandro-colomar alejandro-colomar Jun 6, 2025

Choose a reason for hiding this comment

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

[Not a question about the documentation change, but about the old text (and the program behavior)].

@ikerexxe , @hallyn is this a warning to not introduce bad characters in passwords? Should we add checks like we have for the user name? Care must be taken sounds like it will break something in /etc/passwd, which sounds like we should check.

Comment on lines -125 to +126
The minimum password age is the number of days the user will
have to wait before she will be allowed to change her password
again.
The minimum password age is the number of days the user must wait
before they can change their password again.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ikerexxe , @hallyn

What's the purpose of this feature? It sounds like a security issue: if a password is compromised, and the minimum age doesn't allow changing it, it's a problem.

Should we consider deprecation of this field?

Comment on lines 138 to +143
The maximum password age is the number of days after which the
user will have to change her password.
user must change their password.
</para>
<para>
After this number of days is elapsed, the password may still
be valid. The user should be asked to change her password the
next time she will log in.
After this number of days has elapsed, the password may still be valid.
The user is prompted to change their password at the next login.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hallyn , @ikerexxe

I will propose deprecation of this field eventually. Security guidelines these days say that expiring passwords results in worse passwords.

Copy link
Collaborator

@alejandro-colomar alejandro-colomar left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Reviewed-by: Alejandro Colomar <alx@kernel.org>

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jun 6, 2025

@ikerexxe are you going to fix the git history?

@hallyn , @ikerexxe , This seems to be the only concern still open. Feel free to merge when you're okay with the changes. I'm fine with them.

@ikerexxe
Copy link
Collaborator

ikerexxe commented Jun 8, 2025

I'm ok with the documentation but I think we need to fix the git history a bit before merging. Let's see if I can meet Dominika next week and fix that.

In any case, this shouldn't block any release.

@alejandro-colomar
Copy link
Collaborator

I'm ok with the documentation but I think we need to fix the git history a bit before merging. Let's see if I can meet Dominika next week and fix that.

In any case, this shouldn't block any release.

Yeah, we can merge this after the RC; it's just docs.

Copy link
Collaborator

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

I misunderstood some of the changes and I thought we needed to rewrite a little bit the git history before merging, but this is good as it is. Thank you for the patches! Merging them now.

@ikerexxe ikerexxe merged commit fe49b67 into shadow-maint:master Jun 9, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants