search

eclipse-thingweb / node-wot

Components for building WoT devices or for interacting with them over various IoT protocols
https://thingweb.io
Other
160 stars 78 forks source link

Possible bug in Servient.addCredentials() method? #301

Open erceguder opened 3 years ago

erceguder commented 3 years ago

Hey all!

While I was trying to develop something to test security of the things, I've observed that addCredentials() method of Servient class is behaving in an unexpected way. For example, in the following gist you can see that I have a thing with basic auth. of HTTP (tried with HTTPS btw., same problem) and I have a consumer thing to do -hopefully- pentesting. First, the credentials in the consumer thing is consciously set up in a wrong way. After first denial, credentials are set to the same of exposed thing's (affirmed with getCredentials()), yet it still receives 401 from the exposed thing on the second trial (?). An interesting point here is that if I comment out the first addCredentials() method in consumer thing, then on the second trial it successes.

https://gist.github.com/erceguder/a54adfe2e486f0efd6fd6dcbae8f9efa

danielpeintner commented 3 years ago

@relu91 may I ask you to take a look?

relu91 commented 3 years ago

It looks like a caching problem. The credentials are set in ensureClientSecurity method; here: https://github.com/eclipse/thingweb.node-wot/blob/d272697af7ad836d4f5024dc46f06662ae54b06b/packages/core/src/consumed-thing.ts#L136

However, once the client is created the ensureClientSecurity is never called again (see getClientFor method). Consequently, the next reading operations will not have updated credentials. Now it is quite odd that IF the first addCredentials() is removed the second trial succeeds. My bet is that node-wot is failing to create the client in the first and then re-create in the second one.

@erceguder thanks for providing a test-case! would you mind to paste also the debug output of the consumer thing when the first addCredentials() is commented out?

erceguder commented 3 years ago

@relu91 sure, added to the gist.

relu91 commented 3 years ago

However, once the client is created the ensureClientSecurity is never called again (see getClientFor method). Consequently, the next reading operations will not have updated credentials. Now it is quite odd that IF the first addCredentials() is removed the second trial succeeds. My bet is that node-wot is failing to create the client in the first and then re-create in the second one.

Sorry, I forgot to replay back. basically from your output is pretty clear that my hypothesis is confirmed. I think it should be possible to add credentials also after the client's initilization. However one could also argue that the credentials should be added beforehand. @danielpeintner what do you think? Is it a bug or a feature 😄 ?

danielpeintner commented 3 years ago

I think it should be possible to add credentials also after the client's infantilization

I agree. It would be nice to allow adding credentials later. Having said that, I am not 100% sure about all consequences...