feat: PKCS11 PIN per request#3809
Conversation
Robot Results
|
b265cc2 to
5e8aa94
Compare
This version of secrecy is already used by cryptoki, so we don't actually pull anything more, but need to declare it because cryptoki doesn't reexport it. Current version of secrecy is 0.10.0, but cryptoki doesn't use it yet, so using this old version to not to have to pull in 2 versions. Can upgrade once cryptoki does as well. Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
Parse "pin-value" query attribute if it's present in a PKCS 11 URI. RFC7512 specifies a "pin-value" query attribute that can be used to provide the PIN value, which we can use to pass it in the request. Because it is sensitive, there are security considerations, of interest to us being this part of the RFC: > Section 7.5 of [RFC3986] applies since a PKCS #11 URI may be used in > world-readable command-line arguments to run applications, stored in > public configuration files, or otherwise used in clear text. For > that reason, the "pin-value" attribute should only be used if the URI > string itself is protected with the same level of security as the > token PIN itself otherwise is. In our case, the challenge is in not showing up these values in the logs. On tedge-p11-server side as soon as we're parsing URI we can put in in a secrecy wrapper, to not be able to print it from then on, but there's nothing preventing printing the value before it's parsed on the server, or on the client making the request. Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
Allows the user to provide a PIN value to be used when logging into PKCS11 token as a query attribute in the URI, e.g. when the following value is set as `device.key_uri`: pkcs11:token=mytoken;object=mykey?pin-value=my-pin tedge-p11-server will attempt to login to the token using `my-pin` instead of the default PIN tedge-p11-server was started with. Because it is sensitive, there are security considerations, of interest to us being this part of the RFC: > Section 7.5 of [RFC3986] applies since a PKCS #11 URI may be used in > world-readable command-line arguments to run applications, stored in > public configuration files, or otherwise used in clear text. For > that reason, the "pin-value" attribute should only be used if the URI > string itself is protected with the same level of security as the > token PIN itself otherwise is. In our case, the challenge is in not showing up these values in the logs. On tedge-p11-server side as soon as we're parsing URI we can put in in a secrecy wrapper, to not be able to print it from then on, but there's nothing preventing printing the value before it's parsed on the server, or on the client making the request. Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
5e8aa94 to
3c5f0a4
Compare
3c5f0a4 to
8fc58a9
Compare
| /// A secret string that should not be printed by accident. | ||
| /// | ||
| /// Rolling our own type because `secrecy::SecretString` doesn't impl Serialize, | ||
| /// and we don't need eager zeroizing, we only need to make sure not to print | ||
| /// the value. | ||
| #[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct SecretString(String); |
There was a problem hiding this comment.
I don't fully get the motivation here.
- Something that can be serialized is no more something that cannot be printed.
- A
SecretStringcan be exposed as a string withexpose() - A
SecretStringcan be cast into asecrecy::SecretStringwhich can then be exposed withexpose_secret(). - The secret is stored in
tedge.tomlas clear-text. And is displayed bytedge config get
This sounds overly complicated. Why no simply use String? Or secrecy::SecretString with appropriate can to expose_secret() when sending a request over the unix socket?
There was a problem hiding this comment.
- Something that can be serialized is no more something that cannot be printed.
We just don't want to log it accidentally, i.e. when logging the request with "{:?}" we'd like to see other fields but have PIN be redacted. When serializing, it's only done in connection.rs and when calling .serialize(), one has to provide a serializer, so it's not going to be accidental.
- The secret is stored in
tedge.tomlas clear-text. And is displayed bytedge config get
Indeed config may contain secrets (we also have cryptoki.pin in tedge.toml), but we still don't want to have any secrets in the logs if we can help it (similarly to how cryptoki.pin is redacted when starting tedge-p11-server). Having pin as clear text in tedge.toml is probably not ideal and we could handle it like private keys, i.e. secret in a separate file and path to that file in tedge.toml, which we can always do in the future.
Why no simply use String
Requests and Responses implement Debug and can be logged, so when using String we'd log the pin value in the logs
Or secrecy::SecretString with appropriate call to expose_secret() when sending a request over the unix socket?
We're serializing the entire request to write it to the socket, to call expose_secret() one would have to write impl Serialize manually, which we're currently deriving.
There was a problem hiding this comment.
we could handle it like private keys, i.e. secret in a separate file and path to that file in
tedge.toml, which we can always do in the future.
Why not in this PR? This would makes more sense.
By the way, this is also what is done for c8y.credentials_path.
We just don't want to log it accidentally, i.e. when logging the request with "{:?}" we'd like to see other fields but have PIN be redacted.
I appreciate that. However, I feel the current implementation is over complicated. Do we need the secrecy crate as we have to work around it? Why not simply keep this SecretString wrapper with redacting Display and Debug implementations?
There was a problem hiding this comment.
Removed secrecy and replaced with our own SuperString everywhere, in dca30ef.
Why not in this PR? This would makes more sense.
I think then we'd have to add an option for cryptoki.pin_path as well, so both pin options support it, point users to use this new option and keep the old cryptoki.pin_path for backwards compatibility but say it's not recommended, and then in this PR I could change key_pin to key_pin_path. I could do the cryptoki.pin_path changes in a separate PR for readability.
There was a problem hiding this comment.
As discussed offline, it has been decided to keep with storing the pin within the tedge.toml. The user can still protect this file by changing the permissions so that it is only readable by the tedge user (e.g. chmod 600 /etc/tedge/tedge.toml).
63857df to
8d09ec7
Compare
|
|
||
| // NOTE: combining URI behaviour seems unintuitive and surprising. If a client asks for a key on `token2`, | ||
| // but the server only allows accessing `token1` then it probably should receive a KeyNotFound instead of | ||
| // getting a signature from key on `token1` and then NotAuthorized because the signature is wrong. |
There was a problem hiding this comment.
Maybe this doc comment is trying to say too many things, notably when displayed by tedge config list --doc. Yes PKCS#11 is subtel, but I seems to me that a proper doc would do better that a command helper.
There was a problem hiding this comment.
This is a regular comment, not a doc comment, so it's not displayed in tedge config list --doc. It's related to the behaviour explained in the doc comment, so decided to include it below.
There was a problem hiding this comment.
Unless you don't really mean the NOTE, but also the entire doc comment above talking about how the URIs are combined?
In this case I'd agree that if every option had descriptions of this size, then yeah tedge config list --doc output would be a bit large, but still it's nice to have longer descriptions for the options, explaining their behaviour and also potentially interactions with other options, directly in tedge-config instead of only doing that in Markdown documentation.
My preferred solution would be to either tweak a bit the format of tedge config list --doc such that these longer descriptions look better.
There was a problem hiding this comment.
My preferred solution would be to either tweak a bit the format of
tedge config list --docsuch that these longer descriptions look better.
We can see that later.
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
Expand documentation and configuration setting descriptions how PIN and URIs option can be overridden on a per-key basis. Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
dca30ef to
1f19693
Compare
TODO
documentationexpanded a bit on what the options are doing and what happens when bothcryptoki.pinanddevice.key_pinare setProposed changes
Allows the user to provide a PKCS#11 User PIN value to be used when logging into PKCS11 token using a new
device.key_pinconfig setting.Additionally, pin also may be provided in
device.key_uriusing a standard PKCS#11 URIpin-valuequery attribute:tedge-p11-server will then attempt to login to the token using
my-pininstead of the default PIN tedge-p11-server was started with.When multiple pin values are present, we use: URI -> pin field -> provider pin.
Because it is sensitive, there are security considerations, of interest to us being this part of the RFC7512:
When using the
pin-valueattribute, because URIs are just strings, parts of the code that just pass these URIs may log them, potentially exposing secrets in the logs. On tedge-p11-server side as soon as we're parsing URI we can put in in a secrecy wrapper, to not be able to print it from then on, but there's nothing preventing printing the value before it's parsed on the server, or on the client making the request. For this reason, usingpin-valueis not recommended, although it's supported when settingdevice.key_pinis for some reason not possible, aspin-valueis a standardised attribute so there's no reason for socket or module provider to not support it.When using
device.key_uriconfig setting, the value of the secret appears intedge.toml, but after reading it will not be printed in the logs.Types of changes
Paste Link to the issue
#3716
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments