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

[spaceship] decrease App Store Connect API token issued-at-time to prevent server rejection #21583

Conversation

xinsight
Copy link
Contributor

@xinsight xinsight commented Oct 19, 2023

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.
  • I've added or updated relevant unit tests.

Motivation and Context

I have observed sporatic failures with delivering builds to testflight that result in an error message like this:

Authentication credentials are missing or invalid. - Provide a properly configured and signed bearer token, and make sure that it has not expired. Learn more about Generating Tokens for API Requests https://developer.apple.com/go/?id=api-generating-tokens

After much debugging and with this tip I confirmed that my machine time was slightly ahead of Apple's servers which caused the AppStore Connect token to be rejected as the issued-at-time was in the future.

Similar issues have been reported in #20516 and #21109.

Syncing servers to Apple's servers (using ntp.apple.com) is one solution to this problem, however, not everyone is able or willing to sync their system's clocks to Apple's (and Apple's servers might be out-of-sync!) so a more robust and permanent solution is needed.

Description

By reducing the issued-at-time (iat) in the jwt token, the token will only be rejected if a machine is over a minute in the future. (This value could of course, be increased, but one minute seems like a good starting point.)

Additionally, this fix also addresses a minor race condition in the unit tests where the issued time was being compared to the current time - which would fail if the clock second changed from the time when the token was generated to when the test was run.

Testing Steps

To reproduce this issue, add 60 seconds to the iat value in the payload in: https://github.com/fastlane/fastlane/blob/master/spaceship/lib/spaceship/connect_api/token.rb#L103 Attempting to perform any action that requires a token with spaceship will fail.

For example:

api_key = app_store_connect_api_key(
  key_id: "xxx",
  issuer_id: "xxx",
  key_filepath: "xxx.p8",
  duration: 1200, # optional (maximum 1200)
  in_house: false # optional but may be required if using match/sigh
)

latest_testflight_build_number(api_key: api_key, app_identifier: "xxx")

Repeat the command by removing 60 seconds from iat and it should pass.

@google-cla
Copy link

google-cla bot commented Oct 19, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@xinsight xinsight changed the title Decrease token issued at time to prevent rejection [spaceship] Decrease token issued-at-time to prevent rejection Oct 19, 2023
@artem-alexandrov
Copy link

It would be great to have this merged. I'm receiving this error very often sadly...

@orschaef
Copy link

Same here... please lets get that merged!

@artazar
Copy link

artazar commented Nov 15, 2023

For some reason this started to happen more and more.
It would be great to complete his PR to confirm this is exactly the cause or if it requires a completely separate investigation.
Thanks in advance!

PS. In our case macos build agents are provided by CI and cannot be managed from our side, so it's not possible to sync/change time. Just restart the build on another agent, where it then works fine.

RikSchefferAmsterdam pushed a commit to Amsterdam/amsterdam-app-frontend that referenced this pull request Nov 16, 2023
De release pipeline voor iOS geeft een authenticatie error. Het is een bekend probleem dat dit niet een echte error is, maar veroorzaakt wordt doordat de tijd van de aanvrager (Azure server) en de issuer (Apple server) out of sync zijn. Daardoor is de token verlopen zodra die is uitgereikt...

De fix is om deze sessie duur voor de token aanvraag te verhogen. Uit de spaarzame documentatie en GitHub issues meen ik op te maken dat de max 1200 ms is, dus daar zet ik 'm op zolang er geen echte fix is.

