Skip to content
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

JavaScript instanceOf and different versions of the underlying Encryption SDK modules. #126

Open
seebees opened this issue Jun 22, 2019 · 11 comments

Comments

@seebees
Copy link
Contributor

seebees commented Jun 22, 2019

We use instanceOf to insure some security properties of Cryptographic Materials, and Encrypted Data Keys. However, under some conditions, versions of the dependent packages may result in copies of these files being installed (local node_modules folders) for the packages that have differing versions.
This means that these instanceOf checks may fail. Because while the code is the same, the actual instances are not.
What is the best direction?
How pervasive can this be?

@krisread
Copy link

This is blocking me from adding the @awslabs/node-client module to my node application.

Tracing down failures it seems like instanceOf checks in "needs(...." functions are failing.

@seebees
Copy link
Contributor Author

seebees commented Aug 30, 2019

Are you using jest? Can you give me a little more detail about how you are trying to use the client?

@krisread
Copy link

Hi!

All I have to do is add the library to my existing node project, then just copy/pasting the kms simple example from the examples for node fails.

If I check out this repo, it works

The only difference seems to be other packages, and since it fails complaining about instanceOf checks on KMS responses, I figure it has to be this bug.

For now, to be honest I have to give up on making this SDK work. Isolating which other package in my project is affecting this one would be too time consuming.

@seebees
Copy link
Contributor Author

seebees commented Aug 30, 2019

Is your existing project a public repo? Can you point me to a simple way to reproduce your issue?

instanceOf in Javascript will not work in different execution environments. Mostly this happens when working with frames in a browser.

@derek-pavao
Copy link

Any word on this? This appears to be happening to me as well. I'm passing a string to encrypt and getting an Unsupported plaintext error, emitted from this line https://github.com/aws/aws-encryption-sdk-javascript/blob/master/modules/encrypt-node/src/encrypt.ts#L50

If I'm following the code correctly, when I pass a string, the lib turns it into a Buffer which should make this condition (https://github.com/aws/aws-encryption-sdk-javascript/blob/master/modules/encrypt-node/src/encrypt.ts#L45) be true, a Buffer is an instance of Uint8Array. However I get the error on line 50 anyway.

I'm not exactly sure how to figure out what is causing this, and unfortunately my project is not public :( I can try to repro it in a sample type project, but hopefully I can get some insight before I go that far

@seebees
Copy link
Contributor Author

seebees commented Oct 10, 2019

@dotDeeka I'm not sure how this is an instanceOf issue, per se. This issue deals with the fact that instanceOf does not work on things that are the "same" but running in different execution contexts. A description exists here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof of that issue.

My guess would be that it is not falling into the this https://github.com/aws/aws-encryption-sdk-javascript/blob/master/modules/encrypt-node/src/encrypt.ts#L33.

Why not try changing the argument to a Buffer before passing it to the encrypt function?

@seebees seebees changed the title JavaScript instanceOf and different versions JavaScript instanceOf and different versions of the underlying Encryption SDK modules. Oct 10, 2019
@derek-pavao
Copy link

If I debug my test run I can see that it enters L33 correctly, it's definitely the plaintext instanceof Uint8Array check that is false, and I can see that plaintext is a Buffer.

@derek-pavao
Copy link

The issue appears to be related to Jest. There are some details listed in this issue in the Jest repo jestjs/jest#4422

@seebees
Copy link
Contributor Author

seebees commented Oct 10, 2019

Yes the issue you are having is with jest, you can see what node has to say about it here: nodejs/node#20978 (comment)

And this jestjs/jest#7780 (comment) may help with injecting the globals into jest.

@mogarick
Copy link

mogarick commented Sep 2, 2020

@seebees I opened the issue aws-amplify/amplify-js#6552 ~3weeks ago. Today after digging into AuthenticationHelper and tracing to lib-typedarrays I found that the reason was related to jestjs/jest#7780. I solved my problem injecting globals as I can see you also recommended.
But at this moment I'm stuck in other part, because I'm getting a 403 when calling a graphql operation using authMode=AWS_IAM.
Do you think it could also be related to something similar? (maybe another global that needs to be injected as workaround to the instanceof problem).

@austin-payne
Copy link

Hi, I ran across this issue and have some additional details to add, as well as a potential fix.

We have a monorepo with many different packages owned by different teams, that depend on different node modules (and different versions of the same node modules). In this case, most of our packages were using an old version of @aws-crypto/client-node, and we wanted to upgrade some of them to a newer version. All packages only depend on the top level @aws-crypto/client-node module, none of the underlying aws-crypto modules. When running the code in a lambda function bundled with webpack, we get an error due to an instanceof check as described here.

The error is the same as reported here #638 that couldn't be reproduced. I took texastony's working test case, and was able to make it fail simply by adding other @aws-crypto dependencies to the package.json with a different version than @aws-crypto/client-node:

    "@aws-crypto/client-node": "^2.3.1",
    "@aws-crypto/material-management": "1.9.0",
    "@aws-crypto/material-management-node": "1.9.0",

Which causes the type error, due to a failed instanceof check as mentioned:

     TypeError: this._backingMaterialsManager.getEncryptionMaterials is not a function

The structure of node_modules looks something like this:

node_modules/
    @aws-crypto/
        caching-materials-manager-node@v2
            node_modules/
                @aws-crypto/
                    material-management@v2
                    material-management-node@v2
        client-node@v2
            node_modules/
                @aws-crypto/
                    material-management@v2
                    material-management-node@v2
        material-management@v1
        material-management-node@v1
        ...other @aws-crypto modules

caching-materials-manager-node has an instanceof check on a class from material-management-node. This class is exported by client-node, but caching-materials-manager-node and client-node have duplicate installations of materials-management-node due to a different version already being installed at the root node_modules. Therefore the instanceof returns false.

This reproduction is using mocha, not jest, and like I said we originally ran across this error in a webpacked lambda function at runtime, so this issue has a bigger impact than just jest as previous comments suggest.

I think these issues could be solved with peer dependencies, by making all of the underlying encryption modules depend on each other through peerDependencies. Then @aws-crypto/client-node would specify them all as regular dependencies. This would result in a node_modules structure without underlying dependency duplication, for example:

node_modules/
    @aws-crypto/
        client-node@v2
            node_modules/
                @aws-crypto/
                    caching-materials-manager-node@v2
                    material-management@v2
                    material-management-node@v2
                    ... etc
    some-other-package
        @aws-crypto/
            client-node@v1
                node_modules/
                    ... all of the v1 underlying modules

@seebees thoughts? or should I create another issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants