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.

Sign up for GitHub

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

Jump to bottom

Store encrypted values in new DB column. #3351

Merged
merged 4 commits into from
May 17, 2024
Merged

Store encrypted values in new DB column. #3351

merged 4 commits into from
May 17, 2024

Conversation

dmjb
Copy link
Contributor

@dmjb dmjb commented May 16, 2024 •
edited
Loading

Relates to #3315

When encrypting secrets, store the serialized JSON in the database. When decrypting, attempt to fetch from the new column and then fall back to the old one.

In a subsequent PR, I will migrate all existing secrets to the new structure.

Some more unit testing of the crypto engine was added in this PR.

Tested locally with a mix of old and new credentials.

Summary

Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.

Fixes #(related issue)

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@dmjb dmjb requested a review from a team as a code owner May 16, 2024 15:33
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

@dmjb dmjb marked this pull request as draft May 16, 2024 15:35
@dmjb dmjb force-pushed the store-encrypted-data-in-db branch from 75d4df0 to 3dacf36 Compare May 16, 2024 16:11
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@dmjb dmjb force-pushed the store-encrypted-data-in-db branch from 3dacf36 to 60f0387 Compare May 16, 2024 16:13
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@dmjb
Store encrypted values in new DB column.
58c1982 
Relates to #3315

When encrypting secrets, store the serialized JSON in the database. When
decrypting, attempt to fetch from the new column and then fall back to
the old one.

In a subsequent PR, I will migrate all existing secrets to the new
structure.
@dmjb dmjb force-pushed the store-encrypted-data-in-db branch from 60f0387 to 58c1982 Compare May 16, 2024 16:14
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@dmjb dmjb marked this pull request as ready for review May 16, 2024 16:16
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@dmjb dmjb force-pushed the store-encrypted-data-in-db branch from 417d11c to af9f026 Compare May 16, 2024 16:39
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@dmjb dmjb force-pushed the store-encrypted-data-in-db branch from 2679b8e to fa8af1d Compare May 16, 2024 16:57
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@dmjb dmjb force-pushed the store-encrypted-data-in-db branch from fa8af1d to ce1f26c Compare May 16, 2024 16:57
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

jhrozek
jhrozek reviewed May 16, 2024
View reviewed changes
database/query/session_store.sql
@@ -1,8 +1,8 @@
-- name: CreateSessionState :one
INSERT INTO session_store (provider, project_id, remote_user, session_state, owner_filter, provider_config, redirect_url) VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING *;
INSERT INTO session_store (provider, project_id, remote_user, session_state, owner_filter, provider_config, redirect_url, encrypted_redirect) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING *;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to get rid of the redirect_url column?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

jhrozek
jhrozek reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

looks good, just a couple of questions inline

internal/controlplane/handlers_oauth.go
func (s *Server) decryptRedirect(stateData *db.GetProjectIDBySessionStateRow) (*url.URL, error) {
var err error
// TODO: get rid of this once we migrate all secrets to use the new structure
var encryptedData mcrypto.EncryptedData
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if neither is set, does decryptString handle the situation where mcrypto.EncryptedData is empty gracefully?

Copy link
Contributor Author

@dmjb dmjb May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Anywhere where this method is called, it is wrapped with if stateData.RedirectUrl.Valid so we're good.

I want to drop the old column after migration, so this code can be cleaned up at that point.

stacklokbot
stacklokbot reviewed May 17, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 17, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@coveralls
Copy link

Coverage Status

coverage: 50.259% (+0.1%) from 50.137%
when pulling ceea269 on store-encrypted-data-in-db
into 0016075 on main.

@dmjb dmjb merged commit c0286cd into main May 17, 2024
20 checks passed
@dmjb dmjb deleted the store-encrypted-data-in-db branch May 17, 2024 09:14
dmjb added a commit that referenced this pull request May 17, 2024
@dmjb
Generate random salt per key.
c39a5d3 
Fixes #3316

This PR should not be merged until the changes in #3351 have been rolled
out into production.

Generate a random 16-byte salt for each secret we encrypt, and add some
testing around it.
dmjb added a commit that referenced this pull request May 17, 2024
@dmjb
Generate random salt per key.
a609fa9 
Fixes #3316

This PR should not be merged until the changes in #3351 have been rolled
out into production.

Generate a random 16-byte salt for each secret we encrypt, and add some
testing around it.
@dmjb dmjb mentioned this pull request May 17, 2024
Merged
10 tasks
dmjb added a commit that referenced this pull request May 17, 2024
@dmjb
Generate random salt per key.
feb7c2e 
Fixes #3316

This PR should not be merged until the changes in #3351 have been rolled
out into production.

Generate a random 16-byte salt for each secret we encrypt, and add some
testing around it.
dmjb added a commit that referenced this pull request May 17, 2024
@dmjb
Generate random salt per key.
e1906fb 
Fixes #3316

This PR should not be merged until the changes in #3351 have been rolled
out into production.

Generate a random 16-byte salt for each secret we encrypt, and add some
testing around it.
dmjb added a commit that referenced this pull request May 20, 2024
@dmjb
Generate random salt per key.
23e2415 
Fixes #3316

This PR should not be merged until the changes in #3351 have been rolled
out into production.

Generate a random 16-byte salt for each secret we encrypt, and add some
testing around it.
dmjb added a commit that referenced this pull request May 20, 2024
@dmjb
Generate random salt per key.
63d1b21 
Fixes #3316

This PR should not be merged until the changes in #3351 have been rolled
out into production.

Generate a random 16-byte salt for each secret we encrypt, and add some
testing around it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stacklokbot stacklokbot stacklokbot left review comments

@jhrozek jhrozek jhrozek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmjb @coveralls @jhrozek @stacklokbot