added decryption before task json validation #363
Conversation
|
Build finished. |
yyscamper
left a comment
yyscamper
left a comment
There was a problem hiding this comment.
Same comment with RackHD/on-core#220, the validator is not the reasonable place to put decrypt logic.
DavidjohnBlodgett
force-pushed
the
decryptHotfix
branch
from
70e2c05 to
ac69eb8
Compare
|
@yyscamper Ok Felix, This new code pulls out the decryption from the JSON validator as you asked. Let me know if you have any other concerns! |
|
BUILD on-tasks #1707 : UNSTABLE
|
|
test this please |
|
BUILD on-tasks #1714 : UNSTABLE
|
|
test this please |
|
BUILD on-tasks #1720 : UNSTABLE
|
|
@yyscamper Did @DavidjohnBlodgett address your concerns? Still working through the Jenkins failures, but let us know if you are good from the code change perspective. |
|
@DavidjohnBlodgett @pscharla : With your new code change, your decrypt operation only be triggered if the taskSchema is defined, but you know the taskSchema is optional, so the options will also be encrypted if no schema is defined. Of course, if only for the issue you are trying to resolve, it fails at schema validation, so you change did let the schema validation get rid of such problem, but other functionality will still see the encrypted options as well. Meanwhile, I think the decryption is not the responsibility of task or even task-graph, since we encrypted the data before storing to database, so the code which fetch the encrypted data from database should do the decryption as well. Just like what we did for obm. I spent some time to understand our current design, in my opinion, the best place is https://github.com/RackHD/on-core/blob/master/lib/workflow/stores/mongo.js#L374 But I see the code has already do decryption for task data, I just wondering why the option is still be encrypted?! Is it a bug for |
|
@DavidjohnBlodgett : Today, when I debugged on this issue https://github.com/RackHD/on-taskgraph/issues/179, it also has the same root case you have already identified.But when I try to go deeper on this problem, I found the problem is much more worse than I thought. Every time, when the model calls findAndModifyMongo (https://github.com/RackHD/on-core/blob/master/lib/common/model.js#L201), the data will be encrypted, but there is a lot places the data is not decrypted when query data from database. The whole design of encryption and decryption need be refined. But as a quick solution for some particular problem, your PR can be OK after fix following problem:
|
|
@yyscamper @benbp @brianparry @anhou Please do not merge, drafting a new PR to resolve the concerns. Hey Felix, thanks for the feedback and also looking into that other ticket, sorry about the late response... So I think we're both in alignment, I had some reservations about the impact this encryption/decryption design would have on our code base when I first pushed the PR into master, and I think the issues we are seeing warrant a second look at exactly what we're trying to achieve... Some of my motivation was based around a belief that we had not been encrypting to disk, however, post merging the original PR... I have discovered this to not be the case. I think, I will iron out a new PR to revert the encryption changes and just solve the ticket (the changes intended to resolve redaction of the password field from some northbound api calls) with an approach that should have less of an effect of the functionality of our engine. I'll reference that PR here once I have it up. |
|
@yyscamper Felix, per my comment here is the first of 2 PRs I'll post to hopefully resolve our issues: RackHD/on-core#228 |
|
@yyscamper hey Felix, so now both new PRs are up and I'd love your feedback/merge on those so I can close this PR and resolve the issue. thanks! |
|
on-core reversion PR merged, going to close this PR. |
5 participants
resolves RackHD/RackHD#417
new changes to this PR seem to make this PR irrelevant: RackHD/on-core#220
@RackHD/corecommitters
@keedya