[PATCH] pathname cleanups

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] pathname cleanups

Samba - samba-technical mailing list
I'm getting ready to move the @GMT snapshot
name handling into struct smb_filename to
clean up a lot of the shadow_copy2 code.

Here are some prerequisite patches that
clean up our handling of the SMB1 FLAGS2
header value and remove some unneeded
bool parameters to functions.

The changes are already tested by the
current pathname tests.

Passes full make test locally.

Please review and push if happy !

Cheers,

        Jeremy.

pathname_cleanup (37K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
On Thu, May 18, 2017 at 08:40:57PM -0700, Jeremy Allison via samba-technical wrote:
> I'm getting ready to move the @GMT snapshot
> name handling into struct smb_filename to
> clean up a lot of the shadow_copy2 code.

As you mention it: I've got a report from a customer that a certain
access pattern does not work anymore: GPFS has the ability to show the
.snapshots directory (the one where all the @GMT- subdirs show up) in
all directories, with @GMT- subdirectories being relative to the
current dir.

For clients that do not understand the @GMT- logic or the timewarp
token, essentially all non-windows clients, we need to be able to
access previous versions through the subdir/.snapshots/@GMT-../subdir
path. I think in the shadow_copy2 module we need to also preserve the
non-stripped name and try with that also. Any other ideas?

Thanks, Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
On 05/19/2017 07:54 AM, Volker Lendecke via samba-technical wrote:

> On Thu, May 18, 2017 at 08:40:57PM -0700, Jeremy Allison via samba-technical wrote:
>> I'm getting ready to move the @GMT snapshot
>> name handling into struct smb_filename to
>> clean up a lot of the shadow_copy2 code.
>
> As you mention it: I've got a report from a customer that a certain
> access pattern does not work anymore: GPFS has the ability to show the
> .snapshots directory (the one where all the @GMT- subdirs show up) in
> all directories, with @GMT- subdirectories being relative to the
> current dir.
>
> For clients that do not understand the @GMT- logic or the timewarp
> token, essentially all non-windows clients, we need to be able to
> access previous versions through the subdir/.snapshots/@GMT-../subdir
> path. I think in the shadow_copy2 module we need to also preserve the
> non-stripped name and try with that also. Any other ideas?
>
> Thanks, Volker
>
Actually putting the timewarp timestamp into a separate field would
clean that nicely - if the timestamp is not assigned (zero?), then this
is a regular path and shadow_copy2 does not intervene. If it's assigned,
vfs_shadow_copy2 modifies the path. If we re-enter vfs_shadow_copy2, it
enters as a regular path. SMB2 becomes really clean that way.

With SMB1 clients, the @GMT comes as first path element, so the SMB1
server converts that into a timewarp element and strips it off the path
- that too supports regular clients which want to access
path/.snapshots/@GMT. Admittedly, this involves a bit of heuristics.
There's FLAGS2_REPARSE_PATH bit in FLAGS2 - Windows clients set it and
also Samba client since 4.6.x. The server can decide whether to be
strict about it or use the heuristic (currently smbd ignores this flag).

Thanks,
Uri

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
On Fri, May 19, 2017 at 09:11:49AM +0300, Uri Simchoni via samba-technical wrote:
> With SMB1 clients, the @GMT comes as first path element, so the SMB1

I've seen clients where the @GMT comes next to the last path
component. That was part of the complexity of the previous
shadow_copy2 module.

> server converts that into a timewarp element and strips it off the path
> - that too supports regular clients which want to access
> path/.snapshots/@GMT. Admittedly, this involves a bit of heuristics.
> There's FLAGS2_REPARSE_PATH bit in FLAGS2 - Windows clients set it and
> also Samba client since 4.6.x. The server can decide whether to be
> strict about it or use the heuristic (currently smbd ignores this flag).

To support the dumb clients the heuristic would have to be to try
inserting @GMT at all places where / now is if we lose the place the
client sent the @GMT initially. We can't rely on the flag being set I
believe. We're talking FreeBSD, (I believe AIX) and other custom SMB
clients that used to work.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
On 05/19/2017 09:37 AM, Volker Lendecke wrote:
> On Fri, May 19, 2017 at 09:11:49AM +0300, Uri Simchoni via samba-technical wrote:
>> With SMB1 clients, the @GMT comes as first path element, so the SMB1
>
> I've seen clients where the @GMT comes next to the last path
> component. That was part of the complexity of the previous
> shadow_copy2 module.
>
Oh I get it now. In that case I suppose that won't be inside
shadow_copy2, as interpreting the client's intent moves into the server
(where it should be, irrespective of what backend is used to implement
the previous versions). The SMB1 server might try with a modified path
(@GMT removed and timewarp field has timestamp), and fall back to
unmodified path (fall back if FLAGS2_REPARSE_PATH is not set).


>> server converts that into a timewarp element and strips it off the path
>> - that too supports regular clients which want to access
>> path/.snapshots/@GMT. Admittedly, this involves a bit of heuristics.
>> There's FLAGS2_REPARSE_PATH bit in FLAGS2 - Windows clients set it and
>> also Samba client since 4.6.x. The server can decide whether to be
>> strict about it or use the heuristic (currently smbd ignores this flag).
>
> To support the dumb clients the heuristic would have to be to try
> inserting @GMT at all places where / now is if we lose the place the
> client sent the @GMT initially. We can't rely on the flag being set I
> believe. We're talking FreeBSD, (I believe AIX) and other custom SMB
> clients that used to work.
>
> Volker
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, May 19, 2017 at 06:54:05AM +0200, Volker Lendecke wrote:

> On Thu, May 18, 2017 at 08:40:57PM -0700, Jeremy Allison via samba-technical wrote:
> > I'm getting ready to move the @GMT snapshot
> > name handling into struct smb_filename to
> > clean up a lot of the shadow_copy2 code.
>
> As you mention it: I've got a report from a customer that a certain
> access pattern does not work anymore: GPFS has the ability to show the
> .snapshots directory (the one where all the @GMT- subdirs show up) in
> all directories, with @GMT- subdirectories being relative to the
> current dir.
>
> For clients that do not understand the @GMT- logic or the timewarp
> token, essentially all non-windows clients, we need to be able to
> access previous versions through the subdir/.snapshots/@GMT-../subdir
> path. I think in the shadow_copy2 module we need to also preserve the
> non-stripped name and try with that also. Any other ideas?

Ah. The patch I sent actually will fix that issue
(and I understand what changed - with the previous
security fixes, @GMT canonicalization moved into the filename
parsing instead of only being in the shadow_copy2 module).

Take a look at the last hunk:

diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
index fbe8eb4b325..084849d4ba1 100644
--- a/source3/smbd/filename.c
+++ b/source3/smbd/filename.c
@@ -434,6 +434,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
        bool allow_wcard_last_component =
            (ucf_flags & UCF_ALWAYS_ALLOW_WCARD_LCOMP);
        bool save_last_component = ucf_flags & UCF_SAVE_LCOMP;
+       bool snapshot_path = (ucf_flags & UCF_GMT_PATHNAME);
        NTSTATUS status;
        int ret = -1;
 
@@ -516,7 +517,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
        }
 
        /* Canonicalize any @GMT- paths. */
