Android AES GCM support via preferedCipher option #123
Conversation
oblador
changed the title
|
I'll review this, but I would like to wait until the "cleaning" is done. |
|
@pcoltau Unfortunately, this was @Skvuddudutt's last assignment before his contract ended, so wouldn't assume it will be done. Can maybe help with cleaning up some of it later, but would be good if you could glance at the general approach first and see if you think it's worth continuing working on. |
|
Fair enough - I'll give it a look as soon as time permits.. |
|
Yeah sorry I was unclear. I did the PR to "save" the partial work I was
doing but didnt have time to finish. It hasnt been really tested and still
contains a bunch of logging. Also I think there is an issue with read
behaviour and auto migrating IIRC. I wont be doing any more work on it.
As Joel said, you might want to just look at it and see if it is something
you want to finish or just throw it away.
…On 5 April 2018 at 07:46, Pelle Stenild Coltau ***@***.***> wrote:
@oblador <https://github.com/oblador>
Fair enough - I'll give it a look as soon as time permits..
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#123 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABwN77f1RQnz8RnOEm0VXNdU1tftNrCaks5tla-pgaJpZM4TAKe5>
.
|
|
My review of this is done. As @Skvuddudutt says, we need to address some issue before we can merge this. We just need to figure out who should do that. I can't promise that I'll have the time in the near future.. |
|
@pcoltau can you clarify what issue needs to be addressed? If it's "read |
| } | ||
|
|
||
| private String getPreferedCipherStorage(ReadableMap options) { | ||
| String preferedCipher = options.hasKey("preferedCipher") ? options.getString("preferedCipher") : CipherStorageKeystoreAESGCM.CIPHER_OPTION_NAME; |
There was a problem hiding this comment.
We default to GCM here, which would indicate to the caller of this method that the user prefers GCM, which isn't the case. I would rename this method to getPreferedCipherStorage to getPreferedOrDefaultCipherStorage.
| service = getDefaultServiceIfNull(service); | ||
|
|
||
| CipherStorage currentCipherStorage = getCipherStorageForCurrentAPILevel(); | ||
| Log.w("CIPHER","getting generic password"); |
There was a problem hiding this comment.
I'm guessing all these logs are for debug purposes? If so, then we should remove them.
| } else { // stored encryption data is different from the format we want to use, but we should not auto-migrate | ||
| Log.w("CIPHER","used cipher for "+service+":"+resultSet.cipherStorageName+" is old, silently falling back to use it"); | ||
| CipherStorage oldCipherStorage = getCipherStorageByName(resultSet.cipherStorageName); | ||
| decryptionResult = oldCipherStorage.decrypt(service, resultSet.usernameBytes, resultSet.passwordBytes); |
There was a problem hiding this comment.
If the caller of this method has added the "preferedCipher" option AESGCM, but has used the previous version of this library and thus stored it as AESCBC, then the values won't be migrated here. I'm not sure if they should, but we did that for Conceal (just above).
| } | ||
|
|
||
| // currentCipherStorage now contains cipher with highest API-level, check if user prefer something else | ||
| if (preferedCipher != null && preferedCipher.length() > 0) { |
There was a problem hiding this comment.
All callers of the method use the getPreferedCipherStorage method, which never returns null or the empty string, so this check is not needed.
| "dev": true | ||
| } | ||
| } | ||
| } |
| return typeof serviceOrOptions === 'object' | ||
| ? serviceOrOptions.service | ||
| : serviceOrOptions; | ||
| } | ||
| }*/ |
There was a problem hiding this comment.
Just remove this code instead of commenting it out.
| private String getDefaultServiceIfEmpty(@NonNull String service) { | ||
| return service.isEmpty() ? DEFAULT_SERVICE : service; | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of this class is a near copy-paste of the CipherStorageKeystoreAESCBC class (which is understandable, because the code for de-/encryption is nearly the same) - but perhaps we could move much of the shared functionality to a base CipherStorageKeystoreAES class.
| } | ||
|
|
||
| private String getPreferedCipherStorage(ReadableMap options) { | ||
| String preferedCipher = options.hasKey("preferedCipher") ? options.getString("preferedCipher") : CipherStorageKeystoreAESGCM.CIPHER_OPTION_NAME; |
There was a problem hiding this comment.
Furthermore, we need to both document the preferedCipher string, so that users know that it exists, and perhaps also store it in a constant somewhere, so that it can be used from the RN side.
I forgot to "commit" my review comments 🙄- that is done now. Let me know if you have any questions. |
|
Does anyone continue to work on this topic? |
|
I see this PR #513 is doing the same think. Maybe it's more up to date with the master? |
|
AES GCM will be added with #678 |
by Joel's request.