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 SPM and CI setup to fetch the private WordPress-rs repo #23424

Merged
merged 12 commits into from
Jul 17, 2024

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Jul 16, 2024

Builds on top of #23412.

@mokagio mokagio changed the base branch from trunk to add/wordpress-rs July 16, 2024 23:31
@dangermattic
Copy link
Collaborator

dangermattic commented Jul 16, 2024

1 Warning
⚠️ Modules/Package.swift was changed without updating its corresponding Package.resolved. Please resolve the Swift packages in Xcode.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 17, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23424-c895955
Version25.1
Bundle IDorg.wordpress.alpha
Commitc895955
App Center BuildWPiOS - One-Offs #10304
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio mokagio force-pushed the mokagio/fetch-wordpress-rs-in-ci branch from 53a9f33 to b76d75f Compare July 17, 2024 01:52
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 17, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23424-c895955
Version25.1
Bundle IDcom.jetpack.alpha
Commitc895955
App Center Buildjetpack-installable-builds #9350
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio mokagio marked this pull request as ready for review July 17, 2024 04:04
add_host_to_ssh_known_hosts 'github.com'

export GIT_SSH_COMMAND="ssh -i $PRIVATE_REPO_FETCH_KEY -o IdentitiesOnly=yes"
echo "Git SSH command is now $GIT_SSH_COMMAND"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is tidied up version of what Pocket Casts iOS does, too.

See Automattic/a8c-ci-toolkit-buildkite-plugin#102 to track the possibility of DRYing at the CI plugin level.


Copy link
Contributor Author

Choose a reason for hiding this comment

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

SwiftLint picked this and the ones below in CI, so I addressed them because I like the look of a green build 🤓

@mokagio mokagio added the Tooling Build, Release, and Validation Tools label Jul 17, 2024
@mokagio mokagio requested review from jkmassel and a team July 17, 2024 04:41
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

We might want to discuss the security aspect of this approach 🤔 Maybe this is OK but still wanted to open the debate.

This means that if one of our non-trusted agent got compromised, anyone could get access to this bot private key and do stuff with it. I don't know which of our bots this private SSH you used belongs to, and thus which permissions it has, but if that key also has eg push/write access, that could be quite problematic.

This is exactly why, for the case of CI commands that require push access (eg when we need to du version bumps for Release-on-CI) we developed our secure approach of only running those git push operations on one of our trusted agents and only store the bot private key on those agents (instead of via secrets installed on all agents), to ensure the key is never accessible to non-trusted agents. See paaHJt-6EP-p2 for more details.


If the bot SSH key you're using here only has read only access to private repos and doesn't have write permissions (to git push nor comment or open/close issues/PR etc) then we might still be ok with that approach. But if it has too much permissions we might want to be wary of the security implications (even if our CI agents are not really accessible to an external actor as we have other protections in place, for these kind of things better add more layers of security in case one gets compromised)

Wdyt?

exit 1
fi

echo "--- :git: Change Git SSH key to fetch private dependencies"
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, technically speaking this doesn't change the SSH key (as in: replacing it) but adds it to the agent? (Meaning SSH will still try other keys in the agent if the first one(s) it tries fails?)

Copy link
Contributor

Choose a reason for hiding this comment

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

It changes GIT_SSH_COMMAND so I think it's an accurate comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right 👍

I think I conflated the use of -i alone (which would add the identity but, if used alone, still let ssh try other identities registered in the agent or the config file) with when -i is used in association with -o IdentitiesOnly=yes (like here) to ask the ssh-agent not to offer any keys, and only use the keys provided by -i on the command line, or by the config files.

To be even stricter we should also add -F /dev/null to also tell ssh not to use any keys configured in the config file either, to be sure that the only key it will proposed is the one provided via -i and not try any other key boforehand; but in the case of CI and how Buildkite works wrt to exposing keys to SSH, I don't think any Identity would be configured in the ~/.ssh/config of our agents and risk being used and tried before the bot's private key provided here… so ultimately -F /dev/null would probably unnecessary in this context.