-       if (posix_pathnames == false) {
+       if (snapshot_path) {
                status = canonicalize_snapshot_path(smb_fname);
                if (!NT_STATUS_IS_OK(status)) {
                        goto err;

which will allow non-Windows clients not using unix
extensions to do exactly what you need.

Please review and push if happy !

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, May 19, 2017 at 09:11:49AM +0300, Uri Simchoni wrote:
>
> path/.snapshots/@GMT. Admittedly, this involves a bit of heuristics.
> There's FLAGS2_REPARSE_PATH bit in FLAGS2 - Windows clients set it and
> also Samba client since 4.6.x. The server can decide whether to be
> strict about it or use the heuristic (currently smbd ignores this flag).

That's one of the things this patch fixes :-). Please review
and push if happy !

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, May 19, 2017 at 08:37:03AM +0200, Volker Lendecke wrote:
> To support the dumb clients the heuristic would have to be to try
> inserting @GMT at all places where / now is if we lose the place the
> client sent the @GMT initially. We can't rely on the flag being set I
> believe. We're talking FreeBSD, (I believe AIX) and other custom SMB
> clients that used to work.

With my patch, for clients that don't set the flag, we will preserve the place
they sent the @GMT as smbd will no longer canonicalize the path and it will
be passed down into the VFS as a regular pathname as before.

Then the underlying shadow_copy module can then cope with the strangeness
as required.

Longer term though we have to stop supporting odd clients that don't
set the FLAGS2_REPARSE_PATH flag to mean 'access previous version' - the
complexity of supporting that in our code is an engineering burdon we simply
cannot bear going forward. Especially for a deprecated protocol (SMB1) and
as SMB2 supports previous versions without any pathname modification.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, May 19, 2017 at 08:37:03AM +0200, Volker Lendecke wrote:
>
> To support the dumb clients the heuristic would have to be to try
> inserting @GMT at all places where / now is if we lose the place the
> client sent the @GMT initially. We can't rely on the flag being set I
> believe. We're talking FreeBSD, (I believe AIX) and other custom SMB
> clients that used to work.

Thinking about this some more, the natural fix for this is
for clients that don't set the flag to just pass in the
unmodified pathname into the VFS (which my patch fixes
smbd to do), and shadow_copy2 then will sort this out in the same
way as it always did.

The security changes didn't modify the code that uses
strstr_m(name, "@GMT-") to find the timestamp in the
incoming path, so shadow_copy2 still copes with both
canonicalized and non-canonicalized paths (at least
the non-canonicalized paths work as well as they
ever did, which means there are probably some
failure cases that clients rarely come across :-).

That should fix it for your customer for now.

Then when I add in the time_t value to struct smb_filename,
so long as I simply add the processing of the timestamp
passed in as an alternative to the existing method
of searching for the @GMT timestamp, but don't remove
the @GMT search for struct smb_filenames without the
timestamp, then both the old clients that don't set the
flag, and new clients that use SMB2 or correctly set the
flag in SMB1 will still continue to work !

The only thing that would fail would be for people
who want to use @GMT snapshots via shadow_copy2, but
also have paths that contain @GMT names. This is a very
small subset of customers I think :-) :-).

That does unfortunately keep all the horrible complexity
in vfs_shadow_copy2 that adding the timestamp to
struct smb_filename was supposed to remove, but it's
then no worse than it is today :-).

