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 ability to encrypt session data #818

Closed
wants to merge 2 commits into from
Closed

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Apr 24, 2024

WHY are these changes introduced?

Currently, when storing sessions in a database, we don't provide any utility from @shopify/shopify-api to encrypt the access token, which is easy to rotate for apps but might be annoying for developers if their databases are ever leaked.

WHAT is this pull request doing?

To add an extra layer of security to the way apps store tokens, this PR introduces the ability to serialize and deserialize sessions in a way that handles the encryption for apps, given a CryptoKey.

If a row was previously unencrypted, these functions will still be able to load it and update the row when saving it, which means that the migration path for session storages is simply:

  • loadSession
  • storeSession

for every row in their database, if they don't want to wipe it clean.

I'll add documentation and a changelog entry to this PR once we're aligned on what it will do.

Type of change

  • Minor: New feature (non-breaking change which adds functionality)

Checklist

  • I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

@paulomarg paulomarg requested a review from a team as a code owner April 24, 2024 18:33
@paulomarg paulomarg force-pushed the encrypt_access_tokens branch 2 times, most recently from a425cfe to ebf73ed Compare April 25, 2024 19:25
packages/apps/shopify-api/lib/session/session.ts Outdated Show resolved Hide resolved
}

/**
* Converts the session into an array of key-value pairs, encrypting sensitive data.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could explicitly say it is the access token that is getting encrypted.

If you want to encrypt your access tokens before storing them, the `Session` class provides two methods: `fromEncryptedPropertyArray` and `toEncryptedPropertyArray`.

These behave the same as their non-encrypted counterparts, and take in a `CryptoKey` object. If a session is currently not encrypted in storage, these methods will still load it normally, and will encrypt them when saving them.
Currently, only the `accessToken` is encrypted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Should we be making it flexible to allow folks to encrypt more than just the accessToken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of that too, but haven't thought of how the app would convey that information. I think it would need to be a direct argument to to/fromEncryptedArray that the app would have to pass in to the session storage.

That would mean the app could do something like:

// Evergreen way of encrypting every column we allow
new PrismaSessionStorage(prisma, {encryptionKey: true, encryptColumns: 'all'});

// Explicitly select which columns
new PrismaSessionStorage(prisma, {encryptionKey: true, encryptColumns: ['accessToken', 'userId']});

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it would have to be something like that. I think it would be a nice to have, but does go beyond the scope of this required work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is fairly easy to implement though, I'm going to timebox it and see if I can get it off the ground.

@paulomarg
Copy link
Contributor Author

In the end, we decided not to go forward with this as developers are probably better served by using encryption at the infrastructure level for their production databases, rather than making the API more complex at the software level.

@paulomarg paulomarg closed this Apr 29, 2024
@paulomarg paulomarg deleted the encrypt_access_tokens branch April 29, 2024 17:32
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

Successfully merging this pull request may close these issues.

2 participants