SMB initial version #1274
SMB initial version #1274
Conversation
this will negiotate a session with smbclient abd authenticate little else
needed as reference client to unit test SMB server functions
of interest beyond developers
…sted into 9818-ihaywood3-smb
|
One other thing that I am noticing as I'm looking over these commits: they're not properly associated with an email address verified on your github account, which I think you probably want to fix. (You can add & verify additional addresses on your github account at https://github.com/settings/emails ) |
src/twisted/protocols/_smb/base.py
Outdated
|
|
||
|
|
||
|
|
||
| def nstruct(fields): |
There was a problem hiding this comment.
This idiom is going to be absolute hell to get mypy to typecheck; it's going to complain about dynamic subclasses. Could we possibly do this with annotations and attrs fields instead of inventing a new thing here?
There was a problem hiding this comment.
please look at latest commit which uses attrs based approach to binary packing /unpacking
if it's ok I'll move the code across in ntlm.py and core.py
src/twisted/protocols/_smb/base.py
Outdated
| @@ -0,0 +1,207 @@ | |||
| # Copyright (c) Twisted Matrix Laboratories. | |||
There was a problem hiding this comment.
I appreciate the need for this binary protocol-parsing toolkit and I have a bunch of notes about stuff like mypy and attrs and whatnot, but I think the main important change is to ensure that it's an implementation detail, and that we think carefully about what the shape of the public interface for this stuff is; i.e. let's not include this byte-unpacking framework in the public API for SMB initially.
I realize that we're already in _smb since this is experimental, but I would suggest putting this in _smb/_base.py to clarify that it's not intended to be public API surface even after the package is more complete.
There was a problem hiding this comment.
pretty much all the modules are for internal use (heads up, there's more coming, even just to get to browsing a directory) My hope is only SMBFactory and a few interfaces form the public API and these are promoted through __init__.py
src/twisted/protocols/_smb/core.py
Outdated
| | FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES | ||
| | DELETE | READ_CONTROL | WRITE_DAC | WRITE_OWNER | ||
| | SYNCHRONIZE) | ||
| elif IPipe.providedBy(share): |
There was a problem hiding this comment.
Can it only be one? What happens if it provides more than one? Do we have any error reporting to the user?
There was a problem hiding this comment.
it makes no sense for a share to be more than one type
There was a problem hiding this comment.
The code now throws AssertionError if a share tries to implement more than one type.
|
|
||
| from twisted.protocols._smb.core import SMBFactory | ||
|
|
||
| __all__ = ['SMBFactory'] |
There was a problem hiding this comment.
Why is just this one name promoted up to the package namespace?
There was a problem hiding this comment.
simply so the user can access as twisted.protocols.smb.SMBFactory as opposed to twisted.protocols.smb.core.SMBFactory Over time other classes required for public API (mainly interfaces) would be added.
src/twisted/protocols/_smb/ntlm.py
Outdated
| for i in s: | ||
| flags |= FLAGS[i] | ||
| return flags | ||
|
|
There was a problem hiding this comment.
I think you might get a lot of what you want here out of IntFlag
There was a problem hiding this comment.
Correct. But IntFlag was introduced in 3.6 so it is not yet available, I will shift this code over when 3.5 support requirement is dropped.
change recursion to loop in SMBConnection.processData log.debug messages merged various magic numbers pulled out to named constants use struct.Struct objects for header parsing
add a locked parameter to attrs helpers base.calcsize
approach in core.py
There was a problem hiding this comment.
@ihaywood3 Can you resolve the merge conflicts, and get the commit status to be green? I'm sorry this is lingering so long, but it's a lot of new code to get through
for more information, see https://pre-commit.ci
Modified-by: Ian Haywood <ian@haywood.id.au>
…sted into 9818-ihaywood3-smb
for more information, see https://pre-commit.ci
…sted into 9818-ihaywood3-smb
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
To get a more useful review on the branch as a whole, the tests really need to run, so that's the highest priority to fix. New code like this should have 100% coverage.
Also a little bit of documentation of how to set up SMBClient manually as per the integration test so that I can poke at it and see what happens would be helpful. (Is it possible to point a Mac or Windows machine at this yet?)
I see _smb._base is double-private, indicating that it's not going to be a public interface when this gets promoted to a public package, but if core, ismb, ntlm, and security_blob are all going to be public, the naming convention issues all need to be fixed, i.e. thing_like_this needs to be thingLikeThis, smb_handler_name needs to be smb_handlerName.
There should be some introductory .rst documentation as well, at some point, although that doesn't need to be in this PR.
Thanks again for keeping up with this contribution over the years. 2023 is going to be the year of SMB in Twisted, for sure :)
|
|
||
| from twisted.protocols._smb.core import SMBFactory | ||
|
|
||
| __all__ = ["SMBFactory"] |
There was a problem hiding this comment.
I do love a carefully-minimized public interface that we'll commit to, but it seems likely that maybe a bit more stuff will need to be exported in order to be useful? (This is more of a question, not really a request for changes.)
| # See LICENSE for details. | ||
| # -*- test-case-name: twisted.protocols._smb.tests -*- | ||
| """ | ||
| base classes for SMB networking |
There was a problem hiding this comment.
Minor organizational note: the pattern of "base" modules with base classes that other stuff should inherit from is an unfortunate anti-pattern from the bad old days of treating inheritance as our primary extensibility mechanism.
I will also note that this module of "base classes" appears to contain a bunch of functions and only 2 classes, only one of which appears to be plausibly extensible. So a different name/docstring would seem to be in order.
| @param cls: the class, must be decorated with L{attr.s} and have | ||
| members with the appropriate metadata using the helpers from this | ||
| module. | ||
| @type cls: L{type} |
There was a problem hiding this comment.
@type epydoc field are now redundant with type annotations; pydoctor has supported these since 2020! twisted/pydoctor#136
| DATA = 3 | ||
|
|
||
|
|
||
| def unpack(cls: Any, data: bytes, offset: int = 0, remainder: int = IGNORE) -> Any: |
There was a problem hiding this comment.
There is a way to properly express "takes a type, returns an instance of that type" with mypy; you don't have to use Any here:
| def unpack(cls: Any, data: bytes, offset: int = 0, remainder: int = IGNORE) -> Any: | |
| T = TypeVar("T") | |
| def unpack(cls: Type[T], data: bytes, offset: int = 0, remainder: int = IGNORE) -> T: |
| return (obj, data[offset + strct.size :]) | ||
|
|
||
|
|
||
| def smb_fields(cls: Any) -> List[Any]: |
There was a problem hiding this comment.
coding standard: we don't use underscores like PEP8 in Twisted
| def smb_fields(cls: Any) -> List[Any]: | |
| def smbFields(cls: Type[object]) -> List[Any]: |
| @@ -0,0 +1,363 @@ | |||
| #!/usr/bin/python3 | |||
There was a problem hiding this comment.
This module needs to be renamed as described in the comments above in order to actually run. Codecov's not complaining about this module but I think that's because it emitted so many other warnings that it exceeded its maximum for annotations on a single PR.
|
|
||
|
|
||
| class TestBase(unittest.TestCase): | ||
| def test_base_pack(self) -> None: |
There was a problem hiding this comment.
Tests need docstrings so that maintainers understand what properties they're testing. They can (and should!) be very short, per https://jml.io/test-docstrings/ making a short assertion about the specific property they're testing.
| r.four = b"bob" | ||
| self.assertEqual(_base.pack(r), data) | ||
| with self.assertRaises(AssertionError): | ||
| r = FakeStruct(five=424243) # type: ignore |
There was a problem hiding this comment.
If you need type:ignore they should be as narrow as possible. But why do you need it at all here? This is a very simple expression which should typecheck just fine
|
|
||
|
|
||
| @python_unittest.skipUnless(os.access(SMBCLIENT, os.X_OK), "smbclient unavailable") | ||
| class SambaClientTests(unittest.TestCase): |
There was a problem hiding this comment.
Including the word "integration" here should help future maintainers know that this is going to be slow and potentially flaky, which is useful to understand in advance. It's great to have this test, just want to make sure that it is appropriately categorized and most narrow unit-tests of behavior should be tested elsewhere as well.
| class SambaClientTests(unittest.TestCase): | |
| class SambaClientIntegrationTests(unittest.TestCase): |
| @@ -10,4 +11,5 @@ | |||
| # Note that these features require Git 2.23 (released August 2019). | |||
| 03cabb7f55d54e2b09eefd0e03df2d440f962bc4 | |||
| bc96c774be6e307e8e3e4d39780d37b045d0973a | |||
| 2249eebd81b195c72c9293a1a2224fa1134d4ca3 | |||
There was a problem hiding this comment.
The idea here is that we didn't want to misattribute the authorship of lines when running the initial big syntax-cleanup pass; I don't think that you need to add this here; you actually authored all the lines you're reformatting. (It's fine if you think it's helpful for blame legibility, this PR is big and quite old, I just really don't want to establish the precedent that everyone who runs black starts appending to this file.)
This is the minimal proof-of-concept SMB component . All it can do is negotiate a session and authenticate, no actual file access yet but its already a large commit for review (sorry)
https://twistedmatrix.com/trac/ticket/9818