Sound like a plan ?

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 05/19/2017 06:40 AM, Jeremy Allison via samba-technical wrote:

> I'm getting ready to move the @GMT snapshot
> name handling into struct smb_filename to
> clean up a lot of the shadow_copy2 code.
>
> Here are some prerequisite patches that
> clean up our handling of the SMB1 FLAGS2
> header value and remove some unneeded
> bool parameters to functions.
>
> The changes are already tested by the
> current pathname tests.
>
> Passes full make test locally.
>
> Please review and push if happy !
>
> Cheers,
>
> Jeremy.
>
Patches 1-10, RB+ me - pure refactoring (and a bugfix, thanks).

About the last step, I just want to make sure I understand what we're
trying to do, because I got a little confused by the "dumb clients"
discussion:

If I understand correctly, "dumb clients" are not
previous-version-aware, they just want to reach any path that the OS
exposes below the share's root.

Before the change, if the access path was
"somedir/.snapshots/@GMT-foo/file", we would "canonicalize" that into
"GMT-foo/somedir/.snapshots/file", and then we would not find the file,
regardless of whether shadow_copy2 is loaded or not.

After the change, we do the "canonicalization" only if the client
indicated that it wants a previous version, hence the dumb-client
problem is fixed. Samba clients only do that indication starting at
4.5.0, but even prior to that, the canonicalization doesn't change
anything for Samba clients, which put the @GMT tag at the beginning.

If that's what we want to do - fine, makes sense, RB+ too.

However, some things don't add up which puts my understanding in question:

1. Why do we need this canonicalization at all then? Is there a
previous-versions-aware client that puts the @GMT not at the beginning?
Or we just want to follow the spec to the letter, meaning that the @GMT
is allowed to be anywhere, and the decisive factor of whether it's a
previous version request is that flag.

