[Buildroot] [PATCH] package/openssh: Add option to populate keys on build

[Buildroot] [PATCH] package/openssh: Add option to populate keys on build

[Ramon Fried]

During development phase and on targets with read-only
file systems, generating SSH keys on boot is not an option.

Add option to generate and populate SSH keys during build.

Signed-off-by: Ramon Fried <rfried.dev@gmail.com>
---
 package/openssh/Config.in  | 12 ++++++++++++
 package/openssh/openssh.mk | 11 +++++++++++
 2 files changed, 23 insertions(+)

diff --git a/package/openssh/Config.in b/package/openssh/Config.in
index 683a9c0e51..21bdd40435 100644
--- a/package/openssh/Config.in
+++ b/package/openssh/Config.in
@@ -9,3 +9,15 @@ config BR2_PACKAGE_OPENSSH
 	  friends.
 
 	  http://www.openssh.com/
+
+if BR2_PACKAGE_OPENSSH
+
+config BR2_PACKAGE_OPENSSH_POPULATE_KEYS
+	bool "Populate device keys"
+	help
+	  Populate the image with device keys instead
+		of generating them on each boot.
+		This option has security implications, and
+		should be only used in development or on target
+		with read-only root file-system.
+endif
diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
index d50572128a..908eccf6cd 100644
--- a/package/openssh/openssh.mk
+++ b/package/openssh/openssh.mk
@@ -86,6 +86,17 @@ define OPENSSH_INSTALL_SSH_COPY_ID
 	$(INSTALL) -D -m 755 $(@D)/contrib/ssh-copy-id $(TARGET_DIR)/usr/bin/ssh-copy-id
 endef
 
+define OPENSSH_POPULATE_KEYS
+	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_rsa_key -N '' -t rsa
+	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_ecdsa_key -N '' -t ecdsa
+	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_dsa_key -N '' -t dsa
+	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_ed25519_key -N '' -t ed25519
+endef
+
+ifeq ($(BR2_PACKAGE_OPENSSH_POPULATE_KEYS),y)
+OPENSSH_POST_INSTALL_TARGET_HOOKS += OPENSSH_POPULATE_KEYS
+endif
+
 OPENSSH_POST_INSTALL_TARGET_HOOKS += OPENSSH_INSTALL_SSH_COPY_ID
 
 $(eval $(autotools-package))
-- 
2.26.2

[Buildroot] [PATCH] package/openssh: Add option to populate keys on build

[Thomas Petazzoni]

Hello,

On Wed, 29 Apr 2020 15:41:38 +0300
Ramon Fried <rfried.dev@gmail.com> wrote:

> During development phase and on targets with read-only
> file systems, generating SSH keys on boot is not an option.
> 
> Add option to generate and populate SSH keys during build.
> 
> Signed-off-by: Ramon Fried <rfried.dev@gmail.com>

I don't really have a very well formed opinion on whether we want this
or not. Feedback from other developers/contributors might be useful.

> +if BR2_PACKAGE_OPENSSH
> +
> +config BR2_PACKAGE_OPENSSH_POPULATE_KEYS
> +	bool "Populate device keys"

	bool "Generate host keys during build"

Perhaps.

> +	help
> +	  Populate the image with device keys instead
> +		of generating them on each boot.
> +		This option has security implications, and
> +		should be only used in development or on target
> +		with read-only root file-system.

Please fix the indentation: it should be one tab + two spaces, and the
line should try to use the 72-characters width as much as possible. Run
"make check-package" to check for such coding style issues.

> +endif
> diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
> index d50572128a..908eccf6cd 100644
> --- a/package/openssh/openssh.mk
> +++ b/package/openssh/openssh.mk
> @@ -86,6 +86,17 @@ define OPENSSH_INSTALL_SSH_COPY_ID
>  	$(INSTALL) -D -m 755 $(@D)/contrib/ssh-copy-id $(TARGET_DIR)/usr/bin/ssh-copy-id
>  endef
>  
> +define OPENSSH_POPULATE_KEYS

Should be defined within the ifeq
($(BR2_PACKAGE_OPENSSH_POPULATE_KEYS),y) condition.

> +	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_rsa_key -N '' -t rsa
> +	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_ecdsa_key -N '' -t ecdsa
> +	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_dsa_key -N '' -t dsa
> +	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_ed25519_key -N '' -t ed25519

