[RFC PATCH 0/3] Enforce Shared State Signing when options set

[RFC PATCH 0/3] Enforce Shared State Signing when options set

[Joshua Lock]

There are 2 surprising behaviours in the current shared state signing
implementation which this pull request addresses:
1) when signature verification is enabled a failure to verify doesn't prevent
   the shared state object from being used. A warning is printed and the
   unsigned shared state object is used to accelerate the task.
2) when signing is enabled the taskhash doesn't change and any existing,
   unsigned, shared state objects will not be signed. This is particularly
   problematic when combined with 1.

Please review the following changes for suitability for inclusion. If you have
any objections or suggestions for improvement, please respond to the patches. If
you agree with the changes, please provide your Acked-by.

The following changes since commit 835f7eac0610325e906591cd81890bebe8627580:

  meta/lib/oeqa: Test for bootimg-biosplusefi Source (2019-07-23 22:26:28 +0100)

are available in the Git repository at:

  https://github.com/joshuagl/poky joshuagl/signed-sstate
  https://github.com/joshuagl/poky/tree/joshuagl/signed-sstate

Joshua Lock (3):
  sstate: fix log message
  classes/sstate: don't use unsigned sstate when verification enabled
  classes/sstate: regenerate sstate when signing enabled

 meta/classes/sstate.bbclass | 15 +++++++++++----
 meta/lib/oe/gpg_sign.py     | 10 ++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

-- 
2.21.0

[RFC PATCH 1/3] sstate: fix log message

[Joshua Lock]

Referring to the sstate object as a staging package is an artefact of the
code's origins. Switch to referring to an "Sstate package" in order to be more
accurate and consistent with the rest of the file.

Signed-off-by: Joshua Lock <jlock@vmware.com>
---
 meta/classes/sstate.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/sstate.bbclass b/meta/classes/sstate.bbclass
index b604729d84..d8fdcece6a 100644
--- a/meta/classes/sstate.bbclass
+++ b/meta/classes/sstate.bbclass
@@ -329,7 +329,7 @@ def sstate_installpkg(ss, d):
         pstaging_fetch(sstatefetch, d)
 
     if not os.path.isfile(sstatepkg):
-        bb.note("Staging package %s does not exist" % sstatepkg)
+        bb.note("Sstate package %s does not exist" % sstatepkg)
         return False
 
     sstate_clean(ss, d)
-- 
2.21.0

[RFC PATCH 2/3] classes/sstate: don't use unsigned sstate when verification enabled

[Joshua Lock]

When signature verification of shared state objects is enabled
(SSTATE_VERIFY_SIG) use of an unsigned object, even though it produces a
warning, seems unexpected. Instead skip unsigned objects and force the
non-accelerated task to be run.

Signed-off-by: Joshua Lock <jlock@vmware.com>
---
 meta/classes/sstate.bbclass | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/meta/classes/sstate.bbclass b/meta/classes/sstate.bbclass
index d8fdcece6a..3342c5ef50 100644
--- a/meta/classes/sstate.bbclass
+++ b/meta/classes/sstate.bbclass
@@ -340,7 +340,8 @@ def sstate_installpkg(ss, d):
     if bb.utils.to_boolean(d.getVar("SSTATE_VERIFY_SIG"), False):
         signer = get_signer(d, 'local')
         if not signer.verify(sstatepkg + '.sig'):
-            bb.warn("Cannot verify signature on sstate package %s" % sstatepkg)
+            bb.warn("Cannot verify signature on sstate package %s, skipping acceleration..." % sstatepkg)
+            return False
 
     # Empty sstateinst directory, ensure its clean
     if os.path.exists(sstateinst):
-- 
2.21.0

[RFC PATCH 3/3] classes/sstate: regenerate sstate when signing enabled

[Joshua Lock]

This change ensures that the task signatures changes, and therefore
sstate tasks are rerun, when signing is enabled. This has the
positive outcome that if signing is enabled new signed shared state
objects will be produced, rather than just signing shared state
objects for tasks where no work has been performed yet.

The downside of this change is that enabling/disabling sstate object
signing alters the taskhash and results in rebuilding the world.

Signed-off-by: Joshua Lock <jlock@vmware.com>
---
 meta/classes/sstate.bbclass | 10 ++++++++--
 meta/lib/oe/gpg_sign.py     | 10 ++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/meta/classes/sstate.bbclass b/meta/classes/sstate.bbclass
index 3342c5ef50..b060e15053 100644
--- a/meta/classes/sstate.bbclass
+++ b/meta/classes/sstate.bbclass
@@ -659,8 +659,12 @@ def sstate_package(ss, d):
     if d.getVar('SSTATE_SKIP_CREATION') == '1':
         return
 
+    sstate_create_package = ['sstate_report_unihash', 'sstate_create_package']
+    if d.getVar('SSTATE_SIG_KEY'):
+        sstate_create_package.append('sstate_sign_package')
+
     for f in (d.getVar('SSTATECREATEFUNCS') or '').split() + \
