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

Add SSL Client support #63

Merged
merged 32 commits into from Mar 15, 2015

Conversation

Projects
None yet
5 participants
@mhaas
Contributor

mhaas commented Mar 10, 2015

This pull request adds optional SSL client support when talking to the auth server. This is useful if your auth server only speaks HTTPS or if you just want to use HTTPS in a post-snowden world.

The patch is tested lightly: it works with my server. The code is heavily based on the [http://www.yassl.com/yaSSL/Docs-cyassl-manual-11-ssl-tutorial.html](CyaSSL tutorial). No guarantee regards "security" given :)

Fixes #61

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 11, 2015

Two problems:

  • ca-certificates are not loaded, so SSLNoPeerVerification is required
  • in conf.c, the debug logs fire although they are #ifndef'd with USE_CYASSL

mhaas added some commits Mar 11, 2015

Include #config.h in in conf.c
Otherwise, USE_CYASSL won't be available.
Log cert error code
When loading CA certificates with CyaSSL, print the error code
on failure
src/conf.c Outdated
@@ -96,6 +97,8 @@ typedef enum {
oTrustedMACList,
oHtmlMessageFile,
oProxyPort,
oSSLNoPeerVerification,

This comment has been minimized.

@acv

acv Mar 12, 2015

Contributor

I would suggest a key to disable the use of HTTPS globally when talking to AuthServer for testing purpose.

This comment has been minimized.

@mhaas

mhaas Mar 13, 2015

Contributor

You can simple set SSLAvailable to No in the auth server section. Clients will be redirected to HTTP then, but if it's just for testing, that should be ok.

This comment has been minimized.

@mhaas

mhaas Mar 13, 2015

Contributor

Added a note in wifidog.conf: 4f634ed

src/conf.h Outdated
@@ -67,6 +67,8 @@
#define DEFAULT_AUTHSERVMSGPATHFRAGMENT "gw_message.php?"
#define DEFAULT_AUTHSERVPINGPATHFRAGMENT "ping/?"
#define DEFAULT_AUTHSERVAUTHPATHFRAGMENT "auth/?"
#define DEFAULT_AUTHSERVSSLCERTPATH "/etc/ssl/certs/"
#define DEFAULT_AUTHSERVSSLNOPEERVER 0

This comment has been minimized.

@acv

acv Mar 12, 2015

Contributor

Would suggest making sure the name clearly indicate whether validation will be done or not, as it is, I don't know what 0 means here? Is default on or default off?

This comment has been minimized.

@mhaas

mhaas Mar 13, 2015

Contributor

The option is off (0) by default. The name SSLNoPeerVerification - you have to actively turn it on as a reminder that this is potentially bad.
Is 4545c6a better?

int http_get(const int, char*);
#ifdef USE_CYASSL

This comment has been minimized.

@acv

acv Mar 12, 2015

Contributor

HAVE_CYASSL might be a better name?

This comment has been minimized.

@mhaas

mhaas Mar 12, 2015

Contributor

HAVE_CYASSL is set by AC_HAVE_LIBRARY. In the current patch, autoconf only uses AC_HAVE_LIBRARY(cyassl) if --enable-cyassl is passed, so we can indeed use that. However, I wanted to have a distinction between "cyassl is available" and "we actually want to use ssl".

This comment has been minimized.

@acv

acv Mar 12, 2015

Contributor

That makes sense.

buf[totalbytes] = '\0';
debug(LOG_DEBUG, "HTTP Response from Server: [%s]", buf);
CyaSSL_free(ssl);

This comment has been minimized.

@acv

acv Mar 12, 2015

Contributor

White space.

/* Create CYASSL object */
CYASSL* ssl;

This comment has been minimized.

@acv

acv Mar 12, 2015

Contributor

White space is weird... But more importantly there should be very clear logging of the certificate validation failure along with some info about options (fix cert path, disable check) or at least a pointer to config.

This comment has been minimized.

@mhaas

mhaas Mar 13, 2015

Contributor

Better in 59ef4b3?

This comment has been minimized.

@acv

acv Mar 13, 2015

Contributor

Yes.

} else {
res = http_get(sockfd, request);
}
#endif

This comment has been minimized.

@acv

acv Mar 12, 2015

Contributor

Can use #else

This comment has been minimized.

@mhaas

mhaas Mar 13, 2015

Contributor

How so? Even when compiled with USE_CYASSL, we still have to check if the authserver is configured to use ssl.

debug(LOG_DEBUG, "Level %d: Connecting to auth server %s:%d", level, hostname, auth_server->authserv_http_port);
port = htons(auth_server->authserv_http_port);
}
#endif

