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

channel: add realtime encryption #603

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

andydunstall
Copy link
Contributor

@andydunstall andydunstall commented Jun 22, 2023

DSC-74
@owenpearson @sacOO7 we're looking to support encryption with in the database connector, though AFAICT encryption isn't supported for realtime channels

Looking at rest channels, looks like its just doing:

		cipher, _ := options.GetCipher()
		var err error
		*m, err = (*m).withEncodedData(cipher)
		if err != nil {
			return fmt.Errorf("encoding data for message #%d: %w", i, err)
		}

Though I know why withEncodedData isn't run on non-rest channels?

Just adding draft PR to start discussion - this just copies withEncodedData into RealtimeChannel

@github-actions github-actions bot temporarily deployed to staging/pull/603/features June 22, 2023 12:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/603/godoc June 22, 2023 12:36 Inactive
@sacOO7
Copy link
Collaborator

sacOO7 commented Jun 22, 2023

Need to check on how encryption works in rest channels. You can check and implement if it's easy, we will review.

@andydunstall
Copy link
Contributor Author

You can check and implement if it's easy, we will review.

Thanks - thats what this PR is :)

@andydunstall andydunstall marked this pull request as ready for review June 22, 2023 12:52
@sacOO7
Copy link
Collaborator

sacOO7 commented Jun 22, 2023

You can check and implement if it's easy, we will review.

Thanks - thats what this PR is :)

Can you add integration tests for this? Also, check if changes are not failing existing integration tests.

Thanks!

@andydunstall
Copy link
Contributor Author

Oh I guess Subscribe also needs to handle encrypted messages too

@andydunstall andydunstall force-pushed the feature/add-realtime-encryption branch from 6560d13 to db27254 Compare June 22, 2023 14:34
@github-actions github-actions bot temporarily deployed to staging/pull/603/features June 22, 2023 14:35 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/603/godoc June 22, 2023 14:35 Inactive
@andydunstall andydunstall force-pushed the feature/add-realtime-encryption branch from db27254 to f15ea04 Compare June 22, 2023 14:35
@github-actions github-actions bot temporarily deployed to staging/pull/603/features June 22, 2023 14:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/603/godoc June 22, 2023 14:37 Inactive
Copy link
Member

@owenpearson owenpearson 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 so far, although I'm hesitant to approve because I don't think we're doing enough to test this at the moment. The test added here would pass even if the library just ignores the cipher param entirely. At a minimum, could we add a test which creates two realtimes with a cipher mismatch and checks that the behaviour is correct (see ably-js example). It would also be good to test the behaviour when sending an encrypted message to a client without a cipher and vice versa.

@andydunstall
Copy link
Contributor Author

test the behaviour when sending an encrypted message to a client without a cipher and vice versa

Sure, extended the test to include a channel without a cipher, the that the message doesn't equal the original

@andydunstall
Copy link
Contributor Author

@owenpearson @sacOO7 can I get a review on this?

Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@andydunstall
Copy link
Contributor Author

@owenpearson Thanks can this be merged?

Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

LGTM

@owenpearson owenpearson merged commit 5685343 into main Jul 3, 2023
8 checks passed
@owenpearson owenpearson deleted the feature/add-realtime-encryption branch July 3, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants