| Commit message (Collapse) | Author | Age |
|
|
|
|
|
|
|
|
|
| |
When a PIN is changed using the pin set command, the last action is to
confirm the operation with the previously used PIN. This step will cause
this PIN, which is now stale and no longer valid, to be cached, which in
turn can cause follow up command using the same PIN type to use this
wrong cached entry for authentication.
To fix this problem, this change explicitly clear the PIN entry from the
cache after the PIN has been changed.
|
|
|
|
|
|
|
|
|
|
|
|
| |
So far we have cached secrets in gpg-agent(1) whenever that made sense
to do (i.e., for the two PINs in most contexts but not for passwords).
While there is reason to believe that such caching is desired by the
majority of users, not everybody has a use for it.
To give users an opportunity to opt out of such caching, this change
introduces a new environment variable, NITROCLI_NO_CACHE, that, when
present in the environment, instructs the program to bypass the cache
for all operations that require a secret and to instead inquire such
secrets each time they are needed.
|
|
|
|
|
|
|
|
|
|
| |
The input to the Enum macro is supposed to resemble the definition of an
enum in Rust code. When manually defining an enum (or a struct for that
matter), we typically terminate all branches with a comma, and don't
just omit that on the last line.
To mirror this behavior, this change adjusts the Enum macro to accept
(and in fact, require) a comma-terminated last line as well, as opposed
to not accepting it as had been the case so far.
|
|
|
|
|
|
|
|
|
| |
Macros typically should reference types by their full path and not
assume that they are in scope wherever the macro is expanded. We did
missed one spot where AsRef was not fully qualified in the Enum macro.
While that is not much of an issue here (and there may be more
occurrences, e.g., in the auto derives) lets fix that up for the sake of
consistency.
|
|
|
|
|
|
|
|
|
|
| |
With Rust 1.35 we get compile errors due to doc comments that are added
to macro invocations but not actually included in the expanded output.
The rustc wrongly assumes that we want to document the resulting code
and not just provide details about the invocation itself.
This change explicitly allows for those cases. Alternatively we could
have "downgraded" the doc comments to normal comments or removed them
altogether. There is little difference between those alternatives.
|
|
|
|
|
| |
This change adds a test case for the -V/--version option to the suite of
tests.
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Due to a bug in argparse [0], custom stdout and stderr settings are
ignored when using argparse::Print, as we currently do for the --version
option. This patch adds a workaround for this problem: Instead of using
argparse::Print, we use argparse::StoreTrue for the --version option.
The argument parsing will fail as the command is missing, but the
version variable will still be set to true if the version option was
set. So we ignore the parsing result and discard the argparse output if
the version variable is set.
[0] https://github.com/tailhook/rust-argparse/pull/50
|
|
|
|
|
|
|
|
|
| |
The split between the parse_arguments and the handle_arguments functions
is not really useful for reasoning about the code. In fact, it just adds
additional overhead in the form of complex function signatures into the
picture.
As it provides no real other value, this change merges the functionality
of both functions into a single one: handle_arguments.
|
|
|
|
|
|
|
|
|
| |
To be able to decide whether to print the argparse output depending on
the result of the argument parsing, this patch wraps stdout and stderr
in a BufWriter before invoking argparse. Our BufWriter implementation
only writes to the inner Write if the flush method is called. This
allows us to decide whether the buffered data should be written or
silently dropped.
|
|
|
|
|
|
|
|
|
|
| |
We have introduced the parse function to unify the common parsing
related tasks. In that vein, this change goes one step further and
adjusts the function to actually consume the ArgumentParser object used
by it.
All clients using this function actually do not access the parser
afterwards, and, in fact, some of them have to explicitly drop it
because of borrow conflicts with "referred" arguments.
|
|
|
|
|
| |
This patch changes the error handling in the args' module parse function
to use the Result's map_err instead of a more verbose if let expression.
|
|
|
|
|
|
|
|
| |
The factory reset only clears the slot status. The slot content is
overwritten with random data. Therefore accessing a PWS slot after a
factory reset returns garbage data. We fixed this by always querying
the status before accessing the PWS. This patch adds a corresponding
test case.
|
|
|
|
|
|
|
|
|
|
|
|
| |
The Nitrokey devices do not check whether a PWS slot is programmed
before accessing it (upstream issues [0] [1]). Until this is fixed in
the firmware, we have to manually check the slot status in pws get. This
could have been done in libnitrokey or the nitrokey crate, yet this
would lead to unnecessary commands if we check multiple fields of a slot
at the same time.
[0] https://github.com/Nitrokey/nitrokey-pro-firmware/issues/56
[1] https://github.com/Nitrokey/nitrokey-storage-firmware/issues/81
|
| |
|
|
|
|
|
|
| |
After performing the factory reset, we also build the AES key so that
the device is fully usable. Due to timing issue, we have to add a delay
between the factory reset and building the AES key.
|
|
|
|
|
|
|
|
| |
The -V/--version option prints the nitrocli version to stdout and exits.
In the future, it should also print the used libnitrokey version, but as
the required function is only available with nitrokey 0.3.2 and as the
current interface does not reflect the latest change in version naming,
I skipped that in this patch.
|
|
|
|
|
|
|
| |
The duplicate_associated_type_bindings lint seems to have been removed
with the Rust 1.32 release.
This change removes the lint from the program to prevent the newly
introduced warning from being emitted.
|
|
|
|
|
|
|
|
|
| |
This change adds tests for the lock command. For the Nitrokey Pro we
cannot test too much because the only side-effect is that the password
safe is closed and it will be opened automatically again by virtue of
our non-interactive testing methodology.
For Storage devices we verify that the encrypted volume is closed, which
is a documented side-effect.
|
|
|
|
|
|
|
| |
This change adds a test for the creation, opening, and closing of a
hidden subvolume. In order to support that in a non-interactive fashion,
we introduce and honor the NITROCLI_PASSWORD environment variable, that
prevents an interactive password query.
|
|
|
|
|
|
|
|
|
| |
With this change we implement the storage hidden subcommand. We support
creation, opening, and closing of hidden volumes.
Note that the opening of a hidden volume automatically closes any opened
encrypted volumes and vice versa. To that end, we force file system
level caches to disk even from the storage open and storage hidden open
commands.
|
|
|
|
|
|
|
|
|
| |
This change introduces a new subcommand to the storage command called
'hidden'. This subcommand can be used to interact with hidden volumes.
Right now we support three operations pertaining hidden volumes: create,
open, and close.
This patch merely provides the infrastructure for parsing the commands
and all their arguments, it does not yet implement them fully.
|
|
|
|
|
|
|
|
|
|
|
|
| |
With the required interface for secrets well defined, this change
introduces a second secret type in addition to PINs: passwords. Similar
to a PIN, a password can contain pretty arbitrary characters but
passwords can be retried repeatedly, whereas PINs cause a lockout after
a certain number of failed attempts.
Our first use case for passwords will be for hidden volumes. For those,
we do not want to gpg-agent to cache entries and so a password entry
indicates that it is not to be cached through the previously introduced
mechanism for optional caching.
|
|
|
|
|
|
|
|
| |
Another commonality between a password and a PIN is that they typically
both have a minimum length.
To accommodate for this requirement, this change introduces another
method to the SecretEntry trait that represents the secret's minimum
character length.
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Currently, when we enter a secret (i.e., a PIN) through the pinentry
module, this PIN will automatically be cached and not asked from the
user again on subsequent inquiries. However, caching may not always be
desired. For the upcoming support of passwords used in conjunction with
hidden volumes, we do not want any caching because different passwords
can be entered for different volumes and the user's intention is not
clear until a password has actually been entered.
To accommodate this use case, this change modifies the signature of the
SecretEntry trait's cache_id method to return an optional cache ID. If
none is returned, caching of the entered secret is disabled.
|
|
|
|
|
|
|
|
|
| |
We do not know what kind of data future implementers of the SecretEntry
trait may want to return. For all we know these could just be static
strings, in which case the forced conversion into a String by virtue of
the return type is wasteful.
To be more flexible in the future while gaining some consistency, this
change makes all those trait's methods return a Cow object instead.
|
|
|
|
|
|
|
|
|
|
|
|
| |
Now that we have introduced the notion of a secret abstracting over
whether something is a PIN or a password in terms of terminology, we
need to define what makes a secret in code. From the pinentry module's
perspective, the commonality between the two is that they both can be
entered through a dialog containing a prompt and a description, and they
can be cached.
This change introduces a trait, SecretEntry, that defines methods
representing those properties. Right now only the existing PinEntry
struct implements this trait.
|
|
|
|
|
|
|
|
|
|
|
|
| |
In the past we have worked solely with PINs. PINs in our (or rather, the
Nitrokey's) sense are not necessarily numbers but they can be reasonably
short in length, because they can only be retried a limited number of
times.
In the future, however, we will introduce the notion of a password,
which does not carry such a restriction.
The commonality between the two is that they are secrets and so with
this change we refer to secrets -- rather than PINs -- in places where
both passwords and PINs can conceptually be used.
|
|
|
|
|
|
| |
Various functions in the pinentry module contain an arguably redundant
'_pin' suffix in their name. Examples include inquire_pin and clear_pin.
This change removes this part from their names.
|
|
|
|
|
|
|
|
|
| |
The functionality we have in place for choosing a PIN can arguably be
moved into the pinentry module: it can be considered logic directly
related to working with PINs or secrets and that has no dependencies to
unrelated modules of the program.
This patch moves the choose_pin and check_pin functions into the
pinentry module.
|
|
|
|
|
|
|
|
| |
This change adds two tests for the storage command. The first one
verifies that a proper error message is emitted if a storage command is
attempted on a Pro device. The second one checks the output of the
status subcommand and expected changes to it when opening or closing the
encrypted volume.
|
|
|
|
|
| |
This change adds a set of tests for the pws command. Covered are all
subcommands with the most commonly used parameter combinations.
|
|
|
|
|
|
|
|
|
| |
The previous change to properly format the help text for optional
arguments left one thing out: parameters that are based on an Option as
opposed to an enum. The problem with those is that we cannot simply ask
the value (i.e., the Option) for all the variants of the inner type.
Instead, we have to reference the actual type of the inner enum in order
to retrieve all its possible variants.
|
|
|
|
|
|
|
| |
This change continues the effort of auto-generating more of the help
text content by extending the logic to optional arguments. We make use
of the fmt_enum macro to format the description of the argument with the
available variants (as well as the default, if any) interpolated.
|
|
|
|
|
|
|
|
|
| |
With the ability to fully generate the command enums we use for working
with the argparse crate, we can now take things one step further and
populate the contents of the help string we print for the user that
lists the available commands.
Doing so we also fix a bug where we forgot to mention the "storage
status" command in the help text.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Not too long ago we added a macro to auto generate the command enums and
the required trait implementations from a concise declarative
representation. This change extends this mechanism to the execute method
implementation that some of those enums provide.
When a tuple is specified as the "destination", e.g., here:
> Enum! {ConfigCommand, [
> Get => ("get", config_get),
> Set => ("set", config_set)
> ]}
the second component of this tuple will be interpreted as the function
to invoke when this variant used in the newly generated execute method.
|
|
|
|
|
|
| |
This change adds a set of tests for the config get and set commands. We
cover chosen valid parameter combinations and verify that they work as
expected as well as an invalid one.
|
|
|
|
|
|
|
| |
This change adds a set of tests for the otp command. We cover some
variants of the status, set, get, and clear. Testing all the possible
combinations is out of scope and so only a more or less arbitrary subset
of arguments was chosen.
|
|
|
|
|
|
|
|
|
|
|
| |
We exit the program using the process::exit function. This function just
exits the program directly, without any cleanup. That can be a problem
because IO buffers may not be flushed either. For a (typically) line
buffered entity like stdout that may result in data not terminated by a
newline symbol being not displayed properly.
This change explicitly flushes stdout before exiting the process to
alleviate this problem. Note that stderr output is unaffected, because
stderr is not buffered by design.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The application supports multiple devices both plugged in at the same
time as well as when used after the other. However, the GPG cache ID we
use for storing and retrieving the respective PIN is effectively a
constant. This constraint can cause problems when devices have different
PINs, as the PIN of a previously plugged in device may be reused for an
operation on a different one.
To resolve this problem this change adds the respective device's model
and serial number to the cache ID. As each serial number is supposed to
be different, this will ensure that the correct PIN is used for each
device. With this change we also show the model and serial number of the
currently used device in the pinentry dialog.
Note that because we do not store the serial numbers of all previously
plugged in devices, the pin clear command will only clear the PIN for
the currently plugged in device. If a user wants to make sure that a
cached PIN is cleared, the pin clear command should be invoked before
unplugging the device.
|
|
|
|
|
| |
The implementation of the fmt::Display trait for the PinType is
seemingly unused. Remove it.
|
|
|
|
|
|
| |
This patch implements From<&str> for Error so that we can use
Error::from(s) as a shorthand for Error::Error(s.to_string()). It also
replaces Error::Error with Error::from where possible.
|
|
|
|
|
|
|
|
|
| |
nitrokey 0.3.1 introduced the connect_model function that connects to a
specific model given by an enum variant and returns a DeviceWrapper.
This new function allows us to remove the manual selection of a
connection method from the get_device function. We only have to
implement From<DeviceModel> for nitrokey::Model to be able to convert
our model enum to nitrokey's model enum.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
In the past we have already taken a couple of steps to reduce the size
of the final binary, arguing that binary size is the metric of most
relevance for the program at hand:
- the memory footprint is close to irrelevant because the program does
not stay resident in memory for long
- execution speed is likely dominated by communication with the Nitrokey
itself, which is a slow I/O device
With that in mind, this change decreases the binary size further by
swapping the default allocator we use (typically jemalloc) with the
system allocator (which is malloc based on Unix systems). Given that we
are by no means allocation sensitive, there is no point in wasting
binary size on something that adds no value.
This change decreases the binary size by another 324 KiB (for an already
stripped release mode binary).
|
|
|
|
|
|
|
|
|
|
|
|
| |
So far we have taken all nitrokey::CommandError objects and put them in
formatted form into the Error::Error variant. What we really should do,
though, is to preserve the original error, with the additional context
provided by the caller, and report that up the stack directly. Doing so
has at least the benefit that we are able to check for expected errors
without hard coding the textual representation as maintained by the
nitrokey create.
This change refactors the code accordingly and adds two tests for such
expected error codes.
|
|
|
|
|
|
|
| |
Now that we have the infrastructure for non-interactive PIN supply in
place, we can add tests for commands that require the entry of a PIN.
To that end, this change adds tests for the pin set as well as pin
unblock commands.
|
|
|
|
|
|
|
|
|
|
|
| |
The second source of interactivity comes from the pin set and pin
unblock commands, which also inquire with the pinentry module to ask the
user for a PIN.
This change adjusts the two commands to honor the PINs as available in
the command execution context. It also updates the documentation
to reflect the availability of the newly introduced and honored
environment variables NITROCLI_ADMIN_PIN & NITROCLI_USER_PIN as well as
NITROCLI_NEW_ADMIN_PIN & NITROCLI_NEW_USER_PIN.
|
|
|
|
|
|
|
|
|
|
| |
The try_with_pin_and_data function is used by many commands to ask the
user for a PIN in an interactive manner. Because we do not want to have
any interactivity in our tests, we should honor the command execution's
admin & user PIN fields from this function, if set.
This change adjusts the function to honor the command execution
context's admin & user PIN, if set. In order to do so it also adjusts
the callers to hand through the context to begin with.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
In order to run tests fully non-interactively we need to avoid the need
for using the GPG agent's PIN entry and caching mechanism. To accomplish
that, we first need an alternate way to supply the PINs to use to the
program.
This change offers such a way by extending the execution context with
two fields representing the PINs that are populated by corresponding
environment variables, NITROCLI_ADMIN_PIN & NITROCLI_USER_PIN, if set.
While only two PINs are required right now, because the program allows
for the changing of each of the PINs, we also add two fields
representing new PINs. These latter two fields are populated by the
NITROCLI_NEW_ADMIN_PIN and NITROCLI_NEW_USER_PIN environment variables.
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
In the future we will need to perform a sequence of invocations of the
program for testing purposes, with each having a slightly different
execution context. Such a scheme does not map very well to the existing
design where we essentially just have a function invocation to run the
program. We would either have functions that produce a different
execution context or pass in the data to modify.
Neither of these approaches is appealing and so this change reworks the
code slightly. With it, we now can create a Nitrocli object, which
contains the data that diverges from the default execution context. This
data will eventually be modifiable by callers.
|
|
|
|
|
|
|
|
|
|
| |
The try_with_pin_and_data function is a fairly complex beast. Part of
that complexity stems from the returned Result value, whose error part
not only contains the error but also the previously passed in data. As
it turns out, though, this data as returned is never actually consumed
by any client.
Hence, this change simplifies the logic slightly by removing all the
additional complexity that this tuple return entailed.
|