This comment has been minimized.

@acv

acv Mar 12, 2015

Contributor

Again, #else can be used.

@acv

This comment has been minimized.

Contributor

acv commented Mar 12, 2015

Stylistic thing but generally, it's best to do white space / formatting fix separately to minimize code to review ;-)

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 12, 2015

@avc Sorry about the white space. I tried to minimize his - or are you talking about keeping white space and actual code PRs separate?

Thanks for the thorough review!

@acv

This comment has been minimized.

Contributor

acv commented Mar 12, 2015

Yeah, the latter. White space PRs as separate.

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 12, 2015

Thanks. There are some other proiblems:

  • I can't load SSL certs, the call fails with E_COMPRESS for some reason. Maybe I'm missing zlib?!
  • I might have to enable domain name verification separately
@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 12, 2015

Do you need me to re-do the pull request re whitespace or can we let it slip one time? ;)

@acv

This comment has been minimized.

Contributor

acv commented Mar 12, 2015

Don't worry about it.

florida63 and others added some commits Mar 12, 2015

Fix removal option TrustedMACList
Certainly a mistake among all the recent corrections.

TrustedMACList Isn't deprecated.
Merge pull request #1 from florida63/patch-1
Fix removal option TrustedMACList
Add helpful error message on cert failure
If certificates can't be loaded, then direct user to config options.
Fix: wrong constant, use ASN_UNKNOWN_OID_E
Use ASN_UNKNOWN_OID_E instead of COMPRESS_E
@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 15, 2015

You were right, it is ASN_UNKNOWN_OID_E. I mixed up -184 and -148.

@acv

This comment has been minimized.

Contributor

acv commented Mar 15, 2015

Sweet, so we have a root cause (ECC root certs) and a fix (--enable-ecc).

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 15, 2015

Removed the "error state on socket" message.

cyassl with --enable-ecc works for me now.

I think we can merge. I didn't test against known broken servers (e.g. expired, wrong hostname), though.

@acv

This comment has been minimized.

Contributor

acv commented Mar 15, 2015

I would suppress the logging when numbytes == 0 in the read loop personally but I think it's merge ready now otherwise.

@acv

This comment has been minimized.

Contributor

acv commented Mar 15, 2015

I checked the hostname mismatch case and it prints a meaning full error about peer mismatch.

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 15, 2015

Good. You pull the trigger, we share the blame.

acv added a commit that referenced this pull request Mar 15, 2015

@acv acv merged commit 29207c8 into wifidog:master Mar 15, 2015

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 15, 2015

FYI, i requested that OpenWrt enable ECC: https://dev.openwrt.org/ticket/19188#ticket

@acv

This comment has been minimized.

Contributor

acv commented Mar 15, 2015

Sweet.

@sinkcup

This comment has been minimized.

Member

sinkcup commented on src/conf.c in 6a65ff5 Mar 17, 2015

hello, who add "Invalid mask" back ...
I had delete it for "domain whitelist" #76 commit 43a4590

This comment has been minimized.

Contributor

mhaas replied Mar 17, 2015

That was probably my fault when I did a rather unclean merge. Just remove it.

@sinkcup

This comment has been minimized.

Member

sinkcup commented on src/conf.c in 6a65ff5 Mar 18, 2015

line 580 is same of 557 ?

This comment has been minimized.

Contributor

acv replied Mar 18, 2015

TO_NEXT_WORD(ptr, finish_flag) is a macro that advances the first argument (char *) forward by one word or reaches the end of line trying. It is normal that it repeats.

Definition from earlier in the file (line ~380)

@florida63 florida63 added this to the 1.2.0 milestone Mar 24, 2015

@databeille

This comment has been minimized.

Contributor

databeille commented Mar 27, 2015

Do you have :
(simple_http.c:256) CyaSSL_send failed: don't have enough data to complete task
when trying to ping authserver ?

build for openwrt BB with cyassl 3.3.0 or cyassl 3.3.2 master

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 27, 2015

No. Can you post your config? What platform?

@acv

This comment has been minimized.

Contributor

acv commented Mar 27, 2015

That sounds like connect to a non SSL server with SSL.

This happens when the server response is not the expected size on the TLS handshake:

    if (helloSz < RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ + CLIENT_HELLO_FIRST)
            return INCOMPLETE_DATA;
@databeille

This comment has been minimized.

Contributor

databeille commented Mar 30, 2015

@mhaas I have a ar71xx router, running openwrt BB.

I use https://github.com/indutny/bud as TLS terminator ; will try with a basic apache setting to check.

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 30, 2015

On 03/30/2015 02:33 PM, databeille wrote:

@mhaas https://github.com/mhaas I have a ar71xx router, running
openwrt BB.

I use https://github.com/indutny/bud as TLS terminator, will try with a
basic apache setting to check.

Does it work with a regular browser? Perhaps the problem lies between
the terminator and your webserver.

@databeille

This comment has been minimized.

Contributor

databeille commented Mar 30, 2015

Yes, usually, everything works perfectly :)
With Apache handling SSL-TLS, wifidog-tls works, I got a "Pong".
bud-tls returns many different error codes with wifidog-tls, the "weirdest" being : "no shared cipher".

@mhaas

This comment has been minimized.

Contributor

mhaas commented Mar 30, 2015

Hm, ok. It would be good if you made a ticket with as much information (logs) as you can and I will take a look :)

Am 30. März 2015 16:20:51 MESZ, schrieb databeille notifications@github.com:

Yes, usually, everything works perfectly :)
With Apache handling SSL-TLS, wifidog-tls works, I got a "Pong".
bud-tls returns many different error codes with wifidog-tls, the
"weirdest" being : "no shared cipher".


Reply to this email directly or view it on GitHub:
#63 (comment)

@acv

This comment has been minimized.

Contributor

acv commented Mar 30, 2015

Can you run Qualys' SSL tester against your bud-tls endpoint? https://www.ssllabs.com/ssltest/

@databeille

This comment has been minimized.

Contributor

databeille commented Mar 30, 2015

@acv : yes, it works great !
I try to configure it less severe (+SSL2 +SSL3 +UseClientCypherSuite +...), but nothing good for now...

@acv

This comment has been minimized.

Contributor

acv commented Mar 30, 2015

Wifidog is hard coded to to TLS 1.x or greater only so SSL2/SSL3 is not supported.

@databeille

This comment has been minimized.

Contributor

databeille commented Mar 31, 2015

wifidog-tls works great with Apache+mod_ssl, Apache+stud (https://github.com/bumptech/stud/ ; my previous terminator, not maintained anymore) but miserably fails with bud-tls.
I give up for now.

@mhaas

This comment has been minimized.

Contributor

mhaas commented Apr 1, 2015

@databeille I assume you have the cyassl source handy? In examples/client, there is a test program. Execute something like this:

./client -h yourserver.xzy -p 443

So we can see if it's a wifidog problem or a cyassl+bud-tls problem.

@databeille

This comment has been minimized.

Contributor

databeille commented Apr 1, 2015

It is not related to wifidog.
cyassl's ./client script fails too (master branch).

@databeille

This comment has been minimized.

Contributor

databeille commented Apr 1, 2015

My last post about this problem : "INCOMPLETE_DATA" being wide (cases in cyassl/src/tls.c and cyassl/src/internal.c), the error code returned is related to :

    /* make sure can read the message */
    if (*inOutIdx + size > totalSz)
        return INCOMPLETE_DATA;

in src/internal.c.

Will now push an issue to bud-tls.

Anyway, thanks for your help @mhaas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment