[Buildroot] [PATCHv5] system: allow/disallow root login, accept encoded passwords

[Buildroot] [PATCHv5] system: allow/disallow root login, accept encoded passwords

[Yann E. MORIN]

From: Lorenzo Catucci <lorenzo@sancho.ccd.uniroma2.it>

Currently, there is only two possibilities regarding the root account:
  - it is enabled with no password (the default)
  - it is enabled, using a clear-text, user-provided password

This is deemed insufficient in many cases, especially when the .config
file has to be published (e.g. for the GPL compliance, or any other
reason.).

Fix that in two ways:

  - add a bolean option that allows/disallows root login altogether,
    which defaults to 'y' to keep backward compatibility;

  - accept already-encoded passwords, which we recognise as starting
    with either of $1$, $5$ or $6$ (resp. for md5, sha256 or sha512).

Signed-off-by: Lorenzo M. Catucci <lorenzo@sancho.ccd.uniroma2.it>
[yann.morin.1998 at free.fr:
  - don't add a choice to select between clear-text/encoded password,
    use a single prompt;
  - differentiate in the password hook itself;
  - rewrite parts of the help entry;
  - rewrite and expand the commit log
]
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
Changes v4 -> v5:
  - use makefile syntax instead of shell  (Thomas)
  - typoes  (Thomas)
  - fix up the commit log (it never was possible to disable root login)
---
 system/Config.in | 28 +++++++++++++++++++---------
 system/system.mk | 22 ++++++++++++++++------
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/system/Config.in b/system/Config.in
index 431524d..6ba34ba 100644
--- a/system/Config.in
+++ b/system/Config.in
@@ -177,26 +177,36 @@ endif
 
 if BR2_ROOTFS_SKELETON_DEFAULT
 
+config BR2_TARGET_ENABLE_ROOT_LOGIN
+	bool "Enable root login"
+	default "y"
+	help
+	  Enable root login password
+
 config BR2_TARGET_GENERIC_ROOT_PASSWD
 	string "Root password"
 	default ""
+	depends on BR2_TARGET_ENABLE_ROOT_LOGIN
 	help
-	  Set the initial root password (in clear). It will be md5-encrypted.
+	  Set the initial root password.
 
 	  If set to empty (the default), then no root password will be set,
 	  and root will need no password to log in.
 
-	  WARNING! WARNING!
-	  Although pretty strong, MD5 is now an old hash function, and
-	  suffers from some weaknesses, which makes it susceptible to attacks.
-	  It is showing its age, so this root password should not be trusted
-	  to properly secure any product that can be shipped to the wide,
-	  hostile world.
+	  If the password starts with any of $1$, $5$ or $6$, it is considered
+	  to be already crypt-encoded with respectively md5, sha256 or sha512.
+	  Any other value is taken to be a clear-text value, and is crypt-encoded
+	  as per the "Passwords encoding" scheme, above.
+
+	  Note: "$" signs in the hashed password must be doubled. For example,
+	  if the hashed password is "$1$longsalt$v35DIIeMo4yUfI23yditq0", then
+	  you must enter it as "$$1$$longsalt$$v35DIIeMo4yUfI23yditq0".
 
 	  WARNING! WARNING!
-	  The password appears in clear in the .config file, and may appear
+	  The password appears as-is in the .config file, and may appear
 	  in the build log! Avoid using a valuable password if either the
-	  .config file or the build log may be distributed!
+	  .config file or the build log may be distributed, or at the
+	  very least use a strong cryptographic hash for your password!
 
 choice
 	bool "/bin/sh"
diff --git a/system/system.mk b/system/system.mk
index 4a1eb4a..b500a01 100644
--- a/system/system.mk
+++ b/system/system.mk
@@ -34,7 +34,7 @@ endef
 TARGET_FINALIZE_HOOKS += SYSTEM_ISSUE
 endif
 
-ifneq ($(TARGET_GENERIC_ROOT_PASSWD),)
+ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),y)
 TARGETS += host-mkpasswd
 endif
 