-             ['sstate_report_unihash', 'sstate_create_package', 'sstate_sign_package'] + \
+             sstate_create_package + \
              (d.getVar('SSTATEPOSTCREATEFUNCS') or '').split():
         # All hooks should run in SSTATE_BUILDDIR.
         bb.build.exec_func(f, d, (sstatebuild,))
@@ -774,7 +778,7 @@ sstate_create_package () {
 }
 
 python sstate_sign_package () {
-    from oe.gpg_sign import get_signer
+    from oe.gpg_sign import get_signer, SignFailedError
 
     if d.getVar('SSTATE_SIG_KEY'):
         signer = get_signer(d, 'local')
@@ -783,6 +787,8 @@ python sstate_sign_package () {
             os.unlink(sstate_pkg + '.sig')
         signer.detach_sign(sstate_pkg, d.getVar('SSTATE_SIG_KEY', False), None,
                            d.getVar('SSTATE_SIG_PASSPHRASE'), armor=False)
+    else:
+        raise SignFailedError("Can't sign sstate packages without key, SSTATE_SIG_KEY empty")
 }
 
 python sstate_report_unihash() {
diff --git a/meta/lib/oe/gpg_sign.py b/meta/lib/oe/gpg_sign.py
index 2fd8c3b1ac..ec5ace0dd0 100644
--- a/meta/lib/oe/gpg_sign.py
+++ b/meta/lib/oe/gpg_sign.py
@@ -123,6 +123,16 @@ class LocalSigner(object):
         return ret
 
 
+class SignFailedError(bb.build.FuncFailed):
+    def __init__(self, description, name=None, logfile=None):
+        self.description = description
+        self.name = name
+        self.logfile = logfile
+
+    def __str__(self):
+        return 'Signing failed: %s' % self.description
+
+
 def get_signer(d, backend):
     """Get signer object for the specified backend"""
     # Use local signing by default
-- 
2.21.0

Re: [RFC PATCH 3/3] classes/sstate: regenerate sstate when signing enabled

[Richard Purdie]

On Fri, 2019-07-26 at 11:26 +0000, Joshua Lock via Openembedded-core
wrote:
> This change ensures that the task signatures changes, and therefore
> sstate tasks are rerun, when signing is enabled. This has the
> positive outcome that if signing is enabled new signed shared state
> objects will be produced, rather than just signing shared state
> objects for tasks where no work has been performed yet.
> 
> The downside of this change is that enabling/disabling sstate object
> signing alters the taskhash and results in rebuilding the world.
> 
> Signed-off-by: Joshua Lock <jlock@vmware.com>
> ---
>  meta/classes/sstate.bbclass | 10 ++++++++--
>  meta/lib/oe/gpg_sign.py     | 10 ++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/classes/sstate.bbclass
> b/meta/classes/sstate.bbclass
> index 3342c5ef50..b060e15053 100644
> --- a/meta/classes/sstate.bbclass
> +++ b/meta/classes/sstate.bbclass
> @@ -659,8 +659,12 @@ def sstate_package(ss, d):
>      if d.getVar('SSTATE_SKIP_CREATION') == '1':
>          return
>  
> +    sstate_create_package = ['sstate_report_unihash',
> 'sstate_create_package']
> +    if d.getVar('SSTATE_SIG_KEY'):
> +        sstate_create_package.append('sstate_sign_package')
> +
>      for f in (d.getVar('SSTATECREATEFUNCS') or '').split() + \
> -             ['sstate_report_unihash', 'sstate_create_package',
> 'sstate_sign_package'] + \
> +             sstate_create_package + \
>               (d.getVar('SSTATEPOSTCREATEFUNCS') or '').split():
>          # All hooks should run in SSTATE_BUILDDIR.
>          bb.build.exec_func(f, d, (sstatebuild,))
> @@ -774,7 +778,7 @@ sstate_create_package () {
>  }
>  
>  python sstate_sign_package () {
> -    from oe.gpg_sign import get_signer
> +    from oe.gpg_sign import get_signer, SignFailedError
>  
>      if d.getVar('SSTATE_SIG_KEY'):
>          signer = get_signer(d, 'local')
> @@ -783,6 +787,8 @@ python sstate_sign_package () {
>              os.unlink(sstate_pkg + '.sig')
>          signer.detach_sign(sstate_pkg, d.getVar('SSTATE_SIG_KEY',
> False), None,
>                             d.getVar('SSTATE_SIG_PASSPHRASE'),
> armor=False)
> +    else:
> +        raise SignFailedError("Can't sign sstate packages without
> key, SSTATE_SIG_KEY empty")
>  }
>  
>  python sstate_report_unihash() {
> diff --git a/meta/lib/oe/gpg_sign.py b/meta/lib/oe/gpg_sign.py
> index 2fd8c3b1ac..ec5ace0dd0 100644
> --- a/meta/lib/oe/gpg_sign.py
> +++ b/meta/lib/oe/gpg_sign.py
> @@ -123,6 +123,16 @@ class LocalSigner(object):
>          return ret
>  
>  
> +class SignFailedError(bb.build.FuncFailed):
> +    def __init__(self, description, name=None, logfile=None):
> +        self.description = description
> +        self.name = name
> +        self.logfile = logfile
> +
> +    def __str__(self):
> +        return 'Signing failed: %s' % self.description
> +
> +

Whilst this subclass is quite pythonic, I'm not sure we want to do
this. At the back of my mind is the feeling that FuncFailed is a bit
pointless :/.

Put differently, what does this buy us that bb.fatal() doesn't?

We do have a few users of FuncFailed in OECore but not many (about
8)...

Cheers,

Richard

Re: [RFC PATCH 3/3] classes/sstate: regenerate sstate when signing enabled

[Joshua Lock]

> On 27 Jul 2019, at 11:12, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> On Fri, 2019-07-26 at 11:26 +0000, Joshua Lock via Openembedded-core
> wrote:
>> This change ensures that the task signatures changes, and therefore
>> sstate tasks are rerun, when signing is enabled. This has the
>> positive outcome that if signing is enabled new signed shared state
>> objects will be produced, rather than just signing shared state
>> objects for tasks where no work has been performed yet.
>> 
>> The downside of this change is that enabling/disabling sstate object
>> signing alters the taskhash and results in rebuilding the world.
>> 
>> Signed-off-by: Joshua Lock <jlock@vmware.com>
>> ---
>> meta/classes/sstate.bbclass | 10 ++++++++--
>> meta/lib/oe/gpg_sign.py     | 10 ++++++++++
>> 2 files changed, 18 insertions(+), 2 deletions(-)
>> 
>> diff --git a/meta/classes/sstate.bbclass
>> b/meta/classes/sstate.bbclass
>> index 3342c5ef50..b060e15053 100644
>> --- a/meta/classes/sstate.bbclass
>> +++ b/meta/classes/sstate.bbclass
>> @@ -659,8 +659,12 @@ def sstate_package(ss, d):
>>     if d.getVar('SSTATE_SKIP_CREATION') == '1':
>>         return
>> 
>> +    sstate_create_package = ['sstate_report_unihash',
>> 'sstate_create_package']
>> +    if d.getVar('SSTATE_SIG_KEY'):
>> +        sstate_create_package.append('sstate_sign_package')
>> +
>>     for f in (d.getVar('SSTATECREATEFUNCS') or '').split() + \
>> -             ['sstate_report_unihash', 'sstate_create_package',
>> 'sstate_sign_package'] + \
>> +             sstate_create_package + \
>>              (d.getVar('SSTATEPOSTCREATEFUNCS') or '').split():
>>         # All hooks should run in SSTATE_BUILDDIR.
>>         bb.build.exec_func(f, d, (sstatebuild,))
>> @@ -774,7 +778,7 @@ sstate_create_package () {
>> }
>> 
>> python sstate_sign_package () {
>> -    from oe.gpg_sign import get_signer
>> +    from oe.gpg_sign import get_signer, SignFailedError
>> 
>>     if d.getVar('SSTATE_SIG_KEY'):
>>         signer = get_signer(d, 'local')
>> @@ -783,6 +787,8 @@ python sstate_sign_package () {
>>             os.unlink(sstate_pkg + '.sig')
>>         signer.detach_sign(sstate_pkg, d.getVar('SSTATE_SIG_KEY',
>> False), None,
>>                            d.getVar('SSTATE_SIG_PASSPHRASE'),
>> armor=False)
>> +    else:
>> +        raise SignFailedError("Can't sign sstate packages without
>> key, SSTATE_SIG_KEY empty")
>> }
>> 
>> python sstate_report_unihash() {
>> diff --git a/meta/lib/oe/gpg_sign.py b/meta/lib/oe/gpg_sign.py
>> index 2fd8c3b1ac..ec5ace0dd0 100644
>> --- a/meta/lib/oe/gpg_sign.py
>> +++ b/meta/lib/oe/gpg_sign.py
>> @@ -123,6 +123,16 @@ class LocalSigner(object):
>>         return ret
>> 
>> 
>> +class SignFailedError(bb.build.FuncFailed):
>> +    def __init__(self, description, name=None, logfile=None):
>> +        self.description = description
>> +        self.name = name
>> +        self.logfile = logfile
>> +
>> +    def __str__(self):
>> +        return 'Signing failed: %s' % self.description
>> +
>> +
> 
> Whilst this subclass is quite pythonic, I'm not sure we want to do
> this. At the back of my mind is the feeling that FuncFailed is a bit
> pointless :/.

This is pointless in more ways than one, because above I changed the SSTATECREATEFUNCS to only include sstate_sign_package if SSTATE_SIG_KEY is set this exception shouldn’t ever be raised.

I’ll send an update series with this class and its usage dropped.

Joshua

> Put differently, what does this buy us that bb.fatal() doesn't?
> 
> We do have a few users of FuncFailed in OECore but not many (about
> 8)...
> 
> Cheers,
> 
> Richard
> 
> 
>