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

Implement SSL Support #96

Merged
merged 16 commits into from
Jun 5, 2023
Merged

Implement SSL Support #96

merged 16 commits into from
Jun 5, 2023

Conversation

sylvrs
Copy link
Contributor

@sylvrs sylvrs commented May 28, 2023

Overview

Before this pull request, libasynql lacked the ability to connect to a MySQL server using an SSL connection (as seen by #80). This pull request aims to resolve that by adding a simple wrapper using an SSL credentials class (MysqlSslCredentials) and matching configuration options. These configuration options can be used like so:

database:
  type: mysql
  mysql:
    # ...
    ssl:
      key: ""
      certificate: ""
      ca-certificate: ""
      ca-path: ""
      cipher-algorithms: ""

Testing

Using two instances, one with SSL and one without SSL, I was able to confirm that the functionality remains the same. Furthermore, errors are properly thrown if an SSL instance is not given the proper credentials.

Backwards Compatibility

Compatibility for the library remains the same as the SSL credentials are nullable and prefilled with a null value for both fromArray and the constructor.

Comment on lines 50 to 51
$array["ca-certificate"] ?? "",
$array["ca-path"] ?? "",
Copy link
Member

Choose a reason for hiding this comment

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

These should be nullable. Not familiar with mysqli, but why do we need both caCertificate and caPath? What is the different between them? The typical CA bundle only involves key + cert + CA afaik.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a loose binding to mysqli's ssl_set function. I can remove those parameters if you'd prefer? Regardless, I will make them nullable.

Copy link
Member

@SOF3 SOF3 left a comment

Choose a reason for hiding this comment

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

Please update the documentation too so that users know this feature exists. Or if you think it is too distractive for users, at least document somewhere else, in particular e.g. whether the certificates are paths or binary bytes or what.

README.md Outdated Show resolved Hide resolved
@SOF3 SOF3 merged commit ebe477c into poggit:master Jun 5, 2023
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.

3 participants