Opened 12 years ago
Closed 10 years ago
#277 closed Bug / Defect (fixed)
client-config-dir silently ignored if not readable
Reported by: | jglick | Owned by: | David Sommerseth |
---|---|---|---|
Priority: | minor | Milestone: | release 2.3.5 |
Component: | Configuration | Version: | OpenVPN 2.3.0 (Community Ed) |
Severity: | Not set (select this one, unless your'e a OpenVPN developer) | Keywords: | |
Cc: |
Description
OpenVPN 2.3.0 x86_64-unknown-linux-gnu [SSL (OpenSSL)] [LZO] [EPOLL] [eurephia] [MH] [IPv6] built on Feb 9 2013
I spent a considerable amount of time wondering why an iroute
setup (to export a client’s LAN to the VPN) stopped working after some configuration changes. The clearest symptom was that the server was pushing the route to the client even though the common name on its certificate was correct and a client-config-dir
entry with that common name specified an iroute
. After Peer Connection Initiated
was printed in the server log, MULTI_sva: pool returned …
immediately followed, with no intervening OPTIONS IMPORT: reading client specific options from: …
.
It turns out that the problematic change was adding user nobody
to the server configuration: the directory specified in client-config-dir
was in someone’s home directory and was not world-readable. By the time this file was needed, the openvpn server process could no longer access it.
Fair enough, but why did the server fail to record the fact that it could not traverse one of the parent directories? Simply printing to the server log something like
WARNING: cannot look in … for client configurations; not readable
would have saved a lot of debugging time.
(Even better would be to read the files in this directory when the server starts, before switching users, though some people may be relying on being able to change these files without restarting the server.)
Off topic note: the new user registration for this site refuses legitimate email addresses of the form realname+token@something.com
, which GMail and other services offer to let you create aliased addresses for tracking purposes.
Attachments (1)
Change History (11)
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
Owner: | set to David Sommerseth |
---|---|
Status: | new → assigned |
Dazo, I'm lazy, so I'm just throwing this in your lap.
I know we have code to check all the directories and files and so on nowadays, and that stuff is in 2.3.0 already - shouldn't it catch a non-readable ccd/ directory?
(Actually "x" is sufficient normally as we don't *read* it just try to open a file under it, but still, we should catch it if permissions are insufficient, or one of the directories in the path are not sufficient. Maybe we check for "r" and not "x"?)
comment:3 Changed 11 years ago by
Milestone: | → release 2.3.3 |
---|
comment:5 Changed 11 years ago by
Hmm ... I wonder if this is happening due to the --user/--group usage. Considering commit 0f2bc0dd92f43c91e33bba8a66b06b98f281efc1 introduces a check for --client-config-dir. In addition, commit b77bffe8186647c6fd1f2f76aac41fd45719edb8 adds support for tackling chroot. But they do not check the uid/gid, so that may be what causes this issue.
Implementing that will be a bigger patch and reworking check_file_access(), plus ensuring we check for the uid/gid only where it's needed.
In general, I'm okay with that ... but how much should we do this hand-holding of the administrator?
comment:6 Changed 11 years ago by
Digging a little bit deeper, following options_server_import() and read_config_file(). If options_server_import() is called, read_config_file() will report a read error *if* the CCD file is found and can be opened. However, the test_file() which decides if calling options_server_import() will silently ignore any errors, and just keep quiet.
And that behaviour is kind of fine, IMHO. Because it might not be you want a CCD file for all your clients, and the only way to kill of that error would be to create an empty DEFAULT file in the CCD. We'll definitely get some complaints if we do such a change.
test_file() should probably check an error code more carefully. However, that need careful testing as we use different functions to open files on the *nix platforms and Windows.
Changed 11 years ago by
Attachment: | 0001-Improve-error-reporting-on-file-access-to-client-con.patch added |
---|
[PATCH] Improve error reporting on file access to --client-config-dir and --ccd-exclusive
comment:7 Changed 11 years ago by
Status: | assigned → accepted |
---|
Just sent the attached patch to the openvpn-devel mailing list. You can track the review process here: http://article.gmane.org/gmane.network.openvpn.devel/8538
comment:8 Changed 11 years ago by
Is this ready for inclusion? Has it been tested with the suggested use of openvpn_errno instead of errno (for Windows this expands to GetLastError?() or such)?
As I want to ship 2.3.4 today or tomorrow, this smells like "2.3.5"...
comment:9 Changed 11 years ago by
Milestone: | release 2.3.4 → release 2.3.5 |
---|
ACK needed, but we'll make 3.2.5 :-)
comment:10 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
Patch ACKed, commited to master and release/2.3, will be part of 2.3.5 (but do not ask me for a release date for that :-) ).
commit 4978dadaed4ecf1b9dd256f57c6a5c895691580b (master)
commit 488994b3f65242ec31b015f53e21d935f75b8bee (release/2.3)
Author: David Sommerseth
Message-Id: <1398990504-4239-1-git-send-email-dazo@…>
URL: http://article.gmane.org/gmane.network.openvpn.devel/8688
It seems like a better error message would be nice to have.
Regarding the realname+token email addresses: this is a known issue in the particular version of the registration webapp we're using. Next upgrade of that app should fix it, and that something I need to do it within next few months.