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
+113 −84
Merged

[APT-10393] Support Rotating Content URLs #46

Show file tree
Hide file tree
Changes from 1 commit
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
36 changes: 21 additions & 15 deletions Armadillo/src/main/java/com/scribd/armadillo/encryption/SecureStorage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import android.content.SharedPreferences
import android.util.Base64
import android.util.Log
import com.scribd.armadillo.Constants
import com.scribd.armadillo.error.DrmDownloadException
import com.scribd.armadillo.models.DrmDownload
import com.scribd.armadillo.models.DrmType
import kotlinx.serialization.decodeFromString
Expand All @@ -28,9 +29,9 @@ internal interface SecureStorage {
@Singleton
internal class ArmadilloSecureStorage @Inject constructor(
@Named(Constants.DI.STANDARD_STORAGE) private val legacyStandardStorage: SharedPreferences,
@Named(Constants.DI.STANDARD_SECURE_STORAGE) private val secureStandardStorage: SharedPreferences,
@Named(Constants.DI.STANDARD_SECURE_STORAGE) private val secureStandardStorage: SharedPreferences?,
@Named(Constants.DI.DRM_DOWNLOAD_STORAGE) private val legacyDrmStorage: SharedPreferences,
@Named(Constants.DI.DRM_SECURE_STORAGE) private val secureDrmStorage: SharedPreferences
@Named(Constants.DI.DRM_SECURE_STORAGE) private val secureDrmStorage: SharedPreferences?
) : SecureStorage {
companion object {
const val DOWNLOAD_KEY = "download_key"
Expand All @@ -41,8 +42,8 @@ internal class ArmadilloSecureStorage @Inject constructor(
}

override fun downloadSecretKey(context: Context): ByteArray {
return if (secureStandardStorage.contains(DOWNLOAD_KEY)) {
val storedKey = secureDrmStorage.getString(DOWNLOAD_KEY, DEFAULT)!!
return if (secureStandardStorage?.contains(DOWNLOAD_KEY) == true) {
val storedKey = secureDrmStorage?.getString(DOWNLOAD_KEY, DEFAULT)!!
if (storedKey == DEFAULT) {
Log.e(TAG, "Storage Is Out of Alignment")
}
Expand All @@ -53,13 +54,13 @@ internal class ArmadilloSecureStorage @Inject constructor(
if (storedKey == DEFAULT) {
Log.e(TAG, "Storage Is Out of Alignment")
}
secureStandardStorage.edit().putString(DOWNLOAD_KEY, storedKey).apply()
secureStandardStorage?.edit()?.putString(DOWNLOAD_KEY, storedKey)?.apply()
legacyStandardStorage.edit().remove(DOWNLOAD_KEY).apply()
storedKey.toSecretByteArray
} else {
//no key exists anywhere yet
createRandomString().also {
secureStandardStorage.edit().putString(DOWNLOAD_KEY, it).apply()
secureStandardStorage?.edit()?.putString(DOWNLOAD_KEY, it)?.apply()
}.toSecretByteArray
}
}
Expand All @@ -71,48 +72,53 @@ internal class ArmadilloSecureStorage @Inject constructor(
}

override fun saveDrmDownload(context: Context, audioUrl: String, drmDownload: DrmDownload) {
if(secureDrmStorage == null){
throw DrmDownloadException(UnsupportedOperationException("This device cannot encrypt downloads"))
Copy link
Contributor

Choose a reason for hiding this comment

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

What should the user experience be if this happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Download failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything they can do to fix it?

Copy link
Contributor Author

@kabliz kabliz Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No. If their device is in the failed mode they can't download. They can still stream even this component fails to initialize.

}
val alias = getDrmDownloadAlias(audioUrl, drmDownload.drmType)
val value = Base64.encodeToString(Json.encodeToString(drmDownload).toByteArray(StandardCharsets.UTF_8), Base64.NO_WRAP)
secureDrmStorage.edit().putString(alias, value).apply()
secureDrmStorage.edit()?.putString(alias, value)?.apply()
}

override fun getDrmDownload(context: Context, audioUrl: String, drmType: DrmType): DrmDownload? {
val alias = getDrmDownloadAlias(audioUrl, drmType)
var download = secureDrmStorage.getString(alias, null)?.decodeToDrmDownload()
var download = secureDrmStorage?.getString(alias, null)?.decodeToDrmDownload()
if (download == null && legacyDrmStorage.contains(alias)) {
//migrate old storage to secure storage
val downloadValue = legacyDrmStorage.getString(alias, null)
download = downloadValue?.decodeToDrmDownload()
secureDrmStorage.edit().putString(alias, downloadValue).apply()
legacyDrmStorage.edit().remove(alias).apply()
if(secureDrmStorage != null) {
secureDrmStorage.edit()?.putString(alias, downloadValue)?.apply()
legacyDrmStorage.edit().remove(alias).apply()
}
}
return download
}

override fun getAllDrmDownloads(context: Context): Map<String, DrmDownload> {
val drmDownloads = secureDrmStorage.all.keys.mapNotNull { alias ->
val drmDownloads = secureDrmStorage?.all?.keys?.mapNotNull { alias ->
secureDrmStorage.getString(alias, null)?.let { drmResult ->
alias to drmResult.decodeToDrmDownload()
}
}.toMap()
}?.toMap()
val legacyDownloads = legacyDrmStorage.all.keys.mapNotNull { alias ->
legacyDrmStorage.getString(alias, null)?.let { drmResult ->
alias to drmResult.decodeToDrmDownload()
}
}.toMap()

return drmDownloads.plus(legacyDownloads)
return legacyDownloads.plus(drmDownloads ?: emptyMap())
}

override fun removeDrmDownload(context: Context, audioUrl: String, drmType: DrmType) {
val alias = getDrmDownloadAlias(audioUrl, drmType)
legacyDrmStorage.edit().remove(alias).apply()
secureDrmStorage.edit().remove(alias).apply()
secureDrmStorage?.edit()?.remove(alias)?.apply()
}

override fun removeDrmDownload(context: Context, key: String) {
legacyDrmStorage.edit().remove(key).apply()
secureDrmStorage.edit().remove(key).apply()
secureDrmStorage?.edit()?.remove(key)?.apply()
}

private val String.toSecretByteArray: ByteArray
Expand Down