Re: [PATCH v2 8/8] lib/config.nix: Validate configuration parameters

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 3677 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.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
>  lib/config.nix | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/lib/config.nix b/lib/config.nix
> index 5b6b95013734202b7e2e01d5ffce313080658006..660a2427447fd9851e60e955da6bd1a5d71cfdac 100644
> --- a/lib/config.nix
> +++ b/lib/config.nix
> @@ -19,10 +19,39 @@ let
>      inherit default;
>    } else config;
>    finalConfig = default // callConfig config;
> +
> +  # Only allow unreserved characters, : (for port numbers), /, and %-encoding.
> +  # The rest of the code is allowed to assume that these are the only characters
> +  # in the update URL.
> +  # Do not use [:alnum:] or [:hexdigit:] as they depend on the locale in POSIX.

Did we confirm that's true for Nix?  It would be a big problem if it was.

> +  # Query strings and fragment identifiers break appending
> +  # /SHA256SUMS and /SHA256SUMS.gpg to a URL.
> +  # [, ], {, and } would cause globbing in curl.
> +  url-regex = "^https?://([ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789:./~-]|%[ABCDEFabcdef0123456789]{2})+$";
> +  update-url = finalConfig.update-url;
> +
> +  # Only allow a numeric version for now.
> +  number_re = "(0|[1-9][0-9]{0,2})";
> +  version_re = "^(${number_re}\\.){2}${number_re}$";

Why?  Since this is only to make developers' lives a bit easier, we
should err on the side of allowing stuff through rather than restricting
what might be a valid use case.

>  in
> -  finalConfig // {
> -    update-signing-key = builtins.path {
> -      name = "signing-key";
> -      path = finalConfig.update-signing-key;
> -    };
> -  }
> +  if !builtins.isString update-url then
> +    builtins.abort "Update URL must be a string, not ${builtins.typeOf update-url}"
> +  else if builtins.match "^https?://.*" update-url == null then
> +    builtins.abort "Update URL ${builtins.toJSON update-url} has unsupported scheme (not https:// or http://) or is invalid"
> +  else if builtins.match url-regex update-url == null then
> +    builtins.abort "Update URL ${builtins.toJSON update-url} has forbidden characters"
> +  else if builtins.substring (builtins.stringLength update-url - 1) 1 update-url == "/" then
> +    builtins.abort "Update URL ${builtins.toJSON update-url} must not end with /"
> +  else if !builtins.isString finalConfig.version then
> +    builtins.abort "Version must be a string, not ${builtins.typeOf finalConfig.version}"
> +  else if builtins.match version_re finalConfig.version == null then
> +    builtins.abort "Version ${builtins.toJSON finalConfig.version} is invalid"
> +  else if !builtins.isPath finalConfig.update-signing-key then
> +    builtins.abort "Update verification key file is of type ${builtins.typeOf finalConfig.update-signing-key}, not path"

I wouldn't bother checking the types.  There'll be a decent error
message if the wrong type is used anyway, and I don't want to get into
checking the type of the other config values.

I wonder if it would be more maintainable to have this more local, where
the values are actually used.  That way, it's more likely they'll be
kept up to date.

> +  else
> +    finalConfig // {
> +      update-signing-key = builtins.path {
> +        name = "signing-key";
> +        path = finalConfig.update-signing-key;
> +      };
> +    }
>
> -- 
> 2.51.2

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