Careful handling when loading files#31721
Conversation
jentfoo
requested review from
Tener,
codingllama,
r0mant,
rosstimothy,
timothyb89 and
zmb3
jentfoo
force-pushed
the
jent/symlink_fixes
branch
from
be06b1b to
067bbd0
Compare
|
Independently, these two params are meaningful in this context:
These are available in Linux kernel 3.6+, released 11 years ago, and enabled in most contexts (but not all). I don't think such an option is available on MacOS, though. It surely isn't the default. |
|
From the compatibility point of view, this is a breaking change. I don't think it will affect many configurations, but it is possible, in principle, that one of the paths in which we will now disallow symlinks contains those. For example, you can have |
Could we mention in the changelog, for example, the places where symlinks are not allowed anymore? (Ie, the places affected by the change.) |
|
Compatibility options are being discussed here: https://github.com/gravitational/teleport-private/issues/1009#issuecomment-1716028879 I think we can find a way to backport this improvement while maintaining compatibility. |
I fail to see how the behavioral change would be non-breaking. I'm not saying to not backport, no strong opinions there on my part, just asking that we document whatever gets changed. |
This commit attempts to provide a common API so that the decision of when to follow symlinks is a conscious decision. Because Teleport (particularly the agent) runs in a privilege context, there is risk that following symlinks may allow information disclosure. After review of the cases covered in this commit (and some additional cases where this API was not a natural fit), this does not appear to be a broad problem. This commit however does fix the one known flaw described in the issue https://github.com/gravitational/teleport-private/issues/1009
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Looks good, holding out just in case you do more test changes.
public-teleport-github-review-bot
Bot
removed request for
klizhentas,
r0mant and
rosstimothy
r0mant
left a comment
r0mant
left a comment
There was a problem hiding this comment.
I don't have objections to including this change in Teleport 14 (bar potential compilation issue) but @jentfoo can you please add a section describing the user impact of this change to our draft release notes under the "Breaking changes".
I also assume you did a smoke test with this change (e.g. starting teleport and starting a session)?
|
We also build tctl for windows. We don't ship it yet, but we do build it on push and a lot of work went in to getting this far so please don't break it 🙂 |
Make hardlink count lookup code build conditional to avoid undefined syscall.Stat_t.
| if statT, ok := fi.Sys().(*syscall.Stat_t); ok { | ||
| return true, statT.Nlink | ||
| } else { | ||
| return false, 0 | ||
| } |
There was a problem hiding this comment.
I'd write this slightly differently in order to eliminate the else and align the happy path to the left.
statT, ok := fi.Sys().(*syscall.Stat_t)
if !ok {
return false, 0
}
return true, statT.Nlink
There was a problem hiding this comment.
This seems basically the same to me. I tend to prefer avoiding ! conditionals when logic when both states needs to be handled anyways.
|
@r0mant I have a suggestion in the release notes for how to describe this. I tried to describe it fairly generically, but the only known use case to me is the loading of the Also I have added the |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
6 participants
This PR attempts to provide a common API so that the decision of when to follow symlinks is a conscious decision. Because Teleport (particularly the agent) runs in a privileged context, there is risk that following symlinks may allow information disclosure.
After review of the cases covered in this commit (and some additional cases where this API was not a natural fit), this does not appear to be a broad problem. This PR however does fix the one known flaw described in the issue https://github.com/gravitational/teleport-private/issues/1009
In many cases where this API is being adopted, symlinks are still being allowed. This is only after verifying there is no information disclosure when symlinks are followed. Although symlinks are not likely useful in many of these cases, this choice to still allow symlinks is to maintain compatibility with potentially unknown use cases. Future use of this API will almost certainly always specify a
falsefor allowing symlinks.