Er is wel een PR met een mogelijke fix (fastlane/fastlane#21583), die houd ik in de gaten, wordt vervolgd.
@mletynski
Copy link

Guys can we just merge this PR please ? It's critical for our builds.

@shichen85
Copy link

One more approval review let's go!

Copy link
Member

@max-ott max-ott left a comment

Choose a reason for hiding this comment

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

Looking good! Let's get this merge with the next release!

@KasperAndersson
Copy link

Hi @max-ott , do you know when the next release is planned? Thanks in advance

@b099l3
Copy link

b099l3 commented Nov 30, 2023

Using this workaround, till this fixed is merged:

Support got back to me privately, this is what they recommend as a workaround while engineering investigates:

steps:
  - name: Sync clock
    run: sudo sntp -sS time.windows.com

Surprisingly, the commend succeeds. I asked why, and they said that time.windows.com is permitted in Azure -- where the runners live -- whereas time.apple.com is not currently permitted in Azure.

Since the issue is sporadic, I haven't had enough builds yet to know if it fixes the problem.

actions/runner#2996 (comment)

@bjornoleh
Copy link

bjornoleh commented Nov 30, 2023

I am currently testing by pointing Gemfile to the branch for this PR:

gem 'fastlane', git: 'https://github.com/xinsight/fastlane', branch: 'decrease-token-issued-at-time-to-prevent-rejection'

I am comparing the success rate by running the same workflow on schedule in two different repositories. One with the proposed changes, the other without. I can verify that the PR helps, but unfortunately, I am also seeing failures due to the same error when testing the PR. It seems like there can be more than 60 second time offsets between the GitHub macos-13 runner and the Apple servers. Perhaps the limit should be extended. There ismention of >200 seconds time difference in actions/runner#2996, in case that is the same underlying problem.

The error I get is this:
Authentication credentials are missing or invalid. - Provide a properly configured and signed bearer token, and make sure that it has not expired. Learn more about Generating Tokens for API Requests https://developer.apple.com/go/?id=api-generating-tokens

Results:
Without this PR:
4 failures in 16 runs (25%)

With this PR:
2 failures in 14 runs (14%)

Initial testing of adding the 'Sync clock' step as mentioned by @b099l3 looks promising though.

Edit: Results from testing 'Sync clock' is reported here: actions/runner#2996 (comment)
(time differences from -204 to +100 seconds, and occasionally <1 second)

@kpriemchenko
Copy link

This is a good solution to an annoying problem! Let's merge this PR, please 👍

@bjornoleh
Copy link

This is a good solution to an annoying problem! Let's merge this PR, please 👍

The root cause with GithHub runner machine clocks should already have been sorted: actions/runner#2996 (comment).

Are you still seeing this type of failures?

@rogerluan rogerluan changed the title [spaceship] Decrease token issued-at-time to prevent rejection [spaceship] decrease App Store Connect API token issued-at-time to prevent server rejection Jan 14, 2024
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

I was on the fence in the beginning but this solution seems simple, efficient, and safe, IMO. LGTM 💪

Thanks for fixing this, @xinsight! I see a lot of people are facing this issue 🙏

@bjornoleh
Copy link

I was on the fence in the beginning but this solution seems simple, efficient, and safe, IMO. LGTM 💪

Thanks for fixing this, @xinsight! I see a lot of people are facing this issue 🙏

I think this change is redundant by now. Please see actions/runner#2996 (comment)

@rogerluan
Copy link
Member

Thanks for pointing that out @bjornoleh !

That seems to solve this issue for everyone using fastlane with GitHub Actions, but might not be the case for other CI providers? I'm ok with closing this PR if everyone facing this issue were GHA users though 😮

From a quick search, it seems like #21109 OP was experiencing this in Jenkins 😬

@kpriemchenko
Copy link

No, I'm using Fastlane on a custom CI/CD server and started facing auth rejects from Apple last week. So I'm very much interested in merging this PR.

Syncing time with apple server works for the first command (upload_to_testflight), but next command (waiting for build processing and then setting distributing groups) fails again with the same error:

Authentication credentials are missing or invalid. - Provide a properly configured and signed bearer token, and make sure that it has not expired. Learn more about Generating Tokens for API Requests https://developer.apple.com/go/?id=api-generating-tokens

@lacostej lacostej self-requested a review January 15, 2024 12:19
Copy link
Collaborator

@lacostej lacostej left a comment

Choose a reason for hiding this comment

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

let's add this.

@lacostej lacostej merged commit 2beac83 into fastlane:master Jan 17, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.