Finalize strategy / documentation for storing Twitter Consumer Secret

[samtstern]

268 introduces Twitter as an IDP and has the consumer secret as a resource. We should evaluate the risks of this, find out what industry best practice is, and document our decision along with the tradeoffs.

[amandle]

I've thought a bit more about this. The most correct way to handle the secret is for the user to set up a server and request the consumer secret from there. In my opinion, this is too much to ask of our users given that this library is intended to be a one-stop solution for authentication.

I think we should stick with the current implementation or if we're really worried about it we should remove the Twitter IDP from all platforms.

Thoughts?

[samtstern]

What's the worst someone can do to you if they get this secret? And does Twitter have any recommendations on this or are they just doing "see no evil"?

On Thu, Oct 6, 2016 at 10:01 AM Aaron Mandle notifications@github.com wrote:

I've thought a bit more about this. The most correct way to handle the secret is for the user to set up a server and request the consumer secret from there. In my opinion, this is too much to ask of our users given that this library is intended to be a one-stop solution for authentication.

I think we should stick with the current implementation or if we're really worried about it we should remove the Twitter IDP from all platforms.

Thoughts?

— You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub https://github.com/firebase/FirebaseUI-Android/issues/269#issuecomment-252025042, or mute the thread https://github.com/notifications/unsubscribe-auth/AIEw6juG4e8SZLzPc1jenUtWTgN5aWmdks5qxSlOgaJpZM4JuhX8 .

[samtstern]

I did ~15mins of web searching and it seems that Twitter is basically saying "you shouldn't hardcode it in your app, but here's a tutorial showing how to hardcode it in your app".

I am inclined to just keep things as-is with a big ol' warning above it saying exactly what the risks are. If there is demand, in v1.1 we could provide an optional API called something like Task<String> fetchSecretFromTrustedServer() that the developer could implement to go and get their secret from the internet. I doubt there will be much overlap though between FirebaseUI devs and people running their own secure servers.

[drishi94]

I'm wary of baking this into FirebaseUI since we do consider it our best practices, but since it seems to be the best way to make it feasible for developers to use Twitter and Twitter tutorials itself seem to use it, I think we can move forward as noted. Definitely agree with sam's point that we should keep it, but make explicit what the risks are and look into changes for v1.1 based on developer demand.

[amandle]

Any suggestions for what the warning text should say? Maybe something like

WARNING: By obtaining your Consumer Key and Client Secret other programs can perform API operations as your app. Embedding the Consumer Key and Client Secret in your app, no matter how obfuscated, makes it vulnerable to being stolen.

[SUPERCILEX]

@amandle @samtstern

The most correct way to handle the secret is for the user to set up a server and request the consumer secret from there.

Wouldn't saving the key in the real time database work? We could put in the documentation that you have to add an entry such as twitter_api_secret: the secret (with a security rule that says you can't write to that location and can't read unless you're authenticated) and then then just pull from that location using the realtime database when logging-in to twitter. Or am I missing something?

[SUPERCILEX]

@amandle @samtstern So is there no way to store the Twitter secret in the real-time database now?

[amandle]

@SUPERCILEX we would like to have FirebaseUI-Auth not have dependencies on the db if possible

[SUPERCILEX]

K, thanks!

[brandall76]

Hi understand this issue was closed and your decision made (and I agree with it for the sake of being a simple plug-and-play solution), but I'm not keen on storing the credentials in such plain sight - For those of us that already store and fetch these credentials remotely, would an option to add them in during the IdpConfig building process be a solution:

        public Builder setSomeKey(String someKey) {
            someKey = someKey;
            return this;
        }

If the keys and ids etc are found to be null, you would then check for them in the String resources, as per now?

[samtstern]

@brandall76 the problem as I see it is that we need the Twitter secret at build time to get into the Android Manifest: https://github.com/firebase/FirebaseUI-Android/blob/e96aae57a5eaeed446ff103fd1b4b1063e25549b/auth/src/main/AndroidManifest.xml#L15

Do you know of some alternative to this? If so I agree, adding a builder method for the API key would be ideal.

[brandall76]

Thanks for your response @samtstern

From what I can establish the Manifest Meta-Data tag isn't actually needed for the Twitter authorisation process to function correctly. The tag is only required to supply the Fabric API Key, for use with other aspects of their SDK, such as Crashlytics.

However, if you remove the tag, the process will crash with:

Error dealing with settings
java.lang.IllegalArgumentException: Fabric could not be initialized, API key missing from AndroidManifest.xml. Add the following tag to your Application element 
<meta-data android:name="io.fabric.ApiKey" android:value="YOUR_API_KEY"/>
at io.fabric.sdk.android.services.common.ApiKey.logErrorOrThrowException(ApiKey.java:110)

To work around this issue, you can simply change the Manifest tag to the following:

        <meta-data
            android:name="io.fabric.ApiKey"
            android:value="@string/some_string_with_any_value"/>

This will suppress the error, purely due to a value (any value) being present when checked by the ApiKey class.

To test any further possible dependency, I removed the Twitter Strings and replaced the references to the them in the TwitterProvider constructor, with hard-coded values.

Everything works fine.

To resolve the need for a random String at all, Fabric would need to alter the thrown exception to a warning (logErrorOrWarn). Now they are owned by Google, I assume this is easier for you to ask! 😃

Let me know your thoughts - if you consider the placeholder string a viable release work-around, I'll knock up some code.

Ben

[samtstern]

@brandall76 thanks for investigating! Given that the Fabric is part of Google now, I will ask them if there are any consequences of putting a 'random' string in the meta-data entry.

[ashwinraghav]

Hey Team, Twitter assumes that mobile client keys (and secrets) are public and available to attackers by reverse engineering apps. Most threat models in the company make this assumption and put systems in place to minimize attack vectors. Also, given that it is really complicated to determine if an App's server is definitively talking it's own client and not a reverse engineered client, there is little value in leaving the secret at rest on the server. My understanding is that most clients (even outside of Twitter) that do not have a trusted browser/app that sits between the IDP and them end up having secrets stored on the client itself. Thoughts?

[brandall76]

Thanks @samtstern - while you're at it, any thoughts on decoupling the Twitter authorisation from Fabric entirely? It would remove a 'dependency'.

@ashwinraghav appreciate what you're saying, nothing is sacred... But, I would at least like to make it as hard as possible, if only for the sake of good practice. For this particular issue, the current skill level required to access the secret, is the ability to unzip an APK file....