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.

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

[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
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 29 additions & 0 deletions android/install.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.


For a more secure approach on an macOS device, consider leveraging apple's keychain android could use a gradle keychain reader function. And directly use that variable within top level android/build.gradle.

```groovy
def getPassword(String currentUser, String keyChain) {
def stdout = new ByteArrayOutputStream()
def stderr = new ByteArrayOutputStream()
exec {
commandLine 'security', '-q', 'find-generic-password', '-a', currentUser, '-s', keyChain, '-w'
standardOutput = stdout
errorOutput = stderr
ignoreExitValue true
}
//noinspection GroovyAssignabilityCheck
stdout.toString().trim()
}

def mapboxDownloadToken = getPassword("<app_name>","<app_name>_mapbox_download_secret")
// Same config as the first part with this shared keychain secret used instead
allprojects.repositories.maven {
...(refer to mapbox setup)
credentials {
...
password = mapboxDownloadToken
}
}
```

### Using non default mapbox version

*Warning*: If you set a custom version, make sure you revisit, any time you update @rnmapbox/maps. Setting it to earlier version than what we exepect will likely result in a build error.
Expand Down
2 changes: 2 additions & 0 deletions ios/install.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.



Run `pod install` to download the proper mapbox dependency.

Expand Down
39 changes: 25 additions & 14 deletions plugin/install.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,33 @@ After installing this package, add the [config plugin](https://docs.expo.io/guid
}
```

### Setup Download Token

You'll need to provide `RNMapboxMapsDownloadToken` as well. This secret token requires the `DOWNLOADS:READ` scope. You can refer to the [iOS guide](https://docs.mapbox.com/ios/maps/guides/install/#configure-credentials), which explains how to configure this token under the section `Configure your secret token`.

```json
{
"expo": {
"plugins": [
[
"@rnmapbox/maps",
{
"RNMapboxMapsDownloadToken": "sk.ey...qg"
}
]
]
}
}
```
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.
Comment on lines +35 to +41
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.

Copy link
Contributor Author

@FrederickEngelhardt FrederickEngelhardt Sep 4, 2024

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?


```json
{
"expo": {
<!-- "plugins": [ -->
[
"@rnmapbox/maps",
{
<!-- WARNING This WILL BE PUBLIC if on a public repo -->
"RNMapboxMapsDownloadToken": "sk.ey...qg"
}
]
]
}
}
```

If you want to show the user's current location on the map with the [UserLocation](../docs/UserLocation.md) component, you can use the [expo-location](https://docs.expo.dev/versions/latest/sdk/location/) plugin to configure the required `NSLocationWhenInUseUsageDescription` property. Install the plugin with `npx expo install expo-location` and add its config plugin to the plugins array of your `app.{json,config.js,config.ts}`:

Expand Down
Loading