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

7198: SSH Tunnel: allow using OPENSSH key format #15833

Merged

Conversation

kimerinn
Copy link
Contributor

@kimerinn kimerinn commented Aug 22, 2022

What

This closes #7198 .
Allows to use nonencrypted OPENSSH keys for SSH tunneling

How

Use sshlib library to deal with OPENSSH keys
github.com/connectbot/sshlib

Recommended reading order

  1. airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/ssh/SshTunnel.java

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here
Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here
Connector Generator
  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

@kimerinn kimerinn requested review from a team as code owners August 22, 2022 09:34
@github-actions github-actions bot added area/documentation Improvements or additions to documentation area/platform issues related to the platform area/worker Related to worker normalization labels Aug 22, 2022
@kimerinn
Copy link
Contributor Author

kimerinn commented Aug 22, 2022

/test connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/2902826208

@kimerinn
Copy link
Contributor Author

kimerinn commented Aug 22, 2022

/test connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/2905015207

@kimerinn kimerinn temporarily deployed to more-secrets August 22, 2022 15:15 Inactive
@kimerinn kimerinn temporarily deployed to more-secrets August 23, 2022 09:08 Inactive
@kimerinn
Copy link
Contributor Author

kimerinn commented Aug 23, 2022

/test connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/2910279143

…ing_OPENSSH_key_format_2' into feature/7198_SSH_Tunnel_allow_using_OPENSSH_key_format_2

# Conflicts:
#	docs/understanding-airbyte/basic-normalization.md
@kimerinn
Copy link
Contributor Author

/test connector=connectors/destination-postgres

@kimerinn kimerinn temporarily deployed to more-secrets August 23, 2022 11:45 Inactive
@grishick
Copy link
Contributor

asking @rodireich to review this together with #15635 and #15529 in order to avoid these PRs breaking each other

@rodireich
Copy link
Contributor

This can be done with BouncyCastle I think.
See OpenSSHPrivateKeyUtil.parsePrivateKeyBlob

@kimerinn
Copy link
Contributor Author

kimerinn commented Aug 31, 2022

This can be done with BouncyCastle I think. See OpenSSHPrivateKeyUtil.parsePrivateKeyBlob

@rodireich
The problem is that OpenSSHPrivateKeyUtil.parsePrivateKeyBlob() returns AsymmetricKeyParameter while I need java.security.Keypair . I tried somehow to construct KeyPair from AsymmetricKeyParameter but I failed. I could nod find needed info at bouncycastle docs or on stackoverflow.

@alexandr-shegeda
Copy link
Contributor

@rodireich can you take a look at the comment from @kimerinn?
cc @grishick

@rodireich
Copy link
Contributor

rodireich commented Sep 7, 2022

The following code will turn an SSH-Ed25519 private key into java security's KeyPair using Bouncycastle:

final byte[] keyContent = pemObject.getContent();
final var pkp = OpenSSHPrivateKeyUtil.parsePrivateKeyBlob(keyContent);
if (pkp instanceof Ed25519PrivateKeyParameters) {
            final Ed25519PublicKeyParameters pubKeyParam = ((Ed25519PrivateKeyParameters) pkp).generatePublicKey();
            final AsymmetricCipherKeyPair ackp = new AsymmetricCipherKeyPair(pubKeyParam, pkp);
            final byte[] pkcs8Encoded = PrivateKeyInfoFactory.createPrivateKeyInfo(ackp.getPrivate()).getEncoded();
            final PKCS8EncodedKeySpec pkcs8EncodedKeySpec = new PKCS8EncodedKeySpec(pkcs8Encoded);
            final byte[] spkiEncoded = SubjectPublicKeyInfoFactory.createSubjectPublicKeyInfo(ackp.getPublic()).getEncoded();
            final X509EncodedKeySpec spkiKeySpec = new X509EncodedKeySpec(spkiEncoded);
            final KeyFactory keyFac = KeyFactory.getInstance("Ed25519", "BC");
            return new KeyPair(keyFac.generatePublic(spkiKeySpec), keyFac.generatePrivate(pkcs8EncodedKeySpec));
}```
However we need support other subtypes of OPENSSH keys - mainly the legacy format SSH-RSA.

I'm assigning it to myself to complete implementation if that's OK @kimerinn , @alexandr-shegeda 

@rodireich rodireich self-assigned this Sep 7, 2022
@github-actions github-actions bot removed area/platform issues related to the platform normalization labels Sep 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • destination-oracle
  • source-mysql-strict-encrypt
  • source-cockroachdb
  • destination-meilisearch
  • source-db2-strict-encrypt
  • destination-snowflake
  • destination-csv
  • destination-bigquery
  • destination-tidb
  • source-sftp
  • destination-mongodb-strict-encrypt
  • source-mongodb-v2
  • destination-mssql
  • source-postgres-strict-encrypt
  • destination-redis
  • source-scaffold-java-jdbc
  • source-mssql-strict-encrypt
  • destination-redshift
  • destination-clickhouse
  • destination-e2e-test
  • destination-cassandra
  • source-relational-db
  • destination-pubsub
  • source-postgres
  • destination-bigquery-denormalized
  • source-mssql
  • destination-mongodb
  • source-mysql
  • destination-kinesis
  • source-jdbc
  • destination-jdbc
  • source-kafka
  • source-oracle
  • destination-elasticsearch
  • source-cockroachdb-strict-encrypt
  • destination-keen
  • destination-local-json
  • destination-mariadb-columnstore
  • source-mongodb-strict-encrypt
  • source-snowflake
  • destination-rockset
  • source-elasticsearch
  • source-oracle-strict-encrypt
  • destination-oracle-strict-encrypt
  • destination-azure-blob-storage
  • destination-clickhouse-strict-encrypt
  • source-e2e-test
  • destination-postgres
  • source-redshift
  • source-clickhouse
  • destination-mysql
  • destination-mysql-strict-encrypt
  • destination-mssql-strict-encrypt
  • destination-gcs
  • destination-pulsar
  • source-clickhouse-strict-encrypt
  • source-tidb
  • destination-databricks
  • source-bigquery
  • destination-dynamodb
  • destination-scylla
  • destination-s3
  • destination-postgres-strict-encrypt
  • source-alloydb
  • destination-kafka
  • source-e2e-test-cloud
  • destination-mqtt
  • source-db2
  • destination-dev-null

@github-actions github-actions bot added area/platform issues related to the platform area/worker Related to worker normalization labels Sep 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • destination-bigquery-denormalized
  • destination-mongodb
  • destination-tidb
  • destination-mysql
  • source-jdbc
  • source-db2-strict-encrypt
  • destination-meilisearch
  • destination-s3
  • destination-mqtt
  • source-cockroachdb
  • destination-oracle-strict-encrypt
  • destination-local-json
  • destination-clickhouse-strict-encrypt
  • source-oracle-strict-encrypt
  • source-bigquery
  • destination-mongodb-strict-encrypt
  • source-relational-db
  • source-kafka
  • source-mongodb-strict-encrypt
  • source-mongodb-v2
  • source-mysql-strict-encrypt
  • destination-pulsar
  • source-alloydb
  • destination-csv
  • destination-bigquery
  • source-postgres-strict-encrypt
  • destination-mssql
  • destination-oracle
  • destination-redis
  • source-e2e-test-cloud
  • destination-clickhouse
  • destination-postgres
  • destination-dev-null
  • destination-e2e-test
  • destination-scylla
  • destination-jdbc
  • source-clickhouse
  • destination-mariadb-columnstore
  • source-mssql-strict-encrypt
  • source-cockroachdb-strict-encrypt
  • source-scaffold-java-jdbc
  • destination-cassandra
  • destination-databricks
  • destination-mssql-strict-encrypt
  • source-db2
  • source-mysql
  • source-mssql
  • source-elasticsearch
  • destination-redshift
  • source-sftp
  • source-clickhouse-strict-encrypt
  • destination-gcs
  • destination-elasticsearch
  • destination-kinesis
  • source-tidb
  • destination-dynamodb
  • destination-pubsub
  • source-e2e-test
  • destination-kafka
  • destination-azure-blob-storage
  • source-redshift
  • source-snowflake
  • destination-rockset
  • destination-postgres-strict-encrypt
  • destination-snowflake
  • destination-keen
  • source-oracle
  • destination-mysql-strict-encrypt
  • source-postgres

@rodireich rodireich temporarily deployed to more-secrets September 8, 2022 23:36 Inactive

class SshTunnelTest {

private static final String SSH_ED25519_PRIVATE_KEY = "-----BEGIN OPENSSH PRIVATE KEY-----\\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this information be placed into a separate file? The gut feeling is that maybe this shouldn't just be openly visible in a test file. There are private keys stored in separate files: https://github.com/airbytehq/airbyte/blob/7aa7a373db6d9072d2642b57dbdb42ef47f92234/tools/integrations-test-ssl/mssql.key

Copy link
Contributor

Choose a reason for hiding this comment

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

These are all random keys created with ssh-keygen -t rsa and ssh-keygen -t ed25519.
No actual secrets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added link do doc

return new KeyPair(
converter.getPublicKey(SubjectPublicKeyInfo.getInstance(keypair.getPublicKeyInfo())),
converter.getPrivateKey(keypair.getPrivateKeyInfo()));
KeyPair getPrivateKeyPair() throws IOException, GeneralSecurityException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this is no longer private and if so how come?

Minor nit: not a fan of using kp as a variable name since it's not abundantly clear what that means. I presume it means keypair

Copy link
Contributor

Choose a reason for hiding this comment

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

This is required for testing. Reduced visibility to package private, and changed name.
thanks!

Copy link
Contributor

@ryankfu ryankfu left a comment

Choose a reason for hiding this comment

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

Left some minor comments to address but overall looks good

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • destination-dynamodb
  • source-mongodb-v2
  • source-bigquery
  • destination-databricks
  • destination-mysql
  • destination-clickhouse
  • source-snowflake
  • source-cockroachdb-strict-encrypt
  • source-clickhouse-strict-encrypt
  • source-mysql
  • destination-meilisearch
  • destination-keen
  • destination-azure-blob-storage
  • source-db2-strict-encrypt
  • destination-dev-null
  • destination-scylla
  • destination-bigquery
  • destination-pulsar
  • destination-postgres-strict-encrypt
  • source-scaffold-java-jdbc
  • source-alloydb
  • destination-mongodb-strict-encrypt
  • destination-postgres
  • destination-kinesis
  • destination-tidb
  • source-cockroachdb
  • destination-pubsub
  • destination-bigquery-denormalized
  • source-mongodb-strict-encrypt
  • source-clickhouse
  • source-sftp
  • destination-snowflake
  • source-mysql-strict-encrypt
  • destination-oracle-strict-encrypt
  • source-e2e-test-cloud
  • source-postgres-strict-encrypt
  • source-mssql
  • source-postgres
  • source-relational-db
  • destination-jdbc
  • source-jdbc
  • destination-redshift
  • destination-elasticsearch
  • source-db2
  • destination-mysql-strict-encrypt
  • source-tidb
  • destination-cassandra
  • destination-kafka
  • destination-e2e-test
  • destination-redis
  • destination-mariadb-columnstore
  • destination-csv
  • destination-mssql
  • source-oracle-strict-encrypt
  • destination-mqtt
  • destination-mongodb
  • source-elasticsearch
  • destination-rockset
  • source-kafka
  • destination-gcs
  • source-redshift
  • destination-mssql-strict-encrypt
  • destination-local-json
  • destination-s3
  • source-oracle
  • source-e2e-test
  • destination-oracle
  • source-mssql-strict-encrypt
  • destination-clickhouse-strict-encrypt

@rodireich rodireich temporarily deployed to more-secrets September 9, 2022 16:41 Inactive
@rodireich
Copy link
Contributor

rodireich commented Sep 9, 2022

/test connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/3023988948
✅ connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/3023988948
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
normalization/transform_config/__init__.py                            2      0   100%
normalization/transform_catalog/reserved_keywords.py                 14      0   100%
normalization/transform_catalog/__init__.py                           2      0   100%
normalization/destination_type.py                                    14      0   100%
normalization/__init__.py                                             4      0   100%
normalization/transform_catalog/destination_name_transformer.py     166      8    95%
normalization/transform_catalog/table_name_registry.py              174     34    80%
normalization/transform_config/transform.py                         191     49    74%
normalization/transform_catalog/utils.py                             51     14    73%
normalization/transform_catalog/dbt_macro.py                         22      7    68%
normalization/transform_catalog/catalog_processor.py                147     80    46%
normalization/transform_catalog/transform.py                         61     38    38%
normalization/transform_catalog/stream_processor.py                 589    394    33%
-------------------------------------------------------------------------------------
TOTAL                                                              1437    624    57%

Build Passed

Test summary info:

All Passed

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-cockroachdb
  • destination-gcs
  • source-elasticsearch
  • destination-pubsub
  • source-snowflake
  • destination-oracle
  • destination-rockset
  • source-clickhouse-strict-encrypt
  • destination-snowflake
  • destination-local-json
  • source-mssql-strict-encrypt
  • destination-mariadb-columnstore
  • destination-clickhouse
  • destination-kinesis
  • destination-mssql
  • source-alloydb
  • destination-mongodb-strict-encrypt
  • source-e2e-test
  • source-clickhouse
  • source-postgres
  • source-mongodb-v2
  • source-sftp
  • destination-e2e-test
  • destination-csv
  • source-redshift
  • destination-dev-null
  • destination-pulsar
  • source-oracle
  • destination-bigquery
  • destination-mysql
  • destination-cassandra
  • source-scaffold-java-jdbc
  • destination-scylla
  • source-relational-db
  • destination-azure-blob-storage
  • destination-mssql-strict-encrypt
  • source-mongodb-strict-encrypt
  • destination-redis
  • destination-mysql-strict-encrypt
  • source-mssql
  • source-postgres-strict-encrypt
  • source-jdbc
  • destination-postgres-strict-encrypt
  • destination-mqtt
  • destination-jdbc
  • destination-clickhouse-strict-encrypt
  • destination-mongodb
  • destination-meilisearch
  • destination-postgres
  • destination-dynamodb
  • source-mysql
  • source-db2-strict-encrypt
  • source-e2e-test-cloud
  • destination-redshift
  • destination-databricks
  • source-tidb
  • destination-s3
  • destination-bigquery-denormalized
  • destination-tidb
  • source-mysql-strict-encrypt
  • destination-oracle-strict-encrypt
  • destination-elasticsearch
  • source-bigquery
  • source-cockroachdb-strict-encrypt
  • destination-keen
  • source-db2
  • source-oracle-strict-encrypt
  • source-kafka
  • destination-kafka

@rodireich rodireich temporarily deployed to more-secrets September 9, 2022 16:46 Inactive
@rodireich rodireich temporarily deployed to more-secrets September 9, 2022 16:47 Inactive
@rodireich rodireich removed the request for review from alexandr-shegeda September 9, 2022 17:13
@rodireich rodireich merged commit f81c5aa into master Sep 9, 2022
@rodireich rodireich deleted the feature/7198_SSH_Tunnel_allow_using_OPENSSH_key_format_2 branch September 9, 2022 17:22
@rodireich
Copy link
Contributor

rodireich commented Sep 9, 2022

/publish connector=bases/base-normalization

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
* 7198: SSH Tunnel: allow using OPENSSH key format

* 7198: merge

* 7198: merge fix

* Use apache sshd lib to load private keys for tunnel

* Throw an exception in case private key failed to load

* Fix failing flow when creating NO_TUNNEL wrapper

* bump version numbers

* Address review comments. Fix test

Co-authored-by: Rodi Reich Zilberman <867491+rodireich@users.noreply.github.com>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* 7198: SSH Tunnel: allow using OPENSSH key format

* 7198: merge

* 7198: merge fix

* Use apache sshd lib to load private keys for tunnel

* Throw an exception in case private key failed to load

* Fix failing flow when creating NO_TUNNEL wrapper

* bump version numbers

* Address review comments. Fix test

Co-authored-by: Rodi Reich Zilberman <867491+rodireich@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation area/platform issues related to the platform area/worker Related to worker normalization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSH Tunnel: allow using OPENSSH key format
6 participants