2. Once this is fixed, why would we need to keep all the "horrible
complexity" inside vfs_shadow_copy2? Let's assume our next step is to
remove @GMT into a timestamp field *only if* it's the first component of
the path, *after* it's been canonicalized, and vfs_shadow_copy2 handles
only paths with a timestamp (i.e. it's greatly simplified). What would
not work then?

Beside all that, I have to wonder about file renaming - does it really
make sense to regard the destination file name as a potential DFS path
if the source file name is a DFS path? (off-topic but I did some digging
around rename lately, [MS-FSCC] FILE_RENAME_INFORMATION section doesn't
mention DFS, and I'm not much of a DFS expert...)

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pathname cleanups

Samba - samba-technical mailing list
On Sat, May 20, 2017 at 09:21:10PM +0300, Uri Simchoni wrote:

> On 05/19/2017 06:40 AM, Jeremy Allison via samba-technical wrote:
> > I'm getting ready to move the @GMT snapshot
> > name handling into struct smb_filename to
> > clean up a lot of the shadow_copy2 code.
> >
> > Here are some prerequisite patches that
> > clean up our handling of the SMB1 FLAGS2
> > header value and remove some unneeded
> > bool parameters to functions.
> >
> > The changes are already tested by the
> > current pathname tests.
> >
> > Passes full make test locally.
> >
> > Please review and push if happy !
> >
> > Cheers,
> >
> > Jeremy.
> >
> Patches 1-10, RB+ me - pure refactoring (and a bugfix, thanks).

Thanks !

> About the last step, I just want to make sure I understand what we're
> trying to do, because I got a little confused by the "dumb clients"
> discussion:
>
> If I understand correctly, "dumb clients" are not
> previous-version-aware, they just want to reach any path that the OS
> exposes below the share's root.
>
> Before the change, if the access path was
> "somedir/.snapshots/@GMT-foo/file", we would "canonicalize" that into
> "GMT-foo/somedir/.snapshots/file", and then we would not find the file,
> regardless of whether shadow_copy2 is loaded or not.

Not quite. The security fix change canonicalizes the
name into "@GMT-foo/somedir/.snapshots/file" to ensure
that when we have to do:

chdir("@GMT-foo/somedir/.snapshots/")
open("file")

for security reasons that the @GMT-foo is guaranteed to be
seen by shadow_copy2 (as it starts the path).

Without the canonicalization the @GMT-foo may get lost,
if it was at the end of the path.

> After the change, we do the "canonicalization" only if the client
> indicated that it wants a previous version, hence the dumb-client
> problem is fixed. Samba clients only do that indication starting at
> 4.5.0, but even prior to that, the canonicalization doesn't change
> anything for Samba clients, which put the @GMT tag at the beginning.
>
> If that's what we want to do - fine, makes sense, RB+ too.

Yep - that's the goal. Thanks !

> However, some things don't add up which puts my understanding in question:
>
> 1. Why do we need this canonicalization at all then? Is there a
> previous-versions-aware client that puts the @GMT not at the beginning?
> Or we just want to follow the spec to the letter, meaning that the @GMT
> is allowed to be anywhere, and the decisive factor of whether it's a
> previous version request is that flag.

Yes. The SMB2 code has to put the @GMT at the end, as otherwise
we may lose the incoming \\server\share\dfs\absolute\path (this
was an earlier bugfix). That's why my patch adds in the FLAGS2
flag to the fake SMB1 struct in the SMB2-create-with-timestamp
code path.

The canonicalization allows this to work.

> 2. Once this is fixed, why would we need to keep all the "horrible
> complexity" inside vfs_shadow_copy2? Let's assume our next step is to
> remove @GMT into a timestamp field *only if* it's the first component of
> the path, *after* it's been canonicalized, and vfs_shadow_copy2 handles
> only paths with a timestamp (i.e. it's greatly simplified). What would
> not work then?

So most of the "horrible complexity" inside vfs_shadow_copy2 turns out
to be related to where the snapshot paths are on disk, not in the
code that finds the SMB-layer @GMT- string. So yes, once I've fixed
all VFS pathnames to be struct smb_filename I can then remove the
SMB-layer @GMT-foo IF the flag is set and IF the @GMT is at the
start (after canonicalization). This will simplify one of
the parsing code paths inside vfs_shadow_copy2, but the main thing
it'll allow is fast detection of incoming previous version paths.

On another related note - the reason this will fix Volker's "old SMB1 clients
with no flag issue" is that with the patch the incoming client pathname
will be left completely alone and be passed into vfs_shadow_copy2
as-is, restoring the old behavior.

These old clients might run into the chdir()/open() issue if
they put the @GMT at the end of the path, but if that's really
an issue they can turn off symlink restrictions and remove
the chdir()/open() changes if they simply *MUST* make this
work (at the cost of removing the symlink race protection).

> Beside all that, I have to wonder about file renaming - does it really
> make sense to regard the destination file name as a potential DFS path
> if the source file name is a DFS path? (off-topic but I did some digging
> around rename lately, [MS-FSCC] FILE_RENAME_INFORMATION section doesn't
> mention DFS, and I'm not much of a DFS expert...)

Yep, eventually I'm going to have to add more tests here
(although you may already have found this out by your
pinging of dochelp :-).

Cheers,

        Jeremy.