Use $(...) to reference make variable instead of ${...}. Also, perhaps
a small loop makes sense:

	$(foreach type,rsa ecdsa dsa ed25519,\
		ssh-keygen -q -f $(TARGET_DIR)/etc/ssh/ssh_host_$(type)_key -N '' -t $(type)
	)

But wait: where is ssh-keygen coming from? We're not building it, so
you're relying on the one available on your build machine. This is not
acceptable for Buildroot, as we don't expect ssh-keygen to be available
on the host machine. So if we want to do what you propose, we would
need to build ssh-keygen for the host machine, using a host-openssh
package.

> +endef
> +
> +ifeq ($(BR2_PACKAGE_OPENSSH_POPULATE_KEYS),y)
> +OPENSSH_POST_INSTALL_TARGET_HOOKS += OPENSSH_POPULATE_KEYS
> +endif
> +
>  OPENSSH_POST_INSTALL_TARGET_HOOKS += OPENSSH_INSTALL_SSH_COPY_ID

Please keep this hook registration next to its definition.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] package/openssh: Add option to populate keys on build

[Ramon Fried]

On Wed, Apr 29, 2020 at 11:19 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Wed, 29 Apr 2020 15:41:38 +0300
> Ramon Fried <rfried.dev@gmail.com> wrote:
>
> > During development phase and on targets with read-only
> > file systems, generating SSH keys on boot is not an option.
> >
> > Add option to generate and populate SSH keys during build.
> >
> > Signed-off-by: Ramon Fried <rfried.dev@gmail.com>
>
> I don't really have a very well formed opinion on whether we want this
> or not. Feedback from other developers/contributors might be useful.
This is a real world scenario, I can understand if it doesn't fit upstream.
But I saw several projects during the years that generated keys in the
build time
either by this method, or just by putting them in fs-overlay.

>
> > +if BR2_PACKAGE_OPENSSH
> > +
> > +config BR2_PACKAGE_OPENSSH_POPULATE_KEYS
> > +     bool "Populate device keys"
>
>         bool "Generate host keys during build"
ok.
>
> Perhaps.
>
> > +     help
> > +       Populate the image with device keys instead
> > +             of generating them on each boot.
> > +             This option has security implications, and
> > +             should be only used in development or on target
> > +             with read-only root file-system.
>
> Please fix the indentation: it should be one tab + two spaces, and the
> line should try to use the 72-characters width as much as possible. Run
> "make check-package" to check for such coding style issues.
Sure
>
> > +endif
> > diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
> > index d50572128a..908eccf6cd 100644
> > --- a/package/openssh/openssh.mk
> > +++ b/package/openssh/openssh.mk
> > @@ -86,6 +86,17 @@ define OPENSSH_INSTALL_SSH_COPY_ID
> >       $(INSTALL) -D -m 755 $(@D)/contrib/ssh-copy-id $(TARGET_DIR)/usr/bin/ssh-copy-id
> >  endef
> >
> > +define OPENSSH_POPULATE_KEYS
>
> Should be defined within the ifeq
> ($(BR2_PACKAGE_OPENSSH_POPULATE_KEYS),y) condition.
>
> > +     ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_rsa_key -N '' -t rsa
> > +     ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_ecdsa_key -N '' -t ecdsa
> > +     ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_dsa_key -N '' -t dsa
> > +     ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_ed25519_key -N '' -t ed25519
>
ok.
> Use $(...) to reference make variable instead of ${...}. Also, perhaps
> a small loop makes sense:
>
>         $(foreach type,rsa ecdsa dsa ed25519,\
>                 ssh-keygen -q -f $(TARGET_DIR)/etc/ssh/ssh_host_$(type)_key -N '' -t $(type)
>         )
>
Sure.

> But wait: where is ssh-keygen coming from? We're not building it, so
> you're relying on the one available on your build machine. This is not
> acceptable for Buildroot, as we don't expect ssh-keygen to be available
> on the host machine. So if we want to do what you propose, we would
> need to build ssh-keygen for the host machine, using a host-openssh
> package.
I was wondering this myself as well, I think it's huge overhead to
compile the entire openssh
just to generate keys. Most linux distros have it already installed,
nevertheless, we can document that under
https://buildroot.org/downloads/manual/manual.html#requirement-optional
Right ?
>
> > +endef
> > +
> > +ifeq ($(BR2_PACKAGE_OPENSSH_POPULATE_KEYS),y)
> > +OPENSSH_POST_INSTALL_TARGET_HOOKS += OPENSSH_POPULATE_KEYS
> > +endif
> > +
> >  OPENSSH_POST_INSTALL_TARGET_HOOKS += OPENSSH_INSTALL_SSH_COPY_ID
>
> Please keep this hook registration next to its definition.
ok.
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[Buildroot] [PATCH] package/openssh: Add option to populate keys on build

