Opened 8 years ago

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

Milestone: release 2.3.3

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

Milestone: release 2.3.3release 2.3.4

david...?

comment:5 Changed 7 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 7 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 7 years ago by David Sommerseth

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

comment:7 Changed 7 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 7 years ago by David Sommerseth (previous) (diff)

comment:8 Changed 6 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 6 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 6 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.