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

Jump to bottom

[docs] Add warning about download token exposure relates to #3605 and #3396 #3609

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[docs] Add warning about download token exposure relates to #3605 and #3396 #3609

wants to merge 1 commit into from

Conversation

FrederickEngelhardt
Copy link
Contributor

Description

Fixes #3605 #3396

  • warnings about tokens for public repos using expo which auto generates the keys within some critical files.
  • Reference to the android and iOS mapbox token setup guides for token creation and mention this is the recommended solution for public repos otherwise it violates mapbox private download token policies
  • Add alternative keychain solution for android which completely abstracts the key and removes risk of token theft via plain text.

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

@FrederickEngelhardt
- Add warning about download token exposure relates to rnmapbox#3605 …
31b4849 
…and rnmapbox#3396 github issues

- Reference to the android and iOS mapbox token setup guides for token creation and mention this is the recommended solution for public repos otherwise it violates mapbox private download token policies
- Add alternative keychain solution for android which completely abstracts the key and removes risk of token theft via plain text.
@FrederickEngelhardt FrederickEngelhardt mentioned this pull request Sep 2, 2024
Open
mfazekas
mfazekas reviewed Sep 2, 2024
View reviewed changes
android/install.md
@@ -36,6 +36,35 @@ allprojects {
}
```

#### Maven Token with keychain
Copy link
Contributor

Choose a reason for hiding this comment

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

It's awesome thanks, but I'm concerned about the added complexity. In general we recommend storing your tokens in ~/.netrc and ~/.gradle/gradle.properties, which should be good for most cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I can remove this. Those defined sections are good. Plus keystore does not transfer good across machines without an automation script to generate it.

ios/install.md
@@ -27,6 +27,8 @@ Add the following to your `ios/Podfile`:

You will need to authorize your download of the Maps SDK with a secret access token with the `DOWNLOADS:READ` scope. This [guide](https://docs.mapbox.com/ios/maps/guides/install/#configure-credentials) explains how to configure the secret token under section `Configure your secret token`.

For macOS consider using keychain to store secrets instead. There are cocoapods packages that store and allow secret injections for local builds. Something like [keychainaccess](https://cocoapods.org/pods/KeychainAccess) or [cocoapods-keys](https://github.com/orta/cocoapods-keys) could do this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above in general I don't see a problem with tokens in ~/.netrc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, only risk is it's a hardcoded string that can be read by other processes and apps on the machine. As mentioned above I can remove this.

plugin/install.md
Comment on lines +35 to +41
1. Recommended Approach. Follow the guidelines listed in the offical mapbox-gl guide for [iOS](https://docs.mapbox.com/ios/maps/guides/install/#configure-credentials) and [android](https://docs.mapbox.com/android/maps/guides/install/).
- For internal docs go to [ios-install](../ios/install.md) [android-install](../android/install.md)

2. Alternative approach **for private repos only**.
- :warning: If this is a public repo **DO NOT follow this approach as this will expose the mapbox download token**. Furthermore if the token has addition permissions (which it should not) it could lead to a monitary cost, security risk (stolen tiles etc) :warning:.
- **FYI** _Publicizing this private download token is against mapbox policy._
- Running expo prebuild, a require step, will statically generate the token within ios/Podfile and android/build.gradle. These files typically need to be committed for functionality so be sure that you are okay with sharing these tokens with your **private** team.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the following issues:

  • You're recommended approach doesn't work with eas that is the recommended way to build apps
  • You make the assumption that a public expo repo needs the iOS and android generated code checked in, I don't see why you'd want that. Most would use eas where they'd not see iOS and android generated at all.
  • You don't mention that .apk itself will have the token built in when using eas which is the biggest issue I think: Expo Prebuild Secret Key #3396 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@FrederickEngelhardt also the docs from rnmapbox.github.io. are in the https://github.com/rnmapbox/maps-docs/ repo

Copy link
Contributor Author

@FrederickEngelhardt FrederickEngelhardt Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yikes, my familiarity with EAS is minimal. I'll need to familiarize myself with it a bit.

So with EAS android and ios repos are to be ignored? IE git can have those directories ignored?

@Fuggel
Copy link

Fuggel commented Sep 3, 2024

I am also having troubles hiding my secret key when building my app with npx expo prebuild. This command exposes my secret key automatically. Can i hide that in gradle.properties somehow?

@mfazekas
Copy link
Contributor

mfazekas commented Sep 3, 2024

I am also having troubles hiding my secret key when building my app with npx expo prebuild. This command exposes my secret key automatically. Can i hide that in gradle.properties somehow?

If you don't set RNMapboxMapsDownloadToken and set your secret token in your .netrc and gradle.properties does that work?

RNMapboxMapsDownloadToken was implemented for remote build on eas.

@Fuggel
Copy link

Fuggel commented Sep 3, 2024

I am also having troubles hiding my secret key when building my app with npx expo prebuild. This command exposes my secret key automatically. Can i hide that in gradle.properties somehow?

If you don't set RNMapboxMapsDownloadToken and set your secret token in your .netrc and gradle.properties does that work?

RNMapboxMapsDownloadToken was implemented for remote build on eas.

Do you mean my global gradle.properties? Because Ive done it like this:
set my api key in app.config.js and eas.json.
When I run npx expo prebuild (--clean), it autogenerates new files and exposes my sk key in Podfile and /android/gradle.properties.

Is there a better way of handling it?

@FrederickEngelhardt
Copy link
Contributor Author

FrederickEngelhardt commented Sep 4, 2024 •
edited
Loading

Right so I think I need to

  • swap this recommended approach for brownfield, hybrid apps, or apps that need to commit android ios folders for other library integrations.
  • Add a section for recommended approach for EAS, and Private repos
  • Add a section for public repos mentioning that committing the app.config.js download key in plaintext would be a violation of mapbox policy.
    • Mention creating a local file or using something like dotenvx to inject the secrets in a more uniform way.
    • Ignore the iOS and Android repos built with expo prebuild

Something like...

If the app requires OR commits native directories publicly, then usage of (ios) $HOME/.netrc and (android) android/local.properties / $HOME/.gradle/gradle.properties file for the download secrets would be required in order to adhere to Mapbox's private token policy. Otherwise running expo prebuild will generate the files in plain text.

Apps would require committing their ios or android directories only if they did the following

  • the app is brownfield - a mixture of react-native and android
  • Has integrated other libraries that require custom code and do not support EAS or expo directly

Am I missing anything for these requirements?

Possible alternatives

  • pre-commit to remove / replace the key. Seems more complicated though and could be missed / overridden and then the key is still in a repo's git history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfazekas mfazekas mfazekas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RNMapboxMapsDownloadToken is exposed when using expo:prebuild under build.gradle and podfile
3 participants
@FrederickEngelhardt @Fuggel @mfazekas