search

shadow-maint / shadow

Upstream shadow tree
Other
287 stars 227 forks source link

Remove support for rlogind in login(1), that is, remove the '-r' flag #998

Closed alejandro-colomar closed 4 weeks ago

alejandro-colomar commented 1 month ago

The "quick hack" finally disappeared. Probably nobody noticed. ;) (See the changes in for the context of this pun.)

Probably everybody uses SSH these days for remote login. Let's remove this insecure functionality.

Closes: https://github.com/shadow-maint/shadow/issues/992 Cc: @hallyn Cc: @ikerexxe Cc: @jubalh Cc: @thesamesam Cc: @dataCobra Cc: @dkwo

Revisions:

v1b - Rebase ``` $ git range-diff master..gh/rlogin shadow/master..rlogin 1: 1dab33d9 = 1: f0306cbf Remove support for rlogind in login(1), that is, remove the '-r' flag ```
v1c - Rebase ``` $ git range-diff master..gh/rlogin shadow/master..rlogin 1: f0306cbf ! 1: 2aad6592 Remove support for rlogind in login(1), that is, remove the '-r' flag @@ src/login.c: int main (int argc, char **argv) } -#ifdef RLOGIN - if (rflg) { -- size_t max_size = sysconf(_SC_LOGIN_NAME_MAX); +- size_t max_size; - +- max_size = login_name_max_size(); - assert (NULL == username); - username = XMALLOC(max_size, char); - username[max_size - 1] = '\0'; ```
v1d - Reviewed by @dkwo ``` $ git range-diff shadow/master gh/rlogin rlogin 1: 2aad6592 ! 1: 65265b0c Remove support for rlogind in login(1), that is, remove the '-r' flag @@ Commit message this insecure method. Closes: + Reviewed-by: dkwo Cc: "Serge E. Hallyn" Cc: Iker Pedrosa Cc: Michael Vetter Cc: Sam James Cc: Benedikt Brinkmann - Cc: dkwo Signed-off-by: Alejandro Colomar ## configure.ac ## ```
v1e - Rebase ``` $ git range-diff master..gh/rlogin shadow/master..rlogin 1: 65265b0c = 1: 14d98ddc Remove support for rlogind in login(1), that is, remove the '-r' flag ```
v1f - Rebase - Reviewed-by @ikerexxe ``` $ git range-diff master..gh/rlogin shadow/master..rlogin 1: 14d98ddc ! 1: 7c6b5d6c Remove support for rlogind in login(1), that is, remove the '-r' flag @@ Commit message Closes: Reviewed-by: dkwo + Reviewed-by: Iker Pedrosa Cc: "Serge E. Hallyn" - Cc: Iker Pedrosa Cc: Michael Vetter Cc: Sam James Cc: Benedikt Brinkmann ```
dkwo commented 1 month ago

Looks good to me, thanks!

alejandro-colomar commented 1 month ago

Looks good to me, thanks!

May I take that as a Reviewed-by: ...?

Thank you for checking!

dkwo commented 1 month ago

Yes.

alejandro-colomar commented 1 month ago

I wonder if we could remove PW_RLOGIN and this line of code with it. reason is already set to PW_TELNET previously, and that makes more sense.

I have a patch for removing telnet support (thus, removing -h) for when this one is merged. That one removes those two lines (and much more; even the reason variable is removed).

ikerexxe commented 1 month ago

I have a patch for removing telnet support (thus, removing -h) for when this one is merged. That one removes those two lines (and much more; even the reason variable is removed).

Good!

I think these changes are fine, but I'd like @hallyn to review whether we can remove this functionality.

alejandro-colomar commented 1 month ago

Hi Iker, Serge,

On Tue, Jun 11, 2024 at 07:57:17AM GMT, Iker Pedrosa wrote:

I have a patch for removing telnet support (thus, removing -h) for when this one is merged. That one removes those two lines (and much more; even the reason variable is removed).

Good!

I think these changes are fine, but I'd like @hallyn to review whether we can remove this functionality.

Thanks! Serge, please have a look at this.

Have a lovely day! Alex

-- Reply to this email directly or view it on GitHub: https://github.com/shadow-maint/shadow/pull/998#issuecomment-2160979684 You are receiving this because you authored the thread.

Message ID: @.***>

-- https://www.alejandro-colomar.es/

hallyn commented 1 month ago

Well, some people really do have completely isolated networks where rlogin and telnet are not considered a risk.

I can't think of a better way of finding out other than removing this code, though. If we hear that people are losing functionality, then I guess we can revert it...

alejandro-colomar commented 1 month ago

@iker, @hallyn, please don't merge yet.

alejandro-colomar commented 1 month ago

Hi Serge,

On Tue, Jun 11, 2024 at 08:12:27AM GMT, Serge Hallyn wrote:

Well, some people really do have completely isolated networks where rlogin and telnet are not considered a risk.

I can't think of a better way of finding out other than removing this code, though. If we hear that people are losing functionality, then I guess we can revert it...

Thanks!

Would you mind checking https://github.com/shadow-maint/shadow/issues/1010? I'd like you to release 4.15.2 from master. Since removing rlogin support would be a breaking change, I'd prefer merging this PR after you release that. Currently, there are no breaking changes in master since 4.15.1.

Have a lovely day! Alex

-- Reply to this email directly or view it on GitHub: https://github.com/shadow-maint/shadow/pull/998#issuecomment-2161014838 You are receiving this because you authored the thread.

Message ID: @.***>

-- https://www.alejandro-colomar.es/

alejandro-colomar commented 4 weeks ago

Since we have breaking changes in master already, there's no point in delaying this for after 4.16.0. @hallyn, I'll let you merge, since you're preparing the release.