[Yann E. MORIN]

Thomas, Ramon, All,

On 2020-04-29 22:19 +0200, Thomas Petazzoni spake thusly:
> On Wed, 29 Apr 2020 15:41:38 +0300
> Ramon Fried <rfried.dev@gmail.com> wrote:
> 
> > During development phase and on targets with read-only
> > file systems, generating SSH keys on boot is not an option.
> > 
> > Add option to generate and populate SSH keys during build.
> > 
> > Signed-off-by: Ramon Fried <rfried.dev@gmail.com>
> 
> I don't really have a very well formed opinion on whether we want this
> or not. Feedback from other developers/contributors might be useful.

I would be relatively opposed to that. If we want to generate keys at
build time, then it means we do not really care about the content of
said keys.

In that case, I would prefer we do for openssh like we do for dropbear:
generate the keys at boot, and if the filesystem ir R/O, redirect the
keys to /var/run. See:
    package/dropbear/S50dropbear
    package/dropbear/dropbear.service

If however the content of the keys are important, then they should not
be generated, but installed (e.g. by a package or by an overlay).

In either case, I'm not too much in favour for that patch.

Regards,
Yann E. MORIN.

> > +if BR2_PACKAGE_OPENSSH
> > +
> > +config BR2_PACKAGE_OPENSSH_POPULATE_KEYS
> > +	bool "Populate device keys"
> 
> 	bool "Generate host keys during build"
> 
> Perhaps.
> 
> > +	help
> > +	  Populate the image with device keys instead
> > +		of generating them on each boot.
> > +		This option has security implications, and
> > +		should be only used in development or on target
> > +		with read-only root file-system.
> 
> Please fix the indentation: it should be one tab + two spaces, and the
> line should try to use the 72-characters width as much as possible. Run
> "make check-package" to check for such coding style issues.
> 
> > +endif
> > diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
> > index d50572128a..908eccf6cd 100644
> > --- a/package/openssh/openssh.mk
> > +++ b/package/openssh/openssh.mk
> > @@ -86,6 +86,17 @@ define OPENSSH_INSTALL_SSH_COPY_ID
> >  	$(INSTALL) -D -m 755 $(@D)/contrib/ssh-copy-id $(TARGET_DIR)/usr/bin/ssh-copy-id
> >  endef
> >  
> > +define OPENSSH_POPULATE_KEYS
> 
> Should be defined within the ifeq
> ($(BR2_PACKAGE_OPENSSH_POPULATE_KEYS),y) condition.
> 
> > +	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_rsa_key -N '' -t rsa
> > +	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_ecdsa_key -N '' -t ecdsa
> > +	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_dsa_key -N '' -t dsa
> > +	ssh-keygen -q -f ${TARGET_DIR}/etc/ssh/ssh_host_ed25519_key -N '' -t ed25519
> 
> Use $(...) to reference make variable instead of ${...}. Also, perhaps
> a small loop makes sense:
> 
> 	$(foreach type,rsa ecdsa dsa ed25519,\
> 		ssh-keygen -q -f $(TARGET_DIR)/etc/ssh/ssh_host_$(type)_key -N '' -t $(type)
> 	)
> 
> But wait: where is ssh-keygen coming from? We're not building it, so
> you're relying on the one available on your build machine. This is not
> acceptable for Buildroot, as we don't expect ssh-keygen to be available
> on the host machine. So if we want to do what you propose, we would
> need to build ssh-keygen for the host machine, using a host-openssh
> package.
> 
> > +endef
> > +
> > +ifeq ($(BR2_PACKAGE_OPENSSH_POPULATE_KEYS),y)
> > +OPENSSH_POST_INSTALL_TARGET_HOOKS += OPENSSH_POPULATE_KEYS
> > +endif
> > +
> >  OPENSSH_POST_INSTALL_TARGET_HOOKS += OPENSSH_INSTALL_SSH_COPY_ID
> 
> Please keep this hook registration next to its definition.
> 
> Thanks!
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

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