-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Dormant: Implement PGP encrypt and PGP decrypt operations #89
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.
This looks really useful. One general comment:
The openpgp.js library is quite large (321KB even once minified), so if we're going to include it and make everyone download it, we need to make sure it's pulling its weight. Can we create more operations using its capabilities, for example signing, verifying, creating new keys and anything else it supports?
src/js/config/Categories.js
Outdated
@@ -92,6 +92,8 @@ var Categories = [ | |||
"Substitute", | |||
"Derive PBKDF2 key", | |||
"Derive EVP key", | |||
"PGP Encrypt", | |||
"PGP Decrypt", |
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.
Add these to the 'Public Key' category as well.
src/js/config/OperationConfig.js
Outdated
}, | ||
{ | ||
name: "Private key password", | ||
type: "text", |
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 think this would suit the string
type better. It will then display a single-line input box instead of a full textarea.
src/js/operations/PGP.js
Outdated
* Encrypts the input using PGP. | ||
* | ||
* @param {string} input - plaintext to encrypt | ||
* @param {function} args |
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.
The type here should be Object []
.
src/js/operations/PGP.js
Outdated
// Timeout is so that UI can update before openpgp blocks thread | ||
openpgp.encrypt(options) | ||
.then(function(ciphertext) { | ||
console.log(ciphertext); |
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.
Extraneous console.log
should be removed.
src/js/operations/PGP.js
Outdated
reject("Failed to read public key", error); | ||
} | ||
|
||
// Timeout is so that UI can update before openpgp blocks thread |
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 comment feels out of place. Either include more context or put it where the timeout actually takes place.
src/js/operations/PGP.js
Outdated
* Decrypts the input using PGP. | ||
* | ||
* @param {string} input - ciphertext to decrypt | ||
* @param {function} args |
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.
Type should be Object []
.
src/js/views/html/HTMLApp.js
Outdated
}); | ||
// This timeout is so the UI has time to update the baking status | ||
// before any blocking operations delay it until after the operation. | ||
setTimeout(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.
I understand why you've done this, but I really don't like the idea of adding 10ms of latency to every single bake. It's possible we could just reduce the timeout to 0ms and it would still allow the UI to update due to timeouts allowing the execution of anything left in the processing queue first (see this SO comment). However this is still not ideal. I suspect the answer is web workers, which we've discussed before. If the 0ms trick works, I can accept this for now and we'll engineer a more robust solution in the future. If the 0ms trick does not work, we'll have to think of a different solution before this is merged into master.
Thanks for catching all the careless errors above, I will be more scrupulous from now on. I completely agree about the I hadn't noticed the size of OpenPGP.js but I agree it would be a waste to not include all possible functionality if we are "paying" for it anyway. I had some questions about the new functionality before I pushed my changes: 1. Output format of "PGP Verify".I was going to go with the following (when verification succeeds)
and (when it fails)
This seems consistent with other text producing operations: e.g. Extract email addresses (displaying a total as the first line and then the results afterwards). 2. Exclusion of methods for looking up and uploading to key server.I am under the impression that we do not want (at present) to include any operations involving network connectivity (for security, portability, reproducibility, etc). I think if what you are alluding to in #79 gets implemented then perhaps a module system or similar could be built and there could be a "networked operations" module which would include such things. Right now I think it is best left for another day. 3. Input and output format of "PGP Generate".For input I am currently using the arguments:
For output I have currently implemented
This operation is quite odd because it completely ignores any input and seems to just be providing a convenience (which is fine). I wondered if you had any insight in if it should take arguments. E.g. Password is input. Further at-length discussion of this topic may lead into the "pipe output as argument" rabbit-hole which this PR is probably not the place for. 4. Non-Armored operations.Currently all operations deal with Armored-strings instead of bytes, which (in my limited experience), seems to be the preferred format for all things PGP. Instead of having multiple operations (for armored output and raw output) I was going to attempt to create two operations:
And
Although I think the jury is out on whether people actually do use non-Armored, you and your colleagues probably know more than I do. There is probably an additional debate whether to use "Armored" or "Armoured" too :) |
I do think Web Workers are a good idea, but like you say, there are a number of big changes going through at the moment so let's wait until we're a bit more stable before we introduce another major architectural redesign. In answer to your questions: 1. Output format for "PGP Verify".Are you suggesting that this operation will also decrypt the message? I would personally leave that to the "PGP Decrypt" op and just use this one to list something along these lines:
If it's not valid, you could display the hash that actually gets produced as well as the above information. 2. Exclusion of methods for looking up and uploading to key server.Yes, you're absolutely correct that we don't want anything like this at the moment. Perhaps at some point in the future, but for now we can leave it out. 3. Input and output format of "PGP Generate".The arguments and output you have suggested look ideal. There is currently one other operation that also completely ignores the input: Generate UUID. I'm fine with this model if it makes sense, which I think it does in this case. It would be more confusing to the user to expect them to know to enter the password in the input or have to read the description to find out how to use the operation. Piping output into arguments is another thing that we'll tackle at some point. It's definitely a really good idea, but we'll need to think carefully about the best way to design it. There is an issue for it here: #26. 4. Non-Armored operations.Armoured strings should be the default but, as you've suggested, it might be nice to offer the ability to change the input and output formats to also support raw byte arrays and hex. I would prefer that this be an Regarding the spelling, it doesn't bother me a huge amount, but considering that the rest of CyberChef uses British English spelling (see "Favourites" and "Parse colour code" for example), it makes sense to stick to that convention and use "Armour". Perhaps at some point we can offer language packs ;) |
Added: + PGP Sign cleartext + PGP Verify cleartext + PGP Add ASCII Armor + PGP Remove ASCII Armor + Many tests for all operations Updated: + PGP Encrypt (formatting of error messages) + PGP Decrypt (^^) + PGP Sign (this operation is now exclusively for non-clearsigned) + PGP Verify (^^)
Rarely, if ever, used. It is now trivial to generate and test in the actual web app.
Just updating this with my progress after the most recent commit (still a WIP). I separated out Sign / Sign Cleartext and Verify / Verify Cleartext because I think they are sufficiently different both in terms of the function they provide and from a usability perspective. I've operations are currently as follows: PGP EncryptArguments:
Outputs: PGP DecryptArguments:
Outputs: PGP SignArguments:
Outputs: PGP VerifyArguments:
Outputs:
PGP Sign CleartextArguments:
Outputs:
PGP Verify CleartextArguments:
Outputs:
PGP Generate Key PairUnchanged from above PGP Add ASCII ArmorArguments:
Outputs:
Currently clearsigned messages don't work (see PGP Remove ASCII Armor). PGP Remove ASCII ArmorArguments:
The PGP Armored input can currently be anything in PGP Armor:
Outputs: Detached signaturesCurrently detached signatures are not supported. I'm not sure how they would fit in with the current operations:
I'm leaning towards the latter even though we have so many operations already. It is a better representation of the "detached nature" of detached signatures. If I was to implement this I would add a download link for each of the files (which I think should be done anyway). This method has the advantage operation having an "opposite number" as it were:
And all the operations above would have no optional arguments that may be confusing. Summary of todo list
|
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.
Progress is looking really good. My previous comments about not displaying the decrypted message when you run Verify can probably be ignored. It's clearer now that Sign and Verify are just authenticated versions of Encrypt and Decrypt respectively. I was thinking of them more as helper functions that you could use on top of Encrypt and Decrypt.
Nearly there!
src/js/config/Categories.js
Outdated
"PGP Verify Cleartext", | ||
"PGP Generate Key Pair", | ||
"PGP Add ASCII Armor", | ||
"PGP Remove ASCII Armor", |
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.
Change these to 'Sign PGP Cleartext', 'Verify PGP Cleartext', 'Generate PGP Key Pair', 'Add PGP ASCII Armour' and 'Remove PGP ASCII Armour'.
src/js/operations/PGP.js
Outdated
* | ||
* @param {string} input - signed input to verify | ||
* @param {Object[]} args | ||
* @returns {string} - "true" or "false" depending on the validity of the signature |
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 now returns more than just "true" or "false"
src/js/operations/PGP.js
Outdated
* | ||
* @param {string} input - signed input to verify | ||
* @param {Object[]} args | ||
* @returns {string} - "true" or "false" depending on the validity of the signature |
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 also returns more than "true" or "false"
+ "PGP Sign Cleartext" -> "Sign PGP Cleartext" + "PGP Verify Cleartext" -> "Verify PGP Cleartext" + "PGP Generate Key Pair" -> "Generate PGP Key Pair" + "PGP Add ASCII Armor" -> "Add PGP ASCII Armor" + "PGP Remove ASCII Armor" -> "Remove PGP ASCII Armor"
This operation is like "Remove PGP ASCII Armor" but for detached signatures. It outputs files as HTML similar to "Sign PGP Detached".
I think it's time to merge this into Next steps:
|
Okay, sounds good. When I fetch and merge I'll remove the |
Just an update on where I am right now. I started working on merging upstream changes (node/webpack/babel) a while ago although I haven't had much time. I have run into a couple of issues:
And the If anyone has encountered this before (or similar situations) I'd love to know how you resolved it. Additionally thanks to @graingert I have realised that a lot of the code involving promises that I have written is subpar/redundant. I will rewrite that code after merging the upstream node changes, then we can get this merged (finally). @n1474335 sorry if this is holding you up! |
Hey @tlwr, cheers for the update. Don't worry about holding anything up, I have a long list of other things to work on in the meantime! If you commit what you've got so far, I'll see if I can fix your tests. The import problems can potentially be fixed using the |
Committed where I got up to with integrating my work with your work on node/babel/webpack. I've also taken the liberty of rewriting my old code using promises to use best practices. More suboptimal stuff remains but as it is not related to PGP it is probably outside the purview of this PR. I did see the |
src/core/operations/PGP.js
Outdated
return ciphertext.data; | ||
}) | ||
.catch(function(err) { | ||
throw "Could not encrypt text: " + err; |
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.
It's best to keep the original exception so that we don't lose the stacktrace
.catch(function(err) { | ||
console.error("Chef's promise was rejected, this should never occur"); | ||
}); | ||
}, 10); |
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 set timeout will throw away any exceptions, which should be caught at 155
Surely it would be better to explicitly wait for the ui to update?
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 believe @n1474335 doesn't plan on keeping this in.
There is some discussion above about this problem. A possible solution is web-workers which @n1474335 wants to work on at a later date, I think your input could be quite valuable here (there is almost certainly something that we have missed).
The catch statement referenced here should never be called. The throwing away of the stack trace occurs in Recipe.js. Recipe.js
definitely needs some TLC (on my todo) (both on my bad promises code front, and operationally (manging of stack traces into CyberChef UI focused error messages)).
It is probably prudent, now that the UI is being decoupled from the Chef/Recipe/Operation logic, that we change how we handle errors and stack traces (to work better with node).
I.e. we preserve a stack trace (and operation metadata) and handle the error message formatting (from the actual trace) in App.js
@@ -54,6 +54,11 @@ import Chef from "../src/core/Chef.js"; | |||
output: null, | |||
}; | |||
|
|||
// Remove whitespace helper | |||
var noWS = function(string) { |
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.
Best to avoid the var keyword.
Instead of a comment use:
function removeWhitespace(string) {
...
}
test/tests/operations/PGP.js
Outdated
|
||
var CYBERCHEF_GENERATED_KEY_PAIRS = [ | ||
{ | ||
name: "CyberChef 1", |
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.
You can put all these in a .json file and import it.
Also it's that var again, use const
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.
The test runner (as-is) is very hacky. It was originally built when we could only run operations in the browser. There was an entire process involving node
calling phantomJS
running the actual CyberChef operations, relying upon stdout and stderr piped from phantomJS
to node
and the test runner reporting results correctly. As expected this was a bodge and very fragile at best. (It has moved on a bit since then but is still quite bad)
It will be a good day when we remove all of our home-grown test running infrastructure and use proper test runner. This is now feasible because of @n1474335's work with node. Now we can do something like:
import * as CyberChef from "CyberChef";
CyberChef.bake("input", recipeConfig)
.then(x => console.log(x));
Instead of the TestRegister
that is inflicted upon us currently.
The future of this file (once the test runner is refactored) is not storing key pairs in a JSON file. Generating key pairs is an operation that needs to be tested anyway. We should just generate key pairs and use those for other tests.
Again (as above) this is another area where your JS knowledge could prove useful: picking and implementing a test runner from one of the many available in the ecosystem.
[PGP_TEST_KEY_PAIRS[0], PGP_TEST_KEY_PAIRS[1]], | ||
[PGP_TEST_KEY_PAIRS[1], PGP_TEST_KEY_PAIRS[0]], | ||
].forEach(function(pairOfKeyPairs) { | ||
var alice = pairOfKeyPairs[0]; |
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.
Use const
src/core/operations/PGP.js
Outdated
|
||
return openpgp.encrypt(options) | ||
.then(function(ciphertext) { | ||
return ciphertext.data; |
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.
Use arrows: .then(c => c.data)
You should rebase and clean up your commits |
I've cleaned up I also had to increment the Still banging my head against openpgp + webpack. It definitely feels like something trivial that I am overlooking. |
I've just spent about 5 hours trying to sort out this bug. I've got to the stage where it successfully compiles, but the test suite then hangs, even if I comment out all the PGP tests. It could be something to do with the changes you've made to
So in short: Gruntfile.js
resolve: {
alias: {
- jquery: "jquery/src/jquery"
+ openpgp: "openpgp/src/index"
}
},
package.json
"dependencies": {
+ "asmcrypto-lite": "^1.1.0",
+ "es6-promise": "^4.1.0",
+ "rusha": "^0.8.5", As I said, once this is done the tests will still hang, so there's clearly something else wrong as well but I think I'll call it a night. |
For node you need to configure externals or it will prevent require working
…On 16 Apr 2017 02:15, "n1474335" ***@***.***> wrote:
@tlwr <https://github.com/tlwr>
I've just spent about 5 hours trying to sort out this bug. I've got to the
stage where it successfully compiles, but the test suite then hangs, even
if I comment out all the PGP tests. It could be something to do with the
changes you've made to Recipe.js, although it seems to work fine in the
browser... Either way, I haven't investigated any further. I don't really
want to push my "fix" as it isn't actually complete yet. I'll explain the
steps I've gone through in the hopes it will help someone else fix it
properly.
1.
The cause of the return window.document; bug is jQuery. When I set up
webpack in #95 <#95> I included
a resolve alias for jquery (jquery: "jquery/src/jquery") so that it
would import the uncompiled version. This is recommended as it means
webpack can more easily deduplicate common modules elsewhere in the
project. However, if you remove this resolve alias, it no longer loads the
offending code (node_modules/jquery/src/var/document.js) as its own
module, so it doesn't get executed. I still haven't worked out quite why
it's only causing a problem now when it has been there ever since I pushed
#95 <#95>.
2.
Once this bug is fixed, you'll hit another one complaining that it
can't load the crypto module. This is caused by webpack overriding the
require keyword using variable injection when it inlines the openpgp
module. When OpenPGP detects that it's running in a node environment rather
than a browser, it tries to load native modules like crypto using
require and fails because require is now undefined. This is what
really stumps me. I can't work out why webpack is overriding require
and I can't find a way to stop it. When the webpack target is set to
node, it should allow modules to use require
<https://webpack.js.org/configuration/target/#target>. The only
solution I have is to import the source version of OpenPGP rather than the
compiled distribution version (ironically the exact opposite of the
solution/workaround for the previous bug). The source version is all
written using ES6 modules and doesn't include the stub at the top which
tries to detect which module system you're using (CommonJS, AMD, ES6 etc.)
and then load your modules for you. With this stub missing, it loads
successfully. You will need to npm install --save asmcrypto-lite rusha
es6-promise for the uncompiled version to work.
So in short:
Gruntfile.js
resolve: {
alias: {- jquery: "jquery/src/jquery"+ openpgp: "openpgp/src/index"
}
},
package.json
"dependencies": {+ "asmcrypto-lite": "^1.1.0",+ "es6-promise": "^4.1.0",+ "rusha": "^0.8.5",
As I said, once this is done the tests will still hang, so there's clearly
something else wrong as well but I think I'll call it a night.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#89 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZQTAbqrngL3qn1yWW6_nju9j9AcUotks5rwWu-gaJpZM4MV1SY>
.
|
@graingert Unfortunately that doesn't seem to work, webpack still overrides /* 604 */
/***/ (function(module, exports, __webpack_require__) {
/* WEBPACK VAR INJECTION */(function($) {var require;var require;(function(f)... Where |
It shouldn't even emit any contents of that module
On 16 Apr 2017 12:41, "n1474335" <notifications@github.com> wrote:
@graingert <https://github.com/graingert> Unfortunately that doesn't seem
to work, webpack still overrides require for the openpgp module.
/* 604 *//***/ (function(module, exports, __webpack_require__) {
/* WEBPACK VAR INJECTION */(function($) {var require;var require;(function(f)...
Where (frunction(f)... is the beginning of openpgp.js.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#89 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZQTLF_JL8m-LIUbL3iGT93YWKxe_Eoks5rwf5egaJpZM4MV1SY>
.
|
When in node mode
…On 16 Apr 2017 12:50, "Thomas Grainger" ***@***.***> wrote:
It shouldn't even emit any contents of that module
On 16 Apr 2017 12:41, "n1474335" ***@***.***> wrote:
@graingert <https://github.com/graingert> Unfortunately that doesn't seem
to work, webpack still overrides require for the openpgp module.
/* 604 *//***/ (function(module, exports, __webpack_require__) {
/* WEBPACK VAR INJECTION */(function($) {var require;var require;(function(f)...
Where (frunction(f)... is the beginning of openpgp.js.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#89 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZQTLF_JL8m-LIUbL3iGT93YWKxe_Eoks5rwf5egaJpZM4MV1SY>
.
|
@n1474335 your effort sounds pretty heroic. I've implemented your suggested changes and got the same results as you: progress! I'm curious what Also if you comment out the FlowControl tests do the tests still hang? (I believe |
Closing, will reopen with a cleaned up and separate PR (just for the PGP operations) once #117 is merged. |
This is for #6, implemented using Async and OpenPGP.js.
It adds two operations:
And 6 tests
The test file includes 4 key pairs which were generated for the sole purpose of testing CyberChef's PGP operations.
(A project for the future would be test skipping so we don't have to comment out long running tests).
Screenshots (4096 RSA):


I added a rather hacky
setTimeout
beforeapp.chef.bake
(inHTMLApp.js
) so the UI has time to update the baking status. I think it is sufficiently hacky that a less bodged solution should be found in the future but for now it seems to suffice.