Sftp remote file support #4750
Sftp remote file support #4750
Conversation
…ider to maintain the precedent set by the ftp provider
|
John is right, the gem dependency would be great to be handled by the core chef & this would make things for us at NCR much more simple with dealing with SFTP resources. |
|
Doh! I'll get on those build failures shortly |
|
As I say every time this shows up: put it in a cookbook, if it becomes popular then nominate it for core inclusion. With the new gem metadata there is literally no reason to add this to core as a first step. |
|
Does the gem metadata support gem installation from local file system cache? I can't find anything that says so. Our PCI compliant products are still stuck in the same situation since they have to include gem dependencies in the cookbook payload and are restricted from network resources. It feels like a no-brainer to expand remote_file access protocols, especially more secure protocols but I'm sure every PR you guys get looks like a no-brainer to the author |
|
and the cookbook is here: |
|
Adding any feature to Chef core is an increased management and maintenance load, so it always has to be weighed against the magnitude of the benefit to users. Given the limitations of this implementations (notably that it requires plain-text passwords be floating around in the recipe) I can't see this being a security win for most people and so the end benefit is going to be minimal. Using this effectively would require having an existing secrets management/distribution platform and in most cases you should just use that for access secure data instead of SFTP :) There are totally cases where you would want SFTP that aren't amenable to tools like Vault or KeyWhiz, like moving big files around, but that is a less common use case (most people would use HTTPS for that situation). Anyways, hope that explains my resistance. |
|
Basically mutually authenticated file transfer under heavy automation is a Hard Problem and I don't think this fits the bill well as it requires a lot of support services Chef does not provide (well). |
|
@thommay we should look at this in PR review |
|
@lamont-granquist if there's any information I can provide let me know. I'll get the rubocop issues cleaned up |
|
The rubocop failures are on files not touched in this PR. What's the best practice for that? I don't mind fixing them but I don't want to pollute the PR either. |
|
could you rebase on top of master and force push? the merge commits are making this PR pretty ugly... |
|
Yessir, got caught up in holiday stuff. working on it |
jkerry
force-pushed
the
SFTP_Remote_File_Support
branch
from
dd05825
to
2420f2e
Compare
|
Back to just my changes. My apologies for the mess. |
|
Known issue: the copyright information on lib/chef/provider/remote_file/sftp.rb reads Jesse Campbell as it does in the ftp provider. I'll switch that to my name when I get back to the desk. |
| def filename | ||
| parse_path if @filename.nil? | ||
| @filename | ||
| end |
There was a problem hiding this comment.
this looks copied from the ftp class
the thing is that in #get you've removed the one reference to this method. the filename in parse_path is actually a local variable that gets constructed (hella confusingly -- the ftp class could use a bit of cleanup to make it clearer) and therefore both this #filename code and the #parse_path code are unused in this class that you've written and could simply be deleted.
|
other than the dead code i think this looks fine. 👍 |
|
Yeah. I'm 👍 after a bit of a spring clean. |
|
It is used as a path validation mechanism (utilized directly in the tests). I can probably validate the path more directly and fulfill the same interface |
…nto the validate_path code.
|
@thommay @lamont-granquist is this what you had in mind? |
| raise ArgumentError, "no filename: #{path.inspect}" | ||
| end | ||
|
|
||
| @directories, @filename = directories, filename |
There was a problem hiding this comment.
think you can still ditch this line.
There was a problem hiding this comment.
Derp. Pushing a removal now
|
👍 LGTM |
|
|
|
Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog. Non-GitHub Verified CommittersThere are 1 commit author(s) whose commits are authored by a non-GitHub verified email address. Chef will have to manually verify that they are authorized to contribute. |
This is a PR to fulfill #4043 . The sftp_provider cookbook has gotten at least one PR since creation and sftp usage is strictly required for my employer's security policy. I think there's enough community need to consider a PR into core now. Additionally the sftp_provider cookbook creates an awkward gem dependency race condition that is addressed if that dependency is delivered with the chef-client install. I structured the spec to be as similar to the ftp spec as possible where appropriate to ensure the interface is similar.
Notable gaps include:
These might be addressable but I'm not sure at this time if they are inside the capability of the net-sftp gem.