@@ -69,12 +69,22 @@ TARGET_FINALIZE_HOOKS += SET_NETWORK
 
 ifeq ($(BR2_ROOTFS_SKELETON_DEFAULT),y)
 
-define SYSTEM_ROOT_PASSWD
-	[ -n "$(TARGET_GENERIC_ROOT_PASSWD)" ] && \
-		TARGET_GENERIC_ROOT_PASSWD_HASH=$$($(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)"); \
-	$(SED) "s,^root:[^:]*:,root:$$TARGET_GENERIC_ROOT_PASSWD_HASH:," $(TARGET_DIR)/etc/shadow
+ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),y)
+ifeq ($(TARGET_GENERIC_ROOT_PASSWD),)
+SYSTEM_ROOT_PASSWORD =
+else ifneq ($(or $(filter $$1$$%,$(TARGET_GENERIC_ROOT_PASSWD)),$(filter $$5$$%,$(TARGET_GENERIC_ROOT_PASSWD)),$(filter $$6$$%,$(TARGET_GENERIC_ROOT_PASSWD))),)
+SYSTEM_ROOT_PASSWORD = $(TARGET_GENERIC_ROOT_PASSWD)
+else
+SYSTEM_ROOT_PASSWORD = $(shell $(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)")
+endif
+else # !BR2_TARGET_ENABLE_ROOT_LOGIN
+SYSTEM_ROOT_PASSWORD = *
+endif
+
+define SYSTEM_SET_ROOT_PASSWD
+	$(SED) 's,^root:[^:]*:,root:$(SYSTEM_ROOT_PASSWORD):,' $(TARGET_DIR)/etc/shadow
 endef
-TARGET_FINALIZE_HOOKS += SYSTEM_ROOT_PASSWD
+TARGET_FINALIZE_HOOKS += SYSTEM_SET_ROOT_PASSWD
 
 ifeq ($(BR2_SYSTEM_BIN_SH_NONE),y)
 define SYSTEM_BIN_SH
-- 
1.9.1

[Buildroot] [PATCHv5] system: allow/disallow root login, accept encoded passwords

[Lorenzo M. Catucci]

I've tested the patch, generating four filesystem images:

 - disabled root login:
     $ grep -E 'BR2_TARGET_ENABLE_ROOT_LOGIN|BR2_TARGET_GENERIC_ROOT_PASSWD' \
       .config
     # BR2_TARGET_ENABLE_ROOT_LOGIN is not set
     $ grep ^root  output/target/etc/shadow
     root:*:10933:0:99999:7:::

 - passwordless root login:
     $ grep -E 'BR2_TARGET_ENABLE_ROOT_LOGIN|BR2_TARGET_GENERIC_ROOT_PASSWD' \
       .config
     BR2_TARGET_ENABLE_ROOT_LOGIN=y
     BR2_TARGET_GENERIC_ROOT_PASSWD=""
     $ grep ^root  output/target/etc/shadow
     root::10933:0:99999:7:::

 - plaintext root password:
     $ grep -E 'BR2_TARGET_ENABLE_ROOT_LOGIN|BR2_TARGET_GENERIC_ROOT_PASSWD' \
       .config
       BR2_TARGET_ENABLE_ROOT_LOGIN=y
       BR2_TARGET_GENERIC_ROOT_PASSWD="$testpw"
     $ grep ^root  output/target/etc/shadow
     root:$1$kPABjeFK$vpmTV3i89ILrAZi7tRfAB.:10933:0:99999:7:::

 - md5-crypted root password:
     $ grep -E 'BR2_TARGET_ENABLE_ROOT_LOGIN|BR2_TARGET_GENERIC_ROOT_PASSWD' \
       .config
     BR2_TARGET_ENABLE_ROOT_LOGIN=y
     BR2_TARGET_GENERIC_ROOT_PASSWD="$$1$$testsalt$$AicnET0i370P1baqhE4Pm0"
     $ grep ^root  output/target/etc/shadow
     root:$1$testsalt$AicnET0i370P1baqhE4Pm0:10933:0:99999:7:::

If you like and care so, you can definitely add the:

Tested-by: "Lorenzo M. Catucci" <lorenzo@sancho.ccd.uniroma2.it>

tag and/or the

Acked-by: "Lorenzo M. Catucci" <lorenzo@sancho.ccd.uniroma2.it>

tag.


Thank you very much, yours

	lorenzo
On 10/04/2015 23:42, Yann E. MORIN wrote:
> From: Lorenzo Catucci <lorenzo@sancho.ccd.uniroma2.it>
> 
> Currently, there is only two possibilities regarding the root account:
>   - it is enabled with no password (the default)
>   - it is enabled, using a clear-text, user-provided password
> 
> This is deemed insufficient in many cases, especially when the .config
> file has to be published (e.g. for the GPL compliance, or any other
> reason.).
> 
> Fix that in two ways:
> 
>   - add a bolean option that allows/disallows root login altogether,
>     which defaults to 'y' to keep backward compatibility;
> 
>   - accept already-encoded passwords, which we recognise as starting
>     with either of $1$, $5$ or $6$ (resp. for md5, sha256 or sha512).
> 
> Signed-off-by: Lorenzo M. Catucci <lorenzo@sancho.ccd.uniroma2.it>
> [yann.morin.1998 at free.fr:
>   - don't add a choice to select between clear-text/encoded password,
>     use a single prompt;
>   - differentiate in the password hook itself;
>   - rewrite parts of the help entry;
>   - rewrite and expand the commit log
> ]
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> ---
> Changes v4 -> v5:
>   - use makefile syntax instead of shell  (Thomas)
>   - typoes  (Thomas)
>   - fix up the commit log (it never was possible to disable root login)
> ---
>  system/Config.in | 28 +++++++++++++++++++---------
>  system/system.mk | 22 ++++++++++++++++------
>  2 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/system/Config.in b/system/Config.in
> index 431524d..6ba34ba 100644
> --- a/system/Config.in
> +++ b/system/Config.in
> @@ -177,26 +177,36 @@ endif
>  
>  if BR2_ROOTFS_SKELETON_DEFAULT
>  
> +config BR2_TARGET_ENABLE_ROOT_LOGIN
> +	bool "Enable root login"
> +	default "y"
> +	help
> +	  Enable root login password
> +
>  config BR2_TARGET_GENERIC_ROOT_PASSWD
>  	string "Root password"
>  	default ""
> +	depends on BR2_TARGET_ENABLE_ROOT_LOGIN
>  	help
> -	  Set the initial root password (in clear). It will be md5-encrypted.
> +	  Set the initial root password.
>  
>  	  If set to empty (the default), then no root password will be set,
>  	  and root will need no password to log in.
>  
> -	  WARNING! WARNING!
> -	  Although pretty strong, MD5 is now an old hash function, and
> -	  suffers from some weaknesses, which makes it susceptible to attacks.
> -	  It is showing its age, so this root password should not be trusted
> -	  to properly secure any product that can be shipped to the wide,
> -	  hostile world.
> +	  If the password starts with any of $1$, $5$ or $6$, it is considered
> +	  to be already crypt-encoded with respectively md5, sha256 or sha512.
> +	  Any other value is taken to be a clear-text value, and is crypt-encoded
> +	  as per the "Passwords encoding" scheme, above.
> +
> +	  Note: "$" signs in the hashed password must be doubled. For example,
> +	  if the hashed password is "$1$longsalt$v35DIIeMo4yUfI23yditq0", then
> +	  you must enter it as "$$1$$longsalt$$v35DIIeMo4yUfI23yditq0".
>  
>  	  WARNING! WARNING!
> -	  The password appears in clear in the .config file, and may appear
> +	  The password appears as-is in the .config file, and may appear
>  	  in the build log! Avoid using a valuable password if either the
> -	  .config file or the build log may be distributed!
> +	  .config file or the build log may be distributed, or at the
> +	  very least use a strong cryptographic hash for your password!
>  
>  choice
>  	bool "/bin/sh"
> diff --git a/system/system.mk b/system/system.mk
> index 4a1eb4a..b500a01 100644
> --- a/system/system.mk
> +++ b/system/system.mk
> @@ -34,7 +34,7 @@ endef
>  TARGET_FINALIZE_HOOKS += SYSTEM_ISSUE
>  endif
>  
> -ifneq ($(TARGET_GENERIC_ROOT_PASSWD),)
> +ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),y)
>  TARGETS += host-mkpasswd
>  endif
>  
> @@ -69,12 +69,22 @@ TARGET_FINALIZE_HOOKS += SET_NETWORK
>  
>  ifeq ($(BR2_ROOTFS_SKELETON_DEFAULT),y)
>  
> -define SYSTEM_ROOT_PASSWD
> -	[ -n "$(TARGET_GENERIC_ROOT_PASSWD)" ] && \
> -		TARGET_GENERIC_ROOT_PASSWD_HASH=$$($(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)"); \
> -	$(SED) "s,^root:[^:]*:,root:$$TARGET_GENERIC_ROOT_PASSWD_HASH:," $(TARGET_DIR)/etc/shadow
> +ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),y)
> +ifeq ($(TARGET_GENERIC_ROOT_PASSWD),)
> +SYSTEM_ROOT_PASSWORD =
> +else ifneq ($(or $(filter $$1$$%,$(TARGET_GENERIC_ROOT_PASSWD)),$(filter $$5$$%,$(TARGET_GENERIC_ROOT_PASSWD)),$(filter $$6$$%,$(TARGET_GENERIC_ROOT_PASSWD))),)
> +SYSTEM_ROOT_PASSWORD = $(TARGET_GENERIC_ROOT_PASSWD)
> +else
> +SYSTEM_ROOT_PASSWORD = $(shell $(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)")
> +endif
> +else # !BR2_TARGET_ENABLE_ROOT_LOGIN
> +SYSTEM_ROOT_PASSWORD = *
> +endif
> +
> +define SYSTEM_SET_ROOT_PASSWD
> +	$(SED) 's,^root:[^:]*:,root:$(SYSTEM_ROOT_PASSWORD):,' $(TARGET_DIR)/etc/shadow
>  endef
> -TARGET_FINALIZE_HOOKS += SYSTEM_ROOT_PASSWD
> +TARGET_FINALIZE_HOOKS += SYSTEM_SET_ROOT_PASSWD
>  
>  ifeq ($(BR2_SYSTEM_BIN_SH_NONE),y)
>  define SYSTEM_BIN_SH
> 


-- 
+-------------------------+----------------------------------------------+
| Lorenzo M. Catucci      | Centro di Calcolo e Documentazione           |
| catucci at ccd.uniroma2.it | Universit? degli Studi di Roma "Tor Vergata" |
|                         | Via O. Raimondo 18 ** I-00173 ROMA ** ITALY  |
| Tel. +39 06 7259 2255   | Fax. +39 06 7259 2125                        |
+-------------------------+----------------------------------------------+

[Buildroot] [PATCHv5] system: allow/disallow root login, accept encoded passwords

[Arnout Vandecappelle]

On 10/04/15 23:42, Yann E. MORIN wrote:
> From: Lorenzo Catucci <lorenzo@sancho.ccd.uniroma2.it>
> 
> Currently, there is only two possibilities regarding the root account:
>   - it is enabled with no password (the default)
>   - it is enabled, using a clear-text, user-provided password
> 
> This is deemed insufficient in many cases, especially when the .config
> file has to be published (e.g. for the GPL compliance, or any other
> reason.).
> 
> Fix that in two ways:
> 
>   - add a bolean option that allows/disallows root login altogether,
>     which defaults to 'y' to keep backward compatibility;
> 
>   - accept already-encoded passwords, which we recognise as starting
>     with either of $1$, $5$ or $6$ (resp. for md5, sha256 or sha512).
> 
> Signed-off-by: Lorenzo M. Catucci <lorenzo@sancho.ccd.uniroma2.it>
> [yann.morin.1998 at free.fr:
>   - don't add a choice to select between clear-text/encoded password,
>     use a single prompt;
>   - differentiate in the password hook itself;
>   - rewrite parts of the help entry;
>   - rewrite and expand the commit log
> ]
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> ---
> Changes v4 -> v5:
>   - use makefile syntax instead of shell  (Thomas)
>   - typoes  (Thomas)
>   - fix up the commit log (it never was possible to disable root login)
> ---
>  system/Config.in | 28 +++++++++++++++++++---------
>  system/system.mk | 22 ++++++++++++++++------
>  2 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/system/Config.in b/system/Config.in
> index 431524d..6ba34ba 100644
> --- a/system/Config.in
> +++ b/system/Config.in
> @@ -177,26 +177,36 @@ endif
>  
>  if BR2_ROOTFS_SKELETON_DEFAULT
>  
> +config BR2_TARGET_ENABLE_ROOT_LOGIN
> +	bool "Enable root login"
> +	default "y"

 No quotes around bool values.

 However, since the default is y while it is normally n, and since we have to do
something special for the n case, wouldn't it make more sense to call it
BR2_TARGET_DISABLE_ROOT_LOGIN?

> +	help
> +	  Enable root login password
> +
>  config BR2_TARGET_GENERIC_ROOT_PASSWD
>  	string "Root password"
>  	default ""
> +	depends on BR2_TARGET_ENABLE_ROOT_LOGIN
>  	help
> -	  Set the initial root password (in clear). It will be md5-encrypted.
> +	  Set the initial root password.
>  
>  	  If set to empty (the default), then no root password will be set,
>  	  and root will need no password to log in.
>  
> -	  WARNING! WARNING!
> -	  Although pretty strong, MD5 is now an old hash function, and
> -	  suffers from some weaknesses, which makes it susceptible to attacks.
> -	  It is showing its age, so this root password should not be trusted
> -	  to properly secure any product that can be shipped to the wide,
> -	  hostile world.
> +	  If the password starts with any of $1$, $5$ or $6$, it is considered
> +	  to be already crypt-encoded with respectively md5, sha256 or sha512.
> +	  Any other value is taken to be a clear-text value, and is crypt-encoded
> +	  as per the "Passwords encoding" scheme, above.
> +
> +	  Note: "$" signs in the hashed password must be doubled. For example,
> +	  if the hashed password is "$1$longsalt$v35DIIeMo4yUfI23yditq0", then
> +	  you must enter it as "$$1$$longsalt$$v35DIIeMo4yUfI23yditq0".

 Perhaps explain why:

This is necessary because make will interpret the $ as variable expansion.

>  
>  	  WARNING! WARNING!
> -	  The password appears in clear in the .config file, and may appear
> +	  The password appears as-is in the .config file, and may appear
>  	  in the build log! Avoid using a valuable password if either the
> -	  .config file or the build log may be distributed!
> +	  .config file or the build log may be distributed, or at the
> +	  very least use a strong cryptographic hash for your password!
>  
>  choice
>  	bool "/bin/sh"
> diff --git a/system/system.mk b/system/system.mk
> index 4a1eb4a..b500a01 100644
> --- a/system/system.mk
> +++ b/system/system.mk
> @@ -34,7 +34,7 @@ endef
>  TARGET_FINALIZE_HOOKS += SYSTEM_ISSUE
>  endif
>  
> -ifneq ($(TARGET_GENERIC_ROOT_PASSWD),)
> +ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),y)
>  TARGETS += host-mkpasswd
>  endif
>  
> @@ -69,12 +69,22 @@ TARGET_FINALIZE_HOOKS += SET_NETWORK
>  
>  ifeq ($(BR2_ROOTFS_SKELETON_DEFAULT),y)
>  
> -define SYSTEM_ROOT_PASSWD
> -	[ -n "$(TARGET_GENERIC_ROOT_PASSWD)" ] && \
> -		TARGET_GENERIC_ROOT_PASSWD_HASH=$$($(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)"); \
> -	$(SED) "s,^root:[^:]*:,root:$$TARGET_GENERIC_ROOT_PASSWD_HASH:," $(TARGET_DIR)/etc/shadow
> +ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),y)
> +ifeq ($(TARGET_GENERIC_ROOT_PASSWD),)
> +SYSTEM_ROOT_PASSWORD =
> +else ifneq ($(or $(filter $$1$$%,$(TARGET_GENERIC_ROOT_PASSWD)),$(filter $$5$$%,$(TARGET_GENERIC_ROOT_PASSWD)),$(filter $$6$$%,$(TARGET_GENERIC_ROOT_PASSWD))),)

 filter allows multiple patterns, so:

