search

jborean93 / smbprotocol

Python SMBv2 and v3 Client
MIT License
318 stars 73 forks source link

Dfs refresh referrals #171

Closed mrmaxi closed 2 years ago

mrmaxi commented 2 years ago

fix #170 - STATUS_ACCESS_DENIED when write by dfs path before read

Remember that path if DFS if it discovered once, and than when referral was expired only refresh it. Use persistent _referral_cache as dict, so when referral is expired just refresh it (instead of removing).

codecov[bot] commented 2 years ago

Codecov Report

Merging #171 (0015f39) into master (ad98d67) will decrease coverage by 3.34%. The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   99.09%   95.74%   -3.35%     
==========================================
  Files          23       23              
  Lines        5063     5077      +14     
==========================================
- Hits         5017     4861     -156     
- Misses         46      216     +170
Flag Coverage Δ
macOS 67.10% <8.33%> (-1.12%) :arrow_down:
py3.10 95.74% <33.33%> (-3.33%) :arrow_down:
py3.6 95.74% <33.33%> (-3.33%) :arrow_down:
py3.7 95.74% <33.33%> (-3.33%) :arrow_down:
py3.8 95.73% <33.33%> (-3.34%) :arrow_down:
py3.9 95.73% <33.33%> (-3.36%) :arrow_down:
ubuntu 93.13% <20.83%> (-3.65%) :arrow_down:
windows 95.65% <33.33%> (-3.36%) :arrow_down:
x64 95.74% <33.33%> (-3.35%) :arrow_down:
x86 95.65% <33.33%> (-3.36%) :arrow_down:

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

Impacted Files Coverage Δ
smbclient/_pool.py 82.29% <33.33%> (-13.61%) :arrow_down:
smbclient/shutil.py 41.66% <0.00%> (-53.44%) :arrow_down:
smbclient/path.py 44.18% <0.00%> (-46.52%) :arrow_down:
smbclient/_os.py 96.74% <0.00%> (-1.74%) :arrow_down:
smbclient/_io.py 97.74% <0.00%> (-1.30%) :arrow_down:

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 ad98d67...0015f39. Read the comment docs.

jborean93 commented 2 years ago

Nice picking up on your original problem, I'll have to play around with it to see if I can replicate it as it's certainly not something I would expect. I think we might need a different approach in the PR as keeping the referrals around resulted in a memory bloat reporting in https://github.com/jborean93/smbprotocol/issues/136. Maybe we should keep the existing cache and the expiry checks but have another simple list that contains paths known to be DFS paths as part of the check. That way it still knows to do the DFS referral request even on an expiry but the referral responses aren't stored indefinitely.

mrmaxi commented 2 years ago

Maybe we should keep the existing cache and the expiry checks but have another simple list that contains paths known to be DFS paths as part of the check. That way it still knows to do the DFS referral request even on an expiry but the referral responses aren't stored indefinitely.

I thought about this, but in this case we need to duplicate lookup_referral code for find if requested path is part of dfs. And I don't found simple solution.

jborean93 commented 2 years ago

I've taken some time to look at this further and while this might alleviate the problem temporarily for you the issue still remains where the first request is going to error with STATUS_ACCESS_DENIED causing smbclient to raise the exception. Without the server actually responding with the expected errors to start the DFS process anything on top is going to be a hack really. At a guess your domain controller actually has a share or path that the code is legitimately trying to open, or failing that maybe the DFS share permissions themselves cause it to only work if opening with read access or when being requested to the proper DFS server.

It is due to this problem I see the proper solution is ensuing you've resolved the proper DFS servers for your domain rather than rely on the rudimentary check that smbclient tries to do for you without knowing what your domain controller is called.

As mentioned in https://github.com/jborean93/smbprotocol/issues/85#issuecomment-810841141 and further down that thread, in these cases you should be setting the following at the start of your script to ensure the domain referrals are looked up properly.

import smbclient

smbclient.ClientConfig(domain_controller='dc.hostname.realm')

You can add username and/or password if you can't rely on implicit Kerberos authentication but otherwise what this code does is:

When an actual request is seen to connect to one of these now known domain names it knows that this is for a DFS path and can do the lookup pre-emptively avoiding this problem altogether. The reason why you don't see this problem when running it natively on Windows is that it already knows the domain controller to contact and the domain information. On Linux, and in this pure Python implementation it does not have that luxury so it relies on what the caller has provided or error codes returned back. In this case it gets back a STATUS_ACCESS_DENIED so assumed the path is legitimate but you don't have access to it. Why it returns STATUS_ACCESS_DENIED rather than one of the expected error codes to start the lookup I don't have the slightest idea why and I cannot seem to replicate it myself. If this was working then this wouldn't be an issue because the DFS fallback code would run and check the cache or resend the DFS request as needed.

I apologise for getting back to you so late on the PR but unfortunately this is a workaround that keeps around expired caches causing a leak over time for something that can be solved by setting smbclient.ClientConfig(domain_controller='...') and letting smbclient know for sure what is a DFS request or not initially.