[blivet] Allow specifying minimum entropy when creating LUKS

Anne Mulhern amulhern at redhat.com
Tue Sep 2 15:05:31 UTC 2014





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Sunday, August 31, 2014 3:43:48 PM
> Subject: Re: [blivet] Allow specifying minimum entropy when creating LUKS
> 
> On Fri, 2014-08-29 at 11:02 -0400, Anne Mulhern wrote:
> > 
> > 
> > 
> > ----- Original Message -----
> > > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > > To: anaconda-patches at lists.fedorahosted.org
> > > Sent: Friday, August 29, 2014 8:21:12 AM
> > > Subject: [blivet] Allow specifying minimum entropy when creating LUKS
> > > 
> > > Creating new LUKS format requires high-quality random data and such
> > > quality
> > > may
> > > be specified by minimum entropy of kernel's random data pool required.
> > > Blivet
> > > thus need to check such entropy and wait for it to become high enough if
> > > requested by the user code (Anaconda, OpenLMI, blivet-gui, etc.).
> > > 
> > > In order to allow the user code to give a hint to the users about what's
> > > happening and how they can help with entropy generation, blivet also
> > > needs a
> > > callback that is called in case of LUKS format being created with not
> > > enough
> > > entropy available.
> > > 
> > > Related: rhbz#1073679
> > > Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> > > ---
> > >  blivet/callbacks.py         |  9 ++++++---
> > >  blivet/deviceaction.py      | 13 ++++++++++++-
> > >  blivet/devicefactory.py     | 13 ++++++++++---
> > >  blivet/devicelibs/crypto.py | 11 ++++++++++-
> > >  blivet/formats/luks.py      |  8 +++++++-
> > >  blivet/partitioning.py      | 25 ++++++++++++++++++-------
> > >  blivet/util.py              |  4 ++++
> > >  7 files changed, 67 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/blivet/callbacks.py b/blivet/callbacks.py
> > > index 2f7a9cf..b2e8c3a 100644
> > > --- a/blivet/callbacks.py
> > > +++ b/blivet/callbacks.py
> > > @@ -34,12 +34,14 @@ _CallbacksRegister = namedtuple("_CallbacksRegister",
> > >                                  ["create_format_pre",
> > >                                   "create_format_post",
> > >                                   "resize_format_pre",
> > > -                                 "resize_format_post"])
> > > +                                 "resize_format_post",
> > > +                                 "wait_for_entropy"])
> > >  
> > >  def create_new_callbacks_register(create_format_pre=None,
> > >                                    create_format_post=None,
> > >                                    resize_format_pre=None,
> > > -                                  resize_format_post=None):
> > > +                                  resize_format_post=None,
> > > +                                  wait_for_entropy=None):
> > >      """
> > >      A function for creating a new opaque object holding the references
> > >      to
> > >      callbacks. The point of this function is to hide the implementation
> > >      of
> > >      such
> > > @@ -49,7 +51,8 @@ def
> > > create_new_callbacks_register(create_format_pre=None,
> > >      """
> > >  
> > >      return _CallbacksRegister(create_format_pre, create_format_post,
> > > -                              resize_format_pre, resize_format_post)
> > > +                              resize_format_pre, resize_format_post,
> > > +                              wait_for_entropy)
> > >  
> > >  CreateFormatPreData = namedtuple("CreateFormatPreData",
> > >                                   ["msg"])
> > > diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py
> > > index fff5f39..a17ade1 100644
> > > --- a/blivet/deviceaction.py
> > > +++ b/blivet/deviceaction.py
> > > @@ -24,10 +24,11 @@
> > >  from . import util
> > >  
> > >  from . import udev
> > > +from .util import get_current_entropy
> > >  from .devices import StorageDevice
> > >  from .devices import PartitionDevice
> > >  from .devices import LVMLogicalVolumeDevice
> > > -from .formats import getFormat
> > > +from .formats import getFormat, luks
> > >  from .storage_log import log_exception_info
> > >  from parted import partitionFlag, PARTITION_LBA
> > >  from .i18n import _, N_
> > > @@ -552,6 +553,16 @@ class ActionCreateFormat(DeviceAction):
> > >  
> > >              self.device.disk.format.commitToDisk()
> > >  
> > > +        if isinstance(self.device.format, luks.LUKS):
> > > +            # LUKS needs to wait for random data entropy if it is too
> > > low
> > > +            required_entropy = self.device.format.min_luks_entropy
> > > +            current_entropy = get_current_entropy()
> > > +            if required_entropy > 0 and current_entropy <
> > > required_entropy:
> > 
> > minimum entropy less than 0 treated as expected. Also, might be useful to
> > rename
> > required_entropy min_required_entropy here.
> I think it's quite obvious from the context, but it will make things a
> bit clearer which is always good. Fixing locally.
> 
> > 
> > > +                if callbacks and callbacks.wait_for_entropy:
> > > +                    msg = _("Not enough entropy to create LUKS format. "
> > > +                            "%d bits are needed.") % required_entropy
> > > +                    callbacks.wait_for_entropy(msg, required_entropy)
> > > +
> > >          self.device.format.create(device=self.device.path,
> > >                                    options=self.device.formatArgs)
> > >          # Get the UUID now that the format is created
> > > diff --git a/blivet/devicefactory.py b/blivet/devicefactory.py
> > > index 7926d39..4e123b0 100644
> > > --- a/blivet/devicefactory.py
> > > +++ b/blivet/devicefactory.py
> > > @@ -218,7 +218,7 @@ class DeviceFactory(object):
> > >                   label=None, raid_level=None, encrypted=False,
> > >                   container_encrypted=False, container_name=None,
> > >                   container_raid_level=None,
> > >                   container_size=SIZE_POLICY_AUTO,
> > > -                 name=None, device=None):
> > > +                 name=None, device=None, min_luks_entropy=0):
> > >          """
> > >              :param storage: a Blivet instance
> > >              :type storage: :class:`~.Blivet`
> > > @@ -258,6 +258,9 @@ class DeviceFactory(object):
> > >              :type container_encrypted: bool
> > >              :keyword container_size: requested container size
> > >              :type container_size: :class:`~.size.Size`
> > > +            :keyword min_luks_entropy: minimum entropy in bits required
> > > for
> > > +                                       LUKS format creation
> > > +            :type min_luks_entropy: int
> > >  
> > >          """
> > >  
> > > @@ -297,6 +300,7 @@ class DeviceFactory(object):
> > >  
> > >          self.child_factory = None
> > >          self.parent_factory = None
> > > +        self.min_luks_entropy = min_luks_entropy
> > >  
> > >          # used for error recovery
> > >          self.__devices = []
> > > @@ -607,6 +611,7 @@ class DeviceFactory(object):
> > >  
> > >              fmt = getFormat(self.fstype,
> > >                              mountpoint=self.mountpoint,
> > > +                            min_luks_entropy=self.min_luks_entropy,
> > >                              **fmt_args)
> > >              luks_device = LUKSDevice("luks-" + device.name,
> > >                                       parents=[device], fmt=fmt)
> > > @@ -685,7 +690,8 @@ class DeviceFactory(object):
> > >              if parent_container:
> > >                  parent_container.parents.remove(self.device)
> > >              leaf_format = self.device.format
> > > -            self.storage.formatDevice(self.device, getFormat("luks"))
> > > +            self.storage.formatDevice(self.device, getFormat("luks",
> > > +
> > > min_luks_entropy=self.min_luks_entropy))
> > >              luks_device = LUKSDevice("luks-%s" % self.device.name,
> > >                                       fmt=leaf_format,
> > >                                       parents=self.device)
> > > @@ -1026,7 +1032,8 @@ class PartitionSetFactory(PartitionFactory):
> > >                  if container:
> > >                      container.parents.remove(member)
> > >  
> > > -                self.storage.formatDevice(member, getFormat("luks"))
> > > +                self.storage.formatDevice(member, getFormat("luks",
> > > +
> > > min_luks_entropy=self.min_luks_entropy))
> > >                  luks_member = LUKSDevice("luks-%s" % member.name,
> > >                                      parents=[member],
> > >                                      fmt=getFormat(self.fstype))
> > > diff --git a/blivet/devicelibs/crypto.py b/blivet/devicelibs/crypto.py
> > > index b911e94..ccc8aa3 100644
> > > --- a/blivet/devicelibs/crypto.py
> > > +++ b/blivet/devicelibs/crypto.py
> > > @@ -21,12 +21,15 @@
> > >  #
> > >  
> > >  import random
> > > +import time
> > >  from pycryptsetup import CryptSetup
> > >  
> > >  from ..errors import CryptoError
> > >  from ..size import Size
> > > +from ..util import get_current_entropy
> > >  
> > >  LUKS_METADATA_SIZE = Size("2 MiB")
> > > +MIN_CREATE_ENTROPY = 256 # bits
> > >  
> > >  # Keep the character set size a power of two to make sure all characters
> > >  are
> > >  # equally likely
> > > @@ -65,7 +68,8 @@ def luks_status(name):
> > >  
> > >  def luks_format(device,
> > >                  passphrase=None,
> > > -                cipher=None, key_size=None, key_file=None):
> > > +                cipher=None, key_size=None, key_file=None,
> > > +                min_entropy=0):
> > >      # pylint: disable=unused-argument
> > >      if not passphrase:
> > >          raise ValueError("luks_format requires passphrase")
> > > @@ -88,6 +92,11 @@ def luks_format(device,
> > >      if cipherMode: kwargs["cipherMode"]  = cipherMode
> > >      if   key_size: kwargs["keysize"]  = key_size
> > >  
> > > +    if min_entropy >= 0:
> > 
> > minimum entropy less than zero treated as expected value.
> > 
> > > +        while get_current_entropy() < min_entropy:
> > > +            # wait for entropy to become high enough
> > > +            time.sleep(1)
> > > +
> > >      rc = cs.luksFormat(**kwargs)
> > >      if rc:
> > >          raise CryptoError("luks_format failed for '%s'" % device)
> > > diff --git a/blivet/formats/luks.py b/blivet/formats/luks.py
> > > index bcececb..d01bc6a 100644
> > > --- a/blivet/formats/luks.py
> > > +++ b/blivet/formats/luks.py
> > > @@ -71,6 +71,9 @@ class LUKS(DeviceFormat):
> > >              :type escrow_cert: str
> > >              :keyword add_backup_passphrase: generate a backup
> > >              passphrase?
> > >              :type add_backup_passphrase: bool.
> > > +            :keyword min_luks_entropy: minimum entropy in bits required
> > > for
> > > +                                       format creation
> > > +            :type min_luks_entropy: int
> > >  
> > >              .. note::
> > >  
> > > @@ -97,6 +100,7 @@ class LUKS(DeviceFormat):
> > >          self._key_file = kwargs.get("key_file")
> > >          self.escrow_cert = kwargs.get("escrow_cert")
> > >          self.add_backup_passphrase = kwargs.get("add_backup_passphrase",
> > >          False)
> > > +        self.min_luks_entropy = kwargs.get("min_luks_entropy", 0)
> > >  
> > >          if not self.mapName and self.exists and self.uuid:
> > >              self.mapName = "luks-%s" % self.uuid
> > > @@ -220,7 +224,9 @@ class LUKS(DeviceFormat):
> > >                               passphrase=self.__passphrase,
> > >                               key_file=self._key_file,
> > >                               cipher=self.cipher,
> > > -                             key_size=self.key_size)
> > > +                             key_size=self.key_size,
> > > +                             min_entropy=self.min_luks_entropy)
> > > +
> > >          except Exception:
> > >              raise
> > >          else:
> > > diff --git a/blivet/partitioning.py b/blivet/partitioning.py
> > > index 48d72ff..32fcf68 100644
> > > --- a/blivet/partitioning.py
> > > +++ b/blivet/partitioning.py
> > > @@ -69,7 +69,7 @@ def _getCandidateDisks(storage):
> > >  
> > >      return disks
> > >  
> > > -def _scheduleImplicitPartitions(storage, disks):
> > > +def _scheduleImplicitPartitions(storage, disks, min_luks_entropy=0):
> > >      """ Schedule creation of a lvm/btrfs member partitions for autopart.
> > >  
> > >          We create one such partition on each disk. They are not
> > >          allocated
> > >          until
> > > @@ -79,6 +79,9 @@ def _scheduleImplicitPartitions(storage, disks):
> > >          :type storage: :class:`~.Blivet`
> > >          :param disks: list of partitioned disks with free space
> > >          :type disks: list of :class:`~.devices.StorageDevice`
> > > +        :param min_luks_entropy: minimum entropy in bits required for
> > > +                                 luks format creation
> > > +        :type min_luks_entropy: int
> > >          :returns: list of newly created (unallocated) partitions
> > >          :rtype: list of :class:`~.devices.PartitionDevice`
> > >      """
> > > @@ -95,7 +98,8 @@ def _scheduleImplicitPartitions(storage, disks):
> > >              fmt_args = {"passphrase": storage.encryptionPassphrase,
> > >                          "cipher": storage.encryptionCipher,
> > >                          "escrow_cert": storage.autoPartEscrowCert,
> > > -                        "add_backup_passphrase":
> > > storage.autoPartAddBackupPassphrase}
> > > +                        "add_backup_passphrase":
> > > storage.autoPartAddBackupPassphrase,
> > > +                        "min_luks_entropy": min_luks_entropy}
> > >          else:
> > >              if storage.autoPartType in (AUTOPART_TYPE_LVM,
> > >              AUTOPART_TYPE_LVM_THINP):
> > >                  fmt_type = "lvmpv"
> > > @@ -111,7 +115,7 @@ def _scheduleImplicitPartitions(storage, disks):
> > >  
> > >      return devs
> > >  
> > > -def _schedulePartitions(storage, disks):
> > > +def _schedulePartitions(storage, disks, min_luks_entropy=0):
> > >      """ Schedule creation of autopart partitions.
> > >  
> > >          This only schedules the requests for actual partitions.
> > > @@ -120,6 +124,9 @@ def _schedulePartitions(storage, disks):
> > >          :type storage: :class:`~.Blivet`
> > >          :param disks: list of partitioned disks with free space
> > >          :type disks: list of :class:`~.devices.StorageDevice`
> > > +        :param min_luks_entropy: minimum entropy in bits required for
> > > +                                 luks format creation
> > > +        :type min_luks_entropy: int
> > >          :returns: None
> > >          :rtype: None
> > >      """
> > > @@ -196,7 +203,8 @@ def _schedulePartitions(storage, disks):
> > >              fmt_args = {"passphrase": storage.encryptionPassphrase,
> > >                          "cipher": storage.encryptionCipher,
> > >                          "escrow_cert": storage.autoPartEscrowCert,
> > > -                        "add_backup_passphrase":
> > > storage.autoPartAddBackupPassphrase}
> > > +                        "add_backup_passphrase":
> > > storage.autoPartAddBackupPassphrase,
> > > +                        "min_luks_entropy": min_luks_entropy}
> > >          else:
> > >              fmt_type = request.fstype
> > >              fmt_args = {}
> > > @@ -328,13 +336,16 @@ def _scheduleVolumes(storage, devs):
> > >          # schedule the device for creation
> > >          storage.createDevice(dev)
> > >  
> > > -def doAutoPartition(storage, data):
> > > +def doAutoPartition(storage, data, min_luks_entropy=0):
> > >      """ Perform automatic partitioning.
> > >  
> > >          :param storage: a :class:`~.Blivet` instance
> > >          :type storage: :class:`~.Blivet`
> > >          :param data: kickstart data
> > >          :type data: :class:`pykickstart.BaseHandler`
> > > +        :param min_luks_entropy: minimum entropy in bits required for
> > > +                                 luks format creation
> > > +        :type min_luks_entropy: int
> > >  
> > >          :attr:`Blivet.doAutoPart` controls whether this method creates
> > >          the
> > >          automatic partitioning layout. :attr:`Blivet.autoPartType`
> > >          controls
> > > @@ -369,7 +380,7 @@ def doAutoPartition(storage, data):
> > >          raise NoDisksError(_("No usable disks selected"))
> > >  
> > >      disks = _getCandidateDisks(storage)
> > > -    devs = _scheduleImplicitPartitions(storage, disks)
> > > +    devs = _scheduleImplicitPartitions(storage, disks, min_luks_entropy)
> > >      log.debug("candidate disks: %s", disks)
> > >      log.debug("devs: %s", devs)
> > >  
> > > @@ -377,7 +388,7 @@ def doAutoPartition(storage, data):
> > >          raise NotEnoughFreeSpaceError(_("Not enough free space on disks
> > >          for
> > >          "
> > >                                        "automatic partitioning"))
> > >  
> > > -    _schedulePartitions(storage, disks)
> > > +    _schedulePartitions(storage, disks,
> > > min_luks_entropy=min_luks_entropy)
> > >  
> > >      # run the autopart function to allocate and grow partitions
> > >      doPartitioning(storage)
> > > diff --git a/blivet/util.py b/blivet/util.py
> > > index a15bb4a..649b8a7 100644
> > > --- a/blivet/util.py
> > > +++ b/blivet/util.py
> > > @@ -454,3 +454,7 @@ def variable_copy(obj, memo, omit=None, shallow=None,
> > > duplicate=None):
> > >              setattr(new, attr, copy.deepcopy(value, memo))
> > >  
> > >      return new
> > > +
> > > +def get_current_entropy():
> > > +    with open("/proc/sys/kernel/random/entropy_avail", "r") as fobj:
> > > +        return int(fobj.readline())
> > > --
> > > 1.9.3
> > > 
> > > _______________________________________________
> > > anaconda-patches mailing list
> > > anaconda-patches at lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > > 
> > 
> > 0 is a very good value for minimum entropy.
> > I think that a minimum entropy less than zero should
> > be treated as an exceptional condition (rather than an
> > expected one). I've marked the places where it seems to
> > be being treated as expected.
> I don't think it is wise to raise exceptions in such places. I mean, it
> may reveal some issues in the calling code giving us something obviously
> wrong, on the other hand, I like the approach where a valid value means
> some request and an invalid value means "I don't care". The question is
> if we want to report/log bugs in callers code or not.
> 

I'm sorry, I wasn't clear. I wasn't suggesting that you raise
an exception in those locations.

It seems to me that there are two internally consistent choices:
1) Only non-negative values of min_entropy are considered valid. In that case,
since these values are almost final, raising some kind of
InvalidValue exception where they are set in the constructors
seems like the reasonable thing to do.
The extra checks for required_entropy vs. 0 in the places I've marked can be
eliminated completely in that case, since it can never happen.
The cost of this is having to check for valid values in constructors.

2) Alternatively, all non-positive values, including 0, can represent a single
"don't care" value.

In that case, under the assumption that current entropy will always have
an actual entropy value (not a number representing a don't care condition),
the checks of required_entropy vs. 0 in each
place I've marked are redundant and can be eliminated.

The one drawback of this approach is that if any user code computes a negative
value for minimum entropy accidentally and sets that value in the constructor
it will never be caught by our code.

In either case:

The choice of representation should be clearly documented.

If current entropy ever shows up negative that seems like it ought to be
treated as an exceptional value somehow, although not necessarily by
throwing an exception. But it seems on the face of it to be very unlikely.

- mulhern

> --
> Vratislav Podzimek
> 
> Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 


More information about the anaconda-patches mailing list