for eventlet issue 913 - Dnspython 2.6.0rc1 dns.query.udp() API chang…

[kelvin-j-li]

…e heads-up #913

[codecov]

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (b6f6e7c) 56% compared to head (e21efb1) 56%.

Files	Patch %	Lines
eventlet/support/greendns.py	33%	10 Missing and 2 partials ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##           master   #916   +/-   ##
=====================================
- Coverage      56%    56%   -1%     
=====================================
  Files          89     89           
  Lines        9718   9728   +10     
  Branches     1809   1812    +3     
=====================================
  Hits         5461   5461           
- Misses       3883   3892    +9     
- Partials      374    375    +1     

Flag	Coverage Δ
ipv6	23% <22%> (-1%)	⬇️
py310asyncio	53% <22%> (-1%)	⬇️
py310epolls	53% <22%> (-1%)	⬇️
py310poll	53% <22%> (-1%)	⬇️
py310selects	53% <22%> (-1%)	⬇️
py311asyncio	53% <22%> (-1%)	⬇️
py311epolls	53% <22%> (-1%)	⬇️
py312asyncio	50% <22%> (-1%)	⬇️
py312epolls	51% <22%> (-1%)	⬇️
py37asyncio	50% <22%> (-1%)	⬇️
py37epolls	51% <22%> (-1%)	⬇️
py38asyncio	51% <22%> (-1%)	⬇️
py38epolls	53% <22%> (-1%)	⬇️
py38openssl	52% <22%> (-1%)	⬇️
py38poll	53% <22%> (-1%)	⬇️
py38selects	53% <22%> (-1%)	⬇️
py39asyncio	51% <22%> (-1%)	⬇️
py39dnspython1	51% <22%> (-1%)	⬇️
py39epolls	53% <22%> (-1%)	⬇️
py39poll	53% <22%> (-1%)	⬇️
py39selects	53% <22%> (-1%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[4383]

My local tests are successful with dnspython version 2.5.0 and version 2.6.0rc1:

$ tox -e py312 # ok with version 2.5.0 (the default pulled version)
$ .tox/py312/bin/pip install --force-reinstall -v dnspython==2.6.0rc1 # upgrade
$ tox -e py312 # ok with the newer version 2.6.0rc1

It would be awesome if you can provide a unit test that reflect the fixed problem (https://github.com/eventlet/eventlet/blob/master/tests/greendns_test.py#L233).

[4383]

@kelvin-j-li do you plan to propose related unit test (see my previous suggestion), or can I merge this patch?

[rthalley]

This fix is not enough, and the change at line 837 is a regression without the updated receive code. The main fix for the CVE are the changes to receive_udp in this commit

I don't have a good unit test for these as they involve receiving various invalid things and ignoring them.

[rthalley]

I think eventlet changes should look a lot like this branch

Also, I added some unit tests to dnspython, see class IgnoreErrors if you want to adapt them.

[4383]

@rthalley: Thanks for details
@kelvin-j-li: I let you acknowledge Bob's suggestions.

[kelvin-j-li]

@rthalley: Thanks for details @kelvin-j-li: I let you acknowledge Bob's suggestions.

hi Hervé / Bob:
yes, I agree with @rthalley.

Do you prefer me to copy the code from https://github.com/rthalley/eventlet/tree/tudoor to this PR? I am ok with close this PR and use Bob's change/PR instead from https://github.com/rthalley/eventlet/tree/tudoor

Many thanks!

[4383]

Hi @kelvin-j-li

We have to use the latest version of the Bob's branch with the Truncated changes included. AFAIK Bob designed a fix but do not proposed a pull request. If Bob is ok with copying his changes, you could copy his branch and then co-author the fix, else Bob could simply propose a pull request.

@rthalley do you have any preference?

[rthalley]

I didn't do a PR in part because I don't have time to write the tests; I mostly wanted to show the sort of thing that is needed to keep the CVE fix. I'm ok with just copying from my branch.

[kelvin-j-li]

hi @4383 / @rthalley,
I had copy Bob fix into this PR:

cfe9b7b - (HEAD -> master, origin/master, origin/HEAD) Copied the complete fix for (CVE-2023-29483) and handling of truncated exceptions in greendns.py provided by Bob Halley from https://github.com/rthalley/eventlet/tree/tudoor

thanks!

[4383]

Thanks guys (@kelvin-j-li, @rthalley) for the collaboration.

This patch LGTM, I'm going to merge it.