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

cert validation fixes #52855

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@sivel
Copy link
Member

sivel commented Feb 22, 2019

SUMMARY

[WIP] Start of cert validation fixes

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/urls.py

ADDITIONAL INFORMATION

A few things this PR does:

  1. Doesn't effectively disable SSL verification if no_proxy is true.
  2. Keeps the temporary ca cert bundle through the end of execution, avoiding rebuilding it for each open_url/Request.open call.
@sivel

This comment has been minimized.

Copy link
Member Author

sivel commented Feb 22, 2019

not_bot_skip

@sivel sivel changed the title [WIP] Start of cert validation fixes cert validation fixes Feb 26, 2019

@kbreit

This comment has been minimized.

Copy link
Contributor

kbreit commented Feb 27, 2019

CC @dagwieers

I've done some performance testing on this using my Meraki network modules. There doesn't appear to be any regressions, at least related to the Meraki modules.

I'll test more modules, maybe tonight, but I am seeing some performance improvements when running integration tests. meraki_snmp is showing a 6% performance increase over 3 trials (0:38 and 0:36) and meraki_content_filtering is a 3% performance increase (1:03 and 1:01).

@sivel

This comment has been minimized.

Copy link
Member Author

sivel commented Feb 28, 2019

Just for some statistics, on my laptop, generating the ca cert bundle often took less than 200ms. In some cases it was less than 100ms. But depending on drive speed and such, it could be more significant for others, and many repeated open_url calls will add up to time savings.

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 28, 2019

Let me recap my preferred implementation in this PR in case we have to retain the existing functionality:

  • Once on every module invocation (may be the first request performed), check the timestamp of the shared ca-cert bundle
    • If ca-cert bundle is fairly recent (timeout or ttl) or ca-cert bundle is locked, use it
    • If not, lock ca-cert bundle and check for newer certs
      • If newer certs exist, rebuild ca-cert bundle, unlock ca-cert bundle
      • Otherwise touch ca-cert bundle and unlock

The lock would be there to ensure we don't have multiple modules rebuilding the ca-cert bundle at the same time. The timeout value is there just to avoid the somewhat more expensive check for updated certs.

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.