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

Enable reading SSL parameters from configuration file #552

Merged
merged 1 commit into from Feb 28, 2017

Conversation

Projects
None yet
2 participants
@JAORMX
Copy link
Contributor

commented Feb 27, 2017

This enables the reading of some basic SSL parameters from the
configuration file. It gives precedence to the ssl parameters provided
by the class parameter (the ssl dict).

@JAORMX JAORMX force-pushed the JAORMX:ssl-configuration-file branch from e707ad7 to b07494a Feb 27, 2017

# No SSL parameters were given either via the class parameter or
# the configuration file
if not any(x for x in ssl.values()):
ssl = None

This comment has been minimized.

Copy link
@methane

methane Feb 27, 2017

Member

if not any(ssl.values())

And read_default_file option changes how ssl = {"ca": ""} is considered.
if read_default_file=False, ssl is overwritten to None. Otherwise, passed ssl is used as-is.
I don't like it.

This comment has been minimized.

Copy link
@JAORMX

JAORMX Feb 27, 2017

Author Contributor

Actually, this is for the if read_default_file: case. So it will only be read when that's set.

ssl = None

self.ssl = False
if ssl:

This comment has been minimized.

Copy link
@methane

methane Feb 27, 2017

Member

Mabe, if ssl and any(ssl.values()).

@JAORMX JAORMX force-pushed the JAORMX:ssl-configuration-file branch from b07494a to 7f21f19 Feb 27, 2017

@JAORMX

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@methane I updated the commit. What do you think?

# must have preference.
for key, value in ssl_config_params.items():
if value:
ssl.setdefault(key, value)

This comment has been minimized.

Copy link
@methane

methane Feb 27, 2017

Member

since you used _config when building ssl_config_params,
you can just ssl[key] = value

This comment has been minimized.

Copy link
@JAORMX

JAORMX Feb 27, 2017

Author Contributor

Using ssl[key] = value would overwrite the value that's already there, and this gives priority to the values that are set via the ssl dict.

This comment has been minimized.

Copy link
@JAORMX

JAORMX Feb 27, 2017

Author Contributor

That's why I added the comment above this for loop.

This comment has been minimized.

Copy link
@methane

methane Feb 27, 2017

Member

read _config() function.

@JAORMX JAORMX force-pushed the JAORMX:ssl-configuration-file branch from 7f21f19 to 20ea19b Feb 27, 2017

@JAORMX

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@methane you are right, I changed it accordingly.

}
for key, value in ssl_config_params.items():
if value:
ssl[key] = value

This comment has been minimized.

Copy link
@methane

methane Feb 27, 2017

Member

How about this?

for key in ["ca", "capath", "cert", "key", "cipher"]:
    value = cfg.get(read_default_group, "ssl-" + key)
    if value:
        ssl[key] = value

(I hadn't check mysql options yet.)

This comment has been minimized.

Copy link
@methane

methane Feb 27, 2017

Member

I confirmed options have exactly same name.

@JAORMX

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@methane by the way, I see that in the _create_ssl_ctx function there is a check such as this:
if isinstance(sslp, ssl.SSLContext):
return sslp
should I take that into account as well?

@methane

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

Ah, yes please. Use config only when dict is None or isinstance(ssl, dict)

Enable reading SSL parameters from configuration file
This enables the reading of some basic SSL parameters from the
configuration file. It gives precedence to the ssl parameters provided
by the class parameter (the ssl dict).

@JAORMX JAORMX force-pushed the JAORMX:ssl-configuration-file branch from 20ea19b to d883a44 Feb 27, 2017

@JAORMX

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@methane done; and I really liked your suggestion. I added the minimized loop you suggested as part of the change.

@JAORMX

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

Tested it successfully in my deployment. @methane thanks for the great reviews. Let me know if there's anything else I should change.

@methane methane merged commit 557d0d9 into PyMySQL:master Feb 28, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.009%) to 88.906%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.