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

Add mock for browser.secureStorage proposal #186

Merged
merged 9 commits into from
Jul 7, 2022

Conversation

oliverdunk
Copy link
Member

As suggested in the last meeting, this PR adds a polyfill which shows the expected API behaviour of the browser.secureStorage proposal. This includes the remove call added in #183.

For obvious reasons, we can't currently access hardware backed secure storage. As a result, this polyfill uses localStorage and is for proof of concept purposes only.

Resolves #180

Copy link
Member

@dotproto dotproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of this polyfill, I think we should make two main changes.

  1. Use browser.storage.local or indexedDB instead of localStorage to make the polyfill usable in (extension service) worker environments
  2. Encrypt data before putting it in storage and decrypt it after pulling it out

For 2, I believe we can use SubtleCrypto to handle encryption. For the moment I'm thinking this would be a relatively simple password based encryption scheme. The password could be held in a variable or storeage.session where available. After quickly poking around, it looks like this article describes how (or at least a way) to implement this with SubtleCrypto.

@oliverdunk
Copy link
Member Author

For the sake of this polyfill, I think we should make two main changes.

  1. Use browser.storage.local or indexedDB instead of localStorage to make the polyfill usable in (extension service) worker environments
  2. Encrypt data before putting it in storage and decrypt it after pulling it out

For 2, I believe we can use SubtleCrypto to handle encryption. For the moment I'm thinking this would be a relatively simple password based encryption scheme. The password could be held in a variable or storeage.session where available. After quickly poking around, it looks like this article describes how (or at least a way) to implement this with SubtleCrypto.

Thanks for taking a look @dotproto!

I definitely agree on supporting extension service workers, so I've moved to browser.storage.local. Let me know what you think.

On encrypting data, I think I can add this, but I'm curious what the benefit is. Do you think it just makes things a bit more real and usable? Of course if we include the password in the polyfill it doesn't really achieve anything and we're not doing anything that isn't already known to be possible.

@Rob--W Rob--W added the topic: secure storage Related to the secureStorage API proposal label Mar 31, 2022
@dotproto
Copy link
Member

dotproto commented Apr 1, 2022

Do you think it just makes things a bit more real and usable?

That was the idea.

As I understand it a polyfill (or a prollyfill in this case) is a lib that provides the features of a spec/standard that isn't implemented in a browser. As such, I assumed that this library was aiming to both provide a functional API surface developers could experiment with as well as implement as much of the spec's functionality as reasonably possible.

If you want to instead focus on just the API surface without worrying much about functionality, I'd suggest we tweak the framing of this library to say it's a demo, fake, double, or perhaps something else entirely. (Or even poppyfill?)

@oliverdunk
Copy link
Member Author

@dotproto: I'm going to bring this up at WECG tomorrow but I think I'd rather take the path of changing the framing than add a crypto implementation. Having some sort of implementation is useful to see it working, but trying to implement something that is cryptographically sound (even if we're using Web APIs which do most of that for us) seems like an easy way to get distracted, and also gives the impression that this is real more than I would like to without more scrutiny. Poppyfill all the way!

@oliverdunk
Copy link
Member Author

We discussed this at the last meeting (#215) and agreed that we should merge this as-is after renaming it from a "polyfill" to a "mock", to indicate that there is no cryptographic backing. I've gone ahead and done that so this should be good for another look :)

@oliverdunk oliverdunk changed the title Add polyfill for browser.secureStorage proposal Add mock for browser.secureStorage proposal Jun 8, 2022
@xeenon xeenon self-requested a review June 24, 2022 16:58
Copy link
Member

@dotproto dotproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay on this

@dotproto dotproto merged commit ed94d59 into w3c:main Jul 7, 2022
github-actions bot added a commit that referenced this pull request Jul 7, 2022
SHA: ed94d59
Reason: push, by @dotproto

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: secure storage Related to the secureStorage API proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

browser.secureStorage: Create polyfill for implementation
6 participants