else ifneq ($(filter $$1$$% $$5$$% $$6$$%,$(TARGET_GENERIC_ROOT_PASSWD)),)

> +SYSTEM_ROOT_PASSWORD = $(TARGET_GENERIC_ROOT_PASSWD)
> +else
> +SYSTEM_ROOT_PASSWORD = $(shell $(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)")
> +endif
> +else # !BR2_TARGET_ENABLE_ROOT_LOGIN
> +SYSTEM_ROOT_PASSWORD = *

 Even though Peter prefers positive logic, I think in this case it is more
important to keep the logic close to the condition, i.e.:

ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),)
SYSTEM_ROOT_PASSWORD = *
else ifeq ($(TARGET_GENERIC_ROOT_PASSWD),)
...

 Of course, if it becomes _DISABLE_ then it will be positive logic after all :-)

 Regards,
 Arnout


> +endif
> +
> +define SYSTEM_SET_ROOT_PASSWD
> +	$(SED) 's,^root:[^:]*:,root:$(SYSTEM_ROOT_PASSWORD):,' $(TARGET_DIR)/etc/shadow
>  endef
> -TARGET_FINALIZE_HOOKS += SYSTEM_ROOT_PASSWD
> +TARGET_FINALIZE_HOOKS += SYSTEM_SET_ROOT_PASSWD
>  
>  ifeq ($(BR2_SYSTEM_BIN_SH_NONE),y)
>  define SYSTEM_BIN_SH
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCHv5] system: allow/disallow root login, accept encoded passwords

