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

RUMM-1870 Add data encryption interface #797

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

maxep
Copy link
Member

@maxep maxep commented Mar 31, 2022

What and why?

Allow users to set custom encryption routines for on-disk data storage.

How?

Expect an implementation of the following protocol:

/// Interface that allows storing data in encrypted format. Encryption/decryption round should
/// return exactly the same data as it given for the encryption originally (even if decryption
/// happens in another process/app launch).
public protocol DataEncryption {
    /// Encrypts given `Data` with user-chosen encryption.
    ///
    /// - Parameter data: Data to encrypt.
    /// - Returns: The encrypted data.
    func encrypt(data: Data) throws -> Data

    /// Decrypts given `Data` with user-chosen encryption.
    ///
    /// Beware that data to decrypt could be encrypted in a previous
    /// app launch, so implementation should be aware of the case when decryption could
    /// fail (for example, key used for encryption is different from key used for decryption, if
    /// they are unique for every app launch).
    ///
    /// - Parameter data: Data to decrypt.
    /// - Returns: The decrypted data.
    func decrypt(data: Data) throws -> Data
}

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing change.

@maxep maxep requested a review from a team as a code owner March 31, 2022 13:24
@@ -13,17 +13,17 @@ internal struct DataFormat {
/// Suffixes the batch payload read from file.
let suffixData: Data
/// Separates entities written to file.
let separatorData: Data
let separatorByte: UInt8
Copy link
Member Author

Choose a reason for hiding this comment

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

Enforce single byte separator for data splitting during decryption.

private let internalMonitor: InternalMonitor?

/// Files marked as read.
private var filesRead: [ReadableFile] = []
private var filesRead: Set<String> = []
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor refacto

@maxep maxep self-assigned this Apr 2, 2022
@maxep maxep force-pushed the maxep/RUMM-1870/data-encryption branch from adaa256 to d3be27e Compare April 4, 2022 07:56
@maxep maxep force-pushed the maxep/RUMM-1870/data-encryption branch from d3be27e to 1373bc3 Compare April 4, 2022 08:05
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Everything looks good in code, really well done 👍. I left one blocking feedback on partial mock (it has a bug now).

Also, as we have only add small unit tests in FileWriter (encryption) and FileReader (decryption), but no tests for integration layer, how about adding fuzzy condition to these tests:

in which we could randomize the presence of DataEncryption? Something this:

encryption: Bool.random() ? MockDataEncryption() : nil

These 4 tests assert each feature's payload format on a high level. Their behaviour should remain the same no matter if data encryption is or is not configured.

@@ -3,6 +3,7 @@
* [FEATURE] Web-view tracking. See [#729][]
* [FEATURE] Integration with CI Visibility Tests. See[#761][]
* [FEATURE] Add tvOS Support. See [#793][]
* [FEATURE] Add data encryption interface on-disk data storage. See [#797][]
Copy link
Member

Choose a reason for hiding this comment

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

Should this be part of Unreleased section along with "Web-view tracking" which is already part 1.10.0-beta1? I think that something is out-of-sync about the CHANGELOG.md, no? IMO the 1.10.0-beta1 release should be listed here as regular release - otherwise we will lose track on what's inside 1.10.0 and what not.

Copy link
Member Author

Choose a reason for hiding this comment

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

We currently don't add pre-release tag in the changelog, it is part of our release-process. I also think we should add pre-release sections tho! lets chat for updating the process before the next version.

Sources/Datadog/Core/Persistence/Reading/FileReader.swift Outdated Show resolved Hide resolved
Sources/Datadog/Core/Persistence/Writing/FileWriter.swift Outdated Show resolved Hide resolved
Tests/DatadogTests/Datadog/Mocks/CoreMocks.swift Outdated Show resolved Hide resolved
Sources/Datadog/Core/Persistence/Reading/FileReader.swift Outdated Show resolved Hide resolved
maxep and others added 2 commits April 4, 2022 15:07
Co-authored-by: Maciek Grzybowski <maciek.grzybowski@datadoghq.com>
@maxep maxep requested a review from ncreated April 4, 2022 17:04
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

All good 🚀 👌

@maxep maxep force-pushed the maxep/RUMM-1870/data-encryption branch from 748ea4c to 8851d46 Compare April 5, 2022 11:59
@maxep maxep merged commit 16a9b24 into develop Apr 5, 2022
@maxep maxep deleted the maxep/RUMM-1870/data-encryption branch April 5, 2022 13:28
@ncreated ncreated mentioned this pull request Apr 6, 2022
3 tasks
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