Failure to scan UNC Windows path #4075
Conversation
Signed-off-by: Luis M. Santos <[email protected]>
Minor adjustments to path handling to address a few other edge cases. Closes anchore#4070 Signed-off-by: Luis M. Santos <[email protected]>
luissantosHCIT
force-pushed
the
failure_to_scan_unc_path
branch
from
9d96dbc to
609f527
Compare
…X relies on a path Clean() function which only detects \\ as a valid separator under Windows (understandably). As long as AppendRootTerminator works properly, FromPOSIX is guaranteed to work correctly under Windows. Signed-off-by: Luis M. Santos <[email protected]>
There was a problem hiding this comment.
Thanks for the contribution, @luissantosHCIT ! I left a few comments here. I'm interested to know how we can test this -- I'm not really a Windows user lately. Is this as simple as using paths with \\localhost\c$\?
Also, it won't help today but I'm working on getting some cross-platform build tooling so we can finally run the test suite on Windows in CI, for that reason I might be a little slow to merge this one -- someone will probably want to manually test it, hope to be done very soon, though. 🤞
| }, | ||
| { | ||
| name: "DriveC\\", | ||
| path: "C:\\", |
There was a problem hiding this comment.
Could these tests have some more complicated paths? things with spaces? What about /c/something-style paths, are these handled elsewhere?
There was a problem hiding this comment.
To help clarify this better, yes, \\localhost\c$\ is a valid path and it allows you to access the C drive via a UNC path. The advantage here is that you could potentially scan remote directories (\\myserver\c$\) if you have the permissions to do so. That's how I understood it when discussing with a colleague. I normally stick to *nix systems since they are more sane to me (I have worked on Windows for years before but I still find it insane). You could deploy Syft in a sandboxed windows machine and still scan select remote paths without having the program be installed on those machines. The alternative is to pre-mount a path as a mapped drive via PowerShell and run Syft as usual, but I thought this small bugfix might make Syft more ergonomic for those IT users doing what my colleague does.
No rush with merging. I can do a local build of Syft with my changes if my colleagues wants to move on with the scan of several systems we have.
I am going to add a few more paths for testing and validate that the root portion (\\localhost\c$\) continues to remain valid. An invalid path would be (\\localhost\c$:\).
There was a problem hiding this comment.
So I tried to expand testing but I realized that I did not mean for the new function to handle everything about paths.
It is meant to adjust prior behavior.
Before, it always appended :\ to a root path that was already isolated in FromPOSIX. Now, I simply check if it is a UNC path and append \ instead to avoid later path malformation. The default is to add :\ as it did before.
Other path styles are addressed elsewhere. Giving it a longer path simply leads to appending the terminator at the end. Meaning, if you pass a non-root portion of the path, you will get a no-op behavior with the extra \.
I think what you are thinking of is testing FromPOSIX which should address your concerns but see my previous reply about why I did not stick to testing at the FromPOSIX level. AppendRootTerminator is a very dumb function with a lot of assumptions about os, input, and expectations on the output.
Let me know if that makes sense.
| const windowsGoOS = "windows" | ||
| const windowsUNCPathPrefix = "\\\\" | ||
| const windowsDriveColon = ":" | ||
| const windowsDrivePathTerminator = ":\\" |
There was a problem hiding this comment.
There may be a nuance that could cause issues -- previously, the constant string was using backticks -- see removed line 34. But this is using double quotes. These have different escaping behavior -- the backticks results in :\\ whereas the double quotes escapes the backslash to :\. You can see this with a simple go program:
func main() {
fmt.Printf("%s __ %s", ":\\", `:\\`)
}
The result is that it's no longer translating /c/... to C:\\... but rather C:\, I think.
However, I think that's what you wanted given the comment below, so I just wanted to mention this since it isn't entirely obvious to everyone.
There was a problem hiding this comment.
You are right. I am new to Go and I was also working on a Python project at the same time so I didn't notice that a) there were backticks and b) they are not interchangeable (behave more like string vs. string literal in a shell).
With that said, Clean() should remove the excess separators as far as I could see during debugging and it is inconsequential to the path semantics since C:\\ behaves the same as C:\. This is standard filesystem behavior. /etc//fstab == /etc/fstab. In either case, we should get the same behavior unless there is another bug somewhere in the path manipulation. My intention was definitely to resolve C:\\ to C:\.
Good catch!
| "testing" | ||
| ) | ||
|
|
||
| func TestAppendRootTerminator(t *testing.T) { |
There was a problem hiding this comment.
I think we should be testing FromPosix, either instead of or in addition to this AppendRootTerminator function
There was a problem hiding this comment.
I feel your pain. Haha.
If you look at a previous commit (609f527), you will see that I wanted to do just that, but the automatic unit testing step in the CI failed because Clean() does not treat \ as a separator unless running the test under Windows. Clean will remove the extra \ in Windows but not in Linux so the test would be lying if we used the expected result under Linux instead of testing the actual windows behavior. As a result, I moved the tests to target AppendRootTerminator since that is ultimately the key function I want to guard against regressions and it is platform agnostic. Since this code is under platform specific modules (windows), I do not expect it to be called outside of windows anyways, but I do expect to enforce test of "general purpose" components. My choice here was a compromise.
As a note, I am also trying to keep this as frictionless as possible because we would like to do more wide deployment of Syft to see what interesting information we uncover in our infrastructure. We learned a lot during our initial localized test that we were not expecting to find. So I kinda did not want the fact the unit tests are run under Linux be a show stopper, since the path issue here is what's stopping us from further application of Syft.
… detecting the UNC path but it does not comprehensively handle everything about paths in Windows. It relies on FromPOSIX to handle the rest of hte path manipulation. Signed-off-by: Luis M. Santos <[email protected]>
luissantosHCIT
force-pushed
the
failure_to_scan_unc_path
branch
from
3cc5c11 to
488b8f3
Compare
Signed-off-by: Luis M. Santos <[email protected]>
|
Hello, are the changes here still awaiting for the cross platform tooling? I am gauging whether I should use a custom built executable for my use case or if there is hope in making progress on this PR. Thank you! |
|
@luissantosHCIT This one LGTM. I think I want the teams 🤝 on this one which is why I put |
|
@spiffcs To answer some of your concerns, I am primarily a *nix dev and did not know about accessing drive paths via UNC until a colleague brought it to my attention. The credit goes to him haha. Apparently, this is a way for him to scan the remote app deployments without having to push scripts directly to every machine. It's pretty nifti. For the most part, Syft handles paths well it was just the hard coded addition of the colon that broke the path. I believe I was investigating the issue under Linux initially simply because I was looking at how paths were handled under the assumption that it was left to the kernel and OS filesystem as opposed to the project manipulating the paths directly. I believe the simple test I added should not interfere with *nix systems/*nix CI pipelines since I think I tested it on Ubuntu (it's been a while and I am constantly jumping between projects). I like the livestream idea. You should turn it into a mini podcast so your users can browse back. I didn't consider livestreaming as being another avenue for documenting why some things changed over time. I find the PR review discussion fascinating. Good job! |
|
@spiffcs is running these locally on windows to validate in lieu of CI being setup for windows boxes at the moment. Will merge on confirmation of good vibes from the tests 😄 |
5 participants
Description
Scanning a target directory path over UNC fails to catalog files. Go is capable of handling UNC files since at least 1.21 (golang/go#61910).
Syft adds
:to root path before rejoining and this causes cataloger error when attempting to inspect a file.The issue is caused by
FromPOSIXfunction.Type of change
Checklist: