Re: [PATCH v4 14/14] Validate configuration parameters

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 6893 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> Wrong values for the version or update URL will cause very confusing
> build-time or runtime errors.  Provide a better user experience by
> validating them up-front.
>
> The update URL validator is loose.  It rejects only URLs that cannot
> possibly work: either appending /SHA256SUMS to them doesn't append to
> the path, or they will definitely be rejected by curl due to being
> malformed.
>
> The version validator is in lib/config.nix, as the version number is
> used in many places.  It checks that the version only uses characters
> that are permitted by systemd's version number specification [1] and
> that will not break code that uses them in shell or sed commands.
>
> [1]: https://uapi-group.org/specifications/specs/version_format_specification
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
> Changes since v3:
> - Validate compression level.
>
> Changes since v2:
> - Use loose URL validation: allow anything that might work.
> - Only reject versions that violate the specification.
> ---
>  Documentation/installation/getting-spectrum.adoc |  2 +-
>  host/rootfs/default.nix                          | 19 ++++++++++++++++++-
>  lib/config.nix                                   | 12 +++++++++++-
>  release/combined/eosimages.nix                   |  8 +++++++-
>  4 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/installation/getting-spectrum.adoc b/Documentation/installation/getting-spectrum.adoc
> index 0abc83a9e6fc01084b3faa9b93eb38398b0aef27..919b28f86eddff1b92570d46b62a1fbddc32f2d5 100644
> --- a/Documentation/installation/getting-spectrum.adoc
> +++ b/Documentation/installation/getting-spectrum.adoc
> @@ -93,7 +93,7 @@ you must provide your own.  You can do this via
>  xref:../development/build-configuration.adoc[build configuration].
>  The default sets the signing key to `/dev/null` and the server
>  URL to an invalid value, so updates won't work.  To enable updates,
> -set `update-url` to the URL of your server and `update-signing-key`
> +set `updateUrl` to the URL of your server and `updateSigningKey`

Wrong patch surely?

>  to a binary GnuPG keyring to verify the updates with.  Not all possible
>  URLs will work, but most invalid URLs will cause an error during the
>  build rather than runtime misbehavior.
> diff --git a/host/rootfs/default.nix b/host/rootfs/default.nix
> index 1ebaf11cd7e9d61444b6524de6053a0f3cfb82c8..fed99013f960287c3be3941ca593b22c55a6f79a 100644
> --- a/host/rootfs/default.nix
> +++ b/host/rootfs/default.nix
> @@ -85,6 +85,23 @@ let
>      appvm-systemd-sysupdate = callSpectrumPackage ../../vm/app/systemd-sysupdate {};
>    };
>  
> +  update-url =

As a Nix variable, this should stick to camel case rather than
introducing another variable naming convention.

> +    let update-url = config.updateUrl; in
> +    # Use builtins.fromJSON because it supports \uXXXX escapes.
> +    # This is the same check done by check-url.awk in the update VM.
> +    # The update code is careful to escape any metacharacters, but some
> +    # simply cannot be made to work.  Concatenating the URL with /SHA256SUMS
> +    # must append to the path portion of the URL, and the URL must be one
> +    # that libcurl will accept.  I don't know how Unicode space is handled,
> +    # but it is a bad idea.
> +    if builtins.match (builtins.fromJSON "\"^[^\\u0001- #?\\u007F[:space:]]+$\"" update-url) == null then
> +      builtins.abort ''
> +        Update URL ${builtins.toJSON update-url} has forbidden characters.
> +        Query strings, and fragment specifiers are not supported.
> +        ASCII control characters and whitespace must be %-encoded.
> +        ''
> +    else
> +      update-url;
>    packagesSysroot = runCommand "packages-sysroot" {
>      depsBuildBuild = [ inkscape ];
>      nativeBuildInputs = [ xorg.lndir ];
> @@ -152,7 +169,7 @@ stdenvNoCC.mkDerivation {
>        name = "signing-key";
>        path = config.updateSigningKey;
>      };
> -    UPDATE_URL = config.updateUrl;
> +    UPDATE_URL = update-url;
>      VERSION = config.version;
>    };
>  
> diff --git a/lib/config.nix b/lib/config.nix
> index bc5b42f506b7bfd2f66db48610491809351d1a2c..2065be83ad97f8eb011f070d8c3f3249104d07f4 100644
> --- a/lib/config.nix
> +++ b/lib/config.nix
> @@ -18,6 +18,16 @@ let
>    callConfig = config: if builtins.typeOf config == "lambda" then config {
>      inherit default;
>    } else config;
> +  finalConfig = default // callConfig config;
>  in
>  
> -default // callConfig config
> +# Version is used in many files, so validate it here.
> +# See https://uapi-group.org/specifications/specs/version_format_specification
> +# for allowed version strings.
> +if builtins.match "[[:alnum:]_.~^-]+" finalConfig.version == null then
> +   builtins.abort ''
> +     Version ${builtins.toJSON finalConfig.version} has forbidden characters.
> +     Only ASCII alphanumerics, ".", "_", "~", "^", "+", and "-" are allowed.
> +     ''
> +else
> +  finalConfig

Okay, I suppose that makes sense.  In that case all configuration
options probably ought to be validated here, in one place.  asserts
could at least prevent rightwards drift at the expense of error
messages, which I think would be fine for quickly catching trivial
mistakes as is the goal here.

> diff --git a/release/combined/eosimages.nix b/release/combined/eosimages.nix
> index 9cb35dcecee54c17392b609c493272ec83062e9b..5d1e3a67bb81cbb737823bfa3c75d88f18b31f2a 100644
> --- a/release/combined/eosimages.nix
> +++ b/release/combined/eosimages.nix
> @@ -4,6 +4,12 @@
>  import ../../lib/call-package.nix (
>  { callSpectrumPackage, runCommand, e2fsprogs, tar2ext4, config }:
>  
> +let
> +  compressionLevel = config.compressionLevel;
> +in
> +if compressionLevel < 1 || compressionLevel > 9 then
> +  builtins.abort "Compression level ${builtins.toString compressionLevel} is invalid (< 1 or > 9)"
> +else

Even if we keep compressionLevel, this doesn't really have the same
justification for validation as the others.  A mistake here isn't going
to lead to confusing runtime errors.

>  runCommand "eosimages.img" {
>    nativeBuildInputs = [ e2fsprogs tar2ext4 ];
>    imageName = "Spectrum-0.0-x86_64-generic.0.Live.img";
> @@ -16,7 +22,7 @@ runCommand "eosimages.img" {
>    mkdir dir
>    cd dir
>    ln -s -- "$image" "$imageName"
> -  gzip -${builtins.toString (0 + config.compressionLevel)} < "$image" > "$imageName.gz"
> +  gzip -${builtins.toString compressionLevel} < "$image" > "$imageName.gz"
>    sha256sum -- "$imageName.gz" > "$imageName.gz.sha256"
>    tar -ch -- "$imageName.gz" "$imageName.gz.sha256" | tar2ext4 -o "$out"
>    e2label "$out" eosimages
>
> -- 
> 2.52.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]