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

[APT-10393] Support Rotating Content URLs #46

Merged
merged 4 commits into from
Sep 18, 2024
Merged

[APT-10393] Support Rotating Content URLs #46

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca6f5ed
[APT-10393] Protect EncryptedSharedPref Instability
kabliz Sep 17, 2024
33fd980
[APT-10393] Support Rotating Content URLs
kabliz Sep 18, 2024
e1a5348
[APT-10393] Fallback if EncyrSharedPref fails
kabliz Sep 18, 2024
2c640c6
[APT-10393] Cleanup id copying.
kabliz Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 39 additions & 30 deletions Armadillo/src/main/java/com/scribd/armadillo/di/DownloadModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,41 +121,50 @@ internal class DownloadModule {
@Singleton
@Provides
@Named(Constants.DI.STANDARD_SECURE_STORAGE)
fun standardSecureStorage(context: Context): SharedPreferences {
val keys = MasterKeys.getOrCreate(
KeyGenParameterSpec.Builder("armadilloStandard", PURPOSE_ENCRYPT or PURPOSE_DECRYPT)
.setKeySize(256)
.setBlockModes(BLOCK_MODE_GCM)
.setEncryptionPaddings(ENCRYPTION_PADDING_NONE)
.build()
)
return EncryptedSharedPreferences.create(
"armadillo.standard.secure",
keys,
context,
EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
)
fun standardSecureStorage(context: Context): SharedPreferences? {
return try {
val keys = MasterKeys.getOrCreate(
KeyGenParameterSpec.Builder("armadilloStandard", PURPOSE_ENCRYPT or PURPOSE_DECRYPT)
.setKeySize(256)
.setBlockModes(BLOCK_MODE_GCM)
.setEncryptionPaddings(ENCRYPTION_PADDING_NONE)
.build()
)
EncryptedSharedPreferences.create(
"armadillo.standard.secure",
keys,
context,
EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
)
} catch (ex: Exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What exception can actually happen here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think this one should maybe fall back to unencrypted storage? It seems better than not being able to do anything.

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 dont know exactly, im following second hand reports on problems with EncryptedSharedPrefs.

Failing into an insecure mode would not be appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

tink-crypto/tink#535 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend maybe doing something like this?

        val keySpec = KeyGenParameterSpec.Builder("armadilloStandard", PURPOSE_ENCRYPT or PURPOSE_DECRYPT)
            .setKeySize(256)
            .setBlockModes(BLOCK_MODE_GCM)
            .setEncryptionPaddings(ENCRYPTION_PADDING_NONE)
            .build()

        val keys = try {
            MasterKeys.getOrCreate(keyspec)
        } catch (ex: Exception){
                val keyStore = KeyStore.getInstance("AndroidKeyStore")
                keyStore.load(null)
                keyStore.deleteEntry("armadilloStandard")
                context.getSharedPreferences("armadillo.standard.secure", Context.MODE_PRIVATE).edit().clear().apply()
                MasterKeys.getOrCreate(keySpec)
        }

        return EncryptedSharedPreferences.create(
            "armadillo.standard.secure",
            keys,
            context,
            EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
            EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
        )

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you may be fine to use MasterKeys.AES256_GCM_SPEC for both instead of a custom keyspec for each too, but your call

null
}
}

@Singleton
@Provides
@Named(Constants.DI.DRM_SECURE_STORAGE)
fun drmSecureStorage(context: Context): SharedPreferences {
val keys = MasterKeys.getOrCreate(
KeyGenParameterSpec.Builder("armadillo", PURPOSE_ENCRYPT or PURPOSE_DECRYPT)
.setKeySize(256)
.setBlockModes(BLOCK_MODE_GCM)
.setEncryptionPaddings(ENCRYPTION_PADDING_NONE)
.build()
)
return EncryptedSharedPreferences.create(
"armadillo.download.secure",
keys,
context,
EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
)
fun drmSecureStorage(context: Context): SharedPreferences? {
return try {
val keys = MasterKeys.getOrCreate(
KeyGenParameterSpec.Builder("armadillo", PURPOSE_ENCRYPT or PURPOSE_DECRYPT)
.setKeySize(256)
.setBlockModes(BLOCK_MODE_GCM)
.setEncryptionPaddings(ENCRYPTION_PADDING_NONE)
.build()
)
EncryptedSharedPreferences.create(
"armadillo.download.secure",
keys,
context,
EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
)
}
catch (ex: Exception) {
null
}
}

@Singleton
Expand Down
24 changes: 15 additions & 9 deletions Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ internal interface DownloadEngine {
fun removeDownload(audioPlayable: AudioPlayable)
fun removeAllDownloads()
fun updateProgress()
fun redownloadDrmLicense(request: AudioPlayable.MediaRequest)
fun redownloadDrmLicense(id: String, request: AudioPlayable.MediaRequest)
}

/**
Expand Down Expand Up @@ -70,15 +70,20 @@ internal class ExoplayerDownloadEngine @Inject constructor(
scope.launch(errorHandler) {
launch {
// Download DRM license for offline use
offlineDrmManager.downloadDrmLicenseForOffline(audioPlayable.request)
offlineDrmManager.downloadDrmLicenseForOffline(id = audioPlayable.id.toString(), request = audioPlayable.request)
}

launch {
val downloadHelper = downloadHelper(context, audioPlayable.request)
val downloadHelper = downloadHelper(
id = audioPlayable.id.toString(),
context = context,
mediaRequest = audioPlayable.request
)

downloadHelper.prepare(object : DownloadHelper.Callback {
override fun onPrepared(helper: DownloadHelper) {
val request = helper.getDownloadRequest(audioPlayable.id.encodeInByteArray())
var request = helper.getDownloadRequest(audioPlayable.id.encodeInByteArray())
request = request.copyWithId(audioPlayable.id.toString())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing from the byte array to the string?

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's string everywhere else, and the audioplayable.id is not what's being set as the ID there, its being set as the URL, and then i have to override it back to being the audioplayable.id instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope: I don't think we use the audioPlayable.id.encodeInByteArray() data anywhere, it's a bit of a red herring. I think we can pass null

Also, the var threw me off for a moment thinking there was reason to reassign it.

Any reason not to do:

val request = helper.getDownloadRequest(audioPlayable.id.encodeInByteArray()).copyWithId(audioPlayable.id.toString())?

try {
startDownload(context, request)
} catch (e: Exception) {
Expand All @@ -99,8 +104,8 @@ internal class ExoplayerDownloadEngine @Inject constructor(

override fun removeDownload(audioPlayable: AudioPlayable) {
scope.launch(errorHandler) {
launch { downloadManager.removeDownload(audioPlayable.request.url) }
launch { offlineDrmManager.removeDownloadedDrmLicense(audioPlayable.request) }
launch { downloadManager.removeDownload(audioPlayable.id.toString()) }
launch { offlineDrmManager.removeDownloadedDrmLicense(id = audioPlayable.id.toString(), request = audioPlayable.request) }
}
}

Expand All @@ -113,10 +118,10 @@ internal class ExoplayerDownloadEngine @Inject constructor(

override fun updateProgress() = downloadTracker.updateProgress()

override fun redownloadDrmLicense(request: AudioPlayable.MediaRequest) {
override fun redownloadDrmLicense(id: String, request: AudioPlayable.MediaRequest) {
scope.launch(errorHandler) {
try {
offlineDrmManager.downloadDrmLicenseForOffline(request)
offlineDrmManager.downloadDrmLicenseForOffline(id = id, request = request)
} catch (ex: DrmDownloadException){
//continue to try and use old license - a playback error appears elsewhere
}
Expand All @@ -126,7 +131,7 @@ internal class ExoplayerDownloadEngine @Inject constructor(
private fun startDownload(context: Context, downloadRequest: DownloadRequest) =
DownloadService.sendAddDownload(context, downloadService, downloadRequest, true)

private fun downloadHelper(context: Context, mediaRequest: AudioPlayable.MediaRequest): DownloadHelper {
private fun downloadHelper(id: String, context: Context, mediaRequest: AudioPlayable.MediaRequest): DownloadHelper {
val uri = mediaRequest.url.toUri()
val renderersFactory = createRenderersFactory(context)
val dataSourceFactory = DefaultHttpDataSource.Factory().setUserAgent(Constants.getUserAgent(context))
Expand All @@ -139,6 +144,7 @@ internal class ExoplayerDownloadEngine @Inject constructor(
}
val mediaItem = MediaItem.Builder()
.setUri(uri)
.setMediaId(id)
.build()
return when (@C.ContentType val type = Util.inferContentType(uri)) {
C.TYPE_HLS,
Expand Down
38 changes: 23 additions & 15 deletions Armadillo/src/main/java/com/scribd/armadillo/download/ExoplayerDownloadTracker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.scribd.armadillo.download

import android.net.Uri
import android.util.Log
import androidx.annotation.VisibleForTesting
import com.google.android.exoplayer2.offline.Download
Expand All @@ -14,7 +13,6 @@ import com.scribd.armadillo.actions.StopTrackingDownloadAction
import com.scribd.armadillo.actions.UpdateDownloadAction
import com.scribd.armadillo.error.DownloadFailed
import com.scribd.armadillo.error.UnableToLoadDownloadInfo
import com.scribd.armadillo.extensions.toUri
import com.scribd.armadillo.models.DownloadProgressInfo
import com.scribd.armadillo.models.DownloadState
import kotlinx.coroutines.CoroutineScope
Expand All @@ -35,8 +33,8 @@ import javax.inject.Singleton
*/
internal interface DownloadTracker {
fun init()
fun trackDownload(download: ExoplayerDownload)
fun getDownload(uri: Uri): ExoplayerDownload?
fun trackDownload(id: String, download: ExoplayerDownload)
fun getDownload(id: String, uri: String): ExoplayerDownload?
fun updateProgress()
suspend fun loadDownloads()
}
Expand All @@ -58,7 +56,7 @@ internal class ExoplayerDownloadTracker @Inject constructor(
private const val TAG = "DownloadTracker"
}

private val downloads = HashMap<Uri, ExoplayerDownload>()
private val downloads = HashMap<String, ExoplayerDownload>()
private val downloadIndex = downloadManager.downloadIndex

private var isInitialized = false
Expand Down Expand Up @@ -89,8 +87,8 @@ internal class ExoplayerDownloadTracker @Inject constructor(
.use { loadedDownloads ->
while (loadedDownloads.moveToNext()) {
val download = loadedDownloads.download
downloads[download.request.uri] = download
// If we want to resume downloads we should make a call here to the download service to begin download for this uri
downloads[download.request.id] = download
//If we want to resume downloads we should make a call here to the download service to begin download for this id
}
}
} catch (e: IOException) {
Expand All @@ -102,18 +100,22 @@ internal class ExoplayerDownloadTracker @Inject constructor(
}
}

override fun trackDownload(download: ExoplayerDownload) {
if (downloads.containsKey(download.request.uri)) {
override fun trackDownload(id: String, download: ExoplayerDownload) {
if (downloads.containsKey(id) || downloads.containsKey(download.request.uri.toString())) {
return
}
downloads[download.request.uri] = download
downloads[id] = download
}

override fun getDownload(uri: Uri): ExoplayerDownload? = downloads[uri]
override fun getDownload(id: String, uri: String): ExoplayerDownload? = downloads[id] ?: downloads[uri] //older usage

override fun updateProgress() {
downloadManager.currentDownloads.forEach { download ->
downloads[download.request.uri] = download
if(downloads.containsKey(download.request.uri.toString())){
downloads[download.request.uri.toString()] = download //older usage
} else {
downloads[download.request.id] = download
}
TestableDownloadState(download).toDownloadInfo()?.let {
dispatchActionsForProgress(it)
}
Expand All @@ -127,7 +129,11 @@ internal class ExoplayerDownloadTracker @Inject constructor(

override fun onDownloadChanged(downloadManager: DownloadManager, download: Download, finalException: Exception?) {
Log.v(TAG, "onDownloadChanged")
downloads[download.request.uri] = download
if(downloads.containsKey(download.request.uri.toString())){
downloads[download.request.uri.toString()] = download //older usage
} else {
downloads[download.request.id] = download
}
TestableDownloadState(download).toDownloadInfo()?.let {
dispatchActionsForProgress(it)
}
Expand All @@ -138,7 +144,8 @@ internal class ExoplayerDownloadTracker @Inject constructor(
*/
override fun onDownloadRemoved(downloadManager: DownloadManager, download: ExoplayerDownload) {
Log.v(TAG, "onDownloadRemoved")
downloads.remove(download.request.uri)
downloads.remove(download.request.id)
downloads.remove(download.request.uri.toString()) //older usage
TestableDownloadState(download).toDownloadInfo()?.let {
dispatchActionsForProgress(it)
}
Expand Down Expand Up @@ -175,6 +182,7 @@ internal class ExoplayerDownloadTracker @Inject constructor(
}

private fun stopTracking(downloadInfo: DownloadProgressInfo) {
downloads.remove(downloadInfo.url.toUri())
downloads.remove(downloadInfo.id.toString())
downloads.remove(downloadInfo.url) //older usage
}
}
10 changes: 5 additions & 5 deletions Armadillo/src/main/java/com/scribd/armadillo/download/drm/OfflineDrmManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ internal class OfflineDrmManager @Inject constructor(
private const val TAG = "OfflineDrmManager"
}

suspend fun downloadDrmLicenseForOffline(request: AudioPlayable.MediaRequest) {
suspend fun downloadDrmLicenseForOffline(id: String, request: AudioPlayable.MediaRequest) {
withContext(Dispatchers.IO) {
request.drmInfo?.let { drmInfo ->
val drmResult = when (@C.ContentType val type = Util.inferContentType(request.url.toUri(), null)) {
Expand All @@ -41,18 +41,18 @@ internal class OfflineDrmManager @Inject constructor(
)

// Persist DRM result, which includes the key ID that can be used to retrieve the offline license
secureStorage.saveDrmDownload(context, request.url, drmResult)
secureStorage.saveDrmDownload(context, id, drmResult)
Log.i(TAG, "DRM license ready for offline usage")
}
}
}

suspend fun removeDownloadedDrmLicense(request: AudioPlayable.MediaRequest) {
suspend fun removeDownloadedDrmLicense(id: String, request: AudioPlayable.MediaRequest) {
withContext(Dispatchers.IO) {
request.drmInfo?.let { drmInfo ->
secureStorage.getDrmDownload(context, request.url, drmInfo.drmType)?.let { drmDownload ->
secureStorage.getDrmDownload(context = context, id = id, drmType = drmInfo.drmType)?.let { drmDownload ->
// Remove the persisted download info immediately so audio playback would stop using the offline license
secureStorage.removeDrmDownload(context, request.url, drmInfo.drmType)
secureStorage.removeDrmDownload(context = context, id = id, drmType = drmInfo.drmType)

// Release the DRM license
when (val type = drmDownload.audioType) {
Expand Down
4 changes: 0 additions & 4 deletions ...lo/src/main/java/com/scribd/armadillo/download/drm/events/WidevineSessionEventListener.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import com.scribd.armadillo.StateStore
import com.scribd.armadillo.actions.LicenseAcquiredAction
import com.scribd.armadillo.actions.LicenseReleasedAction
import com.scribd.armadillo.di.Injector
import com.scribd.armadillo.encryption.SecureStorage
import com.scribd.armadillo.models.DrmType
import javax.inject.Inject

Expand All @@ -17,9 +16,6 @@ internal class WidevineSessionEventListener
@Inject
internal lateinit var stateStore: StateStore.Modifier

@Inject
internal lateinit var secureStorage: SecureStorage

@Inject
internal lateinit var context: Context

Expand Down
Loading
Loading