[Yann E. MORIN]

Arnout, All,

On 2015-04-14 00:50 +0200, Arnout Vandecappelle spake thusly:
> On 10/04/15 23:42, Yann E. MORIN wrote:
> > From: Lorenzo Catucci <lorenzo@sancho.ccd.uniroma2.it>
[--SNIP--]
> > diff --git a/system/Config.in b/system/Config.in
> > index 431524d..6ba34ba 100644
> > --- a/system/Config.in
> > +++ b/system/Config.in
> > @@ -177,26 +177,36 @@ endif
> >  
> >  if BR2_ROOTFS_SKELETON_DEFAULT
> >  
> > +config BR2_TARGET_ENABLE_ROOT_LOGIN
> > +	bool "Enable root login"
> > +	default "y"
> 
>  No quotes around bool values.

Dang. Indeed.

>  However, since the default is y while it is normally n, and since we have to do
> something special for the n case, wouldn't it make more sense to call it
> BR2_TARGET_DISABLE_ROOT_LOGIN?

Well, I do really prefer positive naming; and we tend to use such
positive logic about everywhere...

That the internals are "more complex" (and that still has to be proven)
is irrelevant to what we present to the user. At the extreme, we could
always have a hidden variable that just negates the visible one.

[--SNIP--]
> > +	  Note: "$" signs in the hashed password must be doubled. For example,
> > +	  if the hashed password is "$1$longsalt$v35DIIeMo4yUfI23yditq0", then
> > +	  you must enter it as "$$1$$longsalt$$v35DIIeMo4yUfI23yditq0".
> 
>  Perhaps explain why:
> 
> This is necessary because make will interpret the $ as variable expansion.

