[spaceship] create_certificate_signing_request: update from SHA-1 to SHA-256

[jaysoffian]

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

Update the two places where Fastlane creates CSRs to use SHA-256 (sha256WithRSAEncryption). This signature algorithm is accepted by Apple and CSRs created using Keychain Access at least since Ventura use SHA-256.

SHA-1 is increasingly deprecated by OS vendors. I noticed fastlane was still using SHA-1 when attempting to use fastlane pem under RedHat 9 which disables SHA-1 by default¹.

I noticed fastlane was still using SHA-1 when attempting to use fastlane pem under RedHat 9 which disables SHA-1¹. Updating fastlane to SHA-256 allows it to generate CSR under RedHat 9 without having to alter the RedHat 9 default crypto policy¹. SHA-256 is also what Keychain Access uses for CSRs at least since Ventura.

Description

Update the two copies of the create_certificate_signing_request function to use OpenSSL::Digest::SHA256.new instead of OpenSSL::Digest::SHA1.new

Testing Steps

I tested by running the new code under RedHat 9 and macOS Ventura using irb.

I inspected the CSR generated by the new code using openssl req -noout -text and verified that the "Signature Algorithm" is now sha256WithRSAEncryption which matches the algorithm used in CSRs generated by Keychain Access under macOS Ventura.

Footnotes

1. https://wiki.almalinux.org/release-notes/9.0.html#changelog ↩ ↩² ↩³

[lacostej]

Looks good to me

[lacostej]

more thoughts

• the fastlane_core/lib/fastlane_core/cert_checker.rb checks the sha1 certificate. Should we support both hashes now?
• why are these methods duplicated and why are we fixing only one of the doc?

the cert_checker might need fixing

[jaysoffian]

• the fastlane_core/lib/fastlane_core/cert_checker.rb checks the sha1 certificate. Should we support both hashes now?

Probably, but that's out of the scope of this PR. I just needed this code to run correctly on Linux. The cert_checker.rb code is macOS specific.

• why are these methods duplicated

There's quite a bit of duplicated code between Spaceship::Portal and Spaceship::ConnectAPI. I don't know why it was done this way.

and why are we fixing only one of the doc?

Oversight on my part. I've updated the PR to correct the typo in both files.

[rogerluan]

I'm afraid that changing how we create CSRs without changing how we check them might break things for macOS users that use both methods (creating one using sha256 and then attempting to check/validate it using sha1), no?

I understand the motivation was to fix only your Red Hat environment, but if this breaks fastlane for macOS users, then we need to batch all changes in a single PR.

I'm just raising the question here, though, it's actually not 100% clear to me whether it'd break or not. I don't fully grasp this portion of the codebase yet 😬 so any points would be helpful here @jaysoffian @lacostej 🙇

Thanks 🙏

[jaysoffian]

I'm afraid that changing how we create CSRs without changing how we check them might break things for macOS users that use both methods (creating one using sha256 and then attempting to check/validate it using sha1), no?

I understand the motivation was to fix only your Red Hat environment, but if this breaks fastlane for macOS users, then we need to batch all changes in a single PR.

I'm just raising the question here, though, it's actually not 100% clear to me whether it'd break or not. I don't fully grasp this portion of the codebase yet 😬 so any points would be helpful here @jaysoffian @lacostej 🙇

Thanks 🙏

Okay, I looked further at the code. These are totally different things. The change I made in this PR is to create CSRs with a SHA-256 digest.

This is unrelated to CertChecker's use of SHA1 digest. The CertChecker class isn't looking at CSRs. Rather, it's looking at certificates in the keychain and comparing them to on-disk cert files. It uses various invocations of security to enumerate certificates from the keychain and then parses the output of the security tool in order to extract the SHA1 digest. e.g. it makes calls like this:

• security find-identity -v -p codesigning
• security find-certificate -Z -a -c "Developer ID Installer"

The output of the first looks like this:

  1) DA39A3EE5E6B4B0D3255BFEF95601890AFD80709 "iPhone Developer: Joe Q Developer (ABCDEFGHIJ)"
     1 valid identities found

In this case DA39A3EE5E6B4B0D3255BFEF95601890AFD80709 is a SHA-1 digest. Separately, CertChecker uses OpenSSL to get the SHA1 digest of an on-disk file:

    def self.sha1_fingerprint(path)
      file_data = File.read(path.to_s)
      cert = OpenSSL::X509::Certificate.new(file_data)
      return OpenSSL::Digest::SHA1.new(cert.to_der).to_s.upcase
    rescue => error
      UI.error(error)
      UI.user_error!("Error parsing certificate '#{path}'")
    end

It then compares the two digests to see if the on-disk cert is the same as the one in the keychain.

So no, this change won't break CertChecker.

As well, since the CertChecker code is only relevant to macOS, there's no need to update it at this time to switch to a SHA-256 digest. In fact, unless we can get security to emit SHA-256 digests in all cases, we can't even do so.

So two totally unrelated and different use cases. One for the type of digest embedded in a CSR, the other for comparing on-disk certs to those in the keychain.

[rogerluan]

Thanks for your thorough analysis @jaysoffian! Super helpful!
Makes sense then, let's move forward!

Could you rebase your branch? I believe the issue with CI has been fixed in master already :) Thank you!

[jaysoffian]

Could you rebase your branch?

Done.

[jaysoffian]

@rogerluan can this be merged?

[jaysoffian]

@lacostej can this be merged?

[rogerluan]

💪