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 options to control certificate validation for api module #37

Merged
merged 5 commits into from
Jun 28, 2021

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

Follow-up to #34 (comment).

This adds three options:

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

api

@codecov
Copy link

codecov bot commented Jun 20, 2021

Codecov Report

Merging #37 (4419665) into main (39dcf4a) will decrease coverage by 0.53%.
The diff coverage is 15.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
- Coverage   80.23%   79.69%   -0.54%     
==========================================
  Files          11       11              
  Lines        1189     1197       +8     
  Branches      161      163       +2     
==========================================
  Hits          954      954              
- Misses        173      181       +8     
  Partials       62       62              
Impacted Files Coverage Δ
plugins/modules/api.py 72.43% <15.38%> (-3.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39dcf4a...4419665. Read the comment docs.

@felixfontein
Copy link
Collaborator Author

CC @jadyndev @NikolayDachev

@felixfontein felixfontein mentioned this pull request Jun 22, 2021
@felixfontein
Copy link
Collaborator Author

I added some documentation for the new settings, see https://ansible.fontein.de/collections/community/routeros/docsite/api-guide.html for a preview of the extra docsite docs.

@felixfontein
Copy link
Collaborator Author

(It would be great to have a tutorial in there on how to set up such a PKI infrastructure; I might work on that later, but definitely not now :) )

@felixfontein
Copy link
Collaborator Author

@heuels can you take a look at this PR? It seems nobody else is interested in reviewing it :)

@NikolayDachev
Copy link
Collaborator

Hey Sorry, I don't have a free time, overall look ok will do a quick test today

@NikolayDachev
Copy link
Collaborator

No issues so far

@felixfontein
Copy link
Collaborator Author

@NikolayDachev thanks for testing! :)

@NikolayDachev
Copy link
Collaborator

NikolayDachev commented Jun 25, 2021

I notice only a single issue but is not related to the code

when: validate_cert_hostname: true
"msg": "[{'connection': {'username': 'admin', 'hostname': 'chr7b', 'port': 8729, 'ssl': True, 'status': 'error: check_hostname requires server_hostname'}}

I think this is related to urllib

If you have idea why this happen probably we should document it with some requirement ?

I'm sure the problem is with my test vm (not sure how long was not updated but last ansible version was 2.7 also not sure what mess is with other python libs) so from other side we can ignore this 🗡️ :)

@NikolayDachev
Copy link
Collaborator

(It would be great to have a tutorial in there on how to set up such a PKI infrastructure; I might work on that later, but definitely not now :) )

do you mean how to for routeros PKI ?

@felixfontein
Copy link
Collaborator Author

I notice only a single issue but is not related to the code

when: validate_cert_hostname: true
"msg": "[{'connection': {'username': 'admin', 'hostname': 'chr7b', 'port': 8729, 'ssl': True, 'status': 'error: check_hostname requires server_hostname'}}

I think this is related to urllib

It's actually related to ssl.SSLContext, it requires that server_hostname is passed to ctx.wrap_socket. I've added some code which - in case validate_cert_hostname=true - passes hostname as server_hostname to ctx.wrap_socket. With that, it should work.

@NikolayDachev
Copy link
Collaborator

Hey I willbe able to check at Monday

@felixfontein
Copy link
Collaborator Author

I also managed to try it out, it works now. I've tested it both with Python 2.7 and 3.9.

@felixfontein
Copy link
Collaborator Author

I've added instructions on setting up certificates on a RouterOS device (using the community.routeros.command module); see https://ansible.fontein.de/collections/community/routeros/docsite/api-guide.html#installing-a-certificate-on-a-mikrotik-router for how it looks.

@felixfontein
Copy link
Collaborator Author

ready_for_review

@NikolayDachev
Copy link
Collaborator

work ! Thanks

@NikolayDachev
Copy link
Collaborator

NikolayDachev commented Jun 28, 2021

I've added instructions on setting up certificates on a RouterOS device (using the community.routeros.command module); see https://ansible.fontein.de/collections/community/routeros/docsite/api-guide.html#installing-a-certificate-on-a-mikrotik-router for how it looks.

Cool, also you can check my example role in https://galaxy.ansible.com/nikolaydachev/routeros_api for ros_certificates (check the README) .. this is more how to configure them via ros, .. not all features are supported but the basic work

@NikolayDachev
Copy link
Collaborator

NikolayDachev commented Jun 28, 2021

@felixfontein we can merge .. everything look and work ok, let me know when we can do that

@felixfontein
Copy link
Collaborator Author

Thanks a lot for reviewing and testing!

I've added instructions on setting up certificates on a RouterOS device (using the community.routeros.command module); see https://ansible.fontein.de/collections/community/routeros/docsite/api-guide.html#installing-a-certificate-on-a-mikrotik-router for how it looks.

Cool, also you can check my example role in https://galaxy.ansible.com/nikolaydachev/routeros_api for ros_certificates (check the README) .. this is more how to configure them via ros, .. not all features are supported but the basic work

I will take a look, thanks for the link (if anyone wants a direct link to the role: https://github.com/NikolayDachev/ansible_collections/tree/master/nikolaydachev/routeros_api/roles/ros_certificates)!

@felixfontein felixfontein merged commit 75b4b96 into ansible-collections:main Jun 28, 2021
@felixfontein felixfontein deleted the api branch June 28, 2021 18:33
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