Modify

Opened 7 years ago

Closed 7 years ago

#1206 closed enhancement (fixed)

Enable TLS-Auth in OpenVPN

Reported by: Jon "The Nice Guy" Spriggs <jon@…> Owned by:
Priority: normal Milestone: Firmware 2.3.7.0
Component: fon-plugin-openvpn Version: N/A
Severity: normal
Cc: Hardware: both

Description

As per Hardening OpenVPN Security, it might be a good idea if we allow users to enable TLS Authentication of their OpenVPN connections.

Patch attached.

Attachments (1)

patch (2.3 KB) - added by Jon "The Nice Guy" Spriggs <jon@…> 7 years ago.
tls-auth patch

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by Jon "The Nice Guy" Spriggs <jon@…>

tls-auth patch

comment:1 Changed 7 years ago by matthijs

  • Milestone set to Firmware 2.3.7.0
  • Severity changed from unknown to normal
  • Status changed from new to confirmed

Hmm, tls-auth is a bit of a confusing name, I though "We're already using TLS for auth, right?". But it seems that tls-auth means to add additional authentication (HMAC signatures) on the TLS handshaking, which seems like a good plan.

Some comments on the patch (just a quick look, haven't looked at the actual code):

  1. It seems the key is now generated _after_ starting OpenVPN instead of before? I'd say that generating the key makes more sense to do while generating the other keys? Because the hardening can be enabled later, this would require generating ta.key unconditionally, but I don't think that's much of a problem?
  2. I don't really like the duplication in the if/else structure in openvpn-client.sh. Perhaps it would make sense to introduce a FILES variable and add the ta_$1.key to that when it's enabled?
  3. Perhaps the option should be named "TLS handshake hardening (tls-auth)" or something like that?
  4. The option needs some kind of notice that changing it requires regenerating the client configuration (but this is also needed for the port number configuration in another ticket, so I'll have a look at this when committing this. Consider this remark a reminder for myself).

comment:2 Changed 7 years ago by matthijs

Jon, do you think you can have another look at this patch and my comments?

comment:3 Changed 7 years ago by matthijs

(If you're too busy right now, just say so and I'll have stab it myself, btw)

comment:4 Changed 7 years ago by matthijs

I committed this patch, with my above comments taken into account. I'll push it out to SVN this week.

comment:5 Changed 7 years ago by matthijs

  • Status changed from confirmed to testing-fix

comment:6 Changed 7 years ago by matthijs

Found a small (but important) typo in the patch: it should be --genkey, not --gen-key.

comment:7 Changed 7 years ago by matthijs

(In [2177]) openvpn: Slightly restructure client config generation.

This uses a subdirectory of /tmp to collect the files, which removes the need to name all files separately on the zip cmdline. This makes it easier to optionally add files to the zip later on.

References: #1206

comment:8 Changed 7 years ago by matthijs

  • Resolution set to fixed
  • Status changed from testing-fix to closed

(In [2179]) luci-openvpn: Allow enabling tls-auth.

This adds a bit more security to the TLS handshake during the OpenVPN connection setup. Having it disabled does not pose a security risk in any way, but having it enabled only adds another layer of security, just in case.

Thanks to Jon Spriggs for most of this patch.

Closes: #1206

Add Comment

Modify Ticket

Action
as closed The ticket will remain with no owner.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.