OK.

[--SNIP--]
> > +ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),y)
> > +ifeq ($(TARGET_GENERIC_ROOT_PASSWD),)
> > +SYSTEM_ROOT_PASSWORD =
> > +else ifneq ($(or $(filter $$1$$%,$(TARGET_GENERIC_ROOT_PASSWD)),$(filter $$5$$%,$(TARGET_GENERIC_ROOT_PASSWD)),$(filter $$6$$%,$(TARGET_GENERIC_ROOT_PASSWD))),)
> 
>  filter allows multiple patterns, so:
> 
> else ifneq ($(filter $$1$$% $$5$$% $$6$$%,$(TARGET_GENERIC_ROOT_PASSWD)),)

Woot! :-) Thanks, will change.

> > +SYSTEM_ROOT_PASSWORD = $(TARGET_GENERIC_ROOT_PASSWD)
> > +else
> > +SYSTEM_ROOT_PASSWORD = $(shell $(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)")
> > +endif
> > +else # !BR2_TARGET_ENABLE_ROOT_LOGIN
> > +SYSTEM_ROOT_PASSWORD = *
> 
>  Even though Peter prefers positive logic, I think in this case it is more
> important to keep the logic close to the condition, i.e.:
> 
> ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),)
> SYSTEM_ROOT_PASSWORD = *
> else ifeq ($(TARGET_GENERIC_ROOT_PASSWD),)
> ...

