Skip to content

adding tmate session and documentation around it #359

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

Conversation

Gregory-Pereira
Copy link
Collaborator

Addresses #357 and comments in #349
/cc @nerdalert @vishnoianil

@Gregory-Pereira Gregory-Pereira force-pushed the add-tmate-session-to-workflows branch 4 times, most recently from eb82fb2 to 9cba9d5 Compare May 17, 2024 15:29
@Gregory-Pereira
Copy link
Collaborator Author

RFR

@Gregory-Pereira Gregory-Pereira requested a review from nerdalert May 17, 2024 15:32
@Gregory-Pereira Gregory-Pereira force-pushed the add-tmate-session-to-workflows branch from 9cba9d5 to 1638b08 Compare May 17, 2024 15:35
@Gregory-Pereira Gregory-Pereira force-pushed the add-tmate-session-to-workflows branch from 1638b08 to 6752f71 Compare May 17, 2024 15:36

With `limit-access-to-actor` set to `true`, the action look who created the PR, and grab the public SSH keys stored in their Github account.
It will reject connections from any SSH private key that does not match the public key listed in the Github account.
This is recommended, as it prevents others from abusing your runners, but may be dissabled to allow a teamate to ssh instead.
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a really bad idea to allow an arbitrary PR author to ssh into a CI runner. Is that what this does?

Copy link
Collaborator Author

@Gregory-Pereira Gregory-Pereira May 17, 2024

Choose a reason for hiding this comment

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

Yes, it will allow a PR author to ssh into an ephemeral Github hosted runner, it is not to a self-hosted piece of infrastructure, but that is what this does. It is useful for debugging purposes. Should I comment them out or close the PR?

Copy link
Member

Choose a reason for hiding this comment

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

Right -- so bad actor A can open a PR and then ssh in to use it to mine MeowCoin or whatever

or worse, bad actor A gets to ssh in to build and push stuff to our image registry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, closing the issues then. Although I think this is more of a CI permissions issue ... because someone could still just include this action in any pr they run and achieve the same result. Regardless I will close this up now, if we want we can always reopen it.

Copy link
Member

Choose a reason for hiding this comment

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

That's true. I believe we have it set so at least the first time someone submits a PR an approval is required for CI to run. That's a pretty low bar to get past, though. We've got to change the setting to always requiring approval for outside collaborators.

image

Copy link
Member

@russellb russellb left a comment

Choose a reason for hiding this comment

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

Left a question inline

@Gregory-Pereira Gregory-Pereira added training-data-approved generated seed data approved, you may run ilab train. and removed training-data-approved generated seed data approved, you may run ilab train. labels May 17, 2024
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.

2 participants