[GitHub] [superset] betodealmeida commented on issue #21789: [SIP-88] - Enable SSH Tunneling

[GitBox <gi...@apache.org>]

betodealmeida commented on issue #21789:
URL: https://github.com/apache/superset/issues/21789#issuecomment-1286223084

   A few comments:
   
   In (1.i.b) it would be nice to explain that you're planning to add a configuration flag to prevent users from connecting to databases in localhost. There are plenty of legit cases for that, so it should be off by default. It probably only makes sense to turn this on for multi-tenant Superset deploymend (Preset, eg). Though I think this is outside the scope of this SIP
   
   For the table schema:
   
   ```python
   class TunnelConfig(Schema):
   	database_id: int # fk
   	ssh_server: str, # IP address 
           ssh_username: str, # username for ssh
           ssh_password: str, # password
   	remote_server_address: Tuple[str, int] # (REMOTE_SERVER_IP, 443)
           ssh_pkey: Optional[str],
           ssh_private_key_password: Optional[str] 
   ```
   
   - Why are you using `ssh_pkey` but then `ssh_private_key_password`? It's better to be consistent, and I'd say, explicit. Let's have `ssh_private_key` instead of `ssh_pkey`. Even better, let's drop the superfluous `ssh_` prefix and use `private_key`.
   - If you're allowing authenticating with a private key the `password` field should be optional.
   - Not all SSH servers run on port 22. You should add a column for the SSH port.
   - `remove_server_address` is confusing, a better name would be `remote_bind_address`, since this is not the name of the remote server, but of the server/port the tunnel will bind to. Also, I would store this in separate columns, since not all databases support complex types.
   
   My suggestion for the schema would be:
   
   ```python
   class SSHTunnelConfig(Schema):
   
       database_id: int
   
       server_host: str
       server_port: int
       username: str
       password: Optional[str]
       private_key: Optional[str]
       private_key_password: Optional[str]
   
       bind_host: str
       bind_port: int
   ```
   
   This would map to the following SSH command:
   
   ```bash
   ssh -p ${server_port} :${bind_host}:${bind_post} ${username}@${server_host}
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org