-
Notifications
You must be signed in to change notification settings - Fork 795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Plugin interface and Nitrokey Webcrypt support #1567
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
Some high-level comments:
Other ideas:
- Merge
plugin
object into the global/local config structure. To discuss, whether this should be mixed, and passed deep down to cryptographic primitives.
Indeed I would move plugin
to config.plugin
, or maybe config.hooks
. That way, we don't have to pass two objects everywhere.
Further, I would propose that we make the arguments of each plugin function / hook into a single object, with descriptive property names. That way, it can be extended in the future more easily, if necessary. For example, we could have:
const hooks = {
generateKey({ algorithmName, curveName, rsaBits }) {
// ...
}
};
Further, the current sketch of that function returns a private key as well (of all zeros). But, in this case we shouldn't store a private key at all, instead it should probably be a GNU-dummy key. We have some support for that, but not for generating it, I think that will need to be added.
And similarly, the other functions currently take a private key, I assume that should be removed and replaced with a fingerprint, to reference the key on the device?
And finally, some more specific comments below 😊
Thinking about this a bit more, I think what I wrote before is not quite sufficient. The difficult part with this use case is that it's not just a hook that replaces existing functions, but the behavior for these is a bit different, since it's referring to keys on a device, instead of keys handled by OpenPGP.js. That means not just that we have to store the generated key as a dummy key, but also that when selecting keys to use, we should also select dummy keys, instead of ignoring them. So, I think this plugin should also be named a bit more specifically, for example |
Though actually - it seems like hardware keys are not stored exactly the same as dummy keys. https://lists.gnupg.org/pipermail/gnupg-users/2015-February/052747.html has some documentation. Currently, we only implement |
Thank you for the review! I got myself busy with something else, but now I would like to finish this PR asap. I have addressed some of the concerns, and minimized the patch. Further work:
Questions:
The general idea as far as I see is to generate the keys with the plugin field set once, and then allow the future operations on them without constantly setting it on use (as done right now for decryption and signing), but instead having it all defined in the global config object and use I am open for a video call if that would be faster for you - please contact me over |
Please review the latest changes. Would that s2k behavior work for you? |
Squashed commit of the following: commit 5957bab Author: Daniel Huigens <[email protected]> Date: Wed Oct 12 13:15:56 2022 +0200 Allow use of Brainpool curves by default (openpgpjs#1563) These curves have been merged back into the editor's draft of the crypto refresh.
@twiss Hey hey! Friendly ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋 Sorry for the delay! I started writing a response but didn't post it and forgot about it 🙃
Overall, I think the structure is going in the right direction. However, I think the API still needs a bit more thought and care, as this will essentially become part of our public API, and can't actually be changed that easily later (as we'd also have to change the WebCrypt OpenPGP.js plugin which will live in a separate repo).
I've left some more concrete comments below. But, please also spend some time yourself thinking about whether this API will keep working for you in the future (other algorithms, other devices, etc).
Also, on a much more nitpicky note, please use camel case everywhere, and generally please try to match the style of the surrounding code 😊
* Return the creation date of the keys | ||
* @returns {Date} The keys creation date | ||
*/ | ||
date() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a date
parameter, do we need this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await openpgp.generateKey({
curve: 'p256',
userIDs: [{ name: 'Jon Smith', email: '[email protected]' }],
format: 'object',
date: plugin.date(),
config: { hardwareKeys: plugin }
});
I was wondering here, if providing date
function in the hardwareKeys
object would not allow to skip it in the list parameter, which potentially could be less error prone usage wise.
Perhaps I could check that anyway by testing dynamically the member existence, which solves the problem.
Also, as you mentioned elsewhere, technical details should not be exposed hence this will be removed from the hardwareKeys
API.
* }>} Generated signature, 32 bytes in each field | ||
* @async | ||
*/ | ||
async sign({ oid, hashAlgo, data, Q, d, hashed }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This API seems designed only for ECDSA? To make it generic, I would accept
algorithmName, curveName
as below. - The return object's properties might need to depend on the algorithm and curve, so you could just say that it returns an onbject.
- Internally, we allow the function to use either the data+hashAlgo or the hash, in case there's an integrated hash+sign operation (such as in Web Crypto). However, here I assume you're always signing the hash?
- And again, descriptive parameter names would be nicer, and I think "recipient" in the JSDoc doesn't apply here - I guess this was copied from somewhere?
So I would do:
async sign({ oid, hashAlgo, data, Q, d, hashed }) { | |
async sign({ algorithmName, curveName, publicKey, privateKey, hash }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points - will do. I am not testing this with the RSA yet, but planning so in the future.
* The secret key material is not present in the result. Instead, the IV field contains the serial number of the device, | ||
* to which the secret key material processing is delegated to. The privateKey field is returned for the backwards | ||
* compatibility. | ||
* | ||
* @param {Object} obj - An object argument for destructuring | ||
* @param {enums.publicKey} obj.algorithmName - Type of the algorithm | ||
* @param {string} obj.curveName - Curve name | ||
* @param {number} obj.rsaBits - RSA key length in bits | ||
* @returns {Promise<{ | ||
* publicKey: Uint8Array, | ||
* privateKey: Uint8Array | ||
* }>} Generated key material |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't return a dummy privateKey property here, since it's obviously meant to stay on the device - so I would just return the public key only.
if (hardwareKeys_with_data) { | ||
hardwareKeys_with_data.algo = algo; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of setting the algorithm on this object here, please just pass the algorithm name below. (Also, algo
is a number here, it would probably be nicer to pass a string to the plugin. Otherwise, the parameter should be called algorithm
instead of algorithmName
.)
@@ -276,17 +281,17 @@ export function generateParams(algo, bits, oid) { | |||
})); | |||
} | |||
case enums.publicKey.ecdsa: | |||
return publicKey.elliptic.generate(oid).then(({ oid, Q, secret }) => ({ | |||
return publicKey.elliptic.generate(oid, hardwareKeys_with_data).then(({ oid, Q, secret }) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can be hardcoded here, e.g.
return publicKey.elliptic.generate(oid, hardwareKeys_with_data).then(({ oid, Q, secret }) => ({ | |
return publicKey.elliptic.generate('ecdsa', oid, config).then(({ oid, Q, secret }) => ({ |
etc.
Or, if you prefer passing the algorithm enum value, you can just pass algo
here, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer passing around enums
whenever possible (following up to #1410)
Hello 👋 Just a heads up, #1597 will cause some merge conflicts with this PR, since it refactors the S2K code. So we'll need to rebase this PR, if it's merged after that one. I don't necessarily want to burden you with that, we can rebase this PR; but if you did some work on it in the past three weeks, it might be good to push it here first, so that we can (either merge it if it's finished - but no rush, ofc - and then rebase #1597, or) rebase the code in this PR after merging that one 😊 |
Change names to more descriptive. Return only the shared key.
Hey, thank you for the tip! About the ready code, I've followed some style code advises, and will update this PR's branch in a moment. |
PartialConfig have it optional Co-authored-by: larabr <[email protected]>
Regarding the CI error:
Would you like me to merge in the current |
Use defaultConfig as the default value, instead of null.
I think the issue might actually be that since you added (Although, as you already said, it should probably be removed before merging, and then the package-lock.json should be reverted as well, but you can update it for now for testing indeed.) |
I've just run |
Thanks for working on this @szszszsz, it'll open up interesting possibilities! Sorry to chime in only now, I would like to have your opinion on a couple of points. First off, with the current implementation, if Secondly, I was wondering if a higher-level interface would work for you? I am thinking of the possibility of having the lib call out to |
Hi!
This is a working draft implementation of the plugin system loosely described at Nitrokey#1 . Please take a look at the below description of the details, as well as the usage example.
Instead of a global plugin object I've decided to pass it through calls, to minimize potential state-related issues.
Requesting for the review and pointers.
Notes:
To do:
Other ideas:
plugin
object into the global/local config structure. To discuss, whether this should be mixed, and passed deep down to cryptographic primitives.A plugin interface for the private keys' operations is available, which allows to increase the security by storing the
private key on another medium than browser or PC, thus providing much greater, if not complete protection from the
secret material leaks. Additionally, this can increase usage control by introducing user confirmation
(e.g. through a touch button) for the private key operations.
The plugin interface is realized by a plugin object containing callbacks to the defined private key operations: sign, decrypt,
agree, generateKeyPair. The detailed description of each is provided in the Readme example . The plugin object is then passed
to all related main OpenPGPjs operations. If that's omitted, the standard OpenPGPjs behavior will be executed.
In the following implementation the host of the private key operations will be a Nitrokey WebCrypt device
(e.g. Nitrokey 3). Nitrokey WebCrypt uses Webauthn standard to create a communication channel with the security key
handling it, which by design should allow to run it on any modern browser and platform.
At the moment it is tested only in the Chrome-based browsers. Support for the Node.js requires additional implementation.
More details at: