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

Add support for using tls connection #1

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

SndR85
Copy link
Contributor

@SndR85 SndR85 commented Jul 31, 2024

I've notices that it isn't possible to setup a ssl connection with the module of mysql. With this change the parameter ssl can be set. This enables users to set a minion config with the following config:

mysql.ssl:
  ca: /path/to/ca.pem

This will pass the ssl parameter to the underlying library with the dictionary as supported by MySQLdb and PyMySQL

@SndR85 SndR85 changed the title Add support for using tls based connection Add support for using tls connection Jul 31, 2024
@lkubb
Copy link
Member

lkubb commented Aug 13, 2024

@scornelissen85 Sorry it took a while - this extension sadly was not finished migrating when you initially submitted this PR.

Would you mind rebasing on main, writing a quick feature request, adding a changelog file and adding the new SSL param to the documentation at the top of the file? A test would be awesome, but connection params seem to be tested poorly currently, so it's fine if you don't write one.

The changelog entry could be created like this:

echo "Added support for specifying TLS connection parameters to the execution module" > changelog/<feature_request_#, e.g. 2>.added.md

@SndR85 SndR85 force-pushed the feature/add_tls_support branch 2 times, most recently from ce41f37 to 3d5a956 Compare August 15, 2024 10:08
@SndR85
Copy link
Contributor Author

SndR85 commented Aug 16, 2024

I have rebased the code. I was actually looking to add some tests, I thought that it maybe could be added as functional test? I discoved the usage of different MySQL containers. However, I've got a bit stuck in complexity about how to implement that with custom certificates. I could use the cryptography package to create a CA and server cert and key and then mount these files in the container. Then I could write test to verify it connects with the server. But it seems a bit hacky to me.

I have created a feature request (#4) and added a changelog.

@SndR85 SndR85 force-pushed the feature/add_tls_support branch from 3d5a956 to 20a0c78 Compare August 16, 2024 09:50
@lkubb
Copy link
Member

lkubb commented Aug 16, 2024

Thanks! :) I'm merging this in as is since it's a simple and important fix. I suspect the modules here need some updating/refactoring, especially after the migration to an extension. This process could be used to improve test coverage.

I would love a functional test for this in theory, but as you mentioned, the fixture setup complexity is very high:

  1. Create two private keys + certificates (CA + MySQL server) (one could just use the x509_v2 modules though, no need to go as low-level as cryptography)
  2. Ensure CN (SAN?) contains a reachable domain (or somehow disable host name verification)
  3. Mount the files into the container
  4. Configure MySQL to use the generated certificate
  5. Ensure the minion_config contains the CA cert that was used to create the certificate above

@lkubb lkubb merged commit 0683371 into salt-extensions:main Aug 16, 2024
18 checks passed
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