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

Update to use SecKeyCreateRandomKey to generate key pairs #78

Merged
merged 12 commits into from
Nov 19, 2024

Conversation

LowAmmo
Copy link
Contributor

@LowAmmo LowAmmo commented Sep 30, 2022

SecKeyGeneratePair was deprecated with iOS 15

Upgraded to support building with Xcode 14 - with no warnings

Updated minimum supported versions -

  • MacOS - 11.5
  • iOS - 14.5
  • TVOS - 14.5
  • WatchOS - 7.5

Testing

  • Converted to use xctestplans
  • Added support to do iOS testing
    • Had to add compiler flags to enable successful testing on a simulator

Updating to switch from the deprecated SecKeyGeneratePair to SecKeyCreateRandomKey

Description

SecKeyCreateRandomKey

  • Removed use of the SecKeyGeneratePair API (that was deprecated with iOS 15.0) to now use the SecKeyCreateRandomKey

Minimum Versions

  • Updated the minimum OS Versions to more recent versions to address additional build warnings

Unit Testing

  • Added a test target to run unit tests on an iOS device
  • As part of this, had to use some compiler directives to support successfully running tests on the simulator
  • Also converted to using xctestplans - as that is the new standard for Xcode

Motivation and Context

It was done to eliminate warnings that we see in our application, and also to just update the repo to more recent standards

Issue: #77

How Has This Been Tested?

Tested locally by confirming successful unit tests.
Also tested in our application to verify change was passive

Checklist:

  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

Kris Kline added 2 commits September 30, 2022 12:55
SecKeyGeneratePair was deprecated with iOS 15

Upgraded to support building with Xcode 14 - with no warnings

Updated minimum supported versions -
* MacOS - 11.5
* iOS - 14.5
* TVOS - 14.5
* WatchOS - 7.5

Testing
* Converted to use xctestplans
* Added support to do iOS testing
  * Had to add compiler flags to enable successful testing on a simulator
@CLAassistant
Copy link

CLAassistant commented Sep 30, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ LowAmmo
❌ Kris Kline


Kris Kline seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@dannys42 dannys42 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! If there's any way you can exclude formatting changes, it would make it easier to evaluate.

Sources/CryptorRSA/CryptorRSAKey.swift Outdated Show resolved Hide resolved
Comment on lines -11 to +14
s.osx.deployment_target = "10.12"
s.ios.deployment_target = "10.3"
s.tvos.deployment_target = "10.3"
s.watchos.deployment_target = "3.3"
s.osx.deployment_target = "11.5"
s.ios.deployment_target = "14.5"
s.tvos.deployment_target = "14.5"
s.watchos.deployment_target = "7.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking the deployment targets! Just wanted to confirm if these are the minimum versions that will work with these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's a concern...can get away with going with even older versions...

I've just gotten in the habit with Apple of trying to stay as up to date as possible.

Comment on lines 30 to 35
<TestPlans>
<TestPlanReference
reference = "container:Tests/CryptorRSA-iOSTests/CryptorRSA-iOSTests.xctestplan"
default = "YES">
</TestPlanReference>
</TestPlans>
Copy link
Contributor

Choose a reason for hiding this comment

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

I must admit I have not yet used TestPlans, but they look really useful! How can I trigger them? I can't seem to find a way to exercise them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using the .xcodeproj it ties into the scheme. In the scheme 'Test' step, you can choose what "test plan" to run (instead of choosing what test target to run).

One of my favorite features is the "random" order - it just helps to pinpoint any tests that accidentally have a dependency on run order (very helpful if you have multiple people contributing to the same repo).

Otherwise, they just help keep things a little bit cleaner (I wonder if Apple's long term plan is to eliminate to the need to have a Test target as part of the project build??)

README.md Show resolved Hide resolved
Comment on lines 566 to 570
let parameters: [NSObject: Any] = [
kSecAttrKeyType: kSecAttrKeyTypeRSA,
kSecAttrKeySizeInBits: keySize.bits,
kSecPublicKeyAttrs: publicKeyAttributes,
kSecPrivateKeyAttrs: privateKeyAbbributes,
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing doesn't appear uniform here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guessing some got tabs & some got spaces...

I'll work to convert everything to use tabs

Tests/CryptorRSATests/CryptorRSATests.swift Outdated Show resolved Hide resolved
@LowAmmo
Copy link
Contributor Author

LowAmmo commented Oct 3, 2022

@dannys42 - Took a few commits to get the compiler directives right... But, I think the spacing is all good now! 😄

@LowAmmo
Copy link
Contributor Author

LowAmmo commented Feb 28, 2023

@dannys42 - Did similar clean up to remove the generated files and just rely on Package.swift

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@LowAmmo
Copy link
Contributor Author

LowAmmo commented Mar 29, 2023

@dannys42 - cleaned up the build to only build on valid/more recent build machines (to match the updates to the podspecs).

Seeing the odd issue where it suddenly has trouble connecting to the travis CI...but...hoping you might be able to get this (and other PRs merged in sometime soon...)

-Thanks!

@LowAmmo
Copy link
Contributor Author

LowAmmo commented Nov 18, 2024

@jayrhynas , @dannys42 - Any chance this could get reviewed and pulled?

@jayrhynas
Copy link
Contributor

@jayrhynas , @dannys42 - Any chance this could get reviewed and pulled?

Just FYI I'm not a maintainer of this project, I just contributed once :)

@dannys42 dannys42 merged commit a03491f into Kitura:master Nov 19, 2024
1 of 2 checks passed
@LowAmmo
Copy link
Contributor Author

LowAmmo commented Nov 19, 2024

@dannys42 - Not sure if this counts as "looking a gift horse in the mouth"...but any chance you could also do an updated publish to the cocoapod spec repo? 😃

@dannys42
Copy link
Contributor

@LowAmmo I'm happy to add you as the cocoapods maintainer if you would like to manage it for the Kitura repos. Last time I dealt with cocoapods, there was just too many issues with downtime. Also the update process is a bit too cumbersome for me.

@LowAmmo LowAmmo deleted the Issue77-SecKeyCreateRandomKey branch November 20, 2024 15:15
@LowAmmo
Copy link
Contributor Author

LowAmmo commented Nov 20, 2024

@dannys42 - Sure! I know everything is moving to SPM...but our internal build system isn't ready to switch over just yet, so being able to just keep cocoapods up-to-date would be much appreciated!

Let me know what you need from me to get that setup!

-Thanks!

@giovannitrezzi
Copy link

giovannitrezzi commented Nov 21, 2024

We’re also awaiting the updated publish to the CocoaPod spec repo and look forward to its completion.
Thank you very much for your efforts and support!

@giovannitrezzi
Copy link

Hi @dannys42 and @LowAmmo,
Just checking in on the status of the CocoaPods spec repo update. Are there any updates or progress to share?

Thank you for your efforts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants