Opened 11 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)

0001-Improve-error-reporting-on-file-access-to-client-con.patch (2.3 KB) - added by David Sommerseth 10 years ago.
[PATCH] Improve error reporting on file access to --client-config-dir and --ccd-exclusive

Download all attachments as: .zip

Change History (11)

comment:1 Changed 11 years ago by Samuli Seppänen

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.

comment:2 Changed 10 years ago by Gert Döring

Owner: set to David Sommerseth
Status: newassigned

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 10 years ago by Gert Döring

Milestone: release 2.3.3

comment:4 Changed 10 years ago by Gert Döring

Milestone: release 2.3.3release 2.3.4

david...?

comment:5 Changed 10 years ago by David Sommerseth

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 10 years ago by David Sommerseth

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 10 years ago by David Sommerseth

[PATCH] Improve error reporting on file access to --client-config-dir and --ccd-exclusive

comment:7 Changed 10 years ago by David Sommerseth

Status: assignedaccepted

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

Btw! Please try to test this patch as much as possible.

Last edited 10 years ago by David Sommerseth (previous) (diff)

comment:8 Changed 10 years ago by Gert Döring

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 10 years ago by Gert Döring

Milestone: release 2.3.4release 2.3.5

ACK needed, but we'll make 3.2.5 :-)

comment:10 Changed 10 years ago by Gert Döring

Resolution: fixed
Status: acceptedclosed

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

Note: See TracTickets for help on using tickets.