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

Disable use of deprecated Ciphers #34

Merged
merged 2 commits into from
Jun 15, 2021

Conversation

jadyndev
Copy link
Contributor

SUMMARY

Old ciphers shouldn't be used. Devices that use newer ciphers couldn't be reached as the cipher was locked to sslv3.

ISSUE TYPE
  • Bugfix Pull Request

Old ciphers shouldn't be used. Devices that use newer ciphers couldn't be reached as the cipher was locked to sslv3.
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #34 (e159bb7) into main (0b25643) will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #34      +/-   ##
==========================================
+ Coverage   80.16%   80.23%   +0.06%     
==========================================
  Files          11       11              
  Lines        1190     1189       -1     
  Branches      161      161              
==========================================
  Hits          954      954              
+ Misses        174      173       -1     
  Partials       62       62              
Impacted Files Coverage Δ
plugins/modules/api.py 75.70% <ø> (+0.42%) ⬆️

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 0b25643...e159bb7. Read the comment docs.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Indeed, this isn't a good idea. I'm mainly wondering whether we should simply remove this line (as you suggested), or make this behavior configurable.

@@ -451,7 +451,6 @@ def ros_api_connect(self, username, password, host, port, use_ssl):
conn_status["connection"]["port"] = port
ctx = ssl.create_default_context()
ctx.check_hostname = False
ctx.set_ciphers('ADH:@SECLEVEL=0')
Copy link
Collaborator

Choose a reason for hiding this comment

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

@NikolayDachev was there a reason why you added this? Does it not work for you without this line? If yes, we should add a setting for it.

(We should also do that for the line above, check_hostname.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did some testing, with this line I could neither connect to my device when "TLS version" was set to "any", nor when it was set to "only v1.2". So if there's no explanation why this line is needed on some devices, let's just remove it. We can make it configurable if someone reports that it is really needed in some cases.

@felixfontein
Copy link
Collaborator

@jadyndev can you please add a changelog fragment so we can merge this? For that, add a file changelogs/fragments/34-api-ciphers.yml with content:

bugfixes:
- "api - when using TLS/SSL, remove explicit cipher configuration to insecure values, which also makes it impossible to connect to newer RouterOS versions (https://github.com/ansible-collections/community.routeros/pull/34)."

(Or something similar; see the guidelines.)

Copy link
Collaborator

@heuels heuels left a comment

Choose a reason for hiding this comment

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

@jadyndev, thanks for submitting this PR!
@felixfontein, thanks for reviewing!

@felixfontein felixfontein merged commit b45baaa into ansible-collections:main Jun 15, 2021
@felixfontein
Copy link
Collaborator

@jadyndev thanks for your work on this!
@heuels thanks for reviewing!

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