Well, that was what Thomas initially suggested. But like Peter, I do
really prefer positive logic, so I'm heavily tempted to keep what I
wrote, unless others speak up and shout (sooned rather than later!). ;-)

Thanks for the review! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCHv5] system: allow/disallow root login, accept encoded passwords

[Arnout Vandecappelle]

On 14/04/15 19:54, Yann E. MORIN wrote:
[snip]
>>  However, since the default is y while it is normally n, and since we have to do
>> something special for the n case, wouldn't it make more sense to call it
>> BR2_TARGET_DISABLE_ROOT_LOGIN?
> 
> Well, I do really prefer positive naming; and we tend to use such
> positive logic about everywhere...
> 
> That the internals are "more complex" (and that still has to be proven)
> is irrelevant to what we present to the user. At the extreme, we could
> always have a hidden variable that just negates the visible one.

 OK for me. A hidden variable would be overkill.

[snip]
>>> +SYSTEM_ROOT_PASSWORD = $(TARGET_GENERIC_ROOT_PASSWD)
>>> +else
>>> +SYSTEM_ROOT_PASSWORD = $(shell $(MKPASSWD) -m "$(TARGET_GENERIC_PASSWD_METHOD)" "$(TARGET_GENERIC_ROOT_PASSWD)")
>>> +endif
>>> +else # !BR2_TARGET_ENABLE_ROOT_LOGIN
>>> +SYSTEM_ROOT_PASSWORD = *
>>
>>  Even though Peter prefers positive logic, I think in this case it is more
>> important to keep the logic close to the condition, i.e.:
>>
>> ifeq ($(BR2_TARGET_ENABLE_ROOT_LOGIN),)
>> SYSTEM_ROOT_PASSWORD = *
>> else ifeq ($(TARGET_GENERIC_ROOT_PASSWD),)
>> ...
> 
> Well, that was what Thomas initially suggested. But like Peter, I do
> really prefer positive logic, so I'm heavily tempted to keep what I
> wrote, unless others speak up and shout (sooned rather than later!). ;-)

 That's two against two then :-)

 But since you're the patch owner and Peter is the BDFL, I guess it will be
positive logic.

 Regards,
 Arnout

> 
> Thanks for the review! :-)
> 
> Regards,
> Yann E. MORIN.
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F