@jkmassel
Copy link
Contributor

@AliSoftware – great points all around, and as a rule I'd agree.

In this narrow case it's fine because it's going to be an open-source project in a month or two anyway, at which point we'll remove this.

Thanks for contributing to a strong security posture with this!

@jkmassel jkmassel merged commit 474b512 into add/wordpress-rs Jul 17, 2024
24 checks passed
@jkmassel jkmassel deleted the mokagio/fetch-wordpress-rs-in-ci branch July 17, 2024 18:54
@AliSoftware
Copy link
Contributor

AliSoftware commented Jul 17, 2024

Btw, instead of using the SSH key of one of our bot account, couldn't we create a read-only deploy key on wordpress-rs and use that instead?

Deploy Keys are limited to the repo they are declared on, and for SPM cloning we only need read-only, so that seems ideal for the use case we have here, and way safer than using a bot's private SSH key which will have write access to many repos…

(And even if there happen to be multiple private repos that SPM needs to reach at some point (which isn't the case for WPiOS here but could be for other projects on which @mokagio have started to use that approach too already), I think we can just create one deploy key for each, and then either use insteadOf with more specific URL prefixes than just the host (one for each repo to rewrite), or alternatively use the same generic insteadOf that is used in this PR but then ssh-agent add each of the read-only deploy keys to the agent, or provide them all thru multiple -i options, so that SSH will try each key in turn on each of the requests until it finds the one that is accepted.)

jkmassel added a commit that referenced this pull request Jul 17, 2024
* Switch to SSH to fetch WordPress.rs

* Set up private repo fetching in WP Prototype Build CI step

* Extract steps to configure Git in CI and use in all builds

* Try to make `gym` use system SCM...

* Try to bypass dependencies resolution in `gym` altogether

* Source set up script because of the `export` call it makes

* Ensure `set-up-git-for-private-repos.sh` is sourced, not run

* Revert "Try to bypass dependencies resolution in `gym` altogether"

This reverts commit 5271617.

* Revert "Try to make `gym` use system SCM..."

This reverts commit 67585cd.

* Remove trailing whitespaces from a couple of files (SwiftLint)

See build annotation in
https://buildkite.com/automattic/wordpress-ios/builds/23085

* Update .buildkite/commands/set-up-git-for-private-repos.sh

Co-authored-by: Olivier Halligon <[email protected]>

* Fix comment character

---------

Co-authored-by: Jeremy Massel <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
@mokagio
Copy link
Contributor Author

mokagio commented Jul 18, 2024

@jkmassel @AliSoftware thank you for the suggestions and for merging at the same time. I like the iterative approach. Make it work, make it right, make it fast...

As a followup I:

Remaining things I haven't yet done because I need to hop offline:

  • Cleanup: Delete the ad hoc SSH key I created for this from wpmobilebot and from mobile-secrets
  • Drop a note about what other projects are doing with PTA instead of keys and why (for clarity)

jkmassel added a commit that referenced this pull request Jul 27, 2024
* Switch to SSH to fetch WordPress.rs

* Set up private repo fetching in WP Prototype Build CI step

* Extract steps to configure Git in CI and use in all builds

* Try to make `gym` use system SCM...

* Try to bypass dependencies resolution in `gym` altogether

* Source set up script because of the `export` call it makes

* Ensure `set-up-git-for-private-repos.sh` is sourced, not run

* Revert "Try to bypass dependencies resolution in `gym` altogether"

This reverts commit 5271617.

* Revert "Try to make `gym` use system SCM..."

This reverts commit 67585cd.

* Remove trailing whitespaces from a couple of files (SwiftLint)

See build annotation in
https://buildkite.com/automattic/wordpress-ios/builds/23085

* Update .buildkite/commands/set-up-git-for-private-repos.sh

Co-authored-by: Olivier Halligon <[email protected]>

* Fix comment character

---------

Co-authored-by: Jeremy Massel <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
jkmassel added a commit that referenced this pull request Jul 31, 2024
* Switch to SSH to fetch WordPress.rs

* Set up private repo fetching in WP Prototype Build CI step

* Extract steps to configure Git in CI and use in all builds

* Try to make `gym` use system SCM...

* Try to bypass dependencies resolution in `gym` altogether

* Source set up script because of the `export` call it makes

* Ensure `set-up-git-for-private-repos.sh` is sourced, not run

* Revert "Try to bypass dependencies resolution in `gym` altogether"

This reverts commit 5271617.

* Revert "Try to make `gym` use system SCM..."

This reverts commit 67585cd.

* Remove trailing whitespaces from a couple of files (SwiftLint)

See build annotation in
https://buildkite.com/automattic/wordpress-ios/builds/23085

* Update .buildkite/commands/set-up-git-for-private-repos.sh

Co-authored-by: Olivier Halligon <[email protected]>

* Fix comment character

---------

Co-authored-by: Jeremy Massel <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
jkmassel added a commit that referenced this pull request Jul 31, 2024
* Switch to SSH to fetch WordPress.rs

* Set up private repo fetching in WP Prototype Build CI step

* Extract steps to configure Git in CI and use in all builds

* Try to make `gym` use system SCM...

* Try to bypass dependencies resolution in `gym` altogether

* Source set up script because of the `export` call it makes

* Ensure `set-up-git-for-private-repos.sh` is sourced, not run

* Revert "Try to bypass dependencies resolution in `gym` altogether"

This reverts commit 5271617.

* Revert "Try to make `gym` use system SCM..."

This reverts commit 67585cd.

* Remove trailing whitespaces from a couple of files (SwiftLint)

See build annotation in
https://buildkite.com/automattic/wordpress-ios/builds/23085

* Update .buildkite/commands/set-up-git-for-private-repos.sh

Co-authored-by: Olivier Halligon <[email protected]>

* Fix comment character

---------

Co-authored-by: Jeremy Massel <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Aug 1, 2024
* Introduce WordPress RS

* Add URL Autodiscovery in the background to evaluate stability

* Use pre-production wordpress.rs build

* Update SPM and CI setup to fetch the private WordPress-rs repo (#23424)

* Switch to SSH to fetch WordPress.rs

* Set up private repo fetching in WP Prototype Build CI step

* Extract steps to configure Git in CI and use in all builds

* Try to make `gym` use system SCM...

* Try to bypass dependencies resolution in `gym` altogether

* Source set up script because of the `export` call it makes

* Ensure `set-up-git-for-private-repos.sh` is sourced, not run

* Revert "Try to bypass dependencies resolution in `gym` altogether"

This reverts commit 5271617.

* Revert "Try to make `gym` use system SCM..."

This reverts commit 67585cd.

* Remove trailing whitespaces from a couple of files (SwiftLint)

See build annotation in
https://buildkite.com/automattic/wordpress-ios/builds/23085

* Update .buildkite/commands/set-up-git-for-private-repos.sh

Co-authored-by: Olivier Halligon <[email protected]>

* Fix comment character

---------

Co-authored-by: Jeremy Massel <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>

* Rebase on `trunk`

* Remove unnecessary command to add `github.com` to known host in CI

Unnecessary because the SPM command will do that for us later.

* Rename `set-up-git-` `for-private-repos` to `to-fetch-wordpress-rs`

That's the only repos the script is currently configured to fetch

* Switch key used to fetch WordPress rs

* Use the public URL now that the repo is open source

* Remove unused default implementations

* Simplify the wordpress.rs PR so we can land it sooner

* Remove SSH-supporting code

* Remove BlogService+List

* Cleanup

* More rebase fixes

* Fix up events and nullability

---------

Co-authored-by: Gio Lodi <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
Co-authored-by: Gio Lodi <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Aug 7, 2024
* Introduce WordPress RS

* Add URL Autodiscovery in the background to evaluate stability

* Use pre-production wordpress.rs build

* Update SPM and CI setup to fetch the private WordPress-rs repo (#23424)

* Switch to SSH to fetch WordPress.rs

* Set up private repo fetching in WP Prototype Build CI step

* Extract steps to configure Git in CI and use in all builds

* Try to make `gym` use system SCM...

* Try to bypass dependencies resolution in `gym` altogether

* Source set up script because of the `export` call it makes

* Ensure `set-up-git-for-private-repos.sh` is sourced, not run

* Revert "Try to bypass dependencies resolution in `gym` altogether"

This reverts commit 5271617.

* Revert "Try to make `gym` use system SCM..."

This reverts commit 67585cd.

* Remove trailing whitespaces from a couple of files (SwiftLint)

See build annotation in
https://buildkite.com/automattic/wordpress-ios/builds/23085

* Update .buildkite/commands/set-up-git-for-private-repos.sh

Co-authored-by: Olivier Halligon <[email protected]>

* Fix comment character

---------

Co-authored-by: Jeremy Massel <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>

* Rebase on `trunk`

* Remove unnecessary command to add `github.com` to known host in CI

Unnecessary because the SPM command will do that for us later.

* Rename `set-up-git-` `for-private-repos` to `to-fetch-wordpress-rs`

That's the only repos the script is currently configured to fetch

* Switch key used to fetch WordPress rs

* Use the public URL now that the repo is open source

* Remove unused default implementations

* Simplify the wordpress.rs PR so we can land it sooner

* Remove SSH-supporting code

* Remove BlogService+List

* Cleanup

* More rebase fixes

* Fix up events and nullability

* Introduce `ViewLayer`

* Update SPM and CI setup to fetch the private WordPress-rs repo (#23424)

* Switch to SSH to fetch WordPress.rs

* Set up private repo fetching in WP Prototype Build CI step

* Extract steps to configure Git in CI and use in all builds

* Try to make `gym` use system SCM...

* Try to bypass dependencies resolution in `gym` altogether

* Source set up script because of the `export` call it makes

* Ensure `set-up-git-for-private-repos.sh` is sourced, not run

* Revert "Try to bypass dependencies resolution in `gym` altogether"

This reverts commit 5271617.

* Revert "Try to make `gym` use system SCM..."

This reverts commit 67585cd.

* Remove trailing whitespaces from a couple of files (SwiftLint)

See build annotation in
https://buildkite.com/automattic/wordpress-ios/builds/23085

* Update .buildkite/commands/set-up-git-for-private-repos.sh

Co-authored-by: Olivier Halligon <[email protected]>

* Fix comment character

---------

Co-authored-by: Jeremy Massel <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>

* WIP

* WIP

* Add WordPress REST API Helpers (#23450)

* Add keychain protocol and test implementation

This make it way easier to put services that depend on the keychain under test

* Add self-hosted blog utilities

* Casing fix
# Conflicts:
#	WordPress/WordPress.xcodeproj/project.pbxproj

* Fixups

* Add WP client

* Store application passwords separately from actual passwords

* Load blog options before showing new site

* Make unit tests compile

* Add a local feature flag for authenticating using application-password

* Remove set-up-git-for-private-repos.sh

* Break a retain cycle

* Set reachabilty status to unknown when receiving an unknown status

* Use the right app name for application-password authentication

* Remove 'Application Passwords' quick action

* Add a FIXME

* Remove emptry "UIHost" app

* Remove Blog+ParsedUrl.swift (unused)

* Retrieve application password for WordPressSite

* Remove WordPressClient reference from DashboardQuickActionsCardCell

* Tweak SiteSwitcherViewController hierachy

* Fix a retain cycle

* Add timestamp to app_name in the application password auth flow

* Remove OS version check

---------

Co-authored-by: Jeremy Massel <[email protected]>
Co-authored-by: Gio Lodi <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
Co-authored-by: Gio Lodi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tooling Build, Release, and Validation Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants