New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply chmod on UNIX socket after creation #110

Merged
merged 6 commits into from Sep 2, 2018

Conversation

Projects
None yet
3 participants
@webknjaz
Member

webknjaz commented Aug 30, 2018

Fixes #93

  • What kind of change does this PR introduce?

    • bug fix
    • feature
    • docs update
    • tests/coverage improvement
    • refactoring
    • other
  • What is the related issue number (starting with #)
    #93

  • What is the current behavior? (You can also link to an open issue here)
    code unlinks the socket file and then immediately tries to chmod it

  • What is the new behavior (if this is a feature change)?
    it should chmod after bind

  • Other information:
    Very first appearance: 6ced9fd#diff-920de15fced9e5f9697efed08f3c207dR214

  • Checklist:

    • I think the code is well written
    • I wrote good commit messages
    • I have squashed related commits together after the changes have been approved
    • Unit tests for the changes exist
    • Integration tests for the changes exist (if applicable)
    • I used the same coding conventions as the rest of the project
    • The new code doesn't generate linter offenses
    • Documentation reflects the changes
    • The PR relates to only one subject with a clear title
      and description in grammatically correct, complete sentences

This change is Reviewable

@webknjaz webknjaz self-assigned this Aug 30, 2018

@webknjaz webknjaz requested a review from jaraco Aug 30, 2018

@jaraco

Overall, I'm thinking nothing needs to change here except to remove the setting of permissions and let the downstream clients adjust the permissions if needed. Would that be sufficient?

@@ -1805,7 +1793,22 @@ def bind(self, family, type, proto=0):
# this machine's TCP stack
pass
# So we can reuse the socket...
try:
if not IS_WINDOWS and family == socket.AF_UNIX:

This comment has been minimized.

@jaraco

jaraco Aug 31, 2018

Member

Can we omit the IS_WINDOWS check? It feels like a redundant check, especially after having constructed a socket.socket(AF_UNIX). It's also conceivable (though unlikely) that Windows could support this socket type. Better to just let operations fail, I think.

This comment has been minimized.

@webknjaz

webknjaz Aug 31, 2018

Member

As I recall accessing socket.AF_UNIX causes AttributeError under Windows, that's why this guard exists. Windows doesn't have such a concept at all.

@@ -1805,7 +1793,22 @@ def bind(self, family, type, proto=0):
# this machine's TCP stack
pass
# So we can reuse the socket...

This comment has been minimized.

@jaraco

jaraco Aug 31, 2018

Member

I dislike how this functionality is now embedded in the bind method and straddling the bind call.

I'm inclined to leave the unlink operation in prepare socket.

This comment has been minimized.

@webknjaz

webknjaz Aug 31, 2018

Member

Please take a look at this comment of the reporter: #93 (comment)
It looks like these things are quite tightly coupled. Also, it's needed exactly around socket.bind().

What I don't like even more is that bypassing of the bind_addr is done via implicit assignment to the self object attribute.

# So everyone can access the socket...
try:
if not IS_WINDOWS and family == socket.AF_UNIX:
os.chmod(self.bind_addr, 0o777)

This comment has been minimized.

@jaraco

jaraco Aug 31, 2018

Member

Is 0o777 the right permissions to set unconditionally? Why not let the os set the permissions naturally, and leave it to the client to change the permissions if needed?

This comment has been minimized.

@webknjaz

webknjaz Aug 31, 2018

Member

It guarantees that everyone can access it. Also, this PR is not intended to change the behavior. Only fix a mistake.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 31, 2018

I'm not sure about complete removal of chmod recognizing that most of (or all?) other HTTP servers allow bypassing the config with these access privileges while having other sufficient default. Also, I'm not sure about what side-effects this removal may cause.

@codecov

This comment has been minimized.

codecov bot commented Sep 1, 2018

Codecov Report

Merging #110 into master will decrease coverage by 0.6%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
- Coverage   70.28%   69.68%   -0.61%     
==========================================
  Files          20       20              
  Lines        3184     3219      +35     
==========================================
+ Hits         2238     2243       +5     
- Misses        946      976      +30

I've separated bind into 3 stages. Let's review this part again.

@webknjaz

This comment was marked as resolved.

Member

webknjaz commented Sep 1, 2018

This pull request introduces 1 alert when merging d2fbe49 into 5edf9cb - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

Comment posted by LGTM.com

@webknjaz webknjaz force-pushed the bugfix/chmod-unix-sock-after-creation branch from d2fbe49 to 7b49f03 Sep 1, 2018

@webknjaz

This comment was marked as resolved.

Member

webknjaz commented Sep 1, 2018

This pull request introduces 1 alert when merging bc58cdb into 5edf9cb - view on LGTM.com

new alerts:

  • 1 for Unused local variable

Comment posted by LGTM.com

@webknjaz

This comment has been minimized.

Member

webknjaz commented Sep 1, 2018

@jaraco @hvenev I've redesigned this PR a bit. Could you please take a look?

File does not exist, which is the primary goal anyway.
"""
sock = self.prepare_socket(

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

TODO: try setting os.uname before this call.

This comment has been minimized.

@hvenev

hvenev Sep 1, 2018

Do you mean umask or changing the system hostname? umask is per-process, so it might cause problems in multithreaded processes (affecting other threads' created files).

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

Yeah, but maybe we can grab current umask, create a socket and then unset it. I'll probably not do it for now.

host, port = self.bind_addr[:2]
host, port = bind_addr[:2]

This comment has been minimized.

@hvenev

hvenev Sep 1, 2018

I'm not sure these make sense for AF_UNIX. In fact, most of this function should probably be moved to the TCP/IP bind().

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

Same, I don't want to touch it now. That's also further plans for refactoring it and turning it into some struct.

if self.ssl_adapter is not None:
self.socket = self.ssl_adapter.bind(self.socket)
if ssl_adapter is not None:
sock = ssl_adapter.bind(sock)

This comment has been minimized.

@hvenev

hvenev Sep 1, 2018

I'm not sure about this, but we might want to use the SSL adapter for systemd-provided sockets. If so, maybe this should be moved to prepare_socket().

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

That's a fair point, I prefer to think about it outside of this PR to keep changes small.

if self.nodelay and not isinstance(self.bind_addr, str):
self.socket.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
if nodelay and not isinstance(bind_addr, str):

This comment has been minimized.

@hvenev

hvenev Sep 1, 2018

Maybe check against six.string_types similarly to prepare()?

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

It's a good question. I'd rather check family though. I just didn't want to change much.

@jaraco

jaraco approved these changes Sep 1, 2018

I like the direction you've gone with this. @webknjaz has done a good job of factoring out functions for the separate aspects, so kudos for that.

This behavior now encapsulates enough functionality, I'm tempted to think that we should generalize this concept to a BindHandler or SocketBuilder interface, something that clearly encapsulates the different behaviors of prepare_socket.

Maybe that would be worthwhile, but doesn't need to happen with this PR.

I do feel a little uneasy adding all of these changes just to set permissions after the socket is created.

info = [
(socket.AF_UNIX, socket.SOCK_STREAM, 0, '', self.bind_addr)]
self.bind_unix_socket(self.bind_addr)
except socket.error as serr:

This comment has been minimized.

@jaraco

jaraco Sep 1, 2018

Member

This error handling is asymmetric to how a socket error is handled for AF_INET*.

This comment has been minimized.

@webknjaz

webknjaz Sep 2, 2018

Member

What do you mean? Moving socket.close() inside the method? I hope to generalize this stuff sometime in future.

return socket_
@staticmethod
def resolve_real_bind_addr(socket_):

This comment has been minimized.

@jaraco

jaraco Sep 1, 2018

Member

Why not use sock, which is used elsewhere in this module for the socket.socket instance?

This comment has been minimized.

@webknjaz

webknjaz Sep 2, 2018

Member

Because it's a function signature, more or less public interface. So I restricted usage of shortcuts inside functions only,

'AF_UNIX sockets are not supported under Windows.'
)
fs_permissions = 0o777 # TODO: allow changing mode

This comment has been minimized.

@jaraco

jaraco Sep 1, 2018

Member

I'm still uneasy with this as a default. I think the default should be not to change any permissions from whatever the OS sets.

This comment has been minimized.

@webknjaz

webknjaz Sep 2, 2018

Member

All servers I can recall have 0o666 as default permissions.
Also, take into account that if the permissions don't have rw, then nobody will be able to use this socket and our goal is to have it accessible by default. See man -s 7 unix for details:


   Pathname socket ownership and permissions
       In the Linux implementation, pathname sockets honor the permissions of the directory they are in.  Creation of a new
       socket fails if the process does not have write and search (execute) permission on the directory in which the socket
       is created.

       On Linux, connecting to a stream socket object requires write permission on that socket; sending  a  datagram  to  a
       datagram  socket  likewise  requires  write  permission on that socket.  POSIX does not make any statement about the
       effect of the permissions on a socket file, and on some systems (e.g.,  older  BSDs),  the  socket  permissions  are
       ignored.  Portable programs should not rely on this feature for security.

       When creating a new socket, the owner and group of the socket file are set according to the usual rules.  The socket
       file has all permissions enabled, other than those that are turned off by the process umask(2).

       The owner, group, and permissions of a pathname socket can be changed (using chown(2) and chmod(2)).

webknjaz added some commits Aug 30, 2018

Split HTTPServer.bind into 3 stages
* prepare_socket
* bind_socket
* resolve_real_bind_addr

@webknjaz webknjaz force-pushed the bugfix/chmod-unix-sock-after-creation branch 2 times, most recently from 49b0b2d to 0ed0249 Sep 2, 2018

webknjaz added some commits Sep 1, 2018

Implement smart file premission setters
* Attempt pre-populating file permissions for socket
* Resolve sock address before chmod
* Improve socket fs permissions set up fallback mode
* Log a warning about failing to set fs mode

@webknjaz webknjaz force-pushed the bugfix/chmod-unix-sock-after-creation branch from 0ed0249 to a410d9c Sep 2, 2018

@webknjaz webknjaz merged commit 9f77c31 into master Sep 2, 2018

2 of 11 checks passed

LGTM analysis: Python Running analyses for revisions
Details
License Compliance FOSSA is analyzing this commit
Details
ci/circleci: linux-build CircleCI is running your tests
Details
ci/circleci: macos-build Your tests are queued behind your running builds
Details
codeclimate Code Climate is analyzing this code.
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
WIP ready for review
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment