Conversation
There was a problem hiding this comment.
FYI I took this from
teleport/tool/tsh/common/proxy.go
Line 555 in 39af8d3
|
@ravicious @greedy52 this PR is still in draft as I have to handle certs expiration, but this shouldn't block the review. I'd like you to pay special attention to the Go code 🙏. I was trying follow existing patterns, but there is a chance that something should be done differently. |
There was a problem hiding this comment.
We need integration tests for these gateways. Take a look at TestTeletermKubeGateway and testTeletermGatewaysCertRenewal and compare them to existing app access integration tests if there are any. We'll likely need to add a new test function, unless the dumper app can be enabled somehow in tests. In this case, we could reuse the cluster from testTeletermGatewaysCertRenewal, just so that we don't have to spend time on spinning up a new cluster just for app gateways.
There was a problem hiding this comment.
Wait, I was too fast with asking for those tests. The existing integration tests depend on cert expiry being handled. I can add those tests when adding the middleware. If you started any work on this, please DM me the patch.
There was a problem hiding this comment.
We might be able to reuse getConnectionGatewayOperations here instead of adding a new function, no?
There was a problem hiding this comment.
By that I mean that in contexts where we can treat a gateway as a gateway with a generic target, we should do so. After applying the two suggestions mentioned above, getConnectionGatewayAppOperations becomes pretty much identical to getConnectionGatewayOperations. This suggests that we should be able to use TrackedGatewayConnection for both databases and apps. targetUri could be then used to differentiate between them where needed, for example in the UI when saying "DB" vs "APP".
There was a problem hiding this comment.
That's right, those gateway types are really similar. Probably we could go even further and use a single type for a document? 🤔
I'm only thinking about the backward compatibility, what will happen if an older version of Connect will try to open an app connection treating it like a db? (probably nothing good)
OTOH, now the app just crashes when it see an unsupported connection, I don't know what is worse :)
There was a problem hiding this comment.
That's right, those gateway types are really similar. Probably we could go even further and use a single type for a document? 🤔
I wasn't thinking too much about documents because they have considerably different UI, especially Kube vs the rest. OTOH there's a lot of similar actions (like changing the port) which could be refactored to a hook or reused in some other way between db and app gateways (I haven't looked at the UI code for app gateways in this PR yet).
In case of connections, the only meaningful difference between is the label shown in the list, or at least that's what I thought until you mentioned backward compatibility.
I'm only thinking about the backward compatibility, what will happen if an older version of Connect will try to open an app connection treating it like a db? (probably nothing good)
That's a good shout. It's more like forward compatibility I suppose. It's worth checking out, but I think besides UI inconsistencies nothing should make the app unusable? The Electron app will attempt to create a gateway for the given target URI, but tshd should fail at either parsing the target URI (if the given version of tshd supports kube gateways where we inspect if the target URI is of a db or a kube cluster) or finding the database.
There was a problem hiding this comment.
Right now if I go back to the previous version of the app which doesn't support app gateways, I get:
TypeError: Cannot destructure property 'rootClusterUri' of 'this._trackedConnectionOperationsFactory.create(...)' as it is undefined.
at eval (webpack-internal:///./src/ui/services/connectionTracker/connectionTrackerService.ts:165:9)
at Array.map (<anonymous>)
at ConnectionTrackerService.getConnections (webpack-internal:///./src/ui/services/connectionTracker/connectionTrackerService.ts:162:35)
at useConnections (webpack-internal:///./src/ui/TopBar/Connections/useConnections.ts:36:35)
at Connections (webpack-internal:///./src/ui/TopBar/Connections/Connections.tsx:52:86)
at renderWithHooks (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:16305:18)
at mountIndeterminateComponent (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:20069:13)
at beginWork (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:21582:16)
at HTMLUnknownElement.callCallback (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:4164:14)
at Object.invokeGuardedCallbackDev (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:4213:16)
There was a problem hiding this comment.
And once I reopen the docs:
Error: Unhandled case: [object Object]
at assertUnreachable (webpack-internal:///../shared/utils/assertUnreachable.ts:27:9)
at getResourceUri (webpack-internal:///./src/ui/services/workspacesService/documentsService/documentsUtils.ts:64:75)
at useActiveDocumentClusterBreadcrumbs (webpack-internal:///./src/ui/StatusBar/useActiveDocumentClusterBreadcrumbs.ts:41:109)
at StatusBar (webpack-internal:///./src/ui/StatusBar/StatusBar.tsx:46:135)
at renderWithHooks (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:16305:18)
at updateFunctionComponent (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:19583:20)
at beginWork (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:21596:16)
at HTMLUnknownElement.callCallback (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:4164:14)
at Object.invokeGuardedCallbackDev (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:4213:16)
at invokeGuardedCallback (webpack-internal:///../../../node_modules/react-dom/cjs/react-dom.development.js:4277:31)
There was a problem hiding this comment.
I decided to remove doc.gateway_app and connection.app types. They were mostly copies of our regular document gateway and the gateway connection. It is much better to simply differentiate them by their URI.
If we ever need to add more properties to the app gateways, we can always create a new type.
Another advantage is that it fixes the errors that show up after downgrading the app. In case of connection, it will be shown as a db connection. If you try to connect, you will see this nice error:

The only thing that I have to fix, is to use routing.parseClusterUri instead of routing.parseDbUri here, since it can be also an app uri (we could use routing.parseClusterUri from the beginning, we don't even need anything from db at that point).
I will create a PR and backport it.
There was a problem hiding this comment.
thanks for inviting me for a preview 😄 . basic go implementation looks good to me 👍 . I can do some testing once everything is ready. I do have a few comments.
-
Handle expired certs
You can use a similar middleware as db, but there is no need to LocalProxy.CheckCertSubject with db route. I would suggest split LocalProxy.CheckDBCerts to have a separate LocalProxy.CheckCertExpiry without the CheckCertSubject part, and use that for App Access.
-
Definitely a stretch, is Teleport Connect able to render web pages? =p. HTTP apps renders web pages in Web App. Wonder Teleport Connect can do both WebPage and terminal modes.
-
I see cloud apps (AWS/Azure/GCP) are unsupported for now. Likely will need quite some refactoring so the same logic can be shared between
tshand Connect. 😭
This is a copy & paste from other connection kinds.
0cb5380 to
5b1151a
Compare
Thanks for the advice! In the end, we decided to move this part out of this PR and address it separately (you can see all tasks here), so it won't block this PR.
We want to support web apps, but by opening them a browser.
Yes, supporting cloud apps required more work, so we left it unsupported (unfortunately, we don't have much time until v15 release). |
ravicious
left a comment
There was a problem hiding this comment.
I looked through most of the UI code, but I'm yet to actually use it.
ravicious
left a comment
There was a problem hiding this comment.
I left comments about some UI stuff that I think we should address and about a bunch of minor things, but other than that it looks good. 👍
# Conflicts: # web/packages/teleterm/src/services/tshd/types.ts # web/packages/teleterm/src/ui/DocumentGateway/useDocumentGateway.ts # web/packages/teleterm/src/ui/services/clusters/clustersService.ts
|
I merged the PR that unifies gateway offline screens, so this PR now uses the correct screen. |
* Allow creating app gateways in tshd * Add UI for document gateway app * Show apps in connections This is a copy & paste from other connection kinds. * Capture app protocol * Start app proxy when clicking on 'Connect' in app * Remove `removeAppGateway` * `appUri` -> `targetUri` * Add TCP and HTTP constants * Add CLI command for HTTP apps * Add `makeAppGateway` * Specify `handleChangePort` dependencies correctly * Remove `doc.gateway_app` and `connection.app`, instead differentiate gateways by URI * Correctly report protocol usage * Mention that AWS apps are supported in tsh * Rename constants and add godoc * Add a TODO comment about dialogs for connecting to unsupported apps * `onRun` -> `onButtonClick`, `runButtonText` -> `buttonText` * Show a notification after copying to clipboard * Make the message about unsupported gateways more precise * Revert mistakenly removed `document.targetUri` from `getResourceUri` * Remove 'Local app proxy' header * Post-merge fixes * Use proper component for the 'offline gateway' state * Support all gateway types in relogin UI * Fix JSdoc comment * Add a TODO comment about the docs link --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>
* Refactor desktop player Adopt an approach similar to the SSH player (more standard react calls and less event emitting). This also updates the progress bar to be more of a "dumb" component that just renders state, and leaves the smoothing out of progress updates to the client. In addition, this adds support for seeking by dragging the progress bar to a particular portion of the video. Fixes #17199 * Prevents EOF from being reported as a tdp.Notification error at the end of every session * Removes duplicated code, handles errors in handleRDPFastPathPDU, ensures we always spit out some message if an error notification pops up in the UI * Add dynamic/ prefix to server info labels (#36219) This change: - Adds the dynamic/ prefix to labels from a server_info resource created with tctl - Forbids labels with the dynamic/ prefix form being used in deny rules for new roles. Existing roles will generate warnings in tctl as well as a cluster alert. * Address deprecation TODOs for or before v15. (#36473) * Convert insecure-drop to drop for unsupported clients (#35803) This change converts the insecure-drop host user creation mode to drop for clients that don't support it. * discovery: remove update if discovery group differs (#36472) * discovery: remove update if discovery group differs This PR removes code marked to be removed in Teleport 15 that updated unconditionally kubernetes and databases that were discovered using a different discovery group. Until this PR, if the `onCreate` function returned `trace.AlreadyExists` error, the resource ended up being updated without any condition. After this PR, the resource is only updated if the existing resource has an empty discovery group. This behavior is kept to ensure that users that migrate from bad configs don't need to delete resources manually. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com> * Update discovery.go Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * handle code review suggestions --------- Signed-off-by: Tiago Silva <tiago.silva@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * Refactor Kubernetes Exec sessions upgrade logic (#36325) * Refactor Kubernetes Exec sessions upgrade logic Signed-off-by: Tiago Silva <tiago.silva@goteleport.com> * handle code review suggestions * Update lib/kube/proxy/forwarder.go Co-authored-by: Anton Miniailo <anton@goteleport.com> * Update lib/kube/proxy/forwarder.go Co-authored-by: Anton Miniailo <anton@goteleport.com> * handle code review suggestions --------- Signed-off-by: Tiago Silva <tiago.silva@goteleport.com> Co-authored-by: Anton Miniailo <anton@goteleport.com> * Deduplicate yarn.lock (#36560) * Deduplicate yarn.lock * Fix types in ProgressBar * docs: updates to tsh connect your client (#36526) * docs: updates to tsh connect your client * remove extra dash * Add backend code for listing EKS clusters through AWS OIDC integration. (#36489) * Add backend code for listing EKS clusters through AWS OIDC integration. * Remove leftover commented code. Co-authored-by: Marco André Dinis <marco.dinis@goteleport.com> * Add godoc. * Remove unneeded intialization of a parameter. * Reduce nesting. * Rename ExtraLabels to JoinLabels. --------- Co-authored-by: Marco André Dinis <marco.dinis@goteleport.com> * Update devbox. (#36193) * Update AWS account ID used in `update-ami-ids` GHA workflow (#36556) PR #36153 updated the AWS account ID used for pulling AMIs, but the GHA workflow was not updated to use this updated AWS account ID. Ref #34282. * Fix accesslist `tctl` (#36531) * Support unmarshalling accesslists manifests wihtout next_audit_date * Support `tctl get access_lists` * Fix accesslist reference * accountrecovery.go: Unconditionally delete the token after use (#36527) A previous conditional was allowing a replay attack on the recovery process. Although discovery of this token is a high bar for an attacker, we should be able to unconditionally delete this token after it's used. * Add app gateways to Connect (#36393) * Allow creating app gateways in tshd * Add UI for document gateway app * Show apps in connections This is a copy & paste from other connection kinds. * Capture app protocol * Start app proxy when clicking on 'Connect' in app * Remove `removeAppGateway` * `appUri` -> `targetUri` * Add TCP and HTTP constants * Add CLI command for HTTP apps * Add `makeAppGateway` * Specify `handleChangePort` dependencies correctly * Remove `doc.gateway_app` and `connection.app`, instead differentiate gateways by URI * Correctly report protocol usage * Mention that AWS apps are supported in tsh * Rename constants and add godoc * Add a TODO comment about dialogs for connecting to unsupported apps * `onRun` -> `onButtonClick`, `runButtonText` -> `buttonText` * Show a notification after copying to clipboard * Make the message about unsupported gateways more precise * Revert mistakenly removed `document.targetUri` from `getResourceUri` * Remove 'Local app proxy' header * Post-merge fixes * Use proper component for the 'offline gateway' state * Support all gateway types in relogin UI * Fix JSdoc comment * Add a TODO comment about the docs link --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> * Allow configuration of Okta access list importing. (#36569) Configuration options for enabling/disabling Okta access list importing have been added. This defaults to false. This has been added to both to Okta plugin settings and the standalone Okta service. Co-authored-by: Trent Clarke <trent@goteleport.com> * Fix example mysql grant all command (#36519) Fixing example mysql grant all command, because current one fails with `ERROR 1102 (42000): Incorrect database name ' % '` because of extra spaces around `%` * lib/teleterm app access: Add middleware for handling expired certs (#36520) * Add middleware for app gateways * Accommodate for app gateways in integration tests The previous version of tests depended on receiving helpers.TeleInstance from higher up. It was then used to generate valid and expired user certs, as well as to get client.TeleportClient. proxy.Suite (Kube tests) and dbhelpers.DatabasePack (db tests) expose helpers.TeleInstance so it wasn't a problem. However, appaccess.Pack, which we're going to use for app access tests, does not expose it. To work around that, we introduce two new fields to gatewayCertRenewalParams, tc (which accepts client.TeleportClient) and generateAndSetupUserCreds. These two fields get rid of the dependency on helpers.TeleInstance. * Add integration tests for app gateways * Switch to the new account settings screen (#36525) * Migrate `RotateCertAuthority` to gRPC (#36536) * Add RotateCertAuthority to gRPC TrustService. * Add gRPC server implementation. * Move RotateRequest type to api/types. * Mark deprecated HTTP rotate endpoint for deletion. * Add client implemenation and HTTP fallback. * Update go-oidc to get final go-jose v2 -> v3 updates (#36514) * Update go-oidc to get final go-jose v2 updates This updates our replaced go-oidc fork to use a tag with go-jose updated to v3: gravitational/go-oidc#19 This update removes the final usage of v2, and fully addresses the GHSA-2c7c-3mj9-8fqh DoS. * Update gopkg.in/go-jose/go-jose.v2 to 2.6.2 to get p2c DoS fix * Add ClusterDropdown component (#36310) This replaces the current ClusterSelector in the TopBar and adds the functionality to ClusterDropdown component on relevant pages * fix: Verify MFA device locks during authentication (#36471) * Test authn and password change with a locked user * Verify MFA device locks during authentication * Configure a LockWatcher in the passwordSuite setup * Appease linter * Update Toggle component styles (#36535) * Reintroduces the changes in #33273 which were erroneously deleted in #33273 (#36538) * Route to server by public addr (#36584) This change fixes a bug where tsh ssh could not dial a node with its public address. * Add EKS Discover into web testplan. (#36578) * Remove unused static token endpoints. (#36545) * Ensure player finishes at 100% * Fix style for disabled progress bar * Remove unused file * fix import order --------- Signed-off-by: Tiago Silva <tiago.silva@goteleport.com> Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com> Co-authored-by: Andrew Burke <31974658+atburke@users.noreply.github.com> Co-authored-by: Brian Joerger <bjoerger@goteleport.com> Co-authored-by: Tiago Silva <tiago.silva@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Co-authored-by: Anton Miniailo <anton@goteleport.com> Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> Co-authored-by: Steven Martin <steven@goteleport.com> Co-authored-by: Marco André Dinis <marco.dinis@goteleport.com> Co-authored-by: Michael Wilson <mike@mdwn.dev> Co-authored-by: Reed Loden <reed@goteleport.com> Co-authored-by: Hugo Shaka <hugo.hervieux@goteleport.com> Co-authored-by: Mike Jensen <jentfoo@users.noreply.github.com> Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> Co-authored-by: Trent Clarke <trent@goteleport.com> Co-authored-by: Taras <9948629+taraspos@users.noreply.github.com> Co-authored-by: Bartosz Leper <bartosz.leper@goteleport.com> Co-authored-by: Michael <michael.myers@goteleport.com> Co-authored-by: Alan Parra <alan.parra@goteleport.com>
* Backport changes from #36393 * Show UNKNOWN for app connections * Use `connection satisfies never` * Remove `connection satisfies never` check
* Backport changes from #36393 * Show UNKNOWN for app connections * Use `connection satisfies never` * Remove `connection satisfies never` check
Closes #35959
Adds local proxies for TCP and HTTP apps.
The most important commits are:
tsh proxy app.TODO:
Handle expired certs. From what I see, I should useonExpiredCert, but I haven't figured out yet a way to elegantly check if the app cert is not expired (for dbs we haveCheckDBCerts).Changelog: Added App Access support to Teleport Connect.