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

GitHub proxy part 6: proxing Git using SSH transport #49980

Merged
merged 23 commits into from
Jan 14, 2025
Merged

Conversation

greedy52
Copy link
Contributor

Related:

will stack another PR for tsh git ssh/config/clone commands on top of this

@greedy52 greedy52 mentioned this pull request Dec 10, 2024
17 tasks
Base automatically changed from STeve/48762_github_oauth_flow to master December 10, 2024 17:50
@greedy52 greedy52 changed the base branch from master to STeve/48762_audit_log December 11, 2024 02:53
@greedy52 greedy52 changed the base branch from STeve/48762_audit_log to master December 11, 2024 02:53
@greedy52 greedy52 force-pushed the STeve/48762_ssh branch 2 times, most recently from a5cb9a6 to 97bbddf Compare December 12, 2024 01:33
@greedy52 greedy52 marked this pull request as ready for review December 12, 2024 01:59
Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

Are you sure that hooking into lib/srv/forward.Server to essentially add a completely separate mode gated by a flag (or like three different and potentially conflicting flags) is easier than writing something new that's going to be structurally guaranteed to work as intended just for the purpose of forwarding the ssh git protocol?

lib/auth/authclient/clt.go Outdated Show resolved Hide resolved
lib/proxy/router.go Outdated Show resolved Hide resolved
lib/proxy/router.go Show resolved Hide resolved
lib/srv/forward/sshserver.go Outdated Show resolved Hide resolved
lib/srv/forward/sshserver.go Outdated Show resolved Hide resolved
lib/reversetunnel/remotesite.go Outdated Show resolved Hide resolved
lib/srv/authhandlers.go Outdated Show resolved Hide resolved
lib/srv/git/git.go Outdated Show resolved Hide resolved
lib/srv/git/github.go Outdated Show resolved Hide resolved
lib/srv/git/github.go Outdated Show resolved Hide resolved
@greedy52
Copy link
Contributor Author

greedy52 commented Dec 16, 2024

Are you sure that hooking into lib/srv/forward.Server to essentially add a completely separate mode gated by a flag (or like three different and potentially conflicting flags) is easier than writing something new that's going to be structurally guaranteed to work as intended just for the purpose of forwarding the ssh git protocol?

will do 👍

@greedy52
Copy link
Contributor Author

i am splitting out some parts of this PR to separate ones like

@public-teleport-github-review-bot

@greedy52 - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

@greedy52 greedy52 requested a review from espadolini January 2, 2025 14:19
@greedy52 greedy52 requested review from Tener and GavinFrazar and removed request for flyinghermit January 2, 2025 15:09
@greedy52
Copy link
Contributor Author

greedy52 commented Jan 7, 2025

@espadolini PTAL

@greedy52
Copy link
Contributor Author

greedy52 commented Jan 9, 2025

In general, I wonder about the decision to reuse ssh connection functionality for this. If we ever want to support other protocols then suddenly we need to reinvent a lot of things.

I can't speak for the decision to use the git ssh protocol vs the git http protocol, but if we're going to use the git ssh protocol, reusing our existing SSH transport makes sense IMO. My initial recommendation was to actually make the configuration bits real static nodes in a similar fashion to the openssh ones, but we've decided to use a dedicated configuration type for git servers.

The very first git proxy poc is actually http-based for AWS CodeCommit using AWS app access. But AWS CodeCommit is dead now.

Like Edoardo said, this is what we end up with after a few iterations of poc and RFD discussions. GitLab supports SSH CA as well. Even if we have to do HTTP later, I don't think it's that difficult.

@greedy52 greedy52 requested a review from Tener January 9, 2025 21:52
@greedy52
Copy link
Contributor Author

@espadolini @Tener could you take another look? thanks!

api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
@@ -626,6 +626,9 @@ func (s *ServerV2) githubCheckAndSetDefaults() error {
return trace.Wrap(err, "invalid GitHub organization name")
}

// Set SSH host port for connection and "fake" hostname for routing. These
// values are hard-coded and cannot be customized.
s.Spec.Addr = "github.com:22"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have to happen in CASD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could move it to backend create but I would prefer it here as it should not have other values.

lib/auth/authclient/clt.go Outdated Show resolved Hide resolved
lib/proxy/router.go Show resolved Hide resolved
lib/srv/git/forward.go Outdated Show resolved Hide resolved
lib/srv/git/forward.go Outdated Show resolved Hide resolved
lib/sshutils/reply.go Outdated Show resolved Hide resolved
lib/sshutils/exec.go Show resolved Hide resolved
lib/sshutils/reply.go Show resolved Hide resolved
@greedy52 greedy52 requested a review from rosstimothy January 13, 2025 20:17
lib/sshutils/reply.go Outdated Show resolved Hide resolved
lib/srv/git/forward.go Outdated Show resolved Hide resolved
@greedy52
Copy link
Contributor Author

greedy52 commented Jan 14, 2025

@r0mant may i get an admin approval for the large size and an excludeflake?

flaky test detector failed on lib/srv/regular due to timeout:

✓  lib/srv/git (4.288s) (coverage: 62.9% of statements)
✓  lib/sshutils (3.388s) (coverage: 7.7% of statements)
✓  lib/proxy (7.197s) (coverage: 28.8% of statements)
✓  lib/srv (7.295s) (coverage: 2.3% of statements)
✓  lib/services (14.516s) (coverage: 3.0% of statements)
✖  lib/srv/regular (10m0.411s) (-test.shuffle 1736869325460463256)

There is no change to that package besides adding a new param to reverse tunnel server to three existing tests.

@r0mant
Copy link
Collaborator

r0mant commented Jan 14, 2025

/excludeflake *

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Bot.

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from GavinFrazar January 14, 2025 20:11
@greedy52 greedy52 added this pull request to the merge queue Jan 14, 2025
Merged via the queue into master with commit f0abbce Jan 14, 2025
44 checks passed
@greedy52 greedy52 deleted the STeve/48762_ssh branch January 14, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/lg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants