google / webcrypto.dart

Cross-platform implementation of Web Cryptography APIs
https://pub.dev/packages/webcrypto
Apache License 2.0
71 stars 43 forks source link

Add .toString() methods on all private classes #104

Closed jonasfj closed 1 month ago

jonasfj commented 1 month ago

It's sad when print(HmacSecretKey.generateKey(Hash.sha256)) shows Instance of '_HmacSecretKey'.

Perhaps we should override the toString() method on all the private classes.

The user doesn't need to know about _HmacSecretKey, since it's a private class. And we could probably display something more useful for debugging purposes.

I haven't figured this out yet, but we could do something like:

It could be useful if print was helpful when debugging stuff :D

On the flip side, the HmacSecretKey doesn't have a hash property, so if you have an instance of HmacSecretKey and you want to know what hash it uses, the only thing you can do is:

I imagine that we'd prefer to avoid using doing (b). Not necessarily be adding a hash property they don't need, but by forcing them to do (a) :rofl:

I think the point I'm trying to make is that, if we add to much information in toString(), then maybe people will use to inspect an object at runtime and rely on the behavior of toString(). And I don't think we want people to rely on the behavior of toString().

So maybe it's safest to just do: macSecretKey.generateKey(Hash.sha256).toString() == "Instance of 'HmacSecretKey'"

It's boring, it's simple, but it doesn't leak any information that could lure a user into misusing toString(). I could ofcourse be convinced otherwise.

devenderbutani21 commented 1 month ago

Hey Jonas! Can you provide more info to solve? Is it in specific section of code?

HamdaanAliQuatil commented 1 month ago

Agree on the part - "user doesn't need to know about _HmacSecretKey"

imo providing the key with the hash function would be a pretty bad idea (option 1) over concerns of inspection at runtime by malicious actors.

I am yet to find credible evidence of cases where the knowledge of the hash function alone, by Eve could be an issue. Even if we double down on option 2, the output that the user will see is Instance of 'HmacSecretKey' with hash: Instance of '_Sha256'

I would like to avoid this for two reasons:

  1. The information is redundant
  2. It would require implementing an override for `_Sha256' as well. (which ig is the goal of the issue, but point 1 still holds)

I agree on the solution Instance of 'HmacSecretKey'. Its not the most informative one but is the best option that doesnt leak much information.

HamdaanAliQuatil commented 1 month ago

Hey Jonas! Can you provide more info to solve? Is it in specific section of code?

Hey @devenderbutani21 this is a WIP, can you please select other open issues, would be happy to assist you in one. Thanks a lot.

For context of this issue please check the following gist - https://gist.github.com/HamdaanAliQuatil/bd32945af3091ff7ba72071386ff4de7

devenderbutani21 commented 1 month ago

@HamdaanAliQuatil Should I work on https://github.com/google/webcrypto.dart/issues/105 this issue?

HamdaanAliQuatil commented 1 month ago

@HamdaanAliQuatil Should I work on #105 this issue?

Yes, that'd be great. Tysm for contributing 🚀

jonasfj commented 1 month ago

@devenderbutani21 Feel free to ping me @jonasfj for reviews, I'm happy to help merge PRs.

I'm jonasfj88 on discord, see dart_commmunity, @HamdaanAliQuatil and I are hanging out in a thread in the #packages channel.

jonasfj commented 1 month ago

imo providing the key with the hash function would be a pretty bad idea (option 1) over concerns of inspection at runtime by malicious actors.

The hash function isn't exactly secret, and we anyone who can do runtime inspection has probably won. Like that's exactly what we're trying to protect against. More like we attempt to protect against mistakes.