GNOME Bugzilla – Bug 726596
tlscertificate: support Subject Alternate Name IP address
Last modified: 2014-07-21 16:29:50 UTC
g_tls_certificate_verify currently only checks for hostname when verifying the identity. You can connect to a server using the IP address. This IP address can be set as an IP address in the X509v3 Subject Altername Name. If getting the hostname for that IP fails, g_tls_certificate_verify will fail without checking the X509v3 SAN.
Created attachment 272224 [details] [review] add suport for SAN IP addresses
Ping.
Comment on attachment 272224 [details] [review] add suport for SAN IP addresses >+/* Copied from gnutls/lib/x509/output.c */ >+static char *ip_to_string(void *_ip, You don't want this. GLib has its own routines for IP to string, but you actually want the other direction anyway; there are multiple ways of writing a given IPv6 address, so you need to compare the binary forms, not the textual forms. (Use g_inet_address_new_from_string() and then g_inet_address_get_bytes()/g_inet_address_get_native_size().) >+ /* Here we only support an additional check to support IP addresses. */ would be good to check g_hostname_is_ip_address(hostname) first >+ enum { MAX_SAN = 256 }; >+ char san[MAX_SAN]; No real need for a named constant here, and if you were going to use one, it should be a #define anyway.
Created attachment 273197 [details] [review] add suport for SAN IP addresses fixup New patch with fixes from review. Let's see how it looks now. Thanks!
Already have a GNOME git account. Thanks! Should I go ahead and commit my last patch? (comment 4).
Comment on attachment 273197 [details] [review] add suport for SAN IP addresses fixup This makes it so that gnutls_x509_crt_check_hostname() only gets called if g_hostname_is_ip_address() returns TRUE. You should probably do if (!hostname) return G_TLS_CERTIFICATE_BAD_IDENTITY; if (gnutls_x509_crt_check_hostname (gnutls->priv->cert, hostname)) return 0; addr = g_inet_address_new_from_string (hostname); if (addr) ... >+ if (memcmp (addr_bytes, san, addr_size) == 0) >+ return 0; leaks addr
Created attachment 275906 [details] [review] add suport for SAN IP addresses 2nd fixup Thanks for the review! Let's see how it looks now. I have kept. g_hostname_is_ip_address before g_inet_address_new_from_string.
Any reason I shouldn't apply latest patch?
Can you add a test case? Add a certificate to tls/tests/files/, and add a test to tls/tests/certificate.c to read it in and then verify that it verifies as expected?
(In reply to comment #10) > Can you add a test case? Add a certificate to tls/tests/files/, and add a test > to tls/tests/certificate.c to read it in and then verify that it verifies as > expected? That's a good reason, indeed. :-) Can you send me or add the CA key so I can create new client certificates with the same issuer as the others? May be it's already there, but can't find it.
(In reply to comment #11) > Can you send me or add the CA key so I can create new client certificates with > the same issuer as the others? > > May be it's already there, but can't find it. If it's not one of the keys that's already there, then I don't have it.
(In reply to comment #12) > (In reply to comment #11) > > Can you send me or add the CA key so I can create new client certificates with > > the same issuer as the others? > > > > May be it's already there, but can't find it. > > If it's not one of the keys that's already there, then I don't have it. I'm afraid it's not. I'll create a script to generate ca, server and client certificates so we can generate everything easily.
Created attachment 281115 [details] [review] update server certificate with SAN IP and add UT I have filed bug 733365 and a patch to re-generate UT certificates. This patch needs the new certificates to exist before being applied. But the new Subject Alternative Name using IP address test cases is added and working. Let me know if this looks fine and I will also commit these two patches (code + UT). But first, bug 733365.
committed and pushed. thanks