Implement certificate authentication for GNUTLS #29
Conversation
puiterwijk
force-pushed
the
certs
branch
2 times, most recently
from
f3fb7a1 to
b34b52b
Compare
Codecov Report
@@ Coverage Diff @@
## master #29 +/- ##
==========================================
- Coverage 64.87% 64.76% -0.11%
==========================================
Files 13 13
Lines 1170 1456 +286
==========================================
+ Hits 759 943 +184
- Misses 411 513 +102
Continue to review full report at Codecov.
|
puiterwijk
force-pushed
the
certs
branch
2 times, most recently
from
17b77bb to
9a1269e
Compare
This implements TLS certificate authentication in the GNUTLS implementation. It supports mutual authentication, and determining the peer subject DN. Signed-off-by: Patrick Uiterwijk <[email protected]>
There was a problem hiding this comment.
-
Please properly format your commit message.
https://chris.beams.io/posts/git-commit/ -
I'm now reconsidering the callback function approach. There are several reasons for this.
First, we end up having to pass allocated memory across a system API. This isn't great. On some operating systems this is explcitly disallowed (malloc() and free() have to be in the same module).
Second, in the case of bindings it will be easier to pass known values. For example, Python can already pass a string to a setsockopt() call. Callbacks introduce language-specific difficulties.
I'm now thinking we should require some sockopts to be set before we call handshake. Then, when handshake needs authentication, it can return with a known errno (maybe EINPROGRESS?). Then the caller can get and set additional socket options. I think this method is less controversial.
Sorry to pull a change like that on you, but I think it is for the best.
| cert_cb(void *m, | ||
| char **cert_uri, | ||
| char **key_uri, | ||
| char **pin) |
There was a problem hiding this comment.
Please follow argument wrapping style in the code. This goes for all functions below.
| return -1; | ||
| *pin = strdup(o->crtkp); | ||
| if (!pin) | ||
| return -1; |
There was a problem hiding this comment.
Hrm... I know that the key URI can specify the PIN as one of its arguments. I'm not sure how I feel about having a separate PIN parameter.
| @@ -160,6 +203,13 @@ on_conn(options_t *opts, int con, int in, int out, const struct addrinfo *ai) | |||
|
|
|||
| if (opts->psku) | |||
| srv.psk = srv_psk_cb; | |||
| else if (opts->crtf) | |||
| { | |||
There was a problem hiding this comment.
Don't wrap the braces here. Also, we don't do unbalanced braces. If the else if requires braces, the if gets them too.
| fprintf(stderr, "%m: Unable to get peer subject DN!\n"); | ||
| shutdown(con, SHUT_RDWR); | ||
| return STATUS_FAILURE; | ||
| } |
There was a problem hiding this comment.
Please use some blank lines in here to separate the code blocks.
| char **key_uri, | ||
| char **pin); | ||
| int client_certificate_request; // 0 for no client cert, 1 for optional, 2 for required | ||
| const char *cafile; |
There was a problem hiding this comment.
If I understand this correctly, we can use the implicit value of cafile to determine if a client certificate can be validated. This means that client_certificate_request can become bool client_certificate_required.
| gnutls_x509_crt_t crt; | ||
| const gnutls_datum_t *cert_list; | ||
| unsigned int cert_list_size = 0; | ||
| gnutls_datum_t dn = {NULL, 0}; |
There was a problem hiding this comment.
Please sort variable definitions by line width: longest to shortest.
| char **key_uri, | ||
| char **pin); | ||
| const char *cafile; | ||
| int insecure; |
There was a problem hiding this comment.
If I understand the code correctly, insecure means that we trust all certificates. I wonder if we should disallow that mode altogether. In that case, cafile = NULL means trust the system CAs.
This implements TLS certificate authentication in the GNUTLS
implementation.
It supports mutual authentication, and determining the peer
subject DN.
Signed-off-by: Patrick